[PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS

[PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS

[Coia Prant]

This series adds proper SGMII support for the Rockchip RK3568 SoC
using the integrated Synopsys DesignWare XPCS, along with necessary
fixes and refactoring in the stmmac core and XPCS driver.

Motivation
==========
The RK3568 integrates a DW XPCS accessed via APB3 and connected to
a Naneng Combo SerDes PHY.  Several boards (e.g., Ariaboard
Photonicat) use this interface for Gigabit Ethernet.  However, the
current upstream stmmac driver does not support this configuration,
and the XPCS driver has issues in SGMII poll mode that cause the
link to be reported incorrectly.

This series addresses these issues by:
- Refactoring stmmac PCS lifetime management to allow platform drivers
  full control over PCS creation/destruction
- Fixing the XPCS driver's SGMII link recovery
- Adding a Rockchip XPCS platform glue driver and wiring it up in
  dwmac-rk

Series overview
===============

Generic:
  Patch 1: move XPCS lifetime management to platform drivers

PHY:
  Patch 2: DT binding for Naneng Combo PHY SGMII MAC selection
  Patch 3: implement the PHY SGMII MAC selection in driver

RK3568 XPCS/SGMII:
  Patch 4: DT binding for Rockchip RK3568 XPCS
  Patch 5: add XPCS and fixed-clock nodes to rk3568.dtsi
  Patch 6: add ANRESTART support for SGMII link recovery
  Patch 7: implement the Rockchip XPCS platform glue driver
  Patch 8: DT binding for Rockchip DWMAC PCS
  Patch 9: wire up SGMII support in dwmac-rk
  Patch 11: update MAINTAINERS

Board enablement:
  Patch 10: enable SGMII LAN port on Photonicat board

Changelog
=========

Changes since v4:

Patch 1 (net: stmmac: move XPCS lifetime management to platform drivers)
- Corrected commit message: pcs_init/pcs_exit callbacks already exist in
  the baseline; this patch simplifies the dispatch, not introduces them
- Fixed description of the priv->hw->xpcs clobber: it happens on the
  plat->pcs_init branch, not the else branch
- Corrected the list of affected platform drivers: only Intel mGbE moves
  its XPCS creation; renesas-gbeth, rzn1 and socfpga only get the
  explicit NULL assignment in pcs_exit()
- Changed the "No xPCS found" message to a more generic "Failed to
  initialize PCS", since three of the four pcs_init() implementers do
  not create an XPCS
- Dropped the redundant NULL initializer in intel_mgbe_pcs_init()

Patch 3 (phy: rockchip: naneng-combphy)
- Added the TRM bit polarity (0 = GMAC0, 1 = GMAC1) and the hardware
  reset value (1) to the commit message

Patch 4 (dt-bindings: net: pcs: rockchip-rk3568)
- Renamed from rockchip-dwxpcs.yaml to rockchip,rk3568-xpcs.yaml
- Renamed pcs@ / pcs-mii@N to ethernet-pcs@ / ethernet-pcs-mii@N to match
  the sibling snps,dw-xpcs.yaml binding

Patch 5 (arm64: dts: rockchip: rk3568)
- Renamed clock nodes to clock-xpcs-gmac0 / clock-xpcs-gmac1 to match
  the preferred clock-<name> pattern in fixed-clock.yaml
- Aligned the labels (clk_gmac0_xpcs_mii / clk_gmac1_xpcs_mii) with
  clock-output-names

Patch 6 (net: pcs: xpcs: add ANRESTART support)
- Updated the comment in xpcs_config_aneg_c37_sgmii() to explain that
  although the DesignWare databook says AN restart is not needed for
  MAC side SGMII, some implementations (RK3568) require it
- Propagated the return value of xpcs_modify() so errors on the restart
  path are visible

Patch 7 (net: pcs: xpcs: add Rockchip RK3568 platform glue driver)
- Added GENERIC_PHY and PM_GENERIC_DOMAINS dependencies to Kconfig
- Derived the EEE multiplier at runtime from clk_get_rate(), and call
  xpcs_config_eee_mult_fact() in xpcs_rk_create()
- Moved device_link_add() inside the device_lock() critical section to
  close the race window where the supplier could be unbound

Patch 8 (NEW)
- dt-bindings: net: rockchip-dwmac: document pcs-handle

Patch 9 (net: stmmac: dwmac-rk)
- Added a rk3568_set_to_rmii() callback that explicitly clears CON1 bit 7
- Updated the commit message: the clock enable reorder prepares for the
  SGMII path that skips rk_get_phy_intf_sel(), it does not fix a
  pre-existing SError
- Documented the in-band-only limitation of the SGMII support
- Kept supports_sgmii unconditional to match the existing RGMII/RMII
  handling; phy-mode is fixed by DT and does not change at runtime

Patch 10 (arm64: dts: rockchip: rk3568-photonicat)
- Reordered properties in &combphy2, &gmac0 and &xpcs to follow the
  file's convention (alphabetical, status last)
- Corrected the changelog to say the property is added to an already
  enabled combphy2

Key design decisions
====================
- The stmmac core now delegates XPCS creation entirely to platform
  drivers via pcs_init/pcs_exit.  This is necessary because the
  generic XPCS creation logic would override any XPCS set up by the
  platform driver.

- The Rockchip XPCS driver creates a virtual MDIO bus over the APB3
  registers and implements address remapping.  The generic XPCS core
  handles all PCS configuration via phylink_pcs_ops.

- On RK3568 in SGMII mode, the MAC clock is fixed at 125 MHz and
  cannot be dynamically changed.  In-band mode is used, and the
  generic stmmac set_clk_tx_rate callback is disabled to prevent
  incorrect clock updates that would break RX.

- The SerDes and power domain are attached to the XPCS device tree
  node rather than the MAC node. This reflects the actual hardware
  topology and simplifies the dwmac-rk driver by keeping all PCS-related
  resources self-contained. It also prepares for possible future QSGMII
  support, where a single SerDes serves multiple MACs and would be
  more naturally managed under the XPCS node.

Testing
=======
Board: Ariaboard Photonicat (RK3568)
OS: Armbian (trixie)
Kernel: 6.18 (backports)
Result: The SGMII interface obtains an IP address, SSH works, and
        ping traffic passes without loss.

Notes
=====
- When testing out-band mode with set_clk_tx_rate, only 1000Mbps
  works on both TX/RX; 10/100Mbps only works on TX side.

Dependencies
============
None. All patches apply cleanly on top of torvalds master tree (v7.3).

Acknowledgments
===============
This work was inspired by and builds upon the excellent work of others:
- Serge Semin's Synopsys DesignWare XPCS platform driver (pcs-xpcs-plat.c)
- Clément Léger's Renesas MIIC driver (pcs-rzn1-miic.c)
- The Rockchip TRM and downstream OEM drivers

Thanks in advance,
Coia Prant
---
Coia Prant (11):
  net: stmmac: move XPCS lifetime management to platform drivers
  dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel
    property
  phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
  dt-bindings: net: pcs: add rockchip,rk3568-xpcs support
  arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes
  net: pcs: xpcs: add ANRESTART support for SGMII link recovery
  net: pcs: xpcs: add Rockchip RK3568 platform glue driver
  dt-bindings: net: rockchip-dwmac: document pcs-handle
  net: stmmac: dwmac-rk: add SGMII support for RK3568
  arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port
  MAINTAINERS: add entry for Rockchip XPCS driver

 .../net/pcs/rockchip,rk3568-xpcs.yaml         | 110 ++++
 .../bindings/net/rockchip-dwmac.yaml          |  15 +
 .../phy/phy-rockchip-naneng-combphy.yaml      |  13 +
 MAINTAINERS                                   |   9 +
 .../boot/dts/rockchip/rk3568-photonicat.dts   |  74 ++-
 arch/arm64/boot/dts/rockchip/rk3568.dtsi      |  45 ++
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   1 +
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  44 +-
 .../stmicro/stmmac/dwmac-renesas-gbeth.c      |   7 +-
 .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 134 ++++-
 .../net/ethernet/stmicro/stmmac/dwmac-rzn1.c  |   7 +-
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |   7 +-
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c |  39 +-
 drivers/net/pcs/Kconfig                       |  25 +
 drivers/net/pcs/Makefile                      |   5 +-
 drivers/net/pcs/pcs-xpcs-rk.c                 | 559 ++++++++++++++++++
 drivers/net/pcs/pcs-xpcs.c                    |  35 +-
 .../rockchip/phy-rockchip-naneng-combphy.c    |   8 +
 include/linux/pcs/pcs-xpcs-rk.h               |  11 +
 19 files changed, 1076 insertions(+), 72 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
 create mode 100644 drivers/net/pcs/pcs-xpcs-rk.c
 create mode 100644 include/linux/pcs/pcs-xpcs-rk.h

-- 
2.47.3

[PATCH net-next v5 01/11] net: stmmac: move XPCS lifetime management to platform drivers

[Coia Prant]

The current XPCS creation logic in stmmac_pcs_setup() is problematic
for several reasons.

First, if a device tree specifies a "pcs-handle" but no select_pcs()
callback is provided by the platform driver, the created XPCS is never
used. The phylink framework requires select_pcs() to actually return
the PCS to the core, so the pcs-handle property becomes effectively
useless without the matching callback. This is confusing for developers
who expect that specifying a pcs-handle in their device tree should be
sufficient to enable the PCS.

Second, and more critically, when a platform driver sets pcs_init()
and creates an XPCS inside that callback, the common code afterwards
still runs unconditionally and overwrites priv->hw->xpcs with the
local xpcs variable, which stays NULL. The platform driver has no way
to prevent this override because the common code runs after the
platform-specific initialization.

After commit 93f84152e4ae ("net: stmmac: clean up
stmmac_mac_select_pcs()"), the common code no longer falls back to
priv->hw->phylink_pcs if select_pcs() is not set. This change
reinforces that each platform must manage its own PCS life cycle
explicitly, but the XPCS creation code in stmmac_pcs_setup() was not
updated to match this new expectation, leaving a gap where platform
drivers have no clean way to take control of XPCS creation.

Address all of these issues by simplifying the existing pcs_init() and
pcs_exit() dispatch in stmmac_pcs_setup() and stmmac_pcs_clean(). The
common stmmac_pcs_setup() now calls plat->pcs_init() if present, and
stmmac_pcs_clean() calls plat->pcs_exit() if present, removing the
confusing and error-prone XPCS creation logic from the common code.

Platforms that do not need an XPCS simply leave the callbacks as NULL
and no change in behavior occurs. Platforms that do need an XPCS can
now create it with the exact configuration they require, including
wrapping it with custom phylink_pcs_ops when necessary.

The Intel mGbE glue is updated to create its XPCS inside its own
pcs_init() implementation, and the renesas-gbeth, rzn1 and socfpga
pcs_exit() callbacks now explicitly clear priv->hw->phylink_pcs after
destroying the PCS to avoid dangling pointer references.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 44 +++++++++++++++++--
 .../stmicro/stmmac/dwmac-renesas-gbeth.c      |  7 ++-
 .../net/ethernet/stmicro/stmmac/dwmac-rzn1.c  |  7 ++-
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |  7 ++-
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 39 +++-------------
 5 files changed, 62 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index f5f9fa67ecd77..4308dccbf2570 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -603,13 +603,47 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
 	plat->mdio_bus_data->needs_reset = true;
 }
 
+static int intel_mgbe_pcs_init(struct stmmac_priv *priv)
+{
+	struct fwnode_handle *devnode, *pcsnode;
+	struct dw_xpcs *xpcs;
+	int addr;
+
+	devnode = dev_fwnode(priv->device);
+
+	if (fwnode_property_present(devnode, "pcs-handle")) {
+		pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
+		xpcs = xpcs_create_fwnode(pcsnode);
+		fwnode_handle_put(pcsnode);
+	} else {
+		addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
+		xpcs = xpcs_create_mdiodev(priv->mii, addr);
+	}
+
+	if (IS_ERR(xpcs))
+		return PTR_ERR(xpcs);
+
+	xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
+
+	priv->hw->xpcs = xpcs;
+	return 0;
+}
+
+static void intel_mgbe_pcs_exit(struct stmmac_priv *priv)
+{
+	if (!priv->hw->xpcs)
+		return;
+
+	xpcs_destroy(priv->hw->xpcs);
+	priv->hw->xpcs = NULL;
+}
+
 static struct phylink_pcs *intel_mgbe_select_pcs(struct stmmac_priv *priv,
 						 phy_interface_t interface)
 {
-	/* plat->mdio_bus_data->has_xpcs has been set true, so there
-	 * should always be an XPCS. The original code would always
-	 * return this if present.
-	 */
+	if (!priv->hw->xpcs)
+		return NULL;
+
 	return xpcs_to_phylink_pcs(priv->hw->xpcs);
 }
 
@@ -733,6 +767,8 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
 	    plat->phy_interface == PHY_INTERFACE_MODE_1000BASEX) {
 		plat->mdio_bus_data->pcs_mask = BIT_U32(INTEL_MGBE_XPCS_ADDR);
 		plat->default_an_inband = true;
+		plat->pcs_init = intel_mgbe_pcs_init;
+		plat->pcs_exit = intel_mgbe_pcs_exit;
 		plat->select_pcs = intel_mgbe_select_pcs;
 	}
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
index 19f34e18bfef2..9af32c26f9c14 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
@@ -81,8 +81,11 @@ static int renesas_gmac_pcs_init(struct stmmac_priv *priv)
 
 static void renesas_gmac_pcs_exit(struct stmmac_priv *priv)
 {
-	if (priv->hw->phylink_pcs)
-		miic_destroy(priv->hw->phylink_pcs);
+	if (!priv->hw->phylink_pcs)
+		return;
+
+	miic_destroy(priv->hw->phylink_pcs);
+	priv->hw->phylink_pcs = NULL;
 }
 
 static struct phylink_pcs *renesas_gmac_select_pcs(struct stmmac_priv *priv,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
index 13634965bc19a..01df4776edb3f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
@@ -35,8 +35,11 @@ static int rzn1_dwmac_pcs_init(struct stmmac_priv *priv)
 
 static void rzn1_dwmac_pcs_exit(struct stmmac_priv *priv)
 {
-	if (priv->hw->phylink_pcs)
-		miic_destroy(priv->hw->phylink_pcs);
+	if (!priv->hw->phylink_pcs)
+		return;
+
+	miic_destroy(priv->hw->phylink_pcs);
+	priv->hw->phylink_pcs = NULL;
 }
 
 static struct phylink_pcs *rzn1_dwmac_select_pcs(struct stmmac_priv *priv,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 1d7f0a57d2889..6d4bc1fe8f751 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -539,8 +539,11 @@ static int socfpga_dwmac_pcs_init(struct stmmac_priv *priv)
 
 static void socfpga_dwmac_pcs_exit(struct stmmac_priv *priv)
 {
-	if (priv->hw->phylink_pcs)
-		lynx_pcs_destroy(priv->hw->phylink_pcs);
+	if (!priv->hw->phylink_pcs)
+		return;
+
+	lynx_pcs_destroy(priv->hw->phylink_pcs);
+	priv->hw->phylink_pcs = NULL;
 }
 
 static struct phylink_pcs *socfpga_dwmac_select_pcs(struct stmmac_priv *priv,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index afe98ff5bdcb0..7396b68899c66 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -426,35 +426,14 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 int stmmac_pcs_setup(struct net_device *ndev)
 {
 	struct stmmac_priv *priv = netdev_priv(ndev);
-	struct fwnode_handle *devnode, *pcsnode;
-	struct dw_xpcs *xpcs = NULL;
-	int addr, ret;
-
-	devnode = dev_fwnode(priv->device);
-
-	if (priv->plat->pcs_init) {
-		ret = priv->plat->pcs_init(priv);
-	} else if (fwnode_property_present(devnode, "pcs-handle")) {
-		pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
-		xpcs = xpcs_create_fwnode(pcsnode);
-		fwnode_handle_put(pcsnode);
-		ret = PTR_ERR_OR_ZERO(xpcs);
-	} else if (priv->plat->mdio_bus_data &&
-		   priv->plat->mdio_bus_data->pcs_mask) {
-		addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
-		xpcs = xpcs_create_mdiodev(priv->mii, addr);
-		ret = PTR_ERR_OR_ZERO(xpcs);
-	} else {
+	int ret;
+
+	if (!priv->plat->pcs_init)
 		return 0;
-	}
 
+	ret = priv->plat->pcs_init(priv);
 	if (ret)
-		return dev_err_probe(priv->device, ret, "No xPCS found\n");
-
-	if (xpcs)
-		xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
-
-	priv->hw->xpcs = xpcs;
+		return dev_err_probe(priv->device, ret, "Failed to initialize PCS\n");
 
 	return 0;
 }
@@ -463,14 +442,10 @@ void stmmac_pcs_clean(struct net_device *ndev)
 {
 	struct stmmac_priv *priv = netdev_priv(ndev);
 
-	if (priv->plat->pcs_exit)
-		priv->plat->pcs_exit(priv);
-
-	if (!priv->hw->xpcs)
+	if (!priv->plat->pcs_exit)
 		return;
 
-	xpcs_destroy(priv->hw->xpcs);
-	priv->hw->xpcs = NULL;
+	priv->plat->pcs_exit(priv);
 }
 
 struct stmmac_clk_rate {
-- 
2.47.3

[PATCH net-next v5 02/11] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property

[Coia Prant]

On RK3568, the SGMII interface can be routed to either GMAC0 or
GMAC1 via the pipe_sgmii_mac_sel bit in the pipe GRF registers.

Add the optional "rockchip,sgmii-mac-sel" property to allow the
device tree to select which GMAC controller is used for SGMII.

The property takes a value of 0 (GMAC0) or 1 (GMAC1). The hardware
reset value is 1 (GMAC1), but this can be overridden by setting the
property to 0 for boards where SGMII is connected to GMAC0.

This is necessary for boards such as the Ariaboard Photonicat, where
the SGMII interface is connected to GMAC0 and needs to be explicitly
configured.

Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 .../bindings/phy/phy-rockchip-naneng-combphy.yaml   | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
index 379b08bd9e97a..8e898bce9af73 100644
--- a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
+++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
@@ -80,6 +80,15 @@ properties:
     description:
       Some additional pipe settings are accessed through GRF regs.
 
+  rockchip,sgmii-mac-sel:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+    default: 1
+    description:
+      Select gmac0 or gmac1 to be used as SGMII controller.
+      The hardware reset value is GMAC1 (1). Set this to 0 to route
+      SGMII to GMAC0.
+
   "#phy-cells":
     const: 1
 
@@ -105,6 +114,10 @@ allOf:
           maxItems: 1
         reset-names:
           maxItems: 1
+        rockchip,sgmii-mac-sel: true
+    else:
+      properties:
+        rockchip,sgmii-mac-sel: false
   - if:
       properties:
         compatible:
-- 
2.47.3

[PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[Coia Prant]

On RK3568, the SGMII interface can be routed to either GMAC0 or
GMAC1 via the GRF register pipe_sgmii_mac_sel.

Add support for this selection by introducing
the "rockchip,sgmii-mac-sel" DT property.

From the RK3568 TRM (Part1, Page 229), the PIPE_GRF_XPCS_CON0
bit 1 (pipe_sgmii_mac_sel) is defined as:

    0: SGMII routed to GMAC0
    1: SGMII routed to GMAC1

The hardware reset value is 1 (GMAC1). If the property is set to 0,
the driver routes SGMII to GMAC0; if set to 1 (or omitted), it
remains at GMAC1.

This is necessary for boards such as the Ariaboard Photonicat, which
uses the SGMII interface connected to GMAC0.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index 7843356a4dd47..7b867e7520064 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
@@ -186,6 +186,7 @@ struct rockchip_combphy_grfcfg {
 	struct combphy_reg pipe_xpcs_phy_ready;
 	struct combphy_reg pipe_pcie1l0_sel;
 	struct combphy_reg pipe_pcie1l1_sel;
+	struct combphy_reg pipe_sgmii_mac_sel;
 	struct combphy_reg u3otg0_port_en;
 	struct combphy_reg u3otg1_port_en;
 };
@@ -212,6 +213,7 @@ struct rockchip_combphy_priv {
 	bool enable_ssc;
 	bool ext_refclk;
 	struct clk *refclk;
+	u32 sgmii_mac_sel;
 };
 
 static void rockchip_combphy_updatel(struct rockchip_combphy_priv *priv,
@@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
 
 	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
 
+	priv->sgmii_mac_sel = 1;
+	device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
+
 	priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
 	/* fallback to old behaviour */
 	if (PTR_ERR(priv->phy_rst) == -ENOENT)
@@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
 		break;
 
 	case PHY_TYPE_SGMII:
+		rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
+					     priv->sgmii_mac_sel > 0);
 		rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
 		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
 		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);
@@ -984,6 +991,7 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
 	.con3_for_sata		= { 0x000c, 15, 0, 0x00, 0x4407 },
 	/* pipe-grf */
 	.pipe_con0_for_sata	= { 0x0000, 15, 0, 0x00, 0x2220 },
+	.pipe_sgmii_mac_sel	= { 0x0040, 1, 1, 0x00, 0x01 },
 	.pipe_xpcs_phy_ready	= { 0x0040, 2, 2, 0x00, 0x01 },
 	.u3otg0_port_en		= { 0x0104, 15, 0, 0x0181, 0x1100 },
 	.u3otg1_port_en		= { 0x0144, 15, 0, 0x0181, 0x1100 },
-- 
2.47.3

[PATCH net-next v5 04/11] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

[Coia Prant]

Add device tree binding documentation for the Synopsys DesignWare
XPCS integrated on the Rockchip RK3568 SoC.

The XPCS is accessed over the APB3 bus and internally connected to
a Naneng Combo SerDes PHY.  It supports 1000BASE-X, SGMII, and
QSGMII modes, with four MII ports.

Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 .../net/pcs/rockchip,rk3568-xpcs.yaml         | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml

diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
new file mode 100644
index 0000000000000..703fcff0e3f70
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/rockchip,rk3568-xpcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip RK3568 Synopsys DesignWare Ethernet PCS
+
+maintainers:
+  - Coia Prant <coiaprant@gmail.com>
+
+description: |
+  Rockchip RK3568 SoC integrates a Synopsys DesignWare Ethernet Physical
+  Coding Sublayer (XPCS).
+  The PCS provides an interface between the Media Access Control (MAC)
+  and the Physical Medium Attachment (PMA) sublayer through a Media
+  Independent Interface (GMII).
+
+  The XPCS is accessed over the APB3 bus and internally connected to a
+  Naneng Combo SerDes PHY.
+  It supports 1000BASE-X, SGMII and QSGMII modes.
+
+  The block contains four MII ports that can be individually enabled and
+  routed to one of the Ethernet GMAC controllers via the pcs-handle
+  property in the MAC device tree node.
+
+properties:
+  compatible:
+    const: rockchip,rk3568-xpcs
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  clocks:
+    items:
+      - description: APB3 bus interface clock (clk_csr_i), required for register access
+      - description: EEE clock (clk_eee_i), required for Energy Efficient Ethernet operation
+
+  clock-names:
+    items:
+      - const: csr
+      - const: eee
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: serdes
+
+  power-domains:
+    maxItems: 1
+
+patternProperties:
+  "^ethernet-pcs-mii@[0-3]$":
+    type: object
+    description:
+      One of the four MII ports of the XPCS. The port is linked to an
+      Ethernet MAC controller via the pcs-handle property in the MAC's
+      device tree node.
+
+    properties:
+      reg:
+        description: MII port number.
+        enum: [0, 1, 2, 3]
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - clocks
+  - clock-names
+  - phys
+  - phy-names
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3568-cru.h>
+    #include <dt-bindings/power/rk3568-power.h>
+    #include <dt-bindings/phy/phy.h>
+
+    ethernet-pcs@fda00000 {
+      compatible = "rockchip,rk3568-xpcs";
+      reg = <0xfda00000 0x200000>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
+      clock-names = "csr", "eee";
+      phys = <&combphy2 PHY_TYPE_SGMII>;
+      phy-names = "serdes";
+      power-domains = <&power RK3568_PD_PIPE>;
+
+      ethernet-pcs-mii@0 {
+        reg = <0>;
+      };
+    };
-- 
2.47.3

[PATCH net-next v5 05/11] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

[Coia Prant]

The RK3568 SoC integrates a Synopsys DesignWare XPCS that provides
the Physical Coding Sublayer for 1000BASE-X, SGMII, and QSGMII
interfaces via its four MII ports.  Add the XPCS device node and
its pcs-mii sub-nodes to the SoC device tree.

The XPCS device is accessed via the APB3 bus at 0xfda00000 and
requires the CSR clock (PCLK_XPCS) for register access and the EEE
clock (CLK_XPCS_EEE) for Energy Efficient Ethernet operation.  The
PD_PIPE power domain must be enabled before any register access.

Also add two fixed-clock nodes (xpcs_gmac0_clk and xpcs_gmac1_clk)
providing the 125 MHz reference clock for the GMACs when operating
with XPCS.  These clocks are used as the assigned-clock-parents
for the respective GMAC nodes in board-level device trees.

The XPCS node and its pcs-mii sub-nodes are disabled by default and
must be enabled at the board level when 1000BASE-X/SGMII/QSGMII is
in use.  The fixed-clock nodes are always present and do not have a
status property, as they are static clock sources.

The XPCS node also requires a reference to the appropriate Naneng
Combo PHY via the phys property at the board level.

Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3568.dtsi | 45 ++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
index 3bc653f027f1f..227d03e336043 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
@@ -110,6 +110,51 @@ sata0: sata@fc000000 {
 		status = "disabled";
 	};
 
+	xpcs: ethernet-pcs@fda00000 {
+		compatible = "rockchip,rk3568-xpcs";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x0 0xfda00000 0x0 0x200000>;
+		clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
+		clock-names = "csr", "eee";
+		power-domains = <&power RK3568_PD_PIPE>;
+		status = "disabled";
+
+		xpcs_mii0: ethernet-pcs-mii@0 {
+			reg = <0>;
+			status = "disabled";
+		};
+
+		xpcs_mii1: ethernet-pcs-mii@1 {
+			reg = <1>;
+			status = "disabled";
+		};
+
+		xpcs_mii2: ethernet-pcs-mii@2 {
+			reg = <2>;
+			status = "disabled";
+		};
+
+		xpcs_mii3: ethernet-pcs-mii@3 {
+			reg = <3>;
+			status = "disabled";
+		};
+	};
+
+	clk_gmac0_xpcs_mii: clock-xpcs-gmac0 {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "clk_gmac0_xpcs_mii";
+		#clock-cells = <0>;
+	};
+
+	clk_gmac1_xpcs_mii: clock-xpcs-gmac1 {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "clk_gmac1_xpcs_mii";
+		#clock-cells = <0>;
+	};
+
 	pipe_phy_grf0: syscon@fdc70000 {
 		compatible = "rockchip,rk3568-pipe-phy-grf", "syscon";
 		reg = <0x0 0xfdc70000 0x0 0x1000>;
-- 
2.47.3

[PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery

[Coia Prant]

On some hardware using the DesignWare XPCS IP (e.g., RK3568 MAC side
SGMII), the PCS does not automatically restart auto-negotiation when the
link goes down and comes back up. Without an explicit ANRESTART, the link
stays down forever.

Add BMCR_ANRESTART in two places:
1. In xpcs_config_aneg_c37_sgmii(), when starting AN, set ANRESTART
   alongside ANENABLE to initiate a fresh negotiation.
2. In xpcs_get_state_c37_sgmii(), when link is down and AN completion is
   detected, clear the interrupt and trigger ANRESTART to restart the
   negotiation process. Propagate the return value of the restart so
   errors are not silently ignored.

Update the comment in xpcs_config_aneg_c37_sgmii() to note that although
the DesignWare databook says AN restart is not needed for MAC side SGMII,
some implementations (e.g. Rockchip RK3568) require it to recover the
link after a disconnect.

This is not a fix for an existing mainline platform: the affected
platform (RK3568 XPCS) is introduced later in the same series.

Tested-by: Jiawen Wu <jiawenwu@trustnetic.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 drivers/net/pcs/pcs-xpcs.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 0337e2bcc0125..8c3875b6985b9 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -761,7 +761,9 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 	 *    DW xPCS used with DW EQoS MAC is always MAC side SGMII.
 	 * 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic
 	 *    speed/duplex mode change by HW after SGMII AN complete)
-	 * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN)
+	 * 5) VR_MII_AN_INTR_STS = 0x0 (Clear CL37 AN complete status)
+	 * 6) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN)
+	 *    VR_MII_MMD_CTRL Bit(9) [AN_RESTART] = 1b (Restart SGMII AN)
 	 *
 	 * Note that VR_MII_MMD_CTRL is MII_BMCR.
 	 *
@@ -769,7 +771,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 	 *	 SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
 	 *	 PHY about the link state change after C28 AN is completed
 	 *	 between PHY and Link Partner. There is also no need to
-	 *	 trigger AN restart for MAC-side SGMII.
+	 *	 trigger AN restart for MAC-side SGMII on most devices.
+	 *
+	 * Note: While the DesignWare databook states that AN restart is
+	 *	 not needed for MAC side SGMII, some implementations (e.g.
+	 *	 Rockchip RK3568) exhibit a timing quirk when integrated with
+	 *	 phylink and do not restart AN automatically when the link
+	 *	 comes back up. An explicit AN restart is required on those
+	 *	 parts to recover the link after a disconnect.
 	 */
 	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
 	if (mdio_ctrl < 0)
@@ -816,9 +825,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 	if (ret < 0)
 		return ret;
 
+	/* Clear CL37 AN complete status */
+	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+	if (ret < 0)
+		return ret;
+
 	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
-				 mdio_ctrl | BMCR_ANENABLE);
+				 mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
 
 	return ret;
 }
@@ -1093,9 +1107,18 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
 		return 0;
 	}
 
-	/* Clear AN complete status or interrupt */
-	if (state->an_complete)
-		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+	if (state->an_complete) {
+		/* Clear AN complete status or interrupt */
+		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+		if (ret < 0)
+			return ret;
+
+		/* Initiate the next round of AN */
+		ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
+				  BMCR_ANRESTART);
+		if (ret < 0)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.47.3

[PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[Coia Prant]

The RK3568 SoC integrates a Synopsys DesignWare XPCS that is accessed
via APB3 memory-mapped registers.
This driver provides the glue logic to make the XPCS accessible to
the generic pcs-xpcs core.

The XPCS block contains four MII ports (0..3), each of which can be
routed to GMAC0 or GMAC1 via the pcs-handle property in the MAC node.
The hardware maps these ports to different MMDs:
  - port 0: MMD 7 (ROCKCHIP_MMD_MII)
  - port 1: MMD 2 (ROCKCHIP_MMD_MII1)
  - port 2: MMD 3 (ROCKCHIP_MMD_MII2)
  - port 3: MMD 4 (ROCKCHIP_MMD_MII3)

This driver creates a virtual MDIO bus that translates MDIO operations
to APB3 register accesses, with proper address remapping for each port.
The generic xpcs driver then creates a phylink_pcs instance on top of
this bus, allowing the MAC to use the PCS via the standard phylink API.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 drivers/net/pcs/Kconfig         |  25 ++
 drivers/net/pcs/Makefile        |   5 +-
 drivers/net/pcs/pcs-xpcs-rk.c   | 559 ++++++++++++++++++++++++++++++++
 include/linux/pcs/pcs-xpcs-rk.h |  11 +
 4 files changed, 598 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/pcs/pcs-xpcs-rk.c
 create mode 100644 include/linux/pcs/pcs-xpcs-rk.h

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index e417fd66f660a..080538f193d59 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -12,6 +12,31 @@ config PCS_XPCS
 	  This module provides a driver and helper functions for Synopsys
 	  DesignWare XPCS controllers.
 
+if PCS_XPCS
+
+config PCS_XPCS_PLATFORM
+	tristate "Generic XPCS controller support"
+	default PCS_XPCS
+	help
+	  Generic DWXPCS driver for platforms that don't require any
+	  platform specific code to function or is using platform
+	  data for setup.
+
+	  If you have a controller with this interface, say Y or M here.
+
+config PCS_XPCS_ROCKCHIP
+	tristate "Rockchip XPCS controller support"
+	default ARCH_ROCKCHIP
+	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
+	depends on GENERIC_PHY || COMPILE_TEST
+	depends on PM_GENERIC_DOMAINS || COMPILE_TEST
+	help
+	  Support for XPCS controller on Rockchip RK356x SoC.
+
+	  If you have a Rockchip SoC with this interface, say Y or M here.
+
+endif # PCS_XPCS
+
 config PCS_LYNX
 	tristate
 	help
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b900..f9f6cf2578d72 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,10 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux PCS drivers
 
-pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-plat.o \
-				   pcs-xpcs-nxp.o pcs-xpcs-wx.o
+pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o
 
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
+obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
+obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
 obj-$(CONFIG_PCS_MTK_LYNXI)	+= pcs-mtk-lynxi.o
 obj-$(CONFIG_PCS_RZN1_MIIC)	+= pcs-rzn1-miic.o
diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
new file mode 100644
index 0000000000000..7fd1408c044cc
--- /dev/null
+++ b/drivers/net/pcs/pcs-xpcs-rk.c
@@ -0,0 +1,559 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Rockchip XPCS platform device driver
+ *
+ * Based on the Synopsys DesignWare XPCS platform driver.
+ * Copyright (C) 2024 Serge Semin
+ *
+ * Adapted for Rockchip SoCs, with reference to the Rockchip OEM driver.
+ * Copyright (C) 2026 Coia Prant
+ */
+
+#include <linux/atomic.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/math.h>
+#include <linux/mdio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pcs/pcs-xpcs-rk.h>
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/sizes.h>
+
+#include "pcs-xpcs.h"
+
+struct dw_xpcs_rk {
+	struct platform_device *pdev;
+	struct mii_bus *bus;
+	void __iomem *reg_base;
+	struct phy *serdes_phy;
+	struct clk *csr_clk;
+	struct clk *eee_clk;
+	u8 eee_mult_fact;
+};
+
+static ptrdiff_t xpcs_rk_addr_format(int dev, int reg)
+{
+	return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);
+}
+
+static int xpcs_rk_read_reg(struct dw_xpcs_rk *pxpcs, int dev, int reg)
+{
+	ptrdiff_t csr;
+	int ret;
+
+	csr = xpcs_rk_addr_format(dev, reg);
+
+	ret = pm_runtime_resume_and_get(&pxpcs->pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = readl(pxpcs->reg_base + (csr << 2)) & 0xffff;
+
+	pm_runtime_put(&pxpcs->pdev->dev);
+	return ret;
+}
+
+static int xpcs_rk_write_reg(struct dw_xpcs_rk *pxpcs, int dev, int reg, u16 val)
+{
+	ptrdiff_t csr;
+	int ret;
+
+	csr = xpcs_rk_addr_format(dev, reg);
+
+	ret = pm_runtime_resume_and_get(&pxpcs->pdev->dev);
+	if (ret)
+		return ret;
+
+	writel(val, pxpcs->reg_base + (csr << 2));
+
+	pm_runtime_put(&pxpcs->pdev->dev);
+	return 0;
+}
+
+#define ROCKCHIP_MMD_MII1	2
+#define ROCKCHIP_MMD_MII2	3
+#define ROCKCHIP_MMD_MII3	4
+#define ROCKCHIP_MMD_PMAPMD	6
+#define ROCKCHIP_MMD_MII	7
+
+static bool xpcs_rk_mdio_addr_validate(int addr)
+{
+	return !(addr < 0 || addr > 3);
+}
+
+static int xpcs_rk_mdio_read_remapping(int addr, int dev, int reg)
+{
+	switch (dev) {
+	case MDIO_MMD_PMAPMD:
+		return ROCKCHIP_MMD_PMAPMD;
+	case MDIO_MMD_VEND2:
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	/* read remapping to MII is performed by HW */
+	switch (addr) {
+	case 0:
+		return ROCKCHIP_MMD_MII;
+	case 1:
+		return ROCKCHIP_MMD_MII1;
+	case 2:
+		return ROCKCHIP_MMD_MII2;
+	case 3:
+		return ROCKCHIP_MMD_MII3;
+	default:
+		return -ENODEV;
+	}
+}
+
+static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
+{
+	switch (dev) {
+	case MDIO_MMD_PMAPMD:
+		return ROCKCHIP_MMD_PMAPMD;
+	case MDIO_MMD_VEND2:
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	/* Writable only on MII */
+	switch (reg) {
+	case DW_VR_MII_AN_CTRL:
+	case DW_VR_MII_AN_INTR_STS:
+	case DW_VR_MII_EEE_MCTRL0:
+	case DW_VR_MII_EEE_MCTRL1:
+	case DW_VR_MII_DIG_CTRL2:
+		return ROCKCHIP_MMD_MII;
+	default:
+		break;
+	}
+
+	switch (addr) {
+	case 0:
+		return ROCKCHIP_MMD_MII;
+	case 1:
+		return ROCKCHIP_MMD_MII1;
+	case 2:
+		return ROCKCHIP_MMD_MII2;
+	case 3:
+		return ROCKCHIP_MMD_MII3;
+	default:
+		return -ENODEV;
+	}
+}
+
+static int xpcs_rk_read_c22(struct mii_bus *bus, int addr, int reg)
+{
+	struct dw_xpcs_rk *pxpcs = bus->priv;
+	int dev;
+
+	if (!xpcs_rk_mdio_addr_validate(addr))
+		return -ENODEV;
+
+	dev = xpcs_rk_mdio_read_remapping(addr, MDIO_MMD_VEND2, reg);
+	if (dev < 0)
+		return 0xffff;
+
+	return xpcs_rk_read_reg(pxpcs, dev, reg);
+}
+
+static int xpcs_rk_write_c22(struct mii_bus *bus, int addr, int reg, u16 val)
+{
+	struct dw_xpcs_rk *pxpcs = bus->priv;
+	int dev;
+
+	if (!xpcs_rk_mdio_addr_validate(addr))
+		return -ENODEV;
+
+	dev = xpcs_rk_mdio_write_remapping(addr, MDIO_MMD_VEND2, reg);
+	if (dev < 0)
+		return 0;
+
+	return xpcs_rk_write_reg(pxpcs, dev, reg, val);
+}
+
+static int xpcs_rk_read_c45(struct mii_bus *bus, int addr, int dev, int reg)
+{
+	struct dw_xpcs_rk *pxpcs = bus->priv;
+
+	if (!xpcs_rk_mdio_addr_validate(addr))
+		return -ENODEV;
+
+	dev = xpcs_rk_mdio_read_remapping(addr, dev, reg);
+	if (dev < 0)
+		return 0xffff;
+
+	return xpcs_rk_read_reg(pxpcs, dev, reg);
+}
+
+static int xpcs_rk_write_c45(struct mii_bus *bus, int addr, int dev, int reg, u16 val)
+{
+	struct dw_xpcs_rk *pxpcs = bus->priv;
+
+	if (!xpcs_rk_mdio_addr_validate(addr))
+		return -ENODEV;
+
+	dev = xpcs_rk_mdio_write_remapping(addr, dev, reg);
+	if (dev < 0)
+		return 0;
+
+	return xpcs_rk_write_reg(pxpcs, dev, reg, val);
+}
+
+static struct dw_xpcs_rk *xpcs_rk_create_data(struct platform_device *pdev)
+{
+	struct dw_xpcs_rk *pxpcs;
+
+	pxpcs = devm_kzalloc(&pdev->dev, sizeof(*pxpcs), GFP_KERNEL);
+	if (!pxpcs)
+		return ERR_PTR(-ENOMEM);
+
+	pxpcs->pdev = pdev;
+
+	dev_set_drvdata(&pdev->dev, pxpcs);
+
+	return pxpcs;
+}
+
+static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
+{
+	struct device *dev = &pxpcs->pdev->dev;
+
+	pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
+	if (IS_ERR(pxpcs->serdes_phy))
+		return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
+					"Failed to get SerDes PHY\n");
+
+	return 0;
+}
+
+static void xpcs_rk_serdes_phy_poweroff(void *data)
+{
+	struct dw_xpcs_rk *pxpcs = data;
+	struct device *dev = &pxpcs->pdev->dev;
+
+	phy_power_off(pxpcs->serdes_phy);
+	phy_exit(pxpcs->serdes_phy);
+
+	dev_pm_genpd_rpm_always_on(dev, false);
+}
+
+static int xpcs_rk_serdes_phy_poweron(struct dw_xpcs_rk *pxpcs)
+{
+	struct device *dev = &pxpcs->pdev->dev;
+	int ret;
+
+	/*
+	 * The power domain is required and must be enabled, which allows us to
+	 * dynamically turn the CSR clock on/off using PM while keeping the PCS
+	 * powered on.
+	 */
+	ret = dev_pm_genpd_rpm_always_on(dev, true);
+	if (ret) {
+		dev_err(dev, "Failed to power on power-domains\n");
+		return ret;
+	}
+
+	ret = phy_init(pxpcs->serdes_phy);
+	if (ret) {
+		dev_err(dev, "Failed to init SerDes PHY\n");
+		goto pm_domain;
+	}
+
+	ret = phy_power_on(pxpcs->serdes_phy);
+	if (ret) {
+		dev_err(dev, "Failed to power on SerDes PHY\n");
+		goto serdes_phy;
+	}
+
+	ret = devm_add_action_or_reset(dev, xpcs_rk_serdes_phy_poweroff, pxpcs);
+	if (ret) {
+		dev_err(dev, "Failed to register devm for SerDes PHY: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+
+serdes_phy:
+	phy_exit(pxpcs->serdes_phy);
+pm_domain:
+	dev_pm_genpd_rpm_always_on(dev, false);
+	return ret;
+}
+
+static int xpcs_rk_init_res(struct dw_xpcs_rk *pxpcs)
+{
+	struct platform_device *pdev = pxpcs->pdev;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "No reg-space found\n");
+		return -EINVAL;
+	}
+
+	if (resource_size(res) < SZ_2M) {
+		dev_err(dev, "Invalid reg-space size\n");
+		return -EINVAL;
+	}
+
+	pxpcs->reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pxpcs->reg_base)) {
+		dev_err(dev, "Failed to map reg-space\n");
+		return PTR_ERR(pxpcs->reg_base);
+	}
+
+	return 0;
+}
+
+static void xpcs_rk_exit_clk(void *data)
+{
+	struct dw_xpcs_rk *pxpcs = data;
+	struct device *dev = &pxpcs->pdev->dev;
+
+	pm_runtime_force_suspend(dev);
+	clk_disable_unprepare(pxpcs->eee_clk);
+}
+
+static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
+{
+	struct device *dev = &pxpcs->pdev->dev;
+	int ret;
+
+	pxpcs->csr_clk = devm_clk_get(dev, "csr");
+	if (IS_ERR(pxpcs->csr_clk))
+		return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
+					 "Failed to get CSR clock\n");
+
+	pxpcs->eee_clk = devm_clk_get(dev, "eee");
+	if (IS_ERR(pxpcs->eee_clk))
+		return dev_err_probe(dev, PTR_ERR(pxpcs->eee_clk),
+					 "Failed to get EEE clock\n");
+
+	ret = clk_prepare_enable(pxpcs->eee_clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable EEE clock\n");
+		return ret;
+	}
+
+	/*
+	 * Compute the multiplier for the EEE clock so that
+	 * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
+	 *
+	 * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
+	 * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
+	 * The reset value is 0 (200 MHz), but derive the value at runtime to
+	 * stay correct if the mux is changed by a board.
+	 */
+	pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
+						 1000000000UL) - 1;
+
+	pm_runtime_set_suspended(dev);
+	pm_runtime_enable(dev);
+
+	ret = devm_add_action_or_reset(dev, xpcs_rk_exit_clk, pxpcs);
+	if (ret) {
+		dev_err(dev, "Failed to register devm for EEE clock: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int xpcs_rk_init_bus(struct dw_xpcs_rk *pxpcs)
+{
+	struct device *dev = &pxpcs->pdev->dev;
+	static atomic_t id = ATOMIC_INIT(-1);
+	struct mii_bus *bus;
+	int ret;
+
+	bus = devm_mdiobus_alloc_size(dev, 0);
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "Rockchip DW XPCS MCI/APB3";
+	bus->read = xpcs_rk_read_c22;
+	bus->write = xpcs_rk_write_c22;
+	bus->read_c45 = xpcs_rk_read_c45;
+	bus->write_c45 = xpcs_rk_write_c45;
+	bus->phy_mask = ~0;
+	bus->parent = dev;
+	bus->priv = pxpcs;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE,
+		 "rockchip_dwxpcs-%x", atomic_inc_return(&id));
+
+	/*
+	 * MDIO-bus here serves as just a back-end engine abstracting out
+	 * the MDIO and MCI/APB3 IO interfaces utilized for the Rockchip DWXPCS CSRs
+	 * access.
+	 */
+	ret = devm_mdiobus_register(dev, bus);
+	if (ret) {
+		dev_err(dev, "Failed to create MDIO bus\n");
+		return ret;
+	}
+
+	pxpcs->bus = bus;
+	return 0;
+}
+
+static int xpcs_rk_probe(struct platform_device *pdev)
+{
+	struct dw_xpcs_rk *pxpcs;
+	int ret;
+
+	pxpcs = xpcs_rk_create_data(pdev);
+	if (IS_ERR(pxpcs))
+		return PTR_ERR(pxpcs);
+
+	/*
+	 * The XPCS may be attached to a power domain (e.g. PD_PIPE). The domain
+	 * must be powered on before any register access, otherwise the SoC will
+	 * trigger a synchronous external abort (SError).
+	 *
+	 * Accessing the XPCS registers also requires a TX clock from the SerDes,
+	 * which is needed for the soft reset.
+	 */
+	ret = xpcs_rk_serdes_phy_init(pxpcs);
+	if (ret)
+		return ret;
+
+	ret = xpcs_rk_serdes_phy_poweron(pxpcs);
+	if (ret)
+		return ret;
+
+	ret = xpcs_rk_init_res(pxpcs);
+	if (ret)
+		return ret;
+
+	ret = xpcs_rk_init_clk(pxpcs);
+	if (ret)
+		return ret;
+
+	ret = xpcs_rk_init_bus(pxpcs);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id xpcs_rk_of_ids[] = {
+	{ .compatible = "rockchip,rk3568-xpcs" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, xpcs_rk_of_ids);
+
+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct device_node *pcs_np;
+	struct dw_xpcs_rk *pxpcs;
+	struct dw_xpcs *xpcs;
+	u32 port;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (of_property_read_u32(np, "reg", &port))
+		return ERR_PTR(-EINVAL);
+
+	if (!xpcs_rk_mdio_addr_validate((int)port))
+		return ERR_PTR(-EINVAL);
+
+	/* The XPCS pdev is attached to the parent node */
+	pcs_np = of_get_parent(np);
+	if (!pcs_np)
+		return ERR_PTR(-ENODEV);
+
+	if (!of_device_is_available(pcs_np)) {
+		of_node_put(pcs_np);
+		return ERR_PTR(-ENODEV);
+	}
+
+	if (!of_match_node(xpcs_rk_of_ids, pcs_np)) {
+		of_node_put(pcs_np);
+		return ERR_PTR(-EINVAL);
+	}
+
+	pdev = of_find_device_by_node(pcs_np);
+	of_node_put(pcs_np);
+	if (!pdev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	device_lock(&pdev->dev);
+	pxpcs = platform_get_drvdata(pdev);
+	if (!pxpcs || !pxpcs->bus) {
+		device_unlock(&pdev->dev);
+		put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
+	if (IS_ERR(xpcs)) {
+		device_unlock(&pdev->dev);
+		put_device(&pdev->dev);
+		return xpcs;
+	}
+
+	if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
+		xpcs_destroy(xpcs);
+		device_unlock(&pdev->dev);
+		put_device(&pdev->dev);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);
+	device_unlock(&pdev->dev);
+	put_device(&pdev->dev);
+	return xpcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_rk_create);
+
+static int xpcs_rk_pm_runtime_suspend(struct device *dev)
+{
+	struct dw_xpcs_rk *pxpcs = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(pxpcs->csr_clk);
+
+	return 0;
+}
+
+static int xpcs_rk_pm_runtime_resume(struct device *dev)
+{
+	struct dw_xpcs_rk *pxpcs = dev_get_drvdata(dev);
+
+	return clk_prepare_enable(pxpcs->csr_clk);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(xpcs_rk_pm_ops,
+			   xpcs_rk_pm_runtime_suspend,
+			   xpcs_rk_pm_runtime_resume,
+			   NULL);
+
+static struct platform_driver xpcs_rk_driver = {
+	.probe = xpcs_rk_probe,
+	.driver = {
+		.name = "rk_xpcs-dwxpcs",
+		.pm = pm_ptr(&xpcs_rk_pm_ops),
+		.of_match_table = xpcs_rk_of_ids,
+	},
+};
+module_platform_driver(xpcs_rk_driver);
+
+MODULE_DESCRIPTION("Rockchip XPCS platform device driver");
+MODULE_AUTHOR("Coia Prant <coiaprant@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/pcs/pcs-xpcs-rk.h b/include/linux/pcs/pcs-xpcs-rk.h
new file mode 100644
index 0000000000000..28723d5bd75cc
--- /dev/null
+++ b/include/linux/pcs/pcs-xpcs-rk.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_PCS_XPCS_ROCKCHIP_H
+#define __LINUX_PCS_XPCS_ROCKCHIP_H
+
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/pcs/pcs-xpcs.h>
+
+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np);
+
+#endif /* __LINUX_PCS_XPCS_ROCKCHIP_H */
-- 
2.47.3

[PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle

[Coia Prant]

The Rockchip dwmac glue driver now supports SGMII on RK3568 and
requires a PCS, referenced through the "pcs-handle" property.  The
property is parsed by rk_pcs_init() and a missing phandle fails the
probe, so describe it in the binding and require it when phy-mode
is "sgmii".

Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 .../devicetree/bindings/net/rockchip-dwmac.yaml   | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
index 80c252845349c..67b21ea77ec96 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
@@ -120,6 +120,12 @@ properties:
     maximum: 0x7F
     default: 0x10
 
+  pcs-handle:
+    description:
+      Specifies a reference to a node representing the PCS device
+      connected to this GMAC. Required when phy-mode is "sgmii".
+    maxItems: 1
+
   phy-supply:
     description: PHY regulator
 
@@ -159,6 +165,15 @@ allOf:
         clocks:
           minItems: 5
 
+  - if:
+      properties:
+        phy-mode:
+          contains:
+            const: sgmii
+    then:
+      required:
+        - pcs-handle
+
 unevaluatedProperties: false
 
 examples:
-- 
2.47.3

[PATCH net-next v5 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568

[Coia Prant]

The RK3568 SoC integrates a Synopsys DesignWare XPCS that can be
connected to GMAC0 or GMAC1 in SGMII mode.  Add the necessary glue
logic to support this configuration.

The current dwmac-rk driver does not support SGMII mode.  SGMII
requires a PCS to handle auto-negotiation and link state reporting,
but the existing driver only supports RGMII and RMII.

Add a set_to_sgmii() callback to configure the GMAC GRF register for
SGMII mode (bit 7 set, interface selection bits 4:6 ignored when set).
Also add a set_to_rmii() callback for rk3568 to explicitly clear bit 7,
since the new SGMII path leaves it set and the RMII branch previously
relied on the SoC reset value.

Provide pcs_init/pcs_exit callbacks to create/destroy the XPCS via
xpcs_rk_create() from the Rockchip XPCS platform driver, and a
select_pcs callback to return the XPCS to phylink.

Reorder rk_gmac_powerup() so that gmac_clk_enable() is called before
the SGMII check.  The SGMII path skips rk_get_phy_intf_sel(), so the
clock must be enabled earlier to cover all register accesses in that
path.  While at it, unify the error unwinding into a single
clk_disable label and add error handling for the default (unhandled
interface) case.

SGMII In-band vs Out-of-band
============================
On RK3568, the MAC clock is fixed at 125 MHz and cannot be dynamically
changed by the stmmac core's set_clk_tx_rate callback.  In-band mode
works because the PCS handles rate adaptation internally.  Out-of-band
mode does not work because the MAC would need to change the clock rate
to 125/12.5/1.25 MHz for 1000/100/10 Mbps respectively, and the clock
is fixed.

Enable default_an_inband for SGMII and disable the generic stmmac
set_clk_tx_rate callback.  This forces phylink to use in-band mode,
where the PCS is responsible for speed/duplex negotiation.

Note that default_an_inband can be overridden by a fixed-link node,
and phylink may also fall back to out-of-band if the PHY does not
support in-band signalling.  Out-of-band SGMII is not supported by
this driver: the MAC clock would stay at 125 MHz for 10/100 Mbps,
giving working TX but failing RX.  Boards must use in-band mode
(managed = "in-band-status" or an in-band capable PHY).

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   1 +
 .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 134 +++++++++++++++---
 2 files changed, 115 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index e3dd5adda5aca..5088acc06982e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -170,6 +170,7 @@ config DWMAC_ROCKCHIP
 	default ARCH_ROCKCHIP
 	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
 	select MFD_SYSCON
+	select PCS_XPCS_ROCKCHIP
 	help
 	  Support for Ethernet controller on Rockchip RK3288 SoC.
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 8d7042e689261..5d501ed8d1429 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -20,6 +20,7 @@
 #include <linux/delay.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/pcs/pcs-xpcs-rk.h>
 #include <linux/pm_runtime.h>
 
 #include "stmmac_platform.h"
@@ -47,6 +48,7 @@ struct rk_gmac_ops {
 	void (*set_to_rgmii)(struct rk_priv_data *bsp_priv,
 			     int tx_delay, int rx_delay);
 	void (*set_to_rmii)(struct rk_priv_data *bsp_priv);
+	void (*set_to_sgmii)(struct rk_priv_data *bsp_priv);
 	int (*set_speed)(struct rk_priv_data *bsp_priv,
 			 phy_interface_t interface, int speed);
 	void (*integrated_phy_powerup)(struct rk_priv_data *bsp_priv);
@@ -63,6 +65,7 @@ struct rk_gmac_ops {
 	bool clock_grf_reg_in_php;
 	bool supports_rgmii;
 	bool supports_rmii;
+	bool supports_sgmii;
 	bool php_grf_required;
 	bool regs_valid;
 	u32 regs[];
@@ -98,6 +101,7 @@ struct rk_priv_data {
 	bool integrated_phy;
 	bool supports_rgmii;
 	bool supports_rmii;
+	bool supports_sgmii;
 
 	struct clk_bulk_data *clks;
 	int num_clks;
@@ -809,6 +813,8 @@ static const struct rk_gmac_ops rk3528_ops = {
 #define RK3568_GRF_GMAC1_CON1		0x038c
 
 /* RK3568_GRF_GMAC0_CON1 && RK3568_GRF_GMAC1_CON1 */
+#define RK3568_GMAC_MODE_RMII_RGMII		GRF_CLR_BIT(7)
+#define RK3568_GMAC_MODE_SGMII_QSGMII		GRF_BIT(7)
 #define RK3568_GMAC_FLOW_CTRL			GRF_BIT(3)
 #define RK3568_GMAC_FLOW_CTRL_CLR		GRF_CLR_BIT(3)
 #define RK3568_GMAC_RXCLK_DLY_ENABLE		GRF_BIT(1)
@@ -836,6 +842,17 @@ static int rk3568_init(struct rk_priv_data *bsp_priv)
 	}
 }
 
+static void rk3568_set_to_rmii(struct rk_priv_data *bsp_priv)
+{
+	u32 con1;
+
+	con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
+				     RK3568_GRF_GMAC0_CON1;
+
+	regmap_write(bsp_priv->grf, con1,
+		     RK3568_GMAC_MODE_RMII_RGMII);
+}
+
 static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
 				int tx_delay, int rx_delay)
 {
@@ -851,19 +868,31 @@ static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
 		     RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
 
 	regmap_write(bsp_priv->grf, con1,
+		     RK3568_GMAC_MODE_RMII_RGMII |
 		     RK3568_GMAC_RXCLK_DLY_ENABLE |
 		     RK3568_GMAC_TXCLK_DLY_ENABLE);
 }
 
+static void rk3568_set_to_sgmii(struct rk_priv_data *bsp_priv)
+{
+	u32 con1;
+
+	con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
+				     RK3568_GRF_GMAC0_CON1;
+
+	regmap_write(bsp_priv->grf, con1, RK3568_GMAC_MODE_SGMII_QSGMII);
+}
+
 static const struct rk_gmac_ops rk3568_ops = {
 	.init = rk3568_init,
+	.set_to_rmii = rk3568_set_to_rmii,
 	.set_to_rgmii = rk3568_set_to_rgmii,
+	.set_to_sgmii = rk3568_set_to_sgmii,
+
 	.set_speed = rk_set_clk_mac_speed,
 
 	.gmac_phy_intf_sel_mask = GENMASK_U16(6, 4),
 
-	.supports_rmii = true,
-
 	.regs_valid = true,
 	.regs = {
 		0xfe2a0000, /* gmac0 */
@@ -1208,6 +1237,43 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
 		dev_err(bsp_priv->dev, "fail to disable phy-supply\n");
 }
 
+static int rk_pcs_init(struct stmmac_priv *priv)
+{
+	struct device_node *np = priv->device->of_node;
+	struct device_node *pcs_node;
+	struct dw_xpcs *xpcs;
+
+	pcs_node = of_parse_phandle(np, "pcs-handle", 0);
+	if (!pcs_node)
+		return -ENODEV;
+
+	xpcs = xpcs_rk_create(priv->device, pcs_node);
+	of_node_put(pcs_node);
+	if (IS_ERR(xpcs))
+		return PTR_ERR(xpcs);
+
+	priv->hw->xpcs = xpcs;
+	return 0;
+}
+
+static void rk_pcs_exit(struct stmmac_priv *priv)
+{
+	if (!priv->hw->xpcs)
+		return;
+
+	xpcs_destroy(priv->hw->xpcs);
+	priv->hw->xpcs = NULL;
+}
+
+static struct phylink_pcs *rk_select_pcs(struct stmmac_priv *priv,
+					 phy_interface_t interface)
+{
+	if (!priv->hw->xpcs)
+		return NULL;
+
+	return xpcs_to_phylink_pcs(priv->hw->xpcs);
+}
+
 static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
 					  struct plat_stmmacenet_data *plat,
 					  const struct rk_gmac_ops *ops)
@@ -1330,6 +1396,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
 
 	bsp_priv->supports_rgmii = ops->supports_rgmii || !!ops->set_to_rgmii;
 	bsp_priv->supports_rmii = ops->supports_rmii || !!ops->set_to_rmii;
+	bsp_priv->supports_sgmii = ops->supports_sgmii || !!ops->set_to_sgmii;
 
 	if (ops->init) {
 		ret = ops->init(bsp_priv);
@@ -1361,6 +1428,10 @@ static int rk_gmac_check_ops(struct rk_priv_data *bsp_priv)
 		if (!bsp_priv->supports_rmii)
 			return -EINVAL;
 		break;
+	case PHY_INTERFACE_MODE_SGMII:
+		if (!bsp_priv->supports_sgmii)
+			return -EINVAL;
+		break;
 	default:
 		dev_err(bsp_priv->dev,
 			"unsupported interface %d", bsp_priv->phy_iface);
@@ -1379,16 +1450,19 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 	if (ret)
 		return ret;
 
+	ret = gmac_clk_enable(bsp_priv, true);
+	if (ret)
+		return ret;
+
+	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
+		goto set_mode;
+
 	ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
 	if (ret < 0)
-		return ret;
+		goto clk_disable;
 
 	intf = ret;
 
-	ret = gmac_clk_enable(bsp_priv, true);
-	if (ret)
-		return ret;
-
 	if (bsp_priv->gmac_phy_intf_sel_mask ||
 	    bsp_priv->gmac_rmii_mode_mask) {
 		/* If defined, encode the phy_intf_sel value */
@@ -1399,10 +1473,8 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 				      bsp_priv->gmac_rmii_mode_mask);
 
 		ret = rk_write_gmac_grf_reg(bsp_priv, val);
-		if (ret < 0) {
-			gmac_clk_enable(bsp_priv, false);
-			return ret;
-		}
+		if (ret < 0)
+			goto clk_disable;
 	}
 
 	if (bsp_priv->clock.rmii_mode_mask) {
@@ -1410,13 +1482,12 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 				     bsp_priv->clock.rmii_mode_mask);
 
 		ret = rk_write_clock_grf_reg(bsp_priv, val);
-		if (ret < 0) {
-			gmac_clk_enable(bsp_priv, false);
-			return ret;
-		}
+		if (ret < 0)
+			goto clk_disable;
 	}
 
-	/*rmii or rgmii*/
+set_mode:
+	/* rmii, rgmii, sgmii */
 	switch (bsp_priv->phy_iface) {
 	case PHY_INTERFACE_MODE_RGMII:
 		dev_info(dev, "init for RGMII\n");
@@ -1447,15 +1518,20 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 		if (bsp_priv->ops->set_to_rmii)
 			bsp_priv->ops->set_to_rmii(bsp_priv);
 		break;
+	case PHY_INTERFACE_MODE_SGMII:
+		dev_info(dev, "init for SGMII\n");
+		if (bsp_priv->ops->set_to_sgmii)
+			bsp_priv->ops->set_to_sgmii(bsp_priv);
+		break;
 	default:
 		dev_err(dev, "NO interface defined!\n");
+		ret = -EINVAL;
+		goto clk_disable;
 	}
 
 	ret = rk_phy_powerup(bsp_priv);
-	if (ret) {
-		gmac_clk_enable(bsp_priv, false);
-		return ret;
-	}
+	if (ret)
+		goto clk_disable;
 
 	pm_runtime_get_sync(dev);
 
@@ -1463,6 +1539,10 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 		bsp_priv->ops->integrated_phy_powerup(bsp_priv);
 
 	return 0;
+
+clk_disable:
+	gmac_clk_enable(bsp_priv, false);
+	return ret;
 }
 
 static void rk_gmac_powerdown(struct rk_priv_data *gmac)
@@ -1486,6 +1566,9 @@ static void rk_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
 
 	if (rk->supports_rmii)
 		__set_bit(PHY_INTERFACE_MODE_RMII, interfaces);
+
+	if (rk->supports_sgmii)
+		__set_bit(PHY_INTERFACE_MODE_SGMII, interfaces);
 }
 
 static int rk_set_clk_tx_rate(void *bsp_priv_, struct clk *clk_tx_i,
@@ -1602,6 +1685,17 @@ static int rk_gmac_probe(struct platform_device *pdev)
 	plat_dat->suspend = rk_gmac_suspend;
 	plat_dat->resume = rk_gmac_resume;
 
+	if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII) {
+		/* SGMII clock always runs at 125 MHz */
+		plat_dat->set_clk_tx_rate = NULL;
+
+		/* SGMII requires a PCS */
+		plat_dat->default_an_inband = true;
+		plat_dat->pcs_init = rk_pcs_init;
+		plat_dat->pcs_exit = rk_pcs_exit;
+		plat_dat->select_pcs = rk_select_pcs;
+	}
+
 	plat_dat->bsp_priv = rk_gmac_setup(pdev, plat_dat, data);
 	if (IS_ERR(plat_dat->bsp_priv))
 		return PTR_ERR(plat_dat->bsp_priv);
-- 
2.47.3

[PATCH net-next v5 10/11] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

[Coia Prant]

The Ariaboard Photonicat has a Motorcomm YT8521SC Gigabit Ethernet PHY
connected to GMAC0 via XPCS SGMII.  Enable the necessary nodes to make
this port functional.

Add rockchip,sgmii-mac-sel = <0> to the already enabled combphy2,
to route the SGMII interface to GMAC0.

Enable the xpcs node and its port 0 sub-node, referencing combphy2
as the SerDes PHY.

Add the mdio0 node with the YT8521SC PHY at address 3, including its
reset GPIO and LED configuration.  Also add LED configuration for the
existing RGMII PHY on mdio1 for consistency.

Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 .../boot/dts/rockchip/rk3568-photonicat.dts   | 74 ++++++++++++++++++-
 1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
index 58c1052ba8ef3..fdaa4a2a4328b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
@@ -3,6 +3,7 @@
 /dts-v1/;
 
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/leds/common.h>
 #include <dt-bindings/pinctrl/rockchip.h>
 #include <dt-bindings/soc/rockchip,vop2.h>
 #include "rk3568.dtsi"
@@ -241,6 +242,7 @@ &combphy1 {
 };
 
 &combphy2 {
+	rockchip,sgmii-mac-sel = <0>;
 	status = "okay";
 };
 
@@ -260,9 +262,18 @@ &cpu3 {
 	cpu-supply = <&vdd_cpu>;
 };
 
-/* Motorcomm YT8521SC LAN port (require SGMII) */
+/* Motorcomm YT8521SC LAN port */
 &gmac0 {
-	status = "disabled";
+	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
+	assigned-clock-parents = <&clk_gmac0_xpcs_mii>;
+	managed = "in-band-status";
+	pcs-handle = <&xpcs_mii0>;
+	phy-handle = <&sgmii_phy>;
+	phy-mode = "sgmii";
+	phy-supply = <&vcc_3v3>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&gmac0_miim>;
+	status = "okay";
 };
 
 /* Motorcomm YT8521SC WAN port */
@@ -341,6 +352,36 @@ &i2s0_8ch {
 	status = "okay";
 };
 
+&mdio0 {
+	sgmii_phy: ethernet-phy@3 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <0x3>;
+		max-speed = <1000>;
+		reset-assert-us = <20000>;
+		reset-deassert-us = <100000>;
+		reset-gpios = <&gpio3 RK_PC6 GPIO_ACTIVE_LOW>;
+
+		leds {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@1 {
+				reg = <1>;
+				color = <LED_COLOR_ID_AMBER>;
+				function = LED_FUNCTION_LAN;
+				default-state = "keep";
+			};
+
+			led@2 {
+				reg = <2>;
+				color = <LED_COLOR_ID_GREEN>;
+				function = LED_FUNCTION_LAN;
+				default-state = "keep";
+			};
+		};
+	};
+};
+
 &mdio1 {
 	rgmii_phy: ethernet-phy@3 {
 		compatible = "ethernet-phy-ieee802.3-c22";
@@ -350,6 +391,25 @@ rgmii_phy: ethernet-phy@3 {
 		reset-gpios = <&gpio4 RK_PC0 GPIO_ACTIVE_LOW>;
 		rx-internal-delay-ps = <1500>;
 		tx-internal-delay-ps = <1500>;
+
+		leds {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@1 {
+				reg = <1>;
+				color = <LED_COLOR_ID_AMBER>;
+				function = LED_FUNCTION_WAN;
+				default-state = "keep";
+			};
+
+			led@2 {
+				reg = <2>;
+				color = <LED_COLOR_ID_GREEN>;
+				function = LED_FUNCTION_WAN;
+				default-state = "keep";
+			};
+		};
 	};
 };
 
@@ -586,3 +646,13 @@ &xin32k {
 	pinctrl-names = "default";
 	pinctrl-0 = <&clk32k_out1>;
 };
+
+&xpcs {
+	phys = <&combphy2 PHY_TYPE_SGMII>;
+	phy-names = "serdes";
+	status = "okay";
+};
+
+&xpcs_mii0 {
+	status = "okay";
+};
-- 
2.47.3

[PATCH net-next v5 11/11] MAINTAINERS: add entry for Rockchip XPCS driver

[Coia Prant]

Add a MAINTAINERS entry for the Rockchip RK3568 XPCS platform driver
and its device tree binding.

Include the relevant mailing lists (netdev and linux-rockchip) so that
future patches are properly distributed.

Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c2414447892c2..960a53afa8357 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23739,6 +23739,15 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/rockchip,rk3576-sai.yaml
 F:	sound/soc/rockchip/rockchip_sai.*
 
+ROCKCHIP XPCS DRIVER
+M:	Coia Prant <coiaprant@gmail.com>
+L:	netdev@vger.kernel.org
+L:	linux-rockchip@lists.infradead.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
+F:	drivers/net/pcs/pcs-xpcs-rk.c
+F:	include/linux/pcs/pcs-xpcs-rk.h
+
 ROCKER DRIVER
 M:	Jiri Pirko <jiri@resnulli.us>
 L:	netdev@vger.kernel.org
-- 
2.47.3

Re: [PATCH net-next v5 01/11] net: stmmac: move XPCS lifetime management to platform drivers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 1

- [Medium] Commit message mischaracterizes the removal of the generic
  XPCS creation as removing dead code.
- [Medium] DT binding vs implementation mismatch: after this patch
  nothing in the stmmac core reads the generic 'pcs-handle' property,…
- [Low] The commit message describes two defects, one framed as "more
  critically" (the common code overwriting priv->hw->xpcs after a…

--

net: stmmac: move XPCS lifetime management to platform drivers

The generic XPCS creation in stmmac_pcs_setup() (pcs-handle lookup and
mdio_bus_data->pcs_mask based xpcs_create_mdiodev()) is removed, leaving
only the plat->pcs_init() / plat->pcs_exit() dispatch. The Intel mGbE glue
grows its own pcs_init()/pcs_exit() that create and destroy the XPCS, and
the renesas-gbeth, rzn1 and socfpga pcs_exit() callbacks now clear
priv->hw->phylink_pcs after destroying the PCS.

A few questions on the commit message and on the removal of the generic
path.

>     First, if a device tree specifies a "pcs-handle" but no select_pcs()
>     callback is provided by the platform driver, the created XPCS is never
>     used. The phylink framework requires select_pcs() to actually return
>     the PCS to the core, so the pcs-handle property becomes effectively
>     useless without the matching callback.

[Severity: Medium]
Is "the created XPCS is never used" accurate?  An XPCS stored in
priv->hw->xpcs has two consumers that do not go through select_pcs().

stmmac_phylink_setup() in stmmac_main.c uses it to widen the phylink
interface sets:

	/* If we have an xpcs, it defines which PHY interfaces are supported. */
	if (priv->hw->xpcs)
		pcs = xpcs_to_phylink_pcs(priv->hw->xpcs);
	else
		pcs = priv->hw->phylink_pcs;

	if (pcs)
		phy_interface_or(config->supported_interfaces, ...

and stmmac_init_phy() uses it to decide whether to attach a PHY at all:

	if (priv->hw->xpcs &&
	    xpcs_get_an_mode(priv->hw->xpcs, mode) == DW_AN_C73)
		return 0;

So for a dwmac instance with a pcs-handle but no plat->pcs_init, does this
change narrow config->supported_interfaces (and config->lpi_interfaces) and
alter the PHY-attach decision, rather than just dropping something unused?
Would it be worth spelling that behavioural change out in the commit
message?

>     Second, and more critically, when a platform driver sets pcs_init()
>     and creates an XPCS inside that callback, the common code afterwards
>     still runs unconditionally and overwrites priv->hw->xpcs with the
>     local xpcs variable, which stays NULL. The platform driver has no way
>     to prevent this override because the common code runs after the
>     platform-specific initialization.

[Severity: Low]
This isn't a bug, but could this paragraph say that no in-tree platform is
affected today?

All three pre-existing pcs_init() implementations store their PCS in
priv->hw->phylink_pcs, not priv->hw->xpcs:

  renesas_gmac_pcs_init()  -> priv->hw->phylink_pcs = miic_create(...)
  rzn1_dwmac_pcs_init()    -> priv->hw->phylink_pcs = miic_create(...)
  socfpga_dwmac_pcs_init() -> priv->hw->phylink_pcs = pcs

so the old "priv->hw->xpcs = xpcs;" on the pcs_init branch overwrote NULL
with NULL.  intel_mgbe_pcs_init() added here is the first in-tree pcs_init()
that stores an XPCS, which makes this a refactor that removes a latent trap
rather than a fix for observable breakage.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index f5f9fa67ecd77..4308dccbf2570 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -603,13 +603,47 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
>  	plat->mdio_bus_data->needs_reset = true;
>  }
>  
> +static int intel_mgbe_pcs_init(struct stmmac_priv *priv)
> +{
> +	struct fwnode_handle *devnode, *pcsnode;
> +	struct dw_xpcs *xpcs;
> +	int addr;
> +
> +	devnode = dev_fwnode(priv->device);
> +
> +	if (fwnode_property_present(devnode, "pcs-handle")) {
> +		pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
> +		xpcs = xpcs_create_fwnode(pcsnode);
> +		fwnode_handle_put(pcsnode);
> +	} else {
> +		addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
> +		xpcs = xpcs_create_mdiodev(priv->mii, addr);
> +	}
> +
> +	if (IS_ERR(xpcs))
> +		return PTR_ERR(xpcs);
> +
> +	xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
> +
> +	priv->hw->xpcs = xpcs;
> +	return 0;
> +}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index afe98ff5bdcb0..7396b68899c66 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -426,35 +426,14 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  int stmmac_pcs_setup(struct net_device *ndev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> -	struct fwnode_handle *devnode, *pcsnode;
> -	struct dw_xpcs *xpcs = NULL;
> -	int addr, ret;
> -
> -	devnode = dev_fwnode(priv->device);
> -
> -	if (priv->plat->pcs_init) {
> -		ret = priv->plat->pcs_init(priv);
> -	} else if (fwnode_property_present(devnode, "pcs-handle")) {
> -		pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
> -		xpcs = xpcs_create_fwnode(pcsnode);
> -		fwnode_handle_put(pcsnode);
> -		ret = PTR_ERR_OR_ZERO(xpcs);
> -	} else if (priv->plat->mdio_bus_data &&
> -		   priv->plat->mdio_bus_data->pcs_mask) {
> -		addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
> -		xpcs = xpcs_create_mdiodev(priv->mii, addr);
> -		ret = PTR_ERR_OR_ZERO(xpcs);
> -	} else {
> +	int ret;
> +
> +	if (!priv->plat->pcs_init)
>  		return 0;
> -	}
>  
> +	ret = priv->plat->pcs_init(priv);
>  	if (ret)
> -		return dev_err_probe(priv->device, ret, "No xPCS found\n");
> -
> -	if (xpcs)
> -		xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
> -
> -	priv->hw->xpcs = xpcs;
> +		return dev_err_probe(priv->device, ret, "Failed to initialize PCS\n");
>  
>  	return 0;
>  }

[Severity: Medium]
After this change, does anything in the stmmac core still read the generic
"pcs-handle" property?

The property is still documented generically in
Documentation/devicetree/bindings/net/ethernet-controller.yaml:

  pcs-handle:
    $ref: /schemas/types.yaml#/definitions/phandle-array
    items:
      maxItems: 1
    description:
      Specifies a reference to a node representing a PCS PHY device on a MDIO
      bus to link with an external PHY (phy-handle) if exists.

and snps,dwmac.yaml $refs ethernet-controller.yaml and ends with
"additionalProperties: true", so pcs-handle stays schema-valid on every
dwmac node regardless of which glue is bound.

On a glue that does not set plat->pcs_init, stmmac_pcs_setup() now returns 0
without looking at the property, priv->hw->xpcs stays NULL, and nothing is
logged - whereas the removed code would at least fail probe with
"No xPCS found" for a bad phandle.  Should the binding be updated to say
pcs-handle is only honoured by specific glue drivers, or should the core
keep a generic path (or emit a warning) so the two agree?

The same question applies to struct stmmac_mdio_bus_data.pcs_mask.
stmmac_mdio_register() still treats it as "a PCS lives at these MDIO
addresses" and masks them out of the bus scan:

	new_bus->phy_mask = mdio_bus_data->phy_mask | mdio_bus_data->pcs_mask;

but no core code creates a PCS for those addresses any more, only
dwmac-intel's private intel_mgbe_pcs_init().  Does a glue that sets
pcs_mask without pcs_init now end up with that address neither scanned as a
PHY nor instantiated as a PCS?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 02/11] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Concern (validated as NOT a defect in the series context): the
  binding adds `rockchip,sgmii-mac-sel` while, at this commit alone, no…

--

dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property

This adds an optional rockchip,sgmii-mac-sel property to the naneng-combphy
binding so a board can select whether the RK3568 SGMII interface is routed
to GMAC0 or GMAC1.  The property is restricted to the RK3568 compatible via
the existing allOf conditional and defaults to 1 (GMAC1).

> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> index 379b08bd9e97a..8e898bce9af73 100644
> --- a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> @@ -80,6 +80,15 @@ properties:
>      description:
>        Some additional pipe settings are accessed through GRF regs.
>  
> +  rockchip,sgmii-mac-sel:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    default: 1
> +    description:
> +      Select gmac0 or gmac1 to be used as SGMII controller.
> +      The hardware reset value is GMAC1 (1). Set this to 0 to route
> +      SGMII to GMAC0.
> +

[Severity: Low]
At this commit alone, is rockchip,sgmii-mac-sel read by anything?

rockchip_combphy_parse_dt() in
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c does not look it up,
struct rockchip_combphy_grfcfg has no pipe_sgmii_mac_sel descriptor, and
no in-tree DTS sets it, so a kernel built from exactly this commit would
silently ignore the property.

This resolves itself later in the same series: "phy: rockchip:
naneng-combphy: add SGMII MAC selection for RK3568" adds the parsing

	priv->sgmii_mac_sel = 1;
	device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);

along with the RK3568 pipe-GRF descriptor

	.pipe_sgmii_mac_sel	= { 0x0040, 1, 1, 0x00, 0x01 },

and the write in the PHY_TYPE_SGMII path of rk3568_combphy_cfg(), and
"arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port" adds the
first user:

	&combphy2 {
		rockchip,sgmii-mac-sel = <0>;
		status = "okay";
	};

So this is noted only for the record, since the binding and its consumer
being split across consecutive patches is the normal series layout.

The remaining point is a design question rather than a bug: is a raw
uint32 MAC index the interface preferred here, or would a phandle to the
GMAC controller node be a better fit for describing the routing?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] In rk3568_combphy_cfg() (drivers/phy/rockchip/phy-rockchip-
  naneng-combphy.c:881-882) the newly added write of pipe_sgmii_mac_sel…

--

phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

This adds a "rockchip,sgmii-mac-sel" DT property to pick whether the RK3568
SGMII interface is routed to GMAC0 or GMAC1 via PIPE_GRF_XPCS_CON0 bit 1.
The value is read in rockchip_combphy_parse_dt() and programmed from the
PHY_TYPE_SGMII branch of rk3568_combphy_cfg().

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)

One question about the commit message wording:

    The hardware reset value is 1 (GMAC1). If the property is set to 0,
    the driver routes SGMII to GMAC0; if set to 1 (or omitted), it
    remains at GMAC1.

Does "or omitted ... it remains at GMAC1" match the code?  The write in
rk3568_combphy_cfg() is unconditional, so in the omitted case the driver
does not leave the bit alone, it actively programs it to 1.  See the
inline comment below.

> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 7843356a4dd47..7b867e7520064 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c

[ ... ]

> @@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
>  
>  	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
>  
> +	priv->sgmii_mac_sel = 1;
> +	device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
> +

[Severity: Low]
Since device_property_read_u32() leaves the output untouched on failure,
rockchip_combphy_parse_dt() ends up with priv->sgmii_mac_sel == 1 both when
the property says 1 and when the property is absent.  Is there a way for the
driver to express "leave the bit as firmware left it"?

Before this patch the kernel never touched PIPE_GRF_XPCS_CON0 bit 1, so a
board whose bootloader routed SGMII to GMAC0 kept that routing.  With this
change, a DT that instantiates the combphy as PHY_TYPE_SGMII but does not
carry the new property has the bit re-programmed to GMAC1 on every
phy_init().  Can that break such a board's SGMII link?

>  	priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
>  	/* fallback to old behaviour */
>  	if (PTR_ERR(priv->phy_rst) == -ENOENT)
> @@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
>  		break;
>  
>  	case PHY_TYPE_SGMII:
> +		rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
> +					     priv->sgmii_mac_sel > 0);

[Severity: Low]
This write always happens, including in the "property omitted" case, and
rockchip_combphy_param_write() sets the write-enable mask along with the
value:

	tmp = en ? reg->enable : reg->disable;
	mask = GENMASK(reg->bitend, reg->bitstart);
	val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);

	return regmap_write(base, reg->offset, val);

so with priv->sgmii_mac_sel defaulting to 1 the register gets
(0x1 << 1) | (GENMASK(1, 1) << 16), i.e. GMAC1 is forced.  Would it be
better to only issue this write when the property is actually present?

A second question: bit 1 of PIPE_GRF_XPCS_CON0 is a single SoC-wide mux,
while the property sits on a per-PHY node.  If two combphy instances that
share the same rockchip,pipe-grf syscon are both brought up as
PHY_TYPE_SGMII with different property values, the last phy_init() wins and
nothing is reported.  Should conflicting values be rejected or at least
warned about?

>  		rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
>  		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
>  		rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);
> @@ -984,6 +991,7 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
>  	.con3_for_sata		= { 0x000c, 15, 0, 0x00, 0x4407 },
>  	/* pipe-grf */
>  	.pipe_con0_for_sata	= { 0x0000, 15, 0, 0x00, 0x2220 },
> +	.pipe_sgmii_mac_sel	= { 0x0040, 1, 1, 0x00, 0x01 },
>  	.pipe_xpcs_phy_ready	= { 0x0040, 2, 2, 0x00, 0x01 },

For what it is worth, the only in-tree SGMII consumer at the end of this
series, rk3568-photonicat.dts, sets rockchip,sgmii-mac-sel = <0>
explicitly, so no upstream DT changes behaviour today; the concern is about
out-of-tree or ported device trees and about the commit message wording.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 04/11] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 5

- [Low] Concern (validated as a false positive): the new schema was said
  to fork the DW XPCS description in two, to lack the conventional…
- [Low] Concern (validated as a false positive): the four `ethernet-pcs-
  mii@[0-3]` child nodes were said to have no in-tree implementation,…
- [Low] Concern (validated as a false positive): the binding omits
  `resets`/`reset-names` for the RK3568 XPCS soft resets (SRST_XPCS,…
- [Low] Concern (validated as a false positive): the mandatory `eee`
  clock is a documented property that no in-tree XPCS code requests…
- [Low] Concern (validated as a false positive): the binding omits `reg-
  names` (direct/indirect) and `reg-io-width`, so the CSR mapping style…

--

dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

This adds a new YAML binding for the Synopsys DesignWare XPCS as
integrated on the RK3568, described as an APB3-mapped block with a
Naneng Combo SerDes PHY and four MII child ports.  All the observations
below are documentation-level questions only; the accompanying driver
patches in the series do implement what the schema describes.

> diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> new file mode 100644
> index 0000000000000..703fcff0e3f70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> @@ -0,0 +1,110 @@

[ ... ]

> +properties:
> +  compatible:
> +    const: rockchip,rk3568-xpcs

[Severity: Low]
This isn't a bug, but would it be worth saying in the commit message why
this gets its own schema and a single compatible, rather than the usual
"rockchip,rk3568-xpcs", "snps,dw-xpcs" fallback?

The glue driver added later in the series maps registers differently from
the generic one:

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_addr_format() {
      return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);
  }

and remaps MDIO_MMD_PMAPMD/MDIO_MMD_VEND2 per port in
xpcs_rk_mdio_read_remapping(), so a snps,dw-xpcs fallback would let
pcs-xpcs-plat bind and touch the wrong MMDs.  Recording that reasoning in
the changelog would save the next reader the same investigation.

> +  reg:
> +    maxItems: 1

[Severity: Low]
The existing snps,dw-xpcs binding describes reg-names (direct/indirect)
and reg-io-width; neither appears here.  Is the intent that the compatible
alone implies a direct-mapped 2 MiB window with a fixed 4-byte stride?

That is what the driver assumes:

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_init_res() {
      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
      ...
      if (resource_size(res) < SZ_2M) {

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_read_reg() {
      ret = readl(pxpcs->reg_base + (csr << 2)) & 0xffff;

Would it help to state the 2 MiB size and the 4-byte register stride in
the description so the omission of reg-names and reg-io-width is clearly
deliberate?

> +  clocks:
> +    items:
> +      - description: APB3 bus interface clock (clk_csr_i), required for register access
> +      - description: EEE clock (clk_eee_i), required for Energy Efficient Ethernet operation
> +
> +  clock-names:
> +    items:
> +      - const: csr
> +      - const: eee

[Severity: Low]
The fixed [csr, eee] list differs from the names the generic XPCS code
looks up ("core" and "pad" in pcs-xpcs.c, "csr" in pcs-xpcs-plat.c).  Is
the exclusion of core/pad intentional here?

Both documented clocks are used by the Rockchip glue:

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_init_clk() {
      pxpcs->csr_clk = devm_clk_get(dev, "csr");
      ...
      pxpcs->eee_clk = devm_clk_get(dev, "eee");
      ...
      pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
                                               1000000000UL) - 1;

so the naming looks consistent with the implementation; a sentence in the
changelog noting that this integration does not use the core/pad clocks
would make the difference from snps,dw-xpcs obvious.

[ ... ]

> +patternProperties:
> +  "^ethernet-pcs-mii@[0-3]$":
> +    type: object
> +    description:
> +      One of the four MII ports of the XPCS. The port is linked to an
> +      Ethernet MAC controller via the pcs-handle property in the MAC's
> +      device tree node.

[Severity: Low]
This isn't a bug, but the changelog doesn't mention the two patches that
consume these child nodes, which makes the new ABI look unimplemented when
the binding patch is read on its own.

The consumers are:

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_create() {
      if (of_property_read_u32(np, "reg", &port))
              return ERR_PTR(-EINVAL);
      ...
      xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);

  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:rk_pcs_init() {
      pcs_node = of_parse_phandle(np, "pcs-handle", 0);
      ...
      xpcs = xpcs_rk_create(priv->device, pcs_node);

Could the commit message reference them?

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - clocks
> +  - clock-names
> +  - phys
> +  - phy-names
> +  - power-domains
> +
> +additionalProperties: false

[Severity: Low]
The RK3568 CRU defines four XPCS soft resets:

  include/dt-bindings/clock/rk3568-cru.h:
      #define SRST_XPCS               150
      #define SRST_XPCS_TX_DIV10      151
      #define SRST_XPCS_RX_DIV10      152
      #define SRST_XPCS_XGXS_RX       153

but the schema has no resets/reset-names, so a device tree cannot express
them.  Is that because bring-up relies only on the power domain, the
SerDes PHY and the XPCS register-level soft reset?

  drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_probe() {
      ret = dev_pm_genpd_rpm_always_on(dev, true);
      ...
      ret = phy_init(pxpcs->serdes_phy);
      ...
      ret = phy_power_on(pxpcs->serdes_phy);

A grep at the end of the series shows SRST_XPCS* matching only the
dt-bindings header, with no driver or DTS user, so this may well be
intentional; a note confirming the resets are left at their SoC defaults
would settle it.

Documenting them now as optional properties would also stay backwards
compatible if they turn out to be needed later.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 05/11] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] DT/binding mismatch: the new `xpcs: ethernet-pcs@fda00000`
  node in `arch/arm64/boot/dts/rockchip/rk3568.dtsi` (line 113) omits…
- [Low] Claimed missing driver/consumer for the new DT nodes: no driver
  matches `rockchip,rk3568-xpcs`, the `ethernet-pcs-mii@N` children are…

--

arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

Adds the RK3568 XPCS node at 0xfda00000 with its four ethernet-pcs-mii
child nodes, all disabled by default, plus two 125 MHz fixed-clock nodes
intended as assigned-clock-parents for gmac0 and gmac1.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> index 3bc653f027f1f..227d03e336043 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> @@ -110,6 +110,51 @@ sata0: sata@fc000000 {
>  		status = "disabled";
>  	};
>  
> +	xpcs: ethernet-pcs@fda00000 {
> +		compatible = "rockchip,rk3568-xpcs";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0x0 0xfda00000 0x0 0x200000>;
> +		clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
> +		clock-names = "csr", "eee";
> +		power-domains = <&power RK3568_PD_PIPE>;
> +		status = "disabled";

[Severity: Medium]
Does this node validate against the binding added earlier in the series?

Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml lists
phys and phy-names in its top-level required array, unconditionally, and
sets additionalProperties: false:

  required:
    - compatible
    - reg
    - "#address-cells"
    - "#size-cells"
    - clocks
    - clock-names
    - phys
    - phy-names
    - power-domains

The node here supplies clocks and power-domains but not phys/phy-names.

dtschema validates nodes irrespective of status = "disabled", so does
make dtbs_check now report "'phys' is a required property" and
"'phy-names' is a required property" for every rk3568 board DTB?  At the
end of the series only rk3568-photonicat.dts fills them in:

  &xpcs {
  	phys = <&combphy2 PHY_TYPE_SGMII>;
  	phy-names = "serdes";
  	status = "okay";
  };

The other 21 device trees that include rk3568.dtsi (rk3568-bpi-r2-pro.dts,
rk3568-rock-3a.dts, rk3568-odroid-m1.dts, rk3568-nanopi-r5s.dtsi,
rk3568-qnap-ts433.dts, ...) do not.

The commit message says "The XPCS node also requires a reference to the
appropriate Naneng Combo PHY via the phys property at the board level."
If that is the intended model, should phys/phy-names be dropped from the
binding's required list (or gated), rather than left mandatory?
Alternatively the SoC dtsi could carry a default phandle for boards to
override.

> +
> +		xpcs_mii0: ethernet-pcs-mii@0 {
> +			reg = <0>;
> +			status = "disabled";
> +		};

[ ... ]

> +	clk_gmac0_xpcs_mii: clock-xpcs-gmac0 {
> +		compatible = "fixed-clock";
> +		clock-frequency = <125000000>;
> +		clock-output-names = "clk_gmac0_xpcs_mii";
> +		#clock-cells = <0>;
> +	};

[Severity: Low]
One point that was checked and turned out not to be an issue, noted here
only for completeness: the compatible string, the ethernet-pcs-mii@N
children and these two fixed clocks all gain in-tree consumers later in
the same series, so there is nothing to change here.

drivers/net/pcs/pcs-xpcs-rk.c matches the compatible:

  static const struct of_device_id xpcs_rk_of_ids[] = {
  	{ .compatible = "rockchip,rk3568-xpcs" },
  	{ /* sentinel */ },
  };

and rk3568-photonicat.dts consumes both the child node and the clock:

  assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
  assigned-clock-parents = <&clk_gmac0_xpcs_mii>;
  managed = "in-band-status";
  pcs-handle = <&xpcs_mii0>;

Until the driver patch lands the nodes are disabled and inert, which is
the usual bindings-then-dtsi-then-driver ordering.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 4 · Low: 0

- [High] In xpcs_get_state_c37_sgmii() the combination
  'DW_VR_MII_AN_STS_C37_ANCMPLT_INTR set + DW_VR_MII_C37_ANSGM_SP_LNKSTS…
- [Medium] Undisclosed functional change: besides the two BMCR_ANRESTART
  additions the commit message advertises, the patch adds a new…
- [Medium] Device-specific quirk applied unconditionally: the commit
  message and the new comment at drivers/net/pcs/pcs-xpcs.c:776-781…
- [Medium] Wrong ordering in xpcs_get_state_c37_sgmii(): the latched AN-
  complete indication is consumed (xpcs_write(DW_VR_MII_AN_INTR_STS, 0),…
- [Medium] The patch turns the read-only-by-contract pcs_get_state()
  callback into a hardware-state-changing operation (BMCR read-modify-…

--

net: pcs: xpcs: add ANRESTART support for SGMII link recovery

The patch sets BMCR_ANRESTART together with BMCR_ANENABLE when
xpcs_config_aneg_c37_sgmii() enables in-band AN, and makes
xpcs_get_state_c37_sgmii() clear DW_VR_MII_AN_INTR_STS and assert
BMCR_ANRESTART when the link is down with AN completion latched.  A new
comment explains that the DesignWare databook says AN restart is not
needed for MAC side SGMII, but that some parts (Rockchip RK3568) need it.

A few questions below.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 0337e2bcc0125..8c3875b6985b9 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -769,7 +771,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
>  	 *	 SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
>  	 *	 PHY about the link state change after C28 AN is completed
>  	 *	 between PHY and Link Partner. There is also no need to
> -	 *	 trigger AN restart for MAC-side SGMII.
> +	 *	 trigger AN restart for MAC-side SGMII on most devices.
> +	 *
> +	 * Note: While the DesignWare databook states that AN restart is
> +	 *	 not needed for MAC side SGMII, some implementations (e.g.
> +	 *	 Rockchip RK3568) exhibit a timing quirk when integrated with
> +	 *	 phylink and do not restart AN automatically when the link
> +	 *	 comes back up. An explicit AN restart is required on those
> +	 *	 parts to recover the link after a disconnect.
>  	 */

[Severity: Medium]
The comment says the restart is only "required on those parts" and that
there is "no need to trigger AN restart for MAC-side SGMII on most
devices", and the commit message says "This is not a fix for an existing
mainline platform".  The code, however, applies the restart on every
device: no gate on xpcs->info.pma or xpcs->desc, and struct dw_xpcs gains
no quirk flag.

The same function already uses that gating idiom a few lines below:

	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
		mask |= DW_VR_MII_AN_CTRL_8BIT;
		val |= DW_VR_MII_AN_CTRL_8BIT;
		/* Hardware requires it to be PHY side SGMII */
		tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;

Note that this TXGBE case is configured as PHY-side SGMII, which the
MAC-side/RK3568 rationale in the comment does not cover.

Should the new restart be gated on the affected PMA (or on a new dw_xpcs
quirk flag) so the code matches what the comment and the commit message
describe?

Also, phylink already has a path for this: pcs_config() may return a
positive value to ask phylink to call .pcs_an_restart, and
xpcs_an_restart() in this same driver performs exactly the same register
write:

	xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
		    BMCR_ANRESTART);

Would using that framework path be preferable to open-coding the restart
in the config and get_state callbacks?

> @@ -816,9 +825,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Clear CL37 AN complete status */
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +	if (ret < 0)
> +		return ret;
> +

[Severity: Medium]
The commit message describes the change as "Add BMCR_ANRESTART in two
places" plus a comment update, but this hunk adds a third functional
change that is only visible as item "5)" in the in-code numbered list.

This write is unconditional: it runs for every xpcs_config() ->
xpcs_do_config() -> xpcs_config_aneg_c37_sgmii() call and for every
neg_mode, including PHYLINK_PCS_NEG_OUTBAND where no AN is subsequently
enabled.  It clears the register that xpcs_get_state_c37_sgmii() uses as
its only source of link/speed/duplex and of state->an_complete:

	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);

It also changes the value returned when neg_mode is not
PHYLINK_PCS_NEG_INBAND_ENABLED: the function now returns the result of
this AN_INTR_STS write instead of the DIG_CTRL1 modify.

Could the commit message mention this status clear and the return value
change?  For reference, xpcs_config_aneg_c37_1000basex() already performs
the identical clear, so the write itself matches existing driver practice.

>  	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
>  		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> -				 mdio_ctrl | BMCR_ANENABLE);
> +				 mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
>  
>  	return ret;
>  }
> @@ -1093,9 +1107,18 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
>  		return 0;
>  	}
>  
> -	/* Clear AN complete status or interrupt */
> -	if (state->an_complete)
> -		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +	if (state->an_complete) {
> +		/* Clear AN complete status or interrupt */
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Initiate the next round of AN */
> +		ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> +				  BMCR_ANRESTART);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: High]
Is the trigger condition here really "stuck AN"?  The state reached at
this point is ANCMPLT latched with LNKSTS clear, taken from the same
register read:

	state->an_complete = ret & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;

	if (ret & DW_VR_MII_C37_ANSGM_SP_LNKSTS) {
		...
		return 0;
	}

For C37/SGMII, isn't that the normal steady state whenever the partner
PHY's media side is down?  The PHY keeps sending config words, the SGMII
AN exchange completes and latches ANCMPLT (bit 0), and the media link
state is carried separately in the received config word via LNKSTS
(bit 4), per pcs-xpcs.h:

	#define DW_VR_MII_AN_STS_C37_ANCMPLT_INTR	BIT(0)
	...
	#define DW_VR_MII_C37_ANSGM_SP_LNKSTS		BIT(4)

If so, then on an unplugged port each restart re-completes within the AN
link timer with LNKSTS still 0 and re-latches ANCMPLT, so the condition
re-arms itself and a fresh AN restart is issued on every poll,
indefinitely.  phylink polls pcs_get_state once per second and
xpcs->pcs.poll is true for everything except WX_TXGBE and MP_FBNIC, so
this affects the existing DW_AN_C37_SGMII users (stmmac/Intel mGbE, NXP
SJA1105/SJA1110, Wangxun), not only RK3568.

Two further consequences: with MAC_AUTO_SW armed, the hardware-selected
speed/duplex is dropped on each cycle, and because the whole status word
is cleared by writing 0, a restart that lands on a just-latched LNKSTS=1
word destroys that link-up indication and costs an extra poll interval.

For the interrupt-driven case there may also be a faster loop.  The SGMII
AN_CTRL modify in xpcs_config_aneg_c37_sgmii() does not include
DW_VR_MII_AN_INTR_EN in its mask, while the 1000BASE-X path sets it:

	if (!xpcs->pcs.poll) {
		mask |= DW_VR_MII_AN_INTR_EN;
		val |= DW_VR_MII_AN_INTR_EN;
	}

so an enable programmed by a previous 1000BASE-X configuration persists
into SGMII mode.  On txgbe the AN event is routed to the link IRQ:

drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c:txgbe_misc_irq_handle() {
	if (eicr & (TXGBE_PX_MISC_ETH_LK | TXGBE_PX_MISC_ETH_LKDN |
		    TXGBE_PX_MISC_ETH_AN)) {
		sub_irq = irq_find_mapping(txgbe->misc.domain, TXGBE_IRQ_LINK);
	...
}

which ends in phylink_pcs_change() -> pcs_get_state(), so the restart can
be re-triggered at hardware rate rather than once per second.

Would gating this on the affected PMA/quirk, or on an explicit "AN is not
active" hardware condition rather than on the latched AN-complete bit,
address this?

[Severity: Medium]
Is the ordering here recoverable if the second access fails?  The latched
AN-complete indication is consumed by the xpcs_write(DW_VR_MII_AN_INTR_STS,
0) before the fallible read-modify-write that actually starts the next
negotiation.

If the write succeeds and the xpcs_modify() of MII_BMCR fails on the
register back-end, the only indication that AN completed is gone and no
new negotiation was started.  The block that would retry is itself gated
on state->an_complete, which is now read back as 0, so the restart is
never attempted again.  The error only reaches xpcs_get_state(), which
logs it from a void callback:

		ret = xpcs_get_state_c37_sgmii(xpcs, state);
		if (ret)
			dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
				"xpcs_get_state_c37_sgmii", ERR_PTR(ret));

and nothing records a pending restart.  This matters most on the hardware
this patch targets, where AN is not restarted automatically, so the port
would stay down until a fresh pcs_config().  The failure is plausible for
these vendor-MMD accesses: the RK3568 back-end added later in this series
returns errors from pm_runtime_resume_and_get() inside
xpcs_rk_write_reg()/xpcs_rk_read_reg().

Would asserting BMCR_ANRESTART before clearing the status word, or
retrying independently of the latch, close this window?

[Severity: Medium]
This makes pcs_get_state() change hardware state rather than only read it,
and phylink calls that callback from contexts with no common lock.

phylink_resolve() holds pl->state_mutex (and the phy locks) but not RTNL,
while these two callers hold only RTNL:

drivers/net/phy/phylink.c:phylink_ethtool_ksettings_get() {
	...
		phylink_mac_pcs_get_state(pl, &link_state);
	...
}

drivers/net/phy/phylink.c:phylink_mii_read() {
	...
			phylink_mac_pcs_get_state(pl, &state);
	...
}

The lockset intersection is empty, and the driver holds no private lock,
so two CPUs can run this block at once and both assert BMCR_ANRESTART,
aborting each other's negotiation.  It can also interleave with the
multi-transaction BMCR sequence in xpcs_config_aneg_c37_sgmii(), which
reads BMCR, writes it with ANENABLE cleared, programs AN_CTRL/DIG_CTRL1,
clears AN_INTR_STS, and finally writes the cached value back:

	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
	...
		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
				 mdio_ctrl & ~BMCR_ANENABLE);

Each xpcs_read/xpcs_write is a separate bus transaction, so an AN restart
can land while AN is disabled and the AN configuration is only half
applied.

Both RTNL-only paths need the PCS-only case (no phydev), which is
reachable for inband SGMII without a PHY, and SIOCGMIIREG and
ETHTOOL_GLINKSETTINGS are unprivileged.  Before this patch these paths
only cleared the AN status word; should the restart be moved to a context
that is serialized with pcs_config(), such as the .pcs_an_restart op?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 3 · Low: 2

- [High] Incorrect MMD write remapping for MII vendor registers breaks
  configuration for ports 1, 2, and 3.
- [Medium] The patch silently re-packages the pre-existing generic
  Synopsys XPCS platform driver.
- [Medium] xpcs_rk_init_clk() (drivers/net/pcs/pcs-xpcs-rk.c:361-362)
  computes `pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 *…
- [Medium] Acquiring `device_lock()` of another device from within a
  probe path causes a lockdep warning (recursive locking).
- [Low] Concern that the newly exported xpcs_rk_create()
  (include/linux/pcs/pcs-xpcs-rk.h) has no in-tree caller and no Kconfig…
- [Low] The rockchip,rk3568-xpcs binding lists `phys` and `phy-names` in
  its `required:` list…

--

net: pcs: xpcs: add Rockchip RK3568 platform glue driver

Adds a platform glue driver for the DesignWare XPCS block found on RK3568,
exposing the APB3-mapped XPCS registers as a virtual MDIO bus with per-port
MMD remapping so the generic pcs-xpcs core can create a phylink_pcs.  Also
adds new Kconfig symbols for the generic and Rockchip XPCS glue and an
exported xpcs_rk_create() helper for MAC drivers.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)

> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index e417fd66f660a..080538f193d59 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -12,6 +12,31 @@ config PCS_XPCS
>  	  This module provides a driver and helper functions for Synopsys
>  	  DesignWare XPCS controllers.
>  
> +if PCS_XPCS
> +
> +config PCS_XPCS_PLATFORM
> +	tristate "Generic XPCS controller support"
> +	default PCS_XPCS
> +	help
> +	  Generic DWXPCS driver for platforms that don't require any
> +	  platform specific code to function or is using platform
> +	  data for setup.
> +
> +	  If you have a controller with this interface, say Y or M here.
> +
> +config PCS_XPCS_ROCKCHIP
> +	tristate "Rockchip XPCS controller support"
> +	default ARCH_ROCKCHIP
> +	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
> +	depends on GENERIC_PHY || COMPILE_TEST
> +	depends on PM_GENERIC_DOMAINS || COMPILE_TEST
> +	help
> +	  Support for XPCS controller on Rockchip RK356x SoC.
> +
> +	  If you have a Rockchip SoC with this interface, say Y or M here.
> +
> +endif # PCS_XPCS
> +
>  config PCS_LYNX
>  	tristate
>  	help
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 4f7920618b900..f9f6cf2578d72 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -1,10 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Makefile for Linux PCS drivers
>  
> -pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-plat.o \
> -				   pcs-xpcs-nxp.o pcs-xpcs-wx.o
> +pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o
>  
>  obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
> +obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
> +obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.o

[Severity: Medium]
Should this repackaging of the pre-existing generic platform glue be a
separate patch, or at least be described in the commit message?

pcs-xpcs-plat.o is moved out of the pcs_xpcs composite object and built as
its own module, and the new PCS_XPCS_PLATFORM tristate makes the snps,dw-xpcs
platform glue user-selectable where it was previously unconditional under
PCS_XPCS.  So the snps,dw-xpcs platform driver ships in pcs-xpcs-plat.ko
instead of pcs_xpcs.ko after this patch.

The commit message only describes adding the Rockchip glue driver and does
not mention this change to existing functionality.  Could the Makefile and
Kconfig refactor be split out with its own rationale?

> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..7fd1408c044cc
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
> @@ -0,0 +1,559 @@

[ ... ]

> +static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +
> +	pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
> +	if (IS_ERR(pxpcs->serdes_phy))
> +		return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
> +					"Failed to get SerDes PHY\n");
> +
> +	return 0;
> +}

[Severity: Low]
xpcs_rk_serdes_phy_init() treats the "serdes" phy as mandatory and fails
probe when it cannot be resolved, and the binding added later in this series
(Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml) lists
phys and phy-names under required:

  required:
    - compatible
    - reg
    ...
    - phys
    - phy-names
    - power-domains

The SoC node added in arch/arm64/boot/dts/rockchip/rk3568.dtsi only has:

	xpcs: ethernet-pcs@fda00000 {
		compatible = "rockchip,rk3568-xpcs";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x0 0xfda00000 0x0 0x200000>;
		clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
		clock-names = "csr", "eee";
		power-domains = <&power RK3568_PD_PIPE>;
		status = "disabled";

CHECK_DTBS validates nodes regardless of status, so does every RK3568 DTB
now report missing phys/phy-names for this node?  Only
rk3568-photonicat.dts adds them, so a board that just flips this node to
status = "okay" would also fail probe with "Failed to get SerDes PHY".

Should the SoC node carry phys/phy-names, or should the binding not mark
them required at the SoC level?

[ ... ]

> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
[ ... ]
> +	/*
> +	 * Compute the multiplier for the EEE clock so that
> +	 * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
> +	 *
> +	 * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
> +	 * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
> +	 * The reset value is 0 (200 MHz), but derive the value at runtime to
> +	 * stay correct if the mux is changed by a board.
> +	 */
> +	pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
> +						 1000000000UL) - 1;

[Severity: Medium]
Can this value overflow the 4-bit register field it ends up in?

For the reset configuration named in the comment (200 MHz, 5 ns) this
computes 100 * 200000000 / 1000000000 - 1 = 19.  That is passed on
unchecked:

	xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);

and the core programs it in xpcs_config_eee():

	drivers/net/pcs/pcs-xpcs.c:
		      FIELD_PREP(DW_VR_MII_EEE_MULT_FACT_100NS,
				 xpcs->eee_mult_fact);

with

	drivers/net/pcs/pcs-xpcs.h:
	#define DW_VR_MII_EEE_MULT_FACT_100NS		GENMASK(11, 8)

FIELD_PREP on a runtime value does no range check, so 19 (0x13) becomes 3.
The hardware is then told the 100 ns reference is 4 EEE clock periods
(20 ns) rather than the 80..120 ns the comment and the kernel-doc of
xpcs_config_eee_mult_fact() require, which affects all derived LPI timers
once EEE is enabled.  Since the field caps the count at 16 periods, is 15
the only representable value for a 5 ns clock, and should this be clamped?

Two more things about the same expression:

- 100 * clk_get_rate() is evaluated in unsigned long, so on 32-bit builds
  100 * 200000000 wraps to 2820130816 and yields mult_fact 2.  Would
  mul_u64_u32_div() or a u64 intermediate be better here?

- If clk_get_rate() returns 0, 0 - 1 stored in a u8 becomes 255, giving
  field value 15.  Is a zero rate worth rejecting?

[ ... ]

> @@ -0,0 +1,559 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip XPCS platform device driver
> + *
> + * Based on the Synopsys DesignWare XPCS platform driver.
> + * Copyright (C) 2024 Serge Semin
> + *
> + * Adapted for Rockchip SoCs, with reference to the Rockchip OEM driver.
> + * Copyright (C) 2026 Coia Prant
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/math.h>
> +#include <linux/mdio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pcs/pcs-xpcs-rk.h>
> +#include <linux/phy.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/sizes.h>
> +
> +#include "pcs-xpcs.h"
> +
> +struct dw_xpcs_rk {
> +	struct platform_device *pdev;
> +	struct mii_bus *bus;
> +	void __iomem *reg_base;
> +	struct phy *serdes_phy;
> +	struct clk *csr_clk;
> +	struct clk *eee_clk;
> +	u8 eee_mult_fact;
> +};
> +
> +static ptrdiff_t xpcs_rk_addr_format(int dev, int reg)
> +{
> +	return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);
> +}
> +
> +static int xpcs_rk_read_reg(struct dw_xpcs_rk *pxpcs, int dev, int reg)
> +{
> +	ptrdiff_t csr;
> +	int ret;
> +
> +	csr = xpcs_rk_addr_format(dev, reg);
> +
> +	ret = pm_runtime_resume_and_get(&pxpcs->pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = readl(pxpcs->reg_base + (csr << 2)) & 0xffff;
> +
> +	pm_runtime_put(&pxpcs->pdev->dev);
> +	return ret;
> +}
> +
> +static int xpcs_rk_write_reg(struct dw_xpcs_rk *pxpcs, int dev, int reg, u16 val)
> +{
> +	ptrdiff_t csr;
> +	int ret;
> +
> +	csr = xpcs_rk_addr_format(dev, reg);
> +
> +	ret = pm_runtime_resume_and_get(&pxpcs->pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	writel(val, pxpcs->reg_base + (csr << 2));
> +
> +	pm_runtime_put(&pxpcs->pdev->dev);
> +	return 0;
> +}
> +
> +#define ROCKCHIP_MMD_MII1	2
> +#define ROCKCHIP_MMD_MII2	3
> +#define ROCKCHIP_MMD_MII3	4
> +#define ROCKCHIP_MMD_PMAPMD	6
> +#define ROCKCHIP_MMD_MII	7
> +
> +static bool xpcs_rk_mdio_addr_validate(int addr)
> +{
> +	return !(addr < 0 || addr > 3);
> +}
> +
> +static int xpcs_rk_mdio_read_remapping(int addr, int dev, int reg)
> +{
> +	switch (dev) {
> +	case MDIO_MMD_PMAPMD:
> +		return ROCKCHIP_MMD_PMAPMD;
> +	case MDIO_MMD_VEND2:
> +		break;
> +	default:
> +		return -ENXIO;
> +	}
> +
> +	/* read remapping to MII is performed by HW */
> +	switch (addr) {
> +	case 0:
> +		return ROCKCHIP_MMD_MII;
> +	case 1:
> +		return ROCKCHIP_MMD_MII1;
> +	case 2:
> +		return ROCKCHIP_MMD_MII2;
> +	case 3:
> +		return ROCKCHIP_MMD_MII3;
> +	default:
> +		return -ENODEV;
> +	}
> +}
> +
> +static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
> +{
> +	switch (dev) {
> +	case MDIO_MMD_PMAPMD:
> +		return ROCKCHIP_MMD_PMAPMD;
> +	case MDIO_MMD_VEND2:
> +		break;
> +	default:
> +		return -ENXIO;
> +	}
> +
> +	/* Writable only on MII */
> +	switch (reg) {
> +	case DW_VR_MII_AN_CTRL:
> +	case DW_VR_MII_AN_INTR_STS:
> +	case DW_VR_MII_EEE_MCTRL0:
> +	case DW_VR_MII_EEE_MCTRL1:
> +	case DW_VR_MII_DIG_CTRL2:
> +		return ROCKCHIP_MMD_MII;
> +	default:
> +		break;
> +	}
> +
> +	switch (addr) {
> +	case 0:
> +		return ROCKCHIP_MMD_MII;
> +	case 1:
> +		return ROCKCHIP_MMD_MII1;
> +	case 2:
> +		return ROCKCHIP_MMD_MII2;
> +	case 3:
> +		return ROCKCHIP_MMD_MII3;
> +	default:
> +		return -ENODEV;
> +	}
> +}
> +
> +static int xpcs_rk_read_c22(struct mii_bus *bus, int addr, int reg)
> +{
> +	struct dw_xpcs_rk *pxpcs = bus->priv;
> +	int dev;
> +
> +	if (!xpcs_rk_mdio_addr_validate(addr))
> +		return -ENODEV;
> +
> +	dev = xpcs_rk_mdio_read_remapping(addr, MDIO_MMD_VEND2, reg);
> +	if (dev < 0)
> +		return 0xffff;
> +
> +	return xpcs_rk_read_reg(pxpcs, dev, reg);
> +}
> +
> +static int xpcs_rk_write_c22(struct mii_bus *bus, int addr, int reg, u16 val)
> +{
> +	struct dw_xpcs_rk *pxpcs = bus->priv;
> +	int dev;
> +
> +	if (!xpcs_rk_mdio_addr_validate(addr))
> +		return -ENODEV;
> +
> +	dev = xpcs_rk_mdio_write_remapping(addr, MDIO_MMD_VEND2, reg);
> +	if (dev < 0)
> +		return 0;
> +
> +	return xpcs_rk_write_reg(pxpcs, dev, reg, val);
> +}
> +
> +static int xpcs_rk_read_c45(struct mii_bus *bus, int addr, int dev, int reg)
> +{
> +	struct dw_xpcs_rk *pxpcs = bus->priv;
> +
> +	if (!xpcs_rk_mdio_addr_validate(addr))
> +		return -ENODEV;
> +
> +	dev = xpcs_rk_mdio_read_remapping(addr, dev, reg);
> +	if (dev < 0)
> +		return 0xffff;
> +
> +	return xpcs_rk_read_reg(pxpcs, dev, reg);
> +}
> +
> +static int xpcs_rk_write_c45(struct mii_bus *bus, int addr, int dev, int reg, u16 val)
> +{
> +	struct dw_xpcs_rk *pxpcs = bus->priv;
> +
> +	if (!xpcs_rk_mdio_addr_validate(addr))
> +		return -ENODEV;
> +
> +	dev = xpcs_rk_mdio_write_remapping(addr, dev, reg);
> +	if (dev < 0)
> +		return 0;
> +
> +	return xpcs_rk_write_reg(pxpcs, dev, reg, val);
> +}
> +
> +static struct dw_xpcs_rk *xpcs_rk_create_data(struct platform_device *pdev)
> +{
> +	struct dw_xpcs_rk *pxpcs;
> +
> +	pxpcs = devm_kzalloc(&pdev->dev, sizeof(*pxpcs), GFP_KERNEL);
> +	if (!pxpcs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pxpcs->pdev = pdev;
> +
> +	dev_set_drvdata(&pdev->dev, pxpcs);
> +
> +	return pxpcs;
> +}
> +
> +static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +
> +	pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
> +	if (IS_ERR(pxpcs->serdes_phy))
> +		return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
> +					"Failed to get SerDes PHY\n");
> +
> +	return 0;
> +}
> +
> +static void xpcs_rk_serdes_phy_poweroff(void *data)
> +{
> +	struct dw_xpcs_rk *pxpcs = data;
> +	struct device *dev = &pxpcs->pdev->dev;
> +
> +	phy_power_off(pxpcs->serdes_phy);
> +	phy_exit(pxpcs->serdes_phy);
> +
> +	dev_pm_genpd_rpm_always_on(dev, false);
> +}
> +
> +static int xpcs_rk_serdes_phy_poweron(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +	int ret;
> +
> +	/*
> +	 * The power domain is required and must be enabled, which allows us to
> +	 * dynamically turn the CSR clock on/off using PM while keeping the PCS
> +	 * powered on.
> +	 */
> +	ret = dev_pm_genpd_rpm_always_on(dev, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to power on power-domains\n");
> +		return ret;
> +	}
> +
> +	ret = phy_init(pxpcs->serdes_phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to init SerDes PHY\n");
> +		goto pm_domain;
> +	}
> +
> +	ret = phy_power_on(pxpcs->serdes_phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to power on SerDes PHY\n");
> +		goto serdes_phy;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, xpcs_rk_serdes_phy_poweroff, pxpcs);
> +	if (ret) {
> +		dev_err(dev, "Failed to register devm for SerDes PHY: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +serdes_phy:
> +	phy_exit(pxpcs->serdes_phy);
> +pm_domain:
> +	dev_pm_genpd_rpm_always_on(dev, false);
> +	return ret;
> +}
> +
> +static int xpcs_rk_init_res(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct platform_device *pdev = pxpcs->pdev;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "No reg-space found\n");
> +		return -EINVAL;
> +	}
> +
> +	if (resource_size(res) < SZ_2M) {
> +		dev_err(dev, "Invalid reg-space size\n");
> +		return -EINVAL;
> +	}
> +
> +	pxpcs->reg_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pxpcs->reg_base)) {
> +		dev_err(dev, "Failed to map reg-space\n");
> +		return PTR_ERR(pxpcs->reg_base);
> +	}
> +
> +	return 0;
> +}
> +
> +static void xpcs_rk_exit_clk(void *data)
> +{
> +	struct dw_xpcs_rk *pxpcs = data;
> +	struct device *dev = &pxpcs->pdev->dev;
> +
> +	pm_runtime_force_suspend(dev);
> +	clk_disable_unprepare(pxpcs->eee_clk);
> +}
> +
> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +	int ret;
> +
> +	pxpcs->csr_clk = devm_clk_get(dev, "csr");
> +	if (IS_ERR(pxpcs->csr_clk))
> +		return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
> +					 "Failed to get CSR clock\n");
> +
> +	pxpcs->eee_clk = devm_clk_get(dev, "eee");
> +	if (IS_ERR(pxpcs->eee_clk))
> +		return dev_err_probe(dev, PTR_ERR(pxpcs->eee_clk),
> +					 "Failed to get EEE clock\n");
> +
> +	ret = clk_prepare_enable(pxpcs->eee_clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable EEE clock\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Compute the multiplier for the EEE clock so that
> +	 * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
> +	 *
> +	 * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
> +	 * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
> +	 * The reset value is 0 (200 MHz), but derive the value at runtime to
> +	 * stay correct if the mux is changed by a board.
> +	 */
> +	pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
> +						 1000000000UL) - 1;
> +
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_enable(dev);
> +
> +	ret = devm_add_action_or_reset(dev, xpcs_rk_exit_clk, pxpcs);
> +	if (ret) {
> +		dev_err(dev, "Failed to register devm for EEE clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xpcs_rk_init_bus(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +	static atomic_t id = ATOMIC_INIT(-1);
> +	struct mii_bus *bus;
> +	int ret;
> +
> +	bus = devm_mdiobus_alloc_size(dev, 0);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "Rockchip DW XPCS MCI/APB3";
> +	bus->read = xpcs_rk_read_c22;
> +	bus->write = xpcs_rk_write_c22;
> +	bus->read_c45 = xpcs_rk_read_c45;
> +	bus->write_c45 = xpcs_rk_write_c45;
> +	bus->phy_mask = ~0;
> +	bus->parent = dev;
> +	bus->priv = pxpcs;
> +
> +	snprintf(bus->id, MII_BUS_ID_SIZE,
> +		 "rockchip_dwxpcs-%x", atomic_inc_return(&id));
> +
> +	/*
> +	 * MDIO-bus here serves as just a back-end engine abstracting out
> +	 * the MDIO and MCI/APB3 IO interfaces utilized for the Rockchip DWXPCS CSRs
> +	 * access.
> +	 */
> +	ret = devm_mdiobus_register(dev, bus);
> +	if (ret) {
> +		dev_err(dev, "Failed to create MDIO bus\n");
> +		return ret;
> +	}
> +
> +	pxpcs->bus = bus;
> +	return 0;
> +}
> +
> +static int xpcs_rk_probe(struct platform_device *pdev)
> +{
> +	struct dw_xpcs_rk *pxpcs;
> +	int ret;
> +
> +	pxpcs = xpcs_rk_create_data(pdev);
> +	if (IS_ERR(pxpcs))
> +		return PTR_ERR(pxpcs);
> +
> +	/*
> +	 * The XPCS may be attached to a power domain (e.g. PD_PIPE). The domain
> +	 * must be powered on before any register access, otherwise the SoC will
> +	 * trigger a synchronous external abort (SError).
> +	 *
> +	 * Accessing the XPCS registers also requires a TX clock from the SerDes,
> +	 * which is needed for the soft reset.
> +	 */
> +	ret = xpcs_rk_serdes_phy_init(pxpcs);
> +	if (ret)
> +		return ret;
> +
> +	ret = xpcs_rk_serdes_phy_poweron(pxpcs);
> +	if (ret)
> +		return ret;
> +
> +	ret = xpcs_rk_init_res(pxpcs);
> +	if (ret)
> +		return ret;
> +
> +	ret = xpcs_rk_init_clk(pxpcs);
> +	if (ret)
> +		return ret;
> +
> +	ret = xpcs_rk_init_bus(pxpcs);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xpcs_rk_of_ids[] = {
> +	{ .compatible = "rockchip,rk3568-xpcs" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, xpcs_rk_of_ids);
> +
> +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *pcs_np;
> +	struct dw_xpcs_rk *pxpcs;
> +	struct dw_xpcs *xpcs;
> +	u32 port;
> +
> +	if (!of_device_is_available(np))
> +		return ERR_PTR(-ENODEV);
> +
> +	if (of_property_read_u32(np, "reg", &port))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!xpcs_rk_mdio_addr_validate((int)port))
> +		return ERR_PTR(-EINVAL);
> +
> +	/* The XPCS pdev is attached to the parent node */
> +	pcs_np = of_get_parent(np);
> +	if (!pcs_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	if (!of_device_is_available(pcs_np)) {
> +		of_node_put(pcs_np);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	if (!of_match_node(xpcs_rk_of_ids, pcs_np)) {
> +		of_node_put(pcs_np);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	pdev = of_find_device_by_node(pcs_np);
> +	of_node_put(pcs_np);
> +	if (!pdev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	device_lock(&pdev->dev);

[Severity: Medium]
xpcs_rk_create() is called from a MAC driver's probe path, which is
already running under device_lock() of the MAC device taken by
really_probe().  Taking device_lock(&pdev->dev) here nests a second
struct device mutex, and since all device mutexes share one lockdep
class unless annotated, lockdep will report "possible recursive
locking detected" on the first probe.  That is a splat in the logs on
any CONFIG_PROVE_LOCKING build, and a panic with panic_on_warn set.

Holding the XPCS device's lock across xpcs_create_mdiodev() and
device_link_add() is also unusual - device_link_add() does its own
locking and the driver core normally expects supplier device locks not
to be held by consumers.

What race is the lock actually protecting against?  pxpcs and
pxpcs->bus are published by xpcs_rk_probe() via dev_set_drvdata() and
xpcs_rk_init_bus(), and the !pxpcs || !pxpcs->bus case already returns
-EPROBE_DEFER, so the consumer retries later; the device link then
keeps the supplier around.  Can the device_lock()/device_unlock() pairs
simply be dropped, or is something else needed here (e.g. checking that
the supplier is bound) to close the window you had in mind?

> +	pxpcs = platform_get_drvdata(pdev);
> +	if (!pxpcs || !pxpcs->bus) {
> +		device_unlock(&pdev->dev);
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
> +	if (IS_ERR(xpcs)) {
> +		device_unlock(&pdev->dev);
> +		put_device(&pdev->dev);
> +		return xpcs;
> +	}
> +
> +	if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +		xpcs_destroy(xpcs);
> +		device_unlock(&pdev->dev);
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);
> +	device_unlock(&pdev->dev);
> +	put_device(&pdev->dev);
> +	return xpcs;
> +}
> +EXPORT_SYMBOL_GPL(xpcs_rk_create);
> +
> +static int xpcs_rk_pm_runtime_suspend(struct device *dev)
> +{
> +	struct dw_xpcs_rk *pxpcs = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(pxpcs->csr_clk);
> +
> +	return 0;
> +}
> +
> +static int xpcs_rk_pm_runtime_resume(struct device *dev)
> +{
> +	struct dw_xpcs_rk *pxpcs = dev_get_drvdata(dev);
> +
> +	return clk_prepare_enable(pxpcs->csr_clk);
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(xpcs_rk_pm_ops,
> +			   xpcs_rk_pm_runtime_suspend,
> +			   xpcs_rk_pm_runtime_resume,
> +			   NULL);
> +
> +static struct platform_driver xpcs_rk_driver = {
> +	.probe = xpcs_rk_probe,
> +	.driver = {
> +		.name = "rk_xpcs-dwxpcs",
> +		.pm = pm_ptr(&xpcs_rk_pm_ops),
> +		.of_match_table = xpcs_rk_of_ids,
> +	},
> +};
> +module_platform_driver(xpcs_rk_driver);
> +
> +MODULE_DESCRIPTION("Rockchip XPCS platform device driver");
> +MODULE_AUTHOR("Coia Prant <coiaprant@gmail.com>");
> +MODULE_LICENSE("GPL");

[Severity: High]
This block in xpcs_rk_mdio_write_remapping() ignores addr entirely:
any write to DW_VR_MII_AN_CTRL, DW_VR_MII_AN_INTR_STS,
DW_VR_MII_EEE_MCTRL0, DW_VR_MII_EEE_MCTRL1 or DW_VR_MII_DIG_CTRL2 is
routed to ROCKCHIP_MMD_MII, which per the commit message is port 0's
MMD (7), regardless of which of the four ports the MDIO address
selects.

So when the core configures a PCS instance created with addr 1, 2 or 3
- xpcs_config_aneg_c37_sgmii() writing DW_VR_MII_AN_CTRL and
DW_VR_MII_DIG_CTRL2, xpcs_config_eee() writing DW_VR_MII_EEE_MCTRL0/1,
and the interrupt-status clear on DW_VR_MII_AN_INTR_STS - the write
lands on port 0 instead.  That has two effects: the intended port is
never configured (so its link stays broken), and port 0's
auto-negotiation/EEE state is silently clobbered by an unrelated
interface, including its pending AN interrupt status.

The read path is not symmetric either: xpcs_rk_mdio_read_remapping()
remaps per addr with the comment "read remapping to MII is performed by
HW", so a write to these registers on port 2 followed by a read back
would not return what was written.  Is that asymmetry deliberate?

Does the TRM say these vendor registers physically exist only in MMD 7
and are shared by all four MII ports?  If so, redirecting writes
silently still leaves per-port configuration impossible, and the driver
probably needs to say so explicitly (reject the access, or document how
the shared block is meant to be programmed) rather than aliasing ports
1..3 onto port 0.  If instead each port has its own copy of these
registers, this switch should be dropped so the addr-based mapping
below applies.  Which is it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 3

- [Low] Concern (verified FALSE POSITIVE, retained for policy): the
  binding makes `pcs-handle` required for `phy-mode = "sgmii"` and the…
- [Low] Concern (verified FALSE POSITIVE, retained for policy): the new
  `pcs-handle` property and the `if phy-mode contains sgmii -> require…
- [Low] Concern (verified FALSE POSITIVE, retained for policy): the new
  conditional's `if` clause tests the value of `phy-mode` without…

--

dt-bindings: net: rockchip-dwmac: document pcs-handle

This adds a pcs-handle property to the Rockchip dwmac binding and adds an
allOf conditional that makes pcs-handle a required property whenever
phy-mode contains "sgmii".  There are no other changes to the schema.

No Link: tags are present in the commit header.

> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> index 80c252845349c..67b21ea77ec96 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> @@ -120,6 +120,12 @@ properties:
>      maximum: 0x7F
>      default: 0x10
>  
> +  pcs-handle:
> +    description:
> +      Specifies a reference to a node representing the PCS device
> +      connected to this GMAC. Required when phy-mode is "sgmii".
> +    maxItems: 1
> +

[Severity: Low]

Should pcs-handle be gated on compatible?  As written the property is
accepted for every Rockchip GMAC compatible selected by this binding,
even though only rockchip,rk3568-gmac integrates the XPCS plus Naneng
combo SerDes.  Other SoC-specific properties in this file use the
else-branch form, for example:

    else:
      properties:
        rockchip,php-grf: false

Checking this further, the un-gated form does not appear to cause a
problem in practice.  At the end of the series only rk3568_ops provides
.set_to_sgmii, and rk_gmac_check_ops() in
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c has:

	case PHY_INTERFACE_MODE_SGMII:
		if (!bsp_priv->supports_sgmii)
			return -EINVAL;

so an sgmii device tree on a Rockchip SoC without SGMII support fails
probe rather than misbehaving.  This binding has also never restricted
phy-mode per compatible, since it inherits the full phy-connection-type
enum through snps,dwmac.yaml, so accepting phy-mode = "sgmii" on other
Rockchip parts predates this patch.  The same construct appears un-gated
in Documentation/devicetree/bindings/net/renesas,rzn1-gmac.yaml, which is
also a snps,dwmac child binding.

>    phy-supply:
>      description: PHY regulator
>  
> @@ -159,6 +165,15 @@ allOf:
>          clocks:
>            minItems: 5
>  
> +  - if:
> +      properties:
> +        phy-mode:
> +          contains:
> +            const: sgmii
> +    then:
> +      required:
> +        - pcs-handle
> +

[Severity: Low]

This conditional makes pcs-handle mandatory, and the commit message
describes its consumer in the present tense:

    The property is parsed by rk_pcs_init() and a missing phandle fails
    the probe

At this revision there is no rk_pcs_init() in
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c, so is the wording ahead
of the tree here?

Looking at the rest of the series answers this: the following patch,
"net: stmmac: dwmac-rk: add SGMII support for RK3568", adds
rk_pcs_init() with exactly the described behaviour:

	pcs_node = of_parse_phandle(np, "pcs-handle", 0);
	if (!pcs_node)
		return -ENODEV;

and rk_gmac_probe() installs plat_dat->pcs_init = rk_pcs_init for SGMII.
Sending the binding before the driver change is the expected ordering, so
this is only visible when the patch is read on its own.

[Severity: Low]

Separately, the if clause tests the value of phy-mode without listing it
in required, for example:

  - if:
      properties:
        phy-mode:
          contains:
            const: sgmii
      required:
        - phy-mode

Without that, JSON Schema also matches nodes that omit phy-mode entirely,
which nominally makes template nodes such as gmac0 in
arch/arm64/boot/dts/rockchip/rk3568.dtsi and gmac1 in rk356x-base.dtsi
require pcs-handle.  Would adding required: [phy-mode] be worthwhile for
clarity?

Following this through, it does not change validation results.  This file
has allOf: - $ref: snps,dwmac.yaml#, and snps,dwmac.yaml already lists:

required:
  - compatible
  - reg
  - interrupts
  - interrupt-names
  - phy-mode

so a node without phy-mode already fails validation on the inherited
required list, and the new conditional only adds a second, redundant
message.  The two dtsi nodes above already omit the pre-existing
mandatory phy-mode, so nothing that previously validated is newly
rejected.  The same un-guarded pattern is used in
Documentation/devicetree/bindings/net/ethernet-controller.yaml.

>  unevaluatedProperties: false
>  
>  examples:

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 3

- [Medium] DWMAC_ROCKCHIP gains an unconditional `select
  PCS_XPCS_ROCKCHIP` without inheriting the selected symbol's…
- [Medium] For SGMII the driver removes every speed-dependent clock
  programming path (rk_gmac_probe() sets plat_dat->set_clk_tx_rate =…
- [Low] rk_get_interfaces() (dwmac-rk.c:1570-1571) now sets
  PHY_INTERFACE_MODE_SGMII in phylink's supported_interfaces for every…
- [Low] The RK3568 XPCS SoC node added by the series ('arm64: dts:
  rockchip: rk3568: add XPCS and fixed-clock nodes') omits properties…
- [Low] rk_select_pcs() (dwmac-rk.c:1268-1275) ignores its `interface`
  argument and returns the SGMII XPCS for every interface mode,…

Pre-existing issues:
- [Medium] DISCARDED (valid but pre-existing and below the reporting
  threshold for pre-existing defects).

--

net: stmmac: dwmac-rk: add SGMII support for RK3568

Adds a set_to_sgmii()/set_to_rmii() pair for rk3568 to program GRF bit 7,
wires up pcs_init/pcs_exit/select_pcs so the Rockchip XPCS is used as the
PCS, and reorders rk_gmac_powerup() so the clocks are enabled before the
interface-specific configuration.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index ab3c4cf964239..c6559e749c0ce 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -170,6 +170,7 @@ config DWMAC_ROCKCHIP
>  	default ARCH_ROCKCHIP
>  	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
>  	select MFD_SYSCON
> +	select PCS_XPCS_ROCKCHIP

[Severity: Medium]
Does this select produce unmet dependency warnings?  PCS_XPCS_ROCKCHIP in
drivers/net/pcs/Kconfig carries three dependency lines:

  config PCS_XPCS_ROCKCHIP
	tristate "Rockchip XPCS controller support"
	default ARCH_ROCKCHIP
	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
	depends on GENERIC_PHY || COMPILE_TEST
	depends on PM_GENERIC_DOMAINS || COMPILE_TEST

while DWMAC_ROCKCHIP only has "depends on OF && (ARCH_ROCKCHIP ||
COMPILE_TEST)".  ARCH_ROCKCHIP in arch/arm64/Kconfig.platforms selects PM
but neither GENERIC_PHY nor PM_GENERIC_DOMAINS, so a config with
ARCH_ROCKCHIP=y, COMPILE_TEST=n and GENERIC_PHY=n (or PM_GENERIC_DOMAINS=n)
force-enables PCS_XPCS_ROCKCHIP with its own depends unsatisfied:

  WARNING: unmet direct dependencies detected for PCS_XPCS_ROCKCHIP

pcs-xpcs-rk.o is then built against the phy stubs where devm_phy_get()
returns ERR_PTR(-ENOSYS), so the probe cannot succeed.

The select is also unconditional, so every Rockchip dwmac build
(RK3288/RK3328/RK3399/RK3588 ...) force-enables the RK356x-only XPCS
driver.  Would inheriting the constraints, or making the selection
conditional, be preferable here?  Note that simply adding the two depends
lines to DWMAC_ROCKCHIP would turn off Ethernet on existing RK3288/RK3399
configurations, so the choice probably needs to be explicit.

The Kconfig change is also not mentioned anywhere in the commit message.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 8d7042e689261..5d501ed8d1429 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

[ ... ]

> @@ -1208,6 +1237,43 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
> +static struct phylink_pcs *rk_select_pcs(struct stmmac_priv *priv,
> +					 phy_interface_t interface)
> +{
> +	if (!priv->hw->xpcs)
> +		return NULL;
> +
> +	return xpcs_to_phylink_pcs(priv->hw->xpcs);
> +}

[Severity: Low]
Should rk_select_pcs() look at its interface argument?  The mac_select_pcs
contract in include/linux/phylink.h is to return the PCS for the specified
interface, or NULL when none is required, but this returns the SGMII XPCS
for any mode.

On an RK3568 port probed as SGMII, rk_get_interfaces() also advertises the
RGMII variants and RMII, and xpcs->pcs.supported_interfaces is non-empty
(filled in by xpcs_get_interfaces()).  When phylink validates one of those
non-SGMII interfaces, phylink_validate_mac_and_pcs() hits:

	if (!phy_interface_empty(pcs->supported_interfaces) &&
	    !test_bit(state->interface, pcs->supported_interfaces)) {
		phylink_err(pl, "MAC returned PCS which does not support %s\n",
			    phy_modes(state->interface));
		return -EINVAL;
	}

Would returning the XPCS only for PHY_INTERFACE_MODE_SGMII and NULL
otherwise avoid that error path?

> @@ -1379,16 +1450,19 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  	if (ret)
>  		return ret;
>  
> +	ret = gmac_clk_enable(bsp_priv, true);
> +	if (ret)
> +		return ret;
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but now that gmac_clk_enable()
is the first resource acquisition in rk_gmac_powerup(), its partial-failure
path becomes more visible.  Do the bulk clocks leak when the clk_phy enable
fails?

drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:gmac_clk_enable() {
	...
		ret = clk_bulk_prepare_enable(bsp_priv->num_clks,
					      bsp_priv->clks);
		if (ret)
			return ret;

		ret = clk_prepare_enable(bsp_priv->clk_phy);
		if (ret)
			return ret;
	...
}

The second failure returns without calling clk_bulk_disable_unprepare() and
without setting bsp_priv->clk_enabled, so the later gmac_clk_enable(bsp_priv,
false) in rk_gmac_powerdown() and at the new clk_disable label is a no-op
because it is gated on clk_enabled.  This only triggers for integrated-PHY
setups where clk_phy is non-NULL.

> +	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> +		goto set_mode;
> +
>  	ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
>  	if (ret < 0)
> -		return ret;
> +		goto clk_disable;

[ ... ]

> @@ -1486,6 +1566,9 @@ static void rk_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
>  
>  	if (rk->supports_rmii)
>  		__set_bit(PHY_INTERFACE_MODE_RMII, interfaces);
> +
> +	if (rk->supports_sgmii)
> +		__set_bit(PHY_INTERFACE_MODE_SGMII, interfaces);
>  }

[Severity: Low]
Is SGMII safe to advertise on a port that was not probed as SGMII?
rk3568_ops populates set_to_sgmii unconditionally, and rk_gmac_setup()
derives supports_sgmii from it, so every RK3568 port now advertises SGMII
in phylink's supported_interfaces (stmmac_phy_setup() copies the
get_interfaces() output verbatim into config->supported_interfaces).

All the SGMII setup, though, is keyed off the probe-time DT phy-mode:

  - rk_gmac_powerup() writes RK3568_GMAC_MODE_SGMII_QSGMII only in the
    "case PHY_INTERFACE_MODE_SGMII:" branch, which runs once at init
  - rk_gmac_probe() installs pcs_init/pcs_exit/select_pcs,
    default_an_inband and clears set_clk_tx_rate only under
    "if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII)"

So on a port probed as rgmii*/rmii, if phylink later selects SGMII (SFP
cage, or a PHY reporting SGMII as its host interface), GRF bit 7 stays
cleared by rk3568_set_to_rgmii()/rk3568_set_to_rmii(), plat->select_pcs is
NULL so stmmac_mac_select_pcs() returns NULL and there is no PCS to run
autonegotiation.  Before this patch SGMII was not in supported_interfaces
and such a configuration was rejected during validation.  Would gating the
__set_bit() on the probe-time interface be closer to what the hardware can
actually do?

> @@ -1602,6 +1685,17 @@ static int rk_gmac_probe(struct platform_device *pdev)
>  	plat_dat->suspend = rk_gmac_suspend;
>  	plat_dat->resume = rk_gmac_resume;
>  
> +	if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> +		/* SGMII clock always runs at 125 MHz */
> +		plat_dat->set_clk_tx_rate = NULL;
> +
> +		/* SGMII requires a PCS */
> +		plat_dat->default_an_inband = true;

[Severity: Medium]
The commit message says:

  "Enable default_an_inband for SGMII and disable the generic stmmac
   set_clk_tx_rate callback.  This forces phylink to use in-band mode,
   where the PCS is responsible for speed/duplex negotiation."

Is "forces" accurate?  default_an_inband is only a default, and the next
paragraph of the message says so itself.  In phylink_parse_mode():

	if (pl->config->default_an_inband)
		pl->cfg_link_an_mode = MLO_AN_INBAND;

	dn = fwnode_get_named_child_node(fwnode, "fixed-link");
	if (dn || fwnode_property_present(fwnode, "fixed-link"))
		pl->cfg_link_an_mode = MLO_AN_FIXED;

and xpcs_inband_caps() advertises out-of-band as acceptable:

	case DW_AN_C37_SGMII:
	case DW_AN_C37_1000BASEX:
		return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;

so phylink_pcs_neg_mode() can settle on PHYLINK_PCS_NEG_OUTBAND for a
fixed-link node or a PHY without in-band support.  With set_clk_tx_rate set
to NULL, rk3568_ops.set_speed = rk_set_clk_mac_speed also becomes
unreachable, so a 10/100 Mbps link is brought up with the clocking left at
the 1 Gbps setting and nothing warns or fails.  Would rejecting a
non-inband neg_mode (for example in mac_config or pcs_validate), or failing
probe when a fixed-link node is present for SGMII, match what the message
describes?

One more observation on the series rather than on this patch: the new
binding Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
lists phys and phy-names in its unconditional required list, but the
ethernet-pcs@fda00000 node added to arch/arm64/boot/dts/rockchip/rk3568.dtsi
supplies only reg, clocks, clock-names and power-domains.  Only
rk3568-photonicat.dts adds them:

  &xpcs {
	phys = <&combphy2 PHY_TYPE_SGMII>;
	phy-names = "serdes";
	status = "okay";
  };

[Severity: Low]
Does dtbs_check report "phys is a required property" for the other RK3568
board DTBs?  dtschema validates nodes regardless of status = "disabled".
The neighbouring sata0 node in the same dtsi does carry phys/phy-names at
SoC level, so would moving them into rk3568.dtsi be an option?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com

Re: [PATCH net-next v5 01/11] net: stmmac: move XPCS lifetime management to platform drivers

[Coia Prant]

<netdev-bot+sashiko@kernel.org> 于2026年9月15日周二 04:24写道：
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 2 · Low: 1
>
> - [Medium] Commit message mischaracterizes the removal of the generic
>   XPCS creation as removing dead code.
> - [Medium] DT binding vs implementation mismatch: after this patch
>   nothing in the stmmac core reads the generic 'pcs-handle' property,…
> - [Low] The commit message describes two defects, one framed as "more
>   critically" (the common code overwriting priv->hw->xpcs after a…
>
> --
>
> net: stmmac: move XPCS lifetime management to platform drivers
>
> The generic XPCS creation in stmmac_pcs_setup() (pcs-handle lookup and
> mdio_bus_data->pcs_mask based xpcs_create_mdiodev()) is removed, leaving
> only the plat->pcs_init() / plat->pcs_exit() dispatch. The Intel mGbE glue
> grows its own pcs_init()/pcs_exit() that create and destroy the XPCS, and
> the renesas-gbeth, rzn1 and socfpga pcs_exit() callbacks now clear
> priv->hw->phylink_pcs after destroying the PCS.
>
> A few questions on the commit message and on the removal of the generic
> path.
>
> >     First, if a device tree specifies a "pcs-handle" but no select_pcs()
> >     callback is provided by the platform driver, the created XPCS is never
> >     used. The phylink framework requires select_pcs() to actually return
> >     the PCS to the core, so the pcs-handle property becomes effectively
> >     useless without the matching callback.
>
> [Severity: Medium]
> Is "the created XPCS is never used" accurate?  An XPCS stored in
> priv->hw->xpcs has two consumers that do not go through select_pcs().
>
> stmmac_phylink_setup() in stmmac_main.c uses it to widen the phylink
> interface sets:
>
>         /* If we have an xpcs, it defines which PHY interfaces are supported. */
>         if (priv->hw->xpcs)
>                 pcs = xpcs_to_phylink_pcs(priv->hw->xpcs);
>         else
>                 pcs = priv->hw->phylink_pcs;
>
>         if (pcs)
>                 phy_interface_or(config->supported_interfaces, ...
>
> and stmmac_init_phy() uses it to decide whether to attach a PHY at all:
>
>         if (priv->hw->xpcs &&
>             xpcs_get_an_mode(priv->hw->xpcs, mode) == DW_AN_C73)
>                 return 0;
>
> So for a dwmac instance with a pcs-handle but no plat->pcs_init, does this
> change narrow config->supported_interfaces (and config->lpi_interfaces) and
> alter the PHY-attach decision, rather than just dropping something unused?
> Would it be worth spelling that behavioural change out in the commit
> message?

The XPCS's runtime functionality is entirely driven by phylink through
pcs_config() / pcs_get_state() / pcs_link_up(), all of which require
the pl->pcs pointer returned by mac_select_pcs(). Without that callback,
pl->pcs stays NULL and none of them are ever called.

The two reads in stmmac_main.c are one-shot setup-time side effects.
They do not exercise the PCS, so I consider the statement accurate in
the sense that matters here.

> [Severity: Low]
> This isn't a bug, but could this paragraph say that no in-tree platform is
> affected today?
>
> All three pre-existing pcs_init() implementations store their PCS in
> priv->hw->phylink_pcs, not priv->hw->xpcs:
>
>   renesas_gmac_pcs_init()  -> priv->hw->phylink_pcs = miic_create(...)
>   rzn1_dwmac_pcs_init()    -> priv->hw->phylink_pcs = miic_create(...)
>   socfpga_dwmac_pcs_init() -> priv->hw->phylink_pcs = pcs
>
> so the old "priv->hw->xpcs = xpcs;" on the pcs_init branch overwrote NULL
> with NULL.  intel_mgbe_pcs_init() added here is the first in-tree pcs_init()
> that stores an XPCS, which makes this a refactor that removes a latent trap
> rather than a fix for observable breakage.

The wording is intentional. All in-tree pcs_init() implementations
except Intel store their PCS in priv->hw->phylink_pcs, so the old
assignment was a latent trap rather than observable breakage. Calling
it out as critical reflects the risk it would have posed for the first
driver that stored an XPCS there.

> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> > index afe98ff5bdcb0..7396b68899c66 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> > @@ -426,35 +426,14 @@ int stmmac_mdio_reset(struct mii_bus *bus)
> >  int stmmac_pcs_setup(struct net_device *ndev)
> >  {
> >       struct stmmac_priv *priv = netdev_priv(ndev);
> > -     struct fwnode_handle *devnode, *pcsnode;
> > -     struct dw_xpcs *xpcs = NULL;
> > -     int addr, ret;
> > -
> > -     devnode = dev_fwnode(priv->device);
> > -
> > -     if (priv->plat->pcs_init) {
> > -             ret = priv->plat->pcs_init(priv);
> > -     } else if (fwnode_property_present(devnode, "pcs-handle")) {
> > -             pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
> > -             xpcs = xpcs_create_fwnode(pcsnode);
> > -             fwnode_handle_put(pcsnode);
> > -             ret = PTR_ERR_OR_ZERO(xpcs);
> > -     } else if (priv->plat->mdio_bus_data &&
> > -                priv->plat->mdio_bus_data->pcs_mask) {
> > -             addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
> > -             xpcs = xpcs_create_mdiodev(priv->mii, addr);
> > -             ret = PTR_ERR_OR_ZERO(xpcs);
> > -     } else {
> > +     int ret;
> > +
> > +     if (!priv->plat->pcs_init)
> >               return 0;
> > -     }
> >
> > +     ret = priv->plat->pcs_init(priv);
> >       if (ret)
> > -             return dev_err_probe(priv->device, ret, "No xPCS found\n");
> > -
> > -     if (xpcs)
> > -             xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
> > -
> > -     priv->hw->xpcs = xpcs;
> > +             return dev_err_probe(priv->device, ret, "Failed to initialize PCS\n");
> >
> >       return 0;
> >  }
>
> [Severity: Medium]
> After this change, does anything in the stmmac core still read the generic
> "pcs-handle" property?
>
> The property is still documented generically in
> Documentation/devicetree/bindings/net/ethernet-controller.yaml:
>
>   pcs-handle:
>     $ref: /schemas/types.yaml#/definitions/phandle-array
>     items:
>       maxItems: 1
>     description:
>       Specifies a reference to a node representing a PCS PHY device on a MDIO
>       bus to link with an external PHY (phy-handle) if exists.
>
> and snps,dwmac.yaml $refs ethernet-controller.yaml and ends with
> "additionalProperties: true", so pcs-handle stays schema-valid on every
> dwmac node regardless of which glue is bound.
>
> On a glue that does not set plat->pcs_init, stmmac_pcs_setup() now returns 0
> without looking at the property, priv->hw->xpcs stays NULL, and nothing is
> logged - whereas the removed code would at least fail probe with
> "No xPCS found" for a bad phandle.  Should the binding be updated to say
> pcs-handle is only honoured by specific glue drivers, or should the core
> keep a generic path (or emit a warning) so the two agree?
>
> The same question applies to struct stmmac_mdio_bus_data.pcs_mask.
> stmmac_mdio_register() still treats it as "a PCS lives at these MDIO
> addresses" and masks them out of the bus scan:
>
>         new_bus->phy_mask = mdio_bus_data->phy_mask | mdio_bus_data->pcs_mask;
>
> but no core code creates a PCS for those addresses any more, only
> dwmac-intel's private intel_mgbe_pcs_init().  Does a glue that sets
> pcs_mask without pcs_init now end up with that address neither scanned as a
> PHY nor instantiated as a PCS?

This is intentional. Only platforms that set pcs_init() are considered
to support a PCS. A glue that sets pcs-handle or pcs_mask without
pcs_init() is misconfigured, and the new behaviour makes that explicit
by not creating anything.

Re: [PATCH net-next v5 02/11] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property

[Coia Prant]

<netdev-bot+sashiko@kernel.org> 于2026年9月15日周二 04:24写道：
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 0 · Low: 1
>
> - [Low] Concern (validated as NOT a defect in the series context): the
>   binding adds `rockchip,sgmii-mac-sel` while, at this commit alone, no…
>
> --
>
> dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
>
> This adds an optional rockchip,sgmii-mac-sel property to the naneng-combphy
> binding so a board can select whether the RK3568 SGMII interface is routed
> to GMAC0 or GMAC1.  The property is restricted to the RK3568 compatible via
> the existing allOf conditional and defaults to 1 (GMAC1).
>
> [Severity: Low]
> At this commit alone, is rockchip,sgmii-mac-sel read by anything?
>
> rockchip_combphy_parse_dt() in
> drivers/phy/rockchip/phy-rockchip-naneng-combphy.c does not look it up,
> struct rockchip_combphy_grfcfg has no pipe_sgmii_mac_sel descriptor, and
> no in-tree DTS sets it, so a kernel built from exactly this commit would
> silently ignore the property.
>
> This resolves itself later in the same series: "phy: rockchip:
> naneng-combphy: add SGMII MAC selection for RK3568" adds the parsing
>
>         priv->sgmii_mac_sel = 1;
>         device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
>
> along with the RK3568 pipe-GRF descriptor
>
>         .pipe_sgmii_mac_sel     = { 0x0040, 1, 1, 0x00, 0x01 },
>
> and the write in the PHY_TYPE_SGMII path of rk3568_combphy_cfg(), and
> "arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port" adds the
> first user:
>
>         &combphy2 {
>                 rockchip,sgmii-mac-sel = <0>;
>                 status = "okay";
>         };
>
> So this is noted only for the record, since the binding and its consumer
> being split across consecutive patches is the normal series layout.
>
> The remaining point is a design question rather than a bug: is a raw
> uint32 MAC index the interface preferred here, or would a phandle to the
> GMAC controller node be a better fit for describing the routing?

The property maps directly onto a single bit in the combo PHY's own
GRF register (PIPE_GRF_XPCS_CON0 bit 1), which selects whether the
SGMII lane is routed to GMAC0 or GMAC1. It describes a mux setting
inside the PHY, not a relationship to the GMAC node. A phandle would
introduce an extra dependency edge between the combo PHY node and the
GMAC node for no benefit, and the raw value matches how the BSP and
the surrounding GRF properties describe similar muxes.

So I'll keep the uint32 form.

Re: [PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[Coia Prant]

<netdev-bot+sashiko@kernel.org> 于2026年9月15日周二 04:24写道：
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 0 · Low: 1
>
> - [Low] In rk3568_combphy_cfg() (drivers/phy/rockchip/phy-rockchip-
>   naneng-combphy.c:881-882) the newly added write of pipe_sgmii_mac_sel…
>
> --
>
> phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
>
> This adds a "rockchip,sgmii-mac-sel" DT property to pick whether the RK3568
> SGMII interface is routed to GMAC0 or GMAC1 via PIPE_GRF_XPCS_CON0 bit 1.
> The value is read in rockchip_combphy_parse_dt() and programmed from the
> PHY_TYPE_SGMII branch of rk3568_combphy_cfg().
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)
>
> One question about the commit message wording:
>
>     The hardware reset value is 1 (GMAC1). If the property is set to 0,
>     the driver routes SGMII to GMAC0; if set to 1 (or omitted), it
>     remains at GMAC1.
>
> Does "or omitted ... it remains at GMAC1" match the code?  The write in
> rk3568_combphy_cfg() is unconditional, so in the omitted case the driver
> does not leave the bit alone, it actively programs it to 1.  See the
> inline comment below.
>
> > diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> > index 7843356a4dd47..7b867e7520064 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>
> [ ... ]
>
> > @@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
> >
> >       priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
> >
> > +     priv->sgmii_mac_sel = 1;
> > +     device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
> > +
>
> [Severity: Low]
> Since device_property_read_u32() leaves the output untouched on failure,
> rockchip_combphy_parse_dt() ends up with priv->sgmii_mac_sel == 1 both when
> the property says 1 and when the property is absent.  Is there a way for the
> driver to express "leave the bit as firmware left it"?
>
> Before this patch the kernel never touched PIPE_GRF_XPCS_CON0 bit 1, so a
> board whose bootloader routed SGMII to GMAC0 kept that routing.  With this
> change, a DT that instantiates the combphy as PHY_TYPE_SGMII but does not
> carry the new property has the bit re-programmed to GMAC1 on every
> phy_init().  Can that break such a board's SGMII link?
>
> >       priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
> >       /* fallback to old behaviour */
> >       if (PTR_ERR(priv->phy_rst) == -ENOENT)
> > @@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
> >               break;
> >
> >       case PHY_TYPE_SGMII:
> > +             rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
> > +                                          priv->sgmii_mac_sel > 0);

The hardware reset value of PIPE_GRF_XPCS_CON0 bit 1 is 1 (GMAC1),
so programming it to 1 when the property is absent produces the same
result as leaving it alone — the bit is already 1 after reset. The
"remains at GMAC1" wording in the commit message reflects that.

SGMII has never been functional on RK3568 in mainline before this
series, so there is no existing in-tree or out-of-tree board whose
SGMII link could be broken by this. A board whose firmware happened to
route the bit to GMAC0 still cannot use SGMII with a mainline kernel
before this series, and will need the new property to describe that
routing once it can.

> [Severity: Low]
> This write always happens, including in the "property omitted" case, and
> rockchip_combphy_param_write() sets the write-enable mask along with the
> value:
>
>         tmp = en ? reg->enable : reg->disable;
>         mask = GENMASK(reg->bitend, reg->bitstart);
>         val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);
>
>         return regmap_write(base, reg->offset, val);
>
> so with priv->sgmii_mac_sel defaulting to 1 the register gets
> (0x1 << 1) | (GENMASK(1, 1) << 16), i.e. GMAC1 is forced.  Would it be
> better to only issue this write when the property is actually present?
>
> A second question: bit 1 of PIPE_GRF_XPCS_CON0 is a single SoC-wide mux,
> while the property sits on a per-PHY node.  If two combphy instances that
> share the same rockchip,pipe-grf syscon are both brought up as
> PHY_TYPE_SGMII with different property values, the last phy_init() wins and
> nothing is reported.  Should conflicting values be rejected or at least
> warned about?
>
> >               rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
> >               rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
> >               rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);
> > @@ -984,6 +991,7 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
> >       .con3_for_sata          = { 0x000c, 15, 0, 0x00, 0x4407 },
> >       /* pipe-grf */
> >       .pipe_con0_for_sata     = { 0x0000, 15, 0, 0x00, 0x2220 },
> > +     .pipe_sgmii_mac_sel     = { 0x0040, 1, 1, 0x00, 0x01 },
> >       .pipe_xpcs_phy_ready    = { 0x0040, 2, 2, 0x00, 0x01 },
>
> For what it is worth, the only in-tree SGMII consumer at the end of this
> series, rk3568-photonicat.dts, sets rockchip,sgmii-mac-sel = <0>
> explicitly, so no upstream DT changes behaviour today; the concern is about
> out-of-tree or ported device trees and about the commit message wording.

There is no in-tree case where two combphy instances are both brought
up as PHY_TYPE_SGMII with different values. This is a single SoC-wide
mux and only one SGMII link exists on RK3568, so the situation cannot
arise on real hardware. I'll leave it as is.

Re: [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery

[Coia Prant]

<netdev-bot+sashiko@kernel.org> 于2026年9月15日周二 04:24写道：
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 4 · Low: 0
>
> - [High] In xpcs_get_state_c37_sgmii() the combination
>   'DW_VR_MII_AN_STS_C37_ANCMPLT_INTR set + DW_VR_MII_C37_ANSGM_SP_LNKSTS…
> - [Medium] Undisclosed functional change: besides the two BMCR_ANRESTART
>   additions the commit message advertises, the patch adds a new…
> - [Medium] Device-specific quirk applied unconditionally: the commit
>   message and the new comment at drivers/net/pcs/pcs-xpcs.c:776-781…
> - [Medium] Wrong ordering in xpcs_get_state_c37_sgmii(): the latched AN-
>   complete indication is consumed (xpcs_write(DW_VR_MII_AN_INTR_STS, 0),…
> - [Medium] The patch turns the read-only-by-contract pcs_get_state()
>   callback into a hardware-state-changing operation (BMCR read-modify-…
>
> --
>
> net: pcs: xpcs: add ANRESTART support for SGMII link recovery
>
> The patch sets BMCR_ANRESTART together with BMCR_ANENABLE when
> xpcs_config_aneg_c37_sgmii() enables in-band AN, and makes
> xpcs_get_state_c37_sgmii() clear DW_VR_MII_AN_INTR_STS and assert
> BMCR_ANRESTART when the link is down with AN completion latched.  A new
> comment explains that the DesignWare databook says AN restart is not
> needed for MAC side SGMII, but that some parts (Rockchip RK3568) need it.
>
> A few questions below.
>
> > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > index 0337e2bcc0125..8c3875b6985b9 100644
> > --- a/drivers/net/pcs/pcs-xpcs.c
> > +++ b/drivers/net/pcs/pcs-xpcs.c
> > @@ -769,7 +771,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
> >        *       SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
> >        *       PHY about the link state change after C28 AN is completed
> >        *       between PHY and Link Partner. There is also no need to
> > -      *       trigger AN restart for MAC-side SGMII.
> > +      *       trigger AN restart for MAC-side SGMII on most devices.
> > +      *
> > +      * Note: While the DesignWare databook states that AN restart is
> > +      *       not needed for MAC side SGMII, some implementations (e.g.
> > +      *       Rockchip RK3568) exhibit a timing quirk when integrated with
> > +      *       phylink and do not restart AN automatically when the link
> > +      *       comes back up. An explicit AN restart is required on those
> > +      *       parts to recover the link after a disconnect.
> >        */
>
> [Severity: Medium]
> The comment says the restart is only "required on those parts" and that
> there is "no need to trigger AN restart for MAC-side SGMII on most
> devices", and the commit message says "This is not a fix for an existing
> mainline platform".  The code, however, applies the restart on every
> device: no gate on xpcs->info.pma or xpcs->desc, and struct dw_xpcs gains
> no quirk flag.
>
> The same function already uses that gating idiom a few lines below:
>
>         if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
>                 mask |= DW_VR_MII_AN_CTRL_8BIT;
>                 val |= DW_VR_MII_AN_CTRL_8BIT;
>                 /* Hardware requires it to be PHY side SGMII */
>                 tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
>
> Note that this TXGBE case is configured as PHY-side SGMII, which the
> MAC-side/RK3568 rationale in the comment does not cover.
>
> Should the new restart be gated on the affected PMA (or on a new dw_xpcs
> quirk flag) so the code matches what the comment and the commit message
> describe?

Rockchip does not own a unique PMA ID, so there is nothing to gate on.
The restart is a standard BMCR bit, and it was tested on Wangxun and on
older XPCS hardware without adverse effect. Enabling AN and restarting
it in the same write is the correct sequence regardless of the return
value.

> Also, phylink already has a path for this: pcs_config() may return a
> positive value to ask phylink to call .pcs_an_restart, and
> xpcs_an_restart() in this same driver performs exactly the same register
> write:
>
>         xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
>                     BMCR_ANRESTART);
>
> Would using that framework path be preferable to open-coding the restart
> in the config and get_state callbacks?

I think enable and restart AN at once whatever returns value is correct.

> > @@ -816,9 +825,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
> >       if (ret < 0)
> >               return ret;
> >
> > +     /* Clear CL37 AN complete status */
> > +     ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> > +     if (ret < 0)
> > +             return ret;
> > +
>
> [Severity: Medium]
> The commit message describes the change as "Add BMCR_ANRESTART in two
> places" plus a comment update, but this hunk adds a third functional
> change that is only visible as item "5)" in the in-code numbered list.
>
> This write is unconditional: it runs for every xpcs_config() ->
> xpcs_do_config() -> xpcs_config_aneg_c37_sgmii() call and for every
> neg_mode, including PHYLINK_PCS_NEG_OUTBAND where no AN is subsequently
> enabled.  It clears the register that xpcs_get_state_c37_sgmii() uses as
> its only source of link/speed/duplex and of state->an_complete:
>
>         ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
>
> It also changes the value returned when neg_mode is not
> PHYLINK_PCS_NEG_INBAND_ENABLED: the function now returns the result of
> this AN_INTR_STS write instead of the DIG_CTRL1 modify.
>
> Could the commit message mention this status clear and the return value
> change?  For reference, xpcs_config_aneg_c37_1000basex() already performs
> the identical clear, so the write itself matches existing driver practice.

The clear is already performed in xpcs_get_state_c37_sgmii() before
this patch, so the behaviour is unchanged for that path. I'll leave the
commit message as is.

> >       if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> >               ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> > -                              mdio_ctrl | BMCR_ANENABLE);
> > +                              mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
> >
> >       return ret;
> >  }
> > @@ -1093,9 +1107,18 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
> >               return 0;
> >       }
> >
> > -     /* Clear AN complete status or interrupt */
> > -     if (state->an_complete)
> > -             xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> > +     if (state->an_complete) {
> > +             /* Clear AN complete status or interrupt */
> > +             ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             /* Initiate the next round of AN */
> > +             ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> > +                               BMCR_ANRESTART);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
>
> [Severity: High]
> Is the trigger condition here really "stuck AN"?  The state reached at
> this point is ANCMPLT latched with LNKSTS clear, taken from the same
> register read:
>
>         state->an_complete = ret & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
>
>         if (ret & DW_VR_MII_C37_ANSGM_SP_LNKSTS) {
>                 ...
>                 return 0;
>         }
>
> For C37/SGMII, isn't that the normal steady state whenever the partner
> PHY's media side is down?

This state must trigger an ANRESTART. On an unplugged SGMII port, the
partner PHY keeps sending a LNKSTS=0 config word, so the PCS completes
AN and latches ANCMPLT while LNKSTS stays clear. Without a restart the
PCS stays in that state indefinitely and never re-runs AN when the
partner comes back, so the link can never recover. Re-issuing ANRESTART
on each poll while the link is down is the mechanism that makes
recovery possible.

> The PHY keeps sending config words, the SGMII
> AN exchange completes and latches ANCMPLT (bit 0), and the media link
> state is carried separately in the received config word via LNKSTS
> (bit 4), per pcs-xpcs.h:
>
>         #define DW_VR_MII_AN_STS_C37_ANCMPLT_INTR       BIT(0)
>         ...
>         #define DW_VR_MII_C37_ANSGM_SP_LNKSTS           BIT(4)
>
> If so, then on an unplugged port each restart re-completes within the AN
> link timer with LNKSTS still 0 and re-latches ANCMPLT, so the condition
> re-arms itself and a fresh AN restart is issued on every poll,
> indefinitely.  phylink polls pcs_get_state once per second and
> xpcs->pcs.poll is true for everything except WX_TXGBE and MP_FBNIC, so
> this affects the existing DW_AN_C37_SGMII users (stmmac/Intel mGbE, NXP
> SJA1105/SJA1110, Wangxun), not only RK3568.
>
> Two further consequences: with MAC_AUTO_SW armed, the hardware-selected
> speed/duplex is dropped on each cycle, and because the whole status word
> is cleared by writing 0, a restart that lands on a just-latched LNKSTS=1
> word destroys that link-up indication and costs an extra poll interval.

The speed/duplex bits in the status word are read-only, so the
"MAC_AUTO_SW result dropped each cycle" concern does not apply: the
hardware re-derives them after each restart.

> For the interrupt-driven case there may also be a faster loop.  The SGMII
> AN_CTRL modify in xpcs_config_aneg_c37_sgmii() does not include
> DW_VR_MII_AN_INTR_EN in its mask, while the 1000BASE-X path sets it:
>
>         if (!xpcs->pcs.poll) {
>                 mask |= DW_VR_MII_AN_INTR_EN;
>                 val |= DW_VR_MII_AN_INTR_EN;
>         }
>
> so an enable programmed by a previous 1000BASE-X configuration persists
> into SGMII mode.  On txgbe the AN event is routed to the link IRQ:
>
> drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c:txgbe_misc_irq_handle() {
>         if (eicr & (TXGBE_PX_MISC_ETH_LK | TXGBE_PX_MISC_ETH_LKDN |
>                     TXGBE_PX_MISC_ETH_AN)) {
>                 sub_irq = irq_find_mapping(txgbe->misc.domain, TXGBE_IRQ_LINK);
>         ...
> }
>
> which ends in phylink_pcs_change() -> pcs_get_state(), so the restart can
> be re-triggered at hardware rate rather than once per second.
>
> Would gating this on the affected PMA/quirk, or on an explicit "AN is not
> active" hardware condition rather than on the latched AN-complete bit,
> address this?

On the loop rate: phylink only registers its own link IRQ for
MLO_AN_FIXED with a link GPIO. For in-band SGMII, phylink either polls
pcs_get_state() through the PCS poll timer when pcs->poll is set, or
waits for the PCS driver to call phylink_pcs_change() from its own
interrupt. On RK3568, xpcs->pcs.poll is set, so this runs at phylink's
resolve interval, roughly once per second. A single MDIO register write
per second is not a concern. On interrupt-driven parts the behaviour
was tested by Wangxun and no adverse effect was observed.

> [Severity: Medium]
> Is the ordering here recoverable if the second access fails?  The latched
> AN-complete indication is consumed by the xpcs_write(DW_VR_MII_AN_INTR_STS,
> 0) before the fallible read-modify-write that actually starts the next
> negotiation.
>
> If the write succeeds and the xpcs_modify() of MII_BMCR fails on the
> register back-end, the only indication that AN completed is gone and no
> new negotiation was started.  The block that would retry is itself gated
> on state->an_complete, which is now read back as 0, so the restart is
> never attempted again.  The error only reaches xpcs_get_state(), which
> logs it from a void callback:
>
>                 ret = xpcs_get_state_c37_sgmii(xpcs, state);
>                 if (ret)
>                         dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
>                                 "xpcs_get_state_c37_sgmii", ERR_PTR(ret));
>
> and nothing records a pending restart.  This matters most on the hardware
> this patch targets, where AN is not restarted automatically, so the port
> would stay down until a fresh pcs_config().  The failure is plausible for
> these vendor-MMD accesses: the RK3568 back-end added later in this series
> returns errors from pm_runtime_resume_and_get() inside
> xpcs_rk_write_reg()/xpcs_rk_read_reg().
>
> Would asserting BMCR_ANRESTART before clearing the status word, or
> retrying independently of the latch, close this window?

If an MDIO access fails at this point, it means an unrecoverable
hardware error until reset. Retrying independently of the latch would
not help in that case.

> [Severity: Medium]
> This makes pcs_get_state() change hardware state rather than only read it,
> and phylink calls that callback from contexts with no common lock.
>
> phylink_resolve() holds pl->state_mutex (and the phy locks) but not RTNL,
> while these two callers hold only RTNL:
>
> drivers/net/phy/phylink.c:phylink_ethtool_ksettings_get() {
>         ...
>                 phylink_mac_pcs_get_state(pl, &link_state);
>         ...
> }
>
> drivers/net/phy/phylink.c:phylink_mii_read() {
>         ...
>                         phylink_mac_pcs_get_state(pl, &state);
>         ...
> }
>
> The lockset intersection is empty, and the driver holds no private lock,
> so two CPUs can run this block at once and both assert BMCR_ANRESTART,
> aborting each other's negotiation.  It can also interleave with the
> multi-transaction BMCR sequence in xpcs_config_aneg_c37_sgmii(), which
> reads BMCR, writes it with ANENABLE cleared, programs AN_CTRL/DIG_CTRL1,
> clears AN_INTR_STS, and finally writes the cached value back:
>
>         mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
>         ...
>                 ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
>                                  mdio_ctrl & ~BMCR_ANENABLE);
>
> Each xpcs_read/xpcs_write is a separate bus transaction, so an AN restart
> can land while AN is disabled and the AN configuration is only half
> applied.
>
> Both RTNL-only paths need the PCS-only case (no phydev), which is
> reachable for inband SGMII without a PHY, and SIOCGMIIREG and
> ETHTOOL_GLINKSETTINGS are unprivileged.  Before this patch these paths
> only cleared the AN status word; should the restart be moved to a context
> that is serialized with pcs_config(), such as the .pcs_an_restart op?

xpcs_get_state_c73() already calls xpcs_soft_reset() and
xpcs_do_config() from the same callback, so changing hardware state
from pcs_get_state() is an established pattern in this driver, not
something this patch introduces. The locking model is phylink's, and
reworking it is out of scope for this series.

Re: [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[Coia Prant]

<netdev-bot+sashiko@kernel.org> 于2026年9月15日周二 04:24写道：
>
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 3 · Low: 2
>
> - [High] Incorrect MMD write remapping for MII vendor registers breaks
>   configuration for ports 1, 2, and 3.
> - [Medium] The patch silently re-packages the pre-existing generic
>   Synopsys XPCS platform driver.
> - [Medium] xpcs_rk_init_clk() (drivers/net/pcs/pcs-xpcs-rk.c:361-362)
>   computes `pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 *…
> - [Medium] Acquiring `device_lock()` of another device from within a
>   probe path causes a lockdep warning (recursive locking).
> - [Low] Concern that the newly exported xpcs_rk_create()
>   (include/linux/pcs/pcs-xpcs-rk.h) has no in-tree caller and no Kconfig…
> - [Low] The rockchip,rk3568-xpcs binding lists `phys` and `phy-names` in
>   its `required:` list…
>
> --
>
> net: pcs: xpcs: add Rockchip RK3568 platform glue driver
>
> Adds a platform glue driver for the DesignWare XPCS block found on RK3568,
> exposing the APB3-mapped XPCS registers as a virtual MDIO bus with per-port
> MMD remapping so the generic pcs-xpcs core can create a phylink_pcs.  Also
> adds new Kconfig symbols for the generic and Rockchip XPCS glue and an
> exported xpcs_rk_create() helper for MAC drivers.
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)
>
> > diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> > index e417fd66f660a..080538f193d59 100644
> > --- a/drivers/net/pcs/Kconfig
> > +++ b/drivers/net/pcs/Kconfig
> > @@ -12,6 +12,31 @@ config PCS_XPCS
> >         This module provides a driver and helper functions for Synopsys
> >         DesignWare XPCS controllers.
> >
> > +if PCS_XPCS
> > +
> > +config PCS_XPCS_PLATFORM
> > +     tristate "Generic XPCS controller support"
> > +     default PCS_XPCS
> > +     help
> > +       Generic DWXPCS driver for platforms that don't require any
> > +       platform specific code to function or is using platform
> > +       data for setup.
> > +
> > +       If you have a controller with this interface, say Y or M here.
> > +
> > +config PCS_XPCS_ROCKCHIP
> > +     tristate "Rockchip XPCS controller support"
> > +     default ARCH_ROCKCHIP
> > +     depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
> > +     depends on GENERIC_PHY || COMPILE_TEST
> > +     depends on PM_GENERIC_DOMAINS || COMPILE_TEST
> > +     help
> > +       Support for XPCS controller on Rockchip RK356x SoC.
> > +
> > +       If you have a Rockchip SoC with this interface, say Y or M here.
> > +
> > +endif # PCS_XPCS
> > +
> >  config PCS_LYNX
> >       tristate
> >       help
> > diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> > index 4f7920618b900..f9f6cf2578d72 100644
> > --- a/drivers/net/pcs/Makefile
> > +++ b/drivers/net/pcs/Makefile
> > @@ -1,10 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  # Makefile for Linux PCS drivers
> >
> > -pcs_xpcs-$(CONFIG_PCS_XPCS)  := pcs-xpcs.o pcs-xpcs-plat.o \
> > -                                pcs-xpcs-nxp.o pcs-xpcs-wx.o
> > +pcs_xpcs-$(CONFIG_PCS_XPCS)  := pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o
> >
> >  obj-$(CONFIG_PCS_XPCS)               += pcs_xpcs.o
> > +obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
> > +obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.o
>
> [Severity: Medium]
> Should this repackaging of the pre-existing generic platform glue be a
> separate patch, or at least be described in the commit message?
>
> pcs-xpcs-plat.o is moved out of the pcs_xpcs composite object and built as
> its own module, and the new PCS_XPCS_PLATFORM tristate makes the snps,dw-xpcs
> platform glue user-selectable where it was previously unconditional under
> PCS_XPCS.  So the snps,dw-xpcs platform driver ships in pcs-xpcs-plat.ko
> instead of pcs_xpcs.ko after this patch.
>
> The commit message only describes adding the Rockchip glue driver and does
> not mention this change to existing functionality.  Could the Makefile and
> Kconfig refactor be split out with its own rationale?

We have added a default value for existing users when selecting PCS_XPCS.

> > +static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
> > +{
> > +     struct device *dev = &pxpcs->pdev->dev;
> > +
> > +     pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
> > +     if (IS_ERR(pxpcs->serdes_phy))
> > +             return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
> > +                                     "Failed to get SerDes PHY\n");
> > +
> > +     return 0;
> > +}
>
> [Severity: Low]
> xpcs_rk_serdes_phy_init() treats the "serdes" phy as mandatory and fails
> probe when it cannot be resolved, and the binding added later in this series
> (Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml) lists
> phys and phy-names under required:
>
>   required:
>     - compatible
>     - reg
>     ...
>     - phys
>     - phy-names
>     - power-domains
>
> The SoC node added in arch/arm64/boot/dts/rockchip/rk3568.dtsi only has:
>
>         xpcs: ethernet-pcs@fda00000 {
>                 compatible = "rockchip,rk3568-xpcs";
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 reg = <0x0 0xfda00000 0x0 0x200000>;
>                 clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
>                 clock-names = "csr", "eee";
>                 power-domains = <&power RK3568_PD_PIPE>;
>                 status = "disabled";
>
> CHECK_DTBS validates nodes regardless of status, so does every RK3568 DTB
> now report missing phys/phy-names for this node?  Only
> rk3568-photonicat.dts adds them, so a board that just flips this node to
> status = "okay" would also fail probe with "Failed to get SerDes PHY".
>
> Should the SoC node carry phys/phy-names, or should the binding not mark
> them required at the SoC level?

As replied earlier, the SerDes connection is board-specific (some
boards use combphy1, others combphy2), so phys/phy-names cannot move
into the SoC dtsi. dtbs_check skips required-property validation for
status = "disabled" nodes, so boards that don't enable XPCS produce no
warnings.

> > +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> > +{
> [ ... ]
> > +     /*
> > +      * Compute the multiplier for the EEE clock so that
> > +      * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
> > +      *
> > +      * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
> > +      * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
> > +      * The reset value is 0 (200 MHz), but derive the value at runtime to
> > +      * stay correct if the mux is changed by a board.
> > +      */
> > +     pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
> > +                                              1000000000UL) - 1;
>
> [Severity: Medium]
> Can this value overflow the 4-bit register field it ends up in?
>
> For the reset configuration named in the comment (200 MHz, 5 ns) this
> computes 100 * 200000000 / 1000000000 - 1 = 19.  That is passed on
> unchecked:
>
>         xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);
>
> and the core programs it in xpcs_config_eee():
>
>         drivers/net/pcs/pcs-xpcs.c:
>                       FIELD_PREP(DW_VR_MII_EEE_MULT_FACT_100NS,
>                                  xpcs->eee_mult_fact);
>
> with
>
>         drivers/net/pcs/pcs-xpcs.h:
>         #define DW_VR_MII_EEE_MULT_FACT_100NS           GENMASK(11, 8)
>
> FIELD_PREP on a runtime value does no range check, so 19 (0x13) becomes 3.
> The hardware is then told the 100 ns reference is 4 EEE clock periods
> (20 ns) rather than the 80..120 ns the comment and the kernel-doc of
> xpcs_config_eee_mult_fact() require, which affects all derived LPI timers
> once EEE is enabled.  Since the field caps the count at 16 periods, is 15
> the only representable value for a 5 ns clock, and should this be clamped?
>
> Two more things about the same expression:
>
> - 100 * clk_get_rate() is evaluated in unsigned long, so on 32-bit builds
>   100 * 200000000 wraps to 2820130816 and yields mult_fact 2.  Would
>   mul_u64_u32_div() or a u64 intermediate be better here?
>
> - If clk_get_rate() returns 0, 0 - 1 stored in a u8 becomes 255, giving
>   field value 15.  Is a zero rate worth rejecting?

You're right, and this is a real bug. Two problems:

1. 100 * clk_get_rate() overflows unsigned long on 32-bit builds.
2. DW_VR_MII_EEE_MULT_FACT_100NS is a 4-bit field (GENMASK(11, 8)),
   so the value is capped at 15. For 200 MHz the formula yields 19,
   which FIELD_PREP truncates to 3.

It will be fixed in the next version.

> > +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> > +{
> > +     struct platform_device *pdev;
> > +     struct device_node *pcs_np;
> > +     struct dw_xpcs_rk *pxpcs;
> > +     struct dw_xpcs *xpcs;
> > +     u32 port;
> > +
> > +     if (!of_device_is_available(np))
> > +             return ERR_PTR(-ENODEV);
> > +
> > +     if (of_property_read_u32(np, "reg", &port))
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     if (!xpcs_rk_mdio_addr_validate((int)port))
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     /* The XPCS pdev is attached to the parent node */
> > +     pcs_np = of_get_parent(np);
> > +     if (!pcs_np)
> > +             return ERR_PTR(-ENODEV);
> > +
> > +     if (!of_device_is_available(pcs_np)) {
> > +             of_node_put(pcs_np);
> > +             return ERR_PTR(-ENODEV);
> > +     }
> > +
> > +     if (!of_match_node(xpcs_rk_of_ids, pcs_np)) {
> > +             of_node_put(pcs_np);
> > +             return ERR_PTR(-EINVAL);
> > +     }
> > +
> > +     pdev = of_find_device_by_node(pcs_np);
> > +     of_node_put(pcs_np);
> > +     if (!pdev)
> > +             return ERR_PTR(-EPROBE_DEFER);
> > +
> > +     device_lock(&pdev->dev);
>
> [Severity: Medium]
> xpcs_rk_create() is called from a MAC driver's probe path, which is
> already running under device_lock() of the MAC device taken by
> really_probe().  Taking device_lock(&pdev->dev) here nests a second
> struct device mutex, and since all device mutexes share one lockdep
> class unless annotated, lockdep will report "possible recursive
> locking detected" on the first probe.  That is a splat in the logs on
> any CONFIG_PROVE_LOCKING build, and a panic with panic_on_warn set.
>
> Holding the XPCS device's lock across xpcs_create_mdiodev() and
> device_link_add() is also unusual - device_link_add() does its own
> locking and the driver core normally expects supplier device locks not
> to be held by consumers.
>
> What race is the lock actually protecting against?  pxpcs and
> pxpcs->bus are published by xpcs_rk_probe() via dev_set_drvdata() and
> xpcs_rk_init_bus(), and the !pxpcs || !pxpcs->bus case already returns
> -EPROBE_DEFER, so the consumer retries later; the device link then
> keeps the supplier around.  Can the device_lock()/device_unlock() pairs
> simply be dropped, or is something else needed here (e.g. checking that
> the supplier is bound) to close the window you had in mind?

Agreed. I'll drop device_lock() and establish the device link first,
then read platform_get_drvdata(). DL_FLAG_MANAGED keeps the supplier
bound while the consumer is, so no extra locking is needed. Failure
paths will call device_link_remove() explicitly, since
DL_FLAG_AUTOREMOVE_CONSUMER only removes the link when the consumer
device is released.

> [Severity: High]
> This block in xpcs_rk_mdio_write_remapping() ignores addr entirely:
> any write to DW_VR_MII_AN_CTRL, DW_VR_MII_AN_INTR_STS,
> DW_VR_MII_EEE_MCTRL0, DW_VR_MII_EEE_MCTRL1 or DW_VR_MII_DIG_CTRL2 is
> routed to ROCKCHIP_MMD_MII, which per the commit message is port 0's
> MMD (7), regardless of which of the four ports the MDIO address
> selects.
>
> So when the core configures a PCS instance created with addr 1, 2 or 3
> - xpcs_config_aneg_c37_sgmii() writing DW_VR_MII_AN_CTRL and
> DW_VR_MII_DIG_CTRL2, xpcs_config_eee() writing DW_VR_MII_EEE_MCTRL0/1,
> and the interrupt-status clear on DW_VR_MII_AN_INTR_STS - the write
> lands on port 0 instead.  That has two effects: the intended port is
> never configured (so its link stays broken), and port 0's
> auto-negotiation/EEE state is silently clobbered by an unrelated
> interface, including its pending AN interrupt status.
>
> The read path is not symmetric either: xpcs_rk_mdio_read_remapping()
> remaps per addr with the comment "read remapping to MII is performed by
> HW", so a write to these registers on port 2 followed by a read back
> would not return what was written.  Is that asymmetry deliberate?
>
> Does the TRM say these vendor registers physically exist only in MMD 7
> and are shared by all four MII ports?  If so, redirecting writes
> silently still leaves per-port configuration impossible, and the driver
> probably needs to say so explicitly (reject the access, or document how
> the shared block is meant to be programmed) rather than aliasing ports
> 1..3 onto port 0.  If instead each port has its own copy of these
> registers, this switch should be dropped so the addr-based mapping
> below applies.  Which is it?

ROCKCHIP_MMD_MII is a management port. The AN_CTRL / AN_INTR_STS /
EEE_MCTRL0/1 / DIG_CTRL2 bits are read-only mirrors on ports 1-3, so
writes can only land in MMD 7. The read path is redirected by hardware
to the port's mirror, which is why the write path is asymmetric.

On RK3568 there is only one SGMII link, so only one port is active at
a time and the aliasing is harmless. QSGMII is not supported by this
driver.

Re: [PATCH net-next v5 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568

[Coia Prant]

<netdev-bot+sashiko@kernel.org> 于2026年9月15日周二 04:24写道：
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 2 (1) · Low: 3
>
> - [Medium] DWMAC_ROCKCHIP gains an unconditional `select
>   PCS_XPCS_ROCKCHIP` without inheriting the selected symbol's…
> - [Medium] For SGMII the driver removes every speed-dependent clock
>   programming path (rk_gmac_probe() sets plat_dat->set_clk_tx_rate =…
> - [Low] rk_get_interfaces() (dwmac-rk.c:1570-1571) now sets
>   PHY_INTERFACE_MODE_SGMII in phylink's supported_interfaces for every…
> - [Low] The RK3568 XPCS SoC node added by the series ('arm64: dts:
>   rockchip: rk3568: add XPCS and fixed-clock nodes') omits properties…
> - [Low] rk_select_pcs() (dwmac-rk.c:1268-1275) ignores its `interface`
>   argument and returns the SGMII XPCS for every interface mode,…
>
> Pre-existing issues:
> - [Medium] DISCARDED (valid but pre-existing and below the reporting
>   threshold for pre-existing defects).
>
> --
>
> net: stmmac: dwmac-rk: add SGMII support for RK3568
>
> Adds a set_to_sgmii()/set_to_rmii() pair for rk3568 to program GRF bit 7,
> wires up pcs_init/pcs_exit/select_pcs so the Rockchip XPCS is used as the
> PCS, and reorders rk_gmac_powerup() so the clocks are enabled before the
> interface-specific configuration.
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
>
> A few questions below.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > index ab3c4cf964239..c6559e749c0ce 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > @@ -170,6 +170,7 @@ config DWMAC_ROCKCHIP
> >       default ARCH_ROCKCHIP
> >       depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
> >       select MFD_SYSCON
> > +     select PCS_XPCS_ROCKCHIP
>
> [Severity: Medium]
> Does this select produce unmet dependency warnings?  PCS_XPCS_ROCKCHIP in
> drivers/net/pcs/Kconfig carries three dependency lines:
>
>   config PCS_XPCS_ROCKCHIP
>         tristate "Rockchip XPCS controller support"
>         default ARCH_ROCKCHIP
>         depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
>         depends on GENERIC_PHY || COMPILE_TEST
>         depends on PM_GENERIC_DOMAINS || COMPILE_TEST
>
> while DWMAC_ROCKCHIP only has "depends on OF && (ARCH_ROCKCHIP ||
> COMPILE_TEST)".  ARCH_ROCKCHIP in arch/arm64/Kconfig.platforms selects PM
> but neither GENERIC_PHY nor PM_GENERIC_DOMAINS, so a config with
> ARCH_ROCKCHIP=y, COMPILE_TEST=n and GENERIC_PHY=n (or PM_GENERIC_DOMAINS=n)
> force-enables PCS_XPCS_ROCKCHIP with its own depends unsatisfied:
>
>   WARNING: unmet direct dependencies detected for PCS_XPCS_ROCKCHIP
>
> pcs-xpcs-rk.o is then built against the phy stubs where devm_phy_get()
> returns ERR_PTR(-ENOSYS), so the probe cannot succeed.
>
> The select is also unconditional, so every Rockchip dwmac build
> (RK3288/RK3328/RK3399/RK3588 ...) force-enables the RK356x-only XPCS
> driver.  Would inheriting the constraints, or making the selection
> conditional, be preferable here?  Note that simply adding the two depends
> lines to DWMAC_ROCKCHIP would turn off Ethernet on existing RK3288/RK3399
> configurations, so the choice probably needs to be explicit.
>
> The Kconfig change is also not mentioned anywhere in the commit message.

You're right. I'll move the dependencies into PCS_XPCS_ROCKCHIP as
selects instead of depends on:

    config PCS_XPCS_ROCKCHIP
        tristate "Rockchip XPCS controller support"
        default ARCH_ROCKCHIP
        depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
        select GENERIC_PHY
        select PM_GENERIC_DOMAINS

This way the symbol pulls in its own dependencies when it is selected
by DWMAC_ROCKCHIP, so the unconditional select in dwmac-rk does not
produce an unmet-dependency warning.

> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > index 8d7042e689261..5d501ed8d1429 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>
> [ ... ]
>
> > @@ -1208,6 +1237,43 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
> > +static struct phylink_pcs *rk_select_pcs(struct stmmac_priv *priv,
> > +                                      phy_interface_t interface)
> > +{
> > +     if (!priv->hw->xpcs)
> > +             return NULL;
> > +
> > +     return xpcs_to_phylink_pcs(priv->hw->xpcs);
> > +}
>
> [Severity: Low]
> Should rk_select_pcs() look at its interface argument?  The mac_select_pcs
> contract in include/linux/phylink.h is to return the PCS for the specified
> interface, or NULL when none is required, but this returns the SGMII XPCS
> for any mode.
>
> On an RK3568 port probed as SGMII, rk_get_interfaces() also advertises the
> RGMII variants and RMII, and xpcs->pcs.supported_interfaces is non-empty
> (filled in by xpcs_get_interfaces()).  When phylink validates one of those
> non-SGMII interfaces, phylink_validate_mac_and_pcs() hits:
>
>         if (!phy_interface_empty(pcs->supported_interfaces) &&
>             !test_bit(state->interface, pcs->supported_interfaces)) {
>                 phylink_err(pl, "MAC returned PCS which does not support %s\n",
>                             phy_modes(state->interface));
>                 return -EINVAL;
>         }
>
> Would returning the XPCS only for PHY_INTERFACE_MODE_SGMII and NULL
> otherwise avoid that error path?

plat_dat->select_pcs is only assigned inside the
if (phy_interface == PHY_INTERFACE_MODE_SGMII) branch of
rk_gmac_probe(). For RGMII/RMII ports the callback is NULL, so
stmmac_mac_select_pcs() returns NULL before reaching rk_select_pcs().

On an SGMII port, phylink_validate() is called with
state->interface = PHY_INTERFACE_MODE_SGMII, so only SGMII is
validated against the PCS. The RGMII/RMII bits in supported_interfaces
are not iterated in this path.

> > @@ -1379,16 +1450,19 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
> >       if (ret)
> >               return ret;
> >
> > +     ret = gmac_clk_enable(bsp_priv, true);
> > +     if (ret)
> > +             return ret;
> > +
> [Severity: Medium]
> This isn't a bug introduced by this patch, but now that gmac_clk_enable()
> is the first resource acquisition in rk_gmac_powerup(), its partial-failure
> path becomes more visible.  Do the bulk clocks leak when the clk_phy enable
> fails?
>
> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:gmac_clk_enable() {
>         ...
>                 ret = clk_bulk_prepare_enable(bsp_priv->num_clks,
>                                               bsp_priv->clks);
>                 if (ret)
>                         return ret;
>
>                 ret = clk_prepare_enable(bsp_priv->clk_phy);
>                 if (ret)
>                         return ret;
>         ...
> }
>
> The second failure returns without calling clk_bulk_disable_unprepare() and
> without setting bsp_priv->clk_enabled, so the later gmac_clk_enable(bsp_priv,
> false) in rk_gmac_powerdown() and at the new clk_disable label is a no-op
> because it is gated on clk_enabled.  This only triggers for integrated-PHY
> setups where clk_phy is non-NULL.

This is pre-existing and unrelated to SGMII.

> > +     if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> > +             goto set_mode;
> > +
> >       ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
> >       if (ret < 0)
> > -             return ret;
> > +             goto clk_disable;
>
> [ ... ]
>
> > @@ -1486,6 +1566,9 @@ static void rk_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
> >
> >       if (rk->supports_rmii)
> >               __set_bit(PHY_INTERFACE_MODE_RMII, interfaces);
> > +
> > +     if (rk->supports_sgmii)
> > +             __set_bit(PHY_INTERFACE_MODE_SGMII, interfaces);
> >  }
> [Severity: Low]
> Is SGMII safe to advertise on a port that was not probed as SGMII?
> rk3568_ops populates set_to_sgmii unconditionally, and rk_gmac_setup()
> derives supports_sgmii from it, so every RK3568 port now advertises SGMII
> in phylink's supported_interfaces (stmmac_phy_setup() copies the
> get_interfaces() output verbatim into config->supported_interfaces).
>
> All the SGMII setup, though, is keyed off the probe-time DT phy-mode:
>
>   - rk_gmac_powerup() writes RK3568_GMAC_MODE_SGMII_QSGMII only in the
>     "case PHY_INTERFACE_MODE_SGMII:" branch, which runs once at init
>   - rk_gmac_probe() installs pcs_init/pcs_exit/select_pcs,
>     default_an_inband and clears set_clk_tx_rate only under
>     "if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII)"
>
> So on a port probed as rgmii*/rmii, if phylink later selects SGMII (SFP
> cage, or a PHY reporting SGMII as its host interface), GRF bit 7 stays
> cleared by rk3568_set_to_rgmii()/rk3568_set_to_rmii(), plat->select_pcs is
> NULL so stmmac_mac_select_pcs() returns NULL and there is no PCS to run
> autonegotiation.  Before this patch SGMII was not in supported_interfaces
> and such a configuration was rejected during validation.  Would gating the
> __set_bit() on the probe-time interface be closer to what the hardware can
> actually do?

phylink_create() receives the DT phy-mode as iface, and
phylink_validate() validates that fixed interface, not every bit in
supported_interfaces. The GRF is programmed from bsp_priv->phy_iface
in rk_gmac_powerup(), which is also the DT phy-mode. Advertising SGMII
in supported_interfaces does not change the interface mode at runtime.

> > @@ -1602,6 +1685,17 @@ static int rk_gmac_probe(struct platform_device *pdev)
> >       plat_dat->suspend = rk_gmac_suspend;
> >       plat_dat->resume = rk_gmac_resume;
> >
> > +     if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> > +             /* SGMII clock always runs at 125 MHz */
> > +             plat_dat->set_clk_tx_rate = NULL;
> > +
> > +             /* SGMII requires a PCS */
> > +             plat_dat->default_an_inband = true;
> [Severity: Medium]
> The commit message says:
>
>   "Enable default_an_inband for SGMII and disable the generic stmmac
>    set_clk_tx_rate callback.  This forces phylink to use in-band mode,
>    where the PCS is responsible for speed/duplex negotiation."
>
> Is "forces" accurate?  default_an_inband is only a default, and the next
> paragraph of the message says so itself.  In phylink_parse_mode():
>
>         if (pl->config->default_an_inband)
>                 pl->cfg_link_an_mode = MLO_AN_INBAND;
>
>         dn = fwnode_get_named_child_node(fwnode, "fixed-link");
>         if (dn || fwnode_property_present(fwnode, "fixed-link"))
>                 pl->cfg_link_an_mode = MLO_AN_FIXED;
>
> and xpcs_inband_caps() advertises out-of-band as acceptable:
>
>         case DW_AN_C37_SGMII:
>         case DW_AN_C37_1000BASEX:
>                 return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
>
> so phylink_pcs_neg_mode() can settle on PHYLINK_PCS_NEG_OUTBAND for a
> fixed-link node or a PHY without in-band support.  With set_clk_tx_rate set
> to NULL, rk3568_ops.set_speed = rk_set_clk_mac_speed also becomes
> unreachable, so a 10/100 Mbps link is brought up with the clocking left at
> the 1 Gbps setting and nothing warns or fails.  Would rejecting a
> non-inband neg_mode (for example in mac_config or pcs_validate), or failing
> probe when a fixed-link node is present for SGMII, match what the message
> describes?

The v5 commit message already notes that default_an_inband can be
overridden by a fixed-link node, and that out-of-band SGMII is not
supported by this driver.

> One more observation on the series rather than on this patch: the new
> binding Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> lists phys and phy-names in its unconditional required list, but the
> ethernet-pcs@fda00000 node added to arch/arm64/boot/dts/rockchip/rk3568.dtsi
> supplies only reg, clocks, clock-names and power-domains.  Only
> rk3568-photonicat.dts adds them:
>
>   &xpcs {
>         phys = <&combphy2 PHY_TYPE_SGMII>;
>         phy-names = "serdes";
>         status = "okay";
>   };
>
> [Severity: Low]
> Does dtbs_check report "phys is a required property" for the other RK3568
> board DTBs?  dtschema validates nodes regardless of status = "disabled".
> The neighbouring sata0 node in the same dtsi does carry phys/phy-names at
> SoC level, so would moving them into rk3568.dtsi be an option?

As replied earlier, the SerDes connection is board-specific and cannot
move into the SoC dtsi. dtbs_check skips required-property validation
for status = "disabled" nodes.