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

[PATCH net-next v10 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 v9:

Patch 5 (arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes)
- Move the two fixed-clock nodes (clk_gmac0_xpcs_mii and
  clk_gmac1_xpcs_mii) from between the XPCS node and pipe_phy_grf0 to
  the top of the root node, alongside the other non-reg nodes, and
  sort them alphabetically. This addresses a DT coding style violation
  reported by the Sashiko AI review. No functional change.

Patch 7 (net: pcs: xpcs: add Rockchip RK3568 platform glue driver)
- Add system suspend/resume callbacks and switch from
  DEFINE_RUNTIME_DEV_PM_OPS to _DEFINE_DEV_PM_OPS so that system
  suspend and runtime suspend can use different callbacks.
- Mark the XPCS as part of the wakeup path in system suspend via
  device_set_wakeup_path(). PD_PIPE is shared with SATA and PCIe;
  without this, genpd would power the domain down once every consumer
  is suspended, killing the SerDes and breaking MAC WoL. Runtime PM
  behaviour is unchanged: the runtime callbacks only gate the CSR
  clock, and dev_pm_genpd_rpm_always_on() already keeps PD_PIPE on at
  runtime.
- Clarify the comment above the power domain access: the XPCS lives in
  PD_PIPE, and the domain must be powered before any register access.

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          |  18 +
 .../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    | 130 +++-
 .../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                 | 619 ++++++++++++++++++
 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, 1135 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 v10 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 cannot be
used by phylink for PCS operations. The framework requires select_pcs()
to return the PCS to the core, so the pcs-handle property becomes
effectively useless for link management without the matching callback.
The XPCS is still consulted by stmmac_phylink_setup() and
stmmac_init_phy(), which makes the configuration silently half-working
rather than clearly broken.

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.

Note that this also removes the generic "pcs-handle" parsing from the
common code. A glue that does not set pcs_init() now leaves
priv->hw->xpcs as NULL, and "pcs-handle" becomes a no-op for it. No
in-tree platform relies on this path: every DTS that pairs a dwmac node
with a PCS goes through a glue that sets pcs_init() (Intel, Renesas,
RZ/N1, SoCFPGA, Rockchip).

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 v10 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 v10 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.

Out-of-range values are rejected by dtschema, so the driver does not
duplicate the range check.

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 v10 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.

The four MII ports are described as ethernet-pcs-mii@N child nodes,
consumed by the Rockchip XPCS glue driver later in this series.

phys and phy-names are required because dtbs_check only validates
required properties for enabled nodes. The SerDes link is a board-level
design choice (combphy1 on some boards, combphy2 on others), so these
properties must be provided by the board device tree, not the SoC dtsi.

The CRU reset lines (SRST_XPCS*) are intentionally not described: no
in-tree user requests them, and bring-up relies on the PD_PIPE power
domain, the SerDes PHY and the in-IP soft reset. They can be added
later as optional without breaking ABI.

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 v10 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 (clock-xpcs-gmac0 and clock-xpcs-gmac1,
labelled clk_gmac0_xpcs_mii and clk_gmac1_xpcs_mii) providing the
125 MHz reference clock for the GMACs when operating with XPCS.  Their
clock-output-names match the CRU mux parent names in clk-rk3568.c, so
boards can reparent SCLK_GMAC0_RX_TX / SCLK_GMAC1_RX_TX through
assigned-clock-parents.

The XPCS node and its 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 requires a reference to the appropriate Naneng Combo PHY
via the phys property.  dtbs_check only validates required properties
for enabled nodes, so the SoC dtsi does not provide phys/phy-names:
boards that enable the XPCS must supply them, since the SerDes link is
a board-level design choice (combphy1 on some boards, combphy2 on
others).

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..2cea108b31a4e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
@@ -8,6 +8,20 @@
 / {
 	compatible = "rockchip,rk3568";
 
+	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>;
+	};
+
 	cpu0_opp_table: opp-table-0 {
 		compatible = "operating-points-v2";
 		opp-shared;
@@ -110,6 +124,37 @@ 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";
+		};
+	};
+
 	pipe_phy_grf0: syscon@fdc70000 {
 		compatible = "rockchip,rk3568-pipe-phy-grf", "syscon";
 		reg = <0x0 0xfdc70000 0x0 0x1000>;
-- 
2.47.3

[PATCH net-next v10 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.

The restart cannot go through the .pcs_an_restart op: phylink only
calls it for 802.3z interfaces, and SGMII is not one. Changing hardware
state from pcs_get_state() is already done elsewhere in this driver
(xpcs_get_state_c73() calls xpcs_soft_reset() and xpcs_do_config()),
so the same pattern is used here.

The latch is cleared before issuing the restart, not after: clearing it
afterwards would discard a freshly latched ANCMPLT from the new
negotiation. If an MDIO access fails at this point, it indicates an
unrecoverable hardware condition until reset.

Also clear DW_VR_MII_AN_INTR_STS in xpcs_config_aneg_c37_sgmii() before
starting AN, matching what xpcs_config_aneg_c37_1000basex() already does.
On the non-inband path the function now returns the result of that write
instead of the DIG_CTRL1 modify.

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 v10 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.

The generic XPCS platform glue (pcs-xpcs-plat.o) is split out of the
pcs_xpcs composite object into its own module, gated behind the new
PCS_XPCS_PLATFORM symbol.  The symbol defaults to PCS_XPCS, so existing
configurations keep the snps,dw-xpcs platform glue enabled without any
change.

PCS_XPCS_ROCKCHIP selects GENERIC_PHY and PM_GENERIC_DOMAINS.
ARCH_ROCKCHIP already selects PM, so the dependency of PM_GENERIC_DOMAINS
on PM is satisfied on the target platform.

The EEE multiplier is derived at runtime from the EEE clock rate instead
of being hardcoded, because the clock is muxed between gpll200 (200 MHz)
and cpll125 (125 MHz) and can be changed by the board or firmware.  A
64-bit intermediate avoids overflow on 32-bit builds, and the result is
clamped to the 4-bit DW_VR_MII_EEE_MULT_FACT_100NS field.

Power management
================
The XPCS, its SerDes PHY and the SATA/PCIe controllers share the PD_PIPE
power domain on RK3568.  genpd powers the domain down during system
suspend once every consumer is suspended, which also kills the SerDes
and the XPCS.

Boards that rely on MAC WoL need the PCS and SerDes alive to receive
magic packets, so PD_PIPE must stay powered across system suspend.  Keep
it on by marking the XPCS as part of the wakeup path via
device_set_wakeup_path().  genpd then leaves the domain powered, because
the Rockchip power domain driver sets GENPD_FLAG_ACTIVE_WAKEUP on
PD_PIPE, which makes genpd check the wakeup path of its consumers during
system suspend.

This is unconditional because the XPCS core currently has no platform
callback through which a consumer could convey the MAC WoL state, and
the state itself is dynamic: WoL is toggled at runtime via ethtool, so
a static DT property cannot express it either.  The affected hardware
is limited to RK3568 boards using SGMII, which are always-on routers
where system suspend is not a realistic use case; the extra power draw
is therefore acceptable.

Runtime PM is unaffected: the runtime callbacks only gate the CSR clock,
and dev_pm_genpd_rpm_always_on() already keeps PD_PIPE on at runtime.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 59, CRU_CLKSEL_CON29)
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   | 619 ++++++++++++++++++++++++++++++++
 include/linux/pcs/pcs-xpcs-rk.h |  11 +
 4 files changed, 658 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..5382afcf95748 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)
+	select GENERIC_PHY
+	select PM_GENERIC_DOMAINS if PM
+	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..35ee980a759e5
--- /dev/null
+++ b/drivers/net/pcs/pcs-xpcs-rk.c
@@ -0,0 +1,619 @@
+// 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 <linux/time.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;
+	}
+
+	/*
+	 * Reads are redirected by hardware to the port's read-only mirror;
+	 * only writes have to be targeted at MII (see the write path).
+	 */
+	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;
+	}
+
+	/*
+	 * These registers physically live only in MII (the management port).
+	 * Ports 1-3 expose read-only mirrors of these bits, so writes must
+	 * always target MII; the read path remaps per address and the
+	 * hardware redirects to the port's mirror.
+	 */
+	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;
+	unsigned long rate;
+	u64 mult;
+	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;
+	}
+
+	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;
+	}
+
+	/*
+	 * 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.
+	 *
+	 * Use a 64-bit intermediate: on 32-bit builds, 100 * 200000000
+	 * does not fit in unsigned long. Clamp to the 4-bit
+	 * DW_VR_MII_EEE_MULT_FACT_100NS field. The mux only provides
+	 * 125 MHz or 200 MHz, so the rate cannot drop below the 5 MHz
+	 * threshold where DIV_ROUND_CLOSEST_ULL() would return 0 and the
+	 * subtraction below would underflow.
+	 */
+	rate = clk_get_rate(pxpcs->eee_clk);
+	if (!rate)
+		return dev_err_probe(dev, -EINVAL, "Invalid EEE clock rate\n");
+
+	mult = DIV_ROUND_CLOSEST_ULL(100ULL * rate, NSEC_PER_SEC) - 1;
+	pxpcs->eee_mult_fact = min_t(u64, mult, 15);
+	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 lives in the PD_PIPE power domain. 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 device_link *link;
+	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);
+
+	/*
+	 * Establish the device link before reading the supplier's drvdata.
+	 * device_link_add() does not fail on a supplier that is unbinding:
+	 * it creates the link in DL_STATE_SUPPLIER_UNBIND. Whether the link
+	 * actually protects the drvdata depends on the supplier's state at
+	 * creation time.
+	 *
+	 * Check link->supplier->links.status right after creation. If the
+	 * supplier was DL_DEV_DRIVER_BOUND, the link is in
+	 * DL_STATE_CONSUMER_PROBE and device_links_unbind_consumers() will
+	 * wait for this probe to finish before unbinding the supplier, so
+	 * the drvdata stays valid for the rest of the function. Any other
+	 * state means the supplier is not usable yet; defer and retry.
+	 *
+	 * The link is released automatically when the consumer device is
+	 * destroyed (DL_FLAG_AUTOREMOVE_CONSUMER), so no explicit
+	 * device_link_remove() is needed on the failure paths.
+	 */
+	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
+	if (!link) {
+		put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	if (READ_ONCE(link->supplier->links.status) != DL_DEV_DRIVER_BOUND) {
+		put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	pxpcs = platform_get_drvdata(pdev);
+	if (!pxpcs || !pxpcs->bus) {
+		put_device(&pdev->dev);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
+	if (IS_ERR(xpcs)) {
+		put_device(&pdev->dev);
+		return xpcs;
+	}
+
+	xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);
+	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 int xpcs_rk_system_suspend(struct device *dev)
+{
+	/*
+	 * Keep the PD_PIPE power domain on during system suspend.
+	 *
+	 * PD_PIPE is shared with SATA/PCIe and would be powered down by
+	 * genpd once all its consumers are suspended, killing the SerDes
+	 * and breaking MAC WoL.  Mark the XPCS as part of the wakeup path
+	 * so genpd keeps the domain on.  Unconditional because the XPCS
+	 * core has no callback to convey the MAC WoL state.
+	 */
+	device_set_wakeup_path(dev);
+	return 0;
+}
+
+static int xpcs_rk_system_resume(struct device *dev)
+{
+	return 0;
+}
+
+static _DEFINE_DEV_PM_OPS(xpcs_rk_pm_ops,
+			  xpcs_rk_system_suspend, xpcs_rk_system_resume,
+			  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 v10 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle

[Coia Prant]

The Rockchip GMAC binding needs to describe the PCS reference used by
the SGMII support added later in this series. The property will be
parsed by rk_pcs_init(), and a missing phandle fails the probe. Add it
and require it when phy-mode is "sgmii" on rockchip,rk3568-gmac, the
only SoC in this binding that has SGMII support.

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

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
index 80c252845349c..bb7540e838033 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,18 @@ allOf:
         clocks:
           minItems: 5
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: rockchip,rk3568-gmac
+        phy-mode:
+          contains:
+            const: sgmii
+    then:
+      required:
+        - pcs-handle
+
 unevaluatedProperties: false
 
 examples:
-- 
2.47.3

[PATCH net-next v10 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. SGMII is not added
to rk_get_interfaces(): it comes from the XPCS's own
supported_interfaces, merged by stmmac_phylink_setup().

The SerDes PHY and the PD_PIPE power domain are owned by the XPCS
driver rather than managed through the stmmac
serdes_poweron/serdes_poweroff callbacks, which are legacy and meant
for single-MAC platforms. On RK3568 the XPCS is the natural owner of
the shared SerDes.

DWMAC_ROCKCHIP selects PCS_XPCS_ROCKCHIP. PCS_XPCS itself is already
selected by STMMAC_ETH, and PM is selected by ARCH_ROCKCHIP, so no
further selects are needed.

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    | 130 +++++++++++++++---
 2 files changed, 111 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..88f09014e3a69 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,16 @@ 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 +867,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 +1236,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 +1395,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 +1427,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 +1449,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 +1472,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 +1481,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 +1517,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 +1538,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)
@@ -1602,6 +1681,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 v10 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.  RK3568 has three Combo PHYs
that can carry SGMII; which one is wired to the XPCS is a board-level
choice, and Photonicat uses combphy2.

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 v10 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 cc3cae2e378b3..085589b8ba65e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23740,6 +23740,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 v10 00/11] net: pcs: add basic support for RK3568 XPCS

[Jakub Kicinski]

On Wed, 23 Sep 2026 04:03:24 +0800 Coia Prant wrote:
> 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.

Please slow down with the reposts.
Not more than 2 a week, and please wait for the AI review to finish
and be published before you repost.

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

[Coia Prant]

Jakub Kicinski <kuba@kernel.org> 于2026年9月23日周三 10:50写道：
>
> On Wed, 23 Sep 2026 04:03:24 +0800 Coia Prant wrote:
> > 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.
>
> Please slow down with the reposts.
> Not more than 2 a week, and please wait for the AI review to finish
> and be published before you repost.

Understood, sorry for the noise. I'll keep reposts within the limit and
wait for the AI review to be published before reposting from now on.

Coia

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit message audit claim: "every DTS that pairs a dwmac node
  with a PCS goes through a glue that sets pcs_init() (Intel, Renesas,…
- [Low] Commit message framing: the removal of the unconditional
  `priv->hw->xpcs = xpcs;` in stmmac_pcs_setup() is described as fixing…

--

net: stmmac: move XPCS lifetime management to platform drivers

This moves XPCS creation and destruction out of the common
stmmac_pcs_setup() and stmmac_pcs_clean(), which now only dispatch to
plat->pcs_init() and plat->pcs_exit(). The Intel mGbE glue gains its own
pcs_init()/pcs_exit() pair, and the renesas-gbeth, rzn1 and socfpga
pcs_exit() callbacks clear priv->hw->phylink_pcs after destroying the PCS.

No Link: tags are present in the commit header.

I looked at the two places in stmmac_pcs_setup() that seemed worth double
checking and did not find anything to report.

> 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;
> -	}

Dropping the generic "pcs-handle" and pcs_mask parsing here matches the
in-tree users I could find: the only dwmac nodes carrying pcs-handle go
through dwmac-rzn1 (r9a06g032.dtsi) and dwmac-renesas-gbeth, both of which
set plat->pcs_init(), and dwmac-intel gains pcs_init() in this same patch.

>  
> +	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;
>  }

Removing the trailing priv->hw->xpcs assignment also looks right. None of
the pcs_init() implementations present before this patch touch
priv->hw->xpcs (renesas_gmac_pcs_init(), rzn1_dwmac_pcs_init() and
socfpga_dwmac_pcs_init() all set priv->hw->phylink_pcs), so no existing
platform changes behaviour, and a glue that does set priv->hw->xpcs from
pcs_init() would previously have had it clobbered.

> @@ -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);
>  }

The Intel side pairs this with intel_mgbe_pcs_exit(), which calls
xpcs_destroy() and clears priv->hw->xpcs, so the destroy path is preserved
for the only platform that was relying on the common code.

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

Re: [PATCH net-next v10 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern (verified FALSE POSITIVE, retained for policy): "The
  binding documents `rockchip,sgmii-mac-sel` but no in-tree code reads…

--

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

This adds an optional rockchip,sgmii-mac-sel property to the Rockchip
naneng-combphy binding so a device tree can select whether the RK3568
SGMII interface is routed to GMAC0 or GMAC1. The property is a uint32
limited to 0 or 1, defaulting to 1, and is only allowed for the RK3568
compatible.

> 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]
This isn't a bug, just a note on ordering within the series.

At this commit alone, nothing in tree reads rockchip,sgmii-mac-sel, so a
device tree setting it to 0 would validate without any effect on the
pipe GRF mux bit.

Checking the rest of the series, the consumer does land immediately
afterwards: rockchip_combphy_parse_dt() in
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c gains

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

and the PHY_TYPE_SGMII case of rk3568_combphy_cfg() programs the bit
described here:

		rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
					     priv->sgmii_mac_sel > 0);

with the matching descriptor added to rk3568_combphy_grfcfgs, so the
behaviour is complete at the end of the series and no change is needed
here.

> [ ... ]

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

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

[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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] DT/binding mismatch: the new `xpcs: ethernet-pcs@fda00000`
  node in arch/arm64/boot/dts/rockchip/rk3568.dtsi omits `phys` and…
- [Medium] The two new nodes `clk_gmac0_xpcs_mii: clock-xpcs-gmac0` and
  `clk_gmac1_xpcs_mii: clock-xpcs-gmac1`…
- [Low] Concern (retained per experiment protocol, judged INVALID): the
  claim that nothing in the tree consumes the DT description added here…

--

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

This adds an ethernet-pcs node at 0xfda00000 for the RK3568 XPCS, with four
ethernet-pcs-mii child nodes, all disabled by default.  It also adds two
125 MHz fixed-clock nodes whose clock-output-names are chosen to match the
CRU gmac RX/TX mux parent name strings so boards can reparent
SCLK_GMAC{0,1}_RX_TX.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> index 3bc653f027f1f..2cea108b31a4e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> @@ -8,6 +8,20 @@
>  / {
>  	compatible = "rockchip,rk3568";
>  
> +	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>;
> +	};

[Severity: Medium]

Is a root-level fixed-clock the right description for this clock?  As the
commit message says, the 125 MHz MII clock is generated inside the XPCS
from the Naneng combo SerDes, so it only exists once PD_PIPE is powered,
PCLK_XPCS is ungated and the SerDes has locked.  The CRU has
SRST_XPCS_TX_DIV10 / SRST_XPCS_RX_DIV10 / SRST_XPCS_XGXS_RX for that
divide-by-10 path, and pcs-xpcs-rk.c later in the series notes:

    /*
     * Accessing the XPCS registers also requires a TX clock from the
     * SerDes, which is needed for the soft reset.
     */

These two nodes carry no status, no gate, no power-domains and no relation
to the xpcs node that produces the clock.

Does that hide the dependency from the clock framework?  A board doing

    assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
    assigned-clock-parents = <&clk_gmac0_xpcs_mii>;

is reparented by of_clk_set_defaults() before the MAC driver's probe body
runs, and the reparent plus clk_get_rate() always succeed with 125 MHz even
when the xpcs node is disabled, PD_PIPE is off, or the SerDes PLL is not
locked.  Instead of -EPROBE_DEFER or an error, wouldn't the MAC just run
against a dead MII clock?  The phantom clock also stays "enabled" across
PD_PIPE power-down.

The other half of this is the coupling mechanism.  The only thing tying
these nodes to the CRU is the global clock name, matched by strcmp in
clk_fetch_parent_index() against the hard-coded strings in
drivers/clk/rockchip/clk-rk3568.c:

    PNAME(mux_gmac0_rx_tx_p)	= { "clk_gmac0_rgmii_speed", "clk_gmac0_rmii_speed", "clk_gmac0_xpcs_mii" };
    PNAME(mux_gmac1_rx_tx_p)	= { "clk_gmac1_rgmii_speed", "clk_gmac1_rmii_speed", "clk_gmac1_xpcs_mii" };

If the RK3568 CRU is later converted to parent_data/fw_name, or those
strings are renamed, board reparenting breaks silently with no build or
schema error.  Would it be preferable to have the real provider export this
clock, either the CRU leg or the xpcs node itself as a clock provider, so
the PD_PIPE / PCLK_XPCS / SerDes dependency is expressible?

[ ... ]

> @@ -110,6 +124,37 @@ 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 match the binding added earlier in the same series?
Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml lists
phys and phy-names unconditionally:

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

and its example supplies:

    phys = <&combphy2 PHY_TYPE_SGMII>;
    phy-names = "serdes";

There is no if/then in that schema exempting disabled nodes, so wouldn't
make dtbs_check report "'phys' is a required property" and "'phy-names' is
a required property" for every rk3566/rk3568 DTB that includes
rk3568.dtsi?  At the end of the series only rk3568-photonicat.dts adds them
via a &xpcs override.

The commit message justification is:

    dtbs_check only validates required properties for enabled nodes, so the
    SoC dtsi does not provide phys/phy-names

Is that accurate?  dt-validate selects schemas by compatible and validates
the nodes present in the DTB; status = "disabled" controls whether the OS
binds a driver, not schema conformance.  The same file appears to
contradict the assumption, since the disabled sata0 node right above this
hunk does carry its combphy reference:

    sata0: sata@fc000000 {
        ...
        phys = <&combphy0 PHY_TYPE_SATA>;
        phy-names = "sata-phy";
        ...
        status = "disabled";
    };

The disabled pcie3x1/pcie3x2 nodes and the disabled USB/SATA nodes in
rk356x-base.dtsi do the same.

Separately from tooling, is the node a complete hardware description
without the SerDes phandle?  xpcs_rk_serdes_phy_init() in
drivers/net/pcs/pcs-xpcs-rk.c does:

    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");

so a board that only flips status to "okay" gets a probe failure.  Would it
make sense to settle this on one side, either dropping phys/phy-names from
the binding's required list, or giving the dtsi node a default SerDes
phandle that boards override?

[Severity: Low]

One note that turned out to be a non-issue, recorded for completeness: at
this commit alone nothing in the tree matches rockchip,rk3568-xpcs, consumes
the "eee" clock-name, or parses the ethernet-pcs-mii@N children.  That is
resolved by later patches in the same series, so there is nothing to change
here.  At the series tip, drivers/net/pcs/pcs-xpcs-rk.c has:

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

xpcs_rk_init_clk() fetches both "csr" and "eee" and derives the EEE
multiplier from clk_get_rate() on the eee clock, xpcs_rk_create() reads the
ethernet-pcs-mii@N child's reg as the port number, and
rk3568-photonicat.dts wires pcs-handle = <&xpcs_mii0> into gmac0.

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

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In xpcs_get_state_c37_sgmii() (drivers/net/pcs/pcs-
  xpcs.c:1110-1121) the ANCMPLT latch is cleared first…
- [Medium] xpcs_get_state_c37_sgmii() now mutates PCS hardware state
  (write-0-to-clear of DW_VR_MII_AN_INTR_STS and the BMCR_ANRESTART…

--

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

This adds BMCR_ANRESTART to the C37 SGMII paths: xpcs_config_aneg_c37_sgmii()
now clears DW_VR_MII_AN_INTR_STS and sets ANRESTART together with ANENABLE,
and xpcs_get_state_c37_sgmii() clears the AN latch and issues an AN restart
when the link is down but AN completion was latched.

> 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

[ ... ]

> @@ -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: Medium]
Can this ordering leave the restart permanently disarmed if the xpcs_modify()
fails?

The latched ANCMPLT bit is the only piece of hardware state this code uses to
decide that a restart is needed, and it is destroyed before the restart is
actually requested.  xpcs_modify() is a real read-modify-write:

xpcs_modify() -> mdiodev_c45_modify() -> mdiobus_c45_modify() ->
__mdiobus_c45_modify_changed()

and that last function returns the read error before issuing any write:

drivers/net/phy/mdio_bus.c:__mdiobus_c45_modify_changed() {
	...
	ret = __mdiobus_c45_read(bus, addr, devad, regnum);
	if (ret < 0)
		return ret;
	...
}

So on a transient register read error the PCS stays AN-complete and link-down
with no negotiation started, while the latch that would have re-triggered this
branch has already been cleared.  Every later poll then sees an_complete == 0
and never re-enters the branch, which is the "link stays down forever" case the
commit message describes.

The commit message says:

	"If an MDIO access fails at this point, it indicates an
	 unrecoverable hardware condition until reset."

Where does that reset happen?  .pcs_get_state is void, and the caller only
logs the value:

drivers/net/pcs/pcs-xpcs.c:xpcs_get_state() {
	...
	case DW_AN_C37_SGMII:
		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));
		break;
	...
}

Nothing sets xpcs->need_reset, resets the PCS or re-arms the restart, so
recovery seems to require an administrative down/up to re-run
xpcs_config_aneg_c37_sgmii().  Would it be safer to issue the ANRESTART first
and only clear the latch once the restart write succeeded, or to set
xpcs->need_reset on failure?

[Severity: Medium]
What serializes this new register write against a concurrent
.pcs_get_state call?

phylink reaches this callback from two paths that share no lock:

drivers/net/phy/phylink.c:phylink_resolve() {
	mutex_lock(&pl->state_mutex);
	...
		phylink_mac_pcs_get_state(pl, &link_state);
	...
}

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

phylink_mii_read() via phylink_mii_ioctl() is in the same ASSERT_RTNL()-only
category.  struct dw_xpcs in drivers/net/pcs/pcs-xpcs.h has no lock of its own,
and the MDIO bus mutex only makes each individual transaction atomic, not the
read of DW_VR_MII_AN_INTR_STS followed by the clear and the ANRESTART.

So can an unprivileged ETHTOOL_GLINKSETTINGS query (or SIOCGMIIREG) racing with
phylink_resolve() clear a latch belonging to a negotiation the other caller just
started, and then issue a second BMCR_ANRESTART that aborts that in-flight
negotiation?

Even without a race, the read of DW_VR_MII_AN_INTR_STS and the subsequent blind
write of 0 are not atomic with respect to the hardware latch, so is an AN
completion latched between those two accesses silently dropped?

Before this patch the callback only cleared the latch, so the state-changing
write is new here.

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

Re: [PATCH net-next v10 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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In drivers/net/pcs/pcs-xpcs-rk.c, xpcs_rk_system_suspend()
  relies solely on device_set_wakeup_path() to keep the PD_PIPE power…
- [Medium] xpcs_rk_mdio_write_remapping() (drivers/net/pcs/pcs-xpcs-
  rk.c:137-150) forces writes of DW_VR_MII_AN_CTRL,…
- [Medium] xpcs_rk_create() (drivers/net/pcs/pcs-xpcs-rk.c:537-546)
  dereferences the struct device_link * returned by device_link_add()…
- [Medium] Concern: xpcs_rk_create() validates the supplier with an
  unlocked read of the driver-core-internal field…
- [Low] Concern: xpcs_rk_create() is EXPORT_SYMBOL_GPL'ed and published
  in a new header include/linux/pcs/pcs-xpcs-rk.h with no in-tree caller…
- [Low] In xpcs_rk_create() (drivers/net/pcs/pcs-xpcs-rk.c:518-558) the
  comment justifying the device_link handling misstates the API…

--

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

This adds a Rockchip RK3568 glue driver for the DesignWare XPCS that
exposes the APB3-mapped CSRs through a virtual MDIO bus, splits the
generic platform glue into its own PCS_XPCS_PLATFORM module, and adds
power-management handling for the shared PD_PIPE power domain.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 59, CRU_CLKSEL_CON29)
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/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..35ee980a759e5
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
> @@ -0,0 +1,619 @@

[ ... ]

> +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;
> +	}
> +
> +	/*
> +	 * Reads are redirected by hardware to the port's read-only mirror;
> +	 * only writes have to be targeted at MII (see the write path).
> +	 */
> +	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;
> +	}
> +
> +	/*
> +	 * These registers physically live only in MII (the management port).
> +	 * Ports 1-3 expose read-only mirrors of these bits, so writes must
> +	 * always target MII; the read path remaps per address and the
> +	 * hardware redirects to the port's mirror.
> +	 */
> +	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;
> +	}

[Severity: Medium]
Reads and writes of the same (addr, reg) pair resolve to different MMDs for
addresses 1..3.  xpcs_rk_mdio_write_remapping() sends AN_CTRL, AN_INTR_STS,
EEE_MCTRL0/1 and DIG_CTRL2 to ROCKCHIP_MMD_MII before the per-address switch
is reached, while xpcs_rk_mdio_read_remapping() returns ROCKCHIP_MMD_MII1/2/3
for those same registers.  Since xpcs_rk_addr_format() folds the MMD into the
APB CSR index, the two accesses land on different physical offsets.

The unmodified core does read-modify-write on exactly these registers, e.g.
in xpcs_config_aneg_c37_sgmii():

	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, mask, val);
	...
	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);

and in xpcs_get_state_c37_sgmii():

	an_intr = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
	if (an_intr & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
		an_intr &= ~DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, an_intr);

For port 1..3, is the value read from the port mirror then written back into
port 0's register, and is the autoneg-complete latch that was observed a
different one from the latch that gets acknowledged?

Two related questions on the same mapping:

AN_CTRL (PCS mode / TX_CONFIG) and the EEE registers become one shared
instance for all four ports with no serialization, yet the commit message
says ports 0..3 can be routed to GMAC0 or GMAC1 independently.  How is
cross-port clobbering avoided when two MACs configure their PCS?

DW_VR_MII_DIG_CTRL1, which holds VR_RST plus 2G5_EN/MAC_AUTO_SW and is
modified by the core in xpcs_config_aneg_c37_sgmii() and
xpcs_config_2500basex(), is not in the list and therefore goes to the
per-port MMD.  Does that contradict the comment's claim that such vendor
registers "physically live only in MII"?

[ ... ]

> +	/*
> +	 * Establish the device link before reading the supplier's drvdata.
> +	 * device_link_add() does not fail on a supplier that is unbinding:
> +	 * it creates the link in DL_STATE_SUPPLIER_UNBIND. Whether the link
> +	 * actually protects the drvdata depends on the supplier's state at
> +	 * creation time.
> +	 *
> +	 * Check link->supplier->links.status right after creation. If the
> +	 * supplier was DL_DEV_DRIVER_BOUND, the link is in
> +	 * DL_STATE_CONSUMER_PROBE and device_links_unbind_consumers() will
> +	 * wait for this probe to finish before unbinding the supplier, so
> +	 * the drvdata stays valid for the rest of the function. Any other
> +	 * state means the supplier is not usable yet; defer and retry.
> +	 *
> +	 * The link is released automatically when the consumer device is
> +	 * destroyed (DL_FLAG_AUTOREMOVE_CONSUMER), so no explicit
> +	 * device_link_remove() is needed on the failure paths.
> +	 */

[Severity: Low]
Two details in this comment in xpcs_rk_create() look inaccurate.

include/linux/device.h describes the flag as:

	/* Remove the link automatically on consumer driver unbind. */

so is "released automatically when the consumer device is destroyed" the
right wording?  The core drops such links from __device_links_no_driver() on
consumer probe failure or driver unbind, not at device destruction.

The claim that a DL_DEV_DRIVER_BOUND supplier implies DL_STATE_CONSUMER_PROBE
only holds while the consumer is DL_DEV_PROBING:

drivers/base/core.c:device_link_init_status() {
	case DL_DEV_DRIVER_BOUND:
		switch (consumer->links.status) {
		case DL_DEV_PROBING:
			link->status = DL_STATE_CONSUMER_PROBE;
	...
}

An already-bound consumer gets DL_STATE_ACTIVE and anything else gets
DL_STATE_AVAILABLE.  Could the comment (or the kernel-doc of the exported
helper) state that xpcs_rk_create() must be called from the consumer's probe?

> +	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +	if (!link) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	if (READ_ONCE(link->supplier->links.status) != DL_DEV_DRIVER_BOUND) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}

[Severity: Medium]
Is it valid to dereference the pointer returned by device_link_add() here?
DL_FLAG_STATELESS is not passed, so this is a managed link, and the
kernel-doc above device_link_add() says:

 * If that flag is not set, however, the caller of this function is handing the
 * management of the link over to the driver core entirely and its return value
 * can only be used to check whether or not the link is present.

No kref is taken for managed links (kref_get() only happens on the stateless
path), so the caller owns no reference on the link object, which the core can
free from device_link_drop_managed() -> kref_put(&link->kref,
__device_link_del) or from device_del() -> device_links_purge().

Since link->supplier is just &pdev->dev, and this function already holds a
reference on pdev from of_find_device_by_node(), would reading
pdev->dev.links.status instead give the same result without touching the
link object?

> +	pxpcs = platform_get_drvdata(pdev);
> +	if (!pxpcs || !pxpcs->bus) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);

[Severity: Medium]
This is a check on a driver-core-internal field (links.status) followed by
use of the supplier's devm-owned data (pxpcs->bus, pxpcs->eee_mult_fact) and
registration of an MDIO device on that bus.

For the in-tree caller the window does look closed: rk_pcs_init() runs from
stmmac_pcs_setup() in __stmmac_dvr_probe(), so the consumer is DL_DEV_PROBING
and the new link is DL_STATE_CONSUMER_PROBE, which makes the supplier wait:

drivers/base/core.c:device_links_unbind_consumers() {
	if (status == DL_STATE_CONSUMER_PROBE) {
		device_links_write_unlock();

		wait_for_device_probe();
		goto start;
	}
}

But xpcs_rk_create() is exported and does not document that precondition.
For a caller outside the consumer's probe the link would be
DL_STATE_AVAILABLE or DL_STATE_ACTIVE, nothing would wait for it, and the
supplier's devres could be released while pxpcs->bus is in use.  Also note
that xpcs_rk_create_data() publishes drvdata before bus and eee_mult_fact
exist:

	pxpcs->pdev = pdev;

	dev_set_drvdata(&pdev->dev, pxpcs);

so the read here relies entirely on the supplier being fully bound.  Would
it be preferable to document the probe-context requirement, or to avoid
inspecting links.status from a driver altogether?

> +EXPORT_SYMBOL_GPL(xpcs_rk_create);

[Severity: Low]
At this commit xpcs_rk_create() has no in-tree caller, and the new header
include/linux/pcs/pcs-xpcs-rk.h declares it without a
!CONFIG_PCS_XPCS_ROCKCHIP stub.  The caller does arrive later in the same
series (rk_pcs_init() in drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
calls it and is installed as plat_dat->pcs_init for
PHY_INTERFACE_MODE_SGMII), and DWMAC_ROCKCHIP does "select
PCS_XPCS_ROCKCHIP", which forces the tristate to at least the selector's
value, so there is no y/m link mismatch either.  Noting it only because the
symbol is unused at this point in the series.

[ ... ]

> +static int xpcs_rk_system_suspend(struct device *dev)
> +{
> +	/*
> +	 * Keep the PD_PIPE power domain on during system suspend.
> +	 *
> +	 * PD_PIPE is shared with SATA/PCIe and would be powered down by
> +	 * genpd once all its consumers are suspended, killing the SerDes
> +	 * and breaking MAC WoL.  Mark the XPCS as part of the wakeup path
> +	 * so genpd keeps the domain on.  Unconditional because the XPCS
> +	 * core has no callback to convey the MAC WoL state.
> +	 */
> +	device_set_wakeup_path(dev);
> +	return 0;
> +}
> +
> +static int xpcs_rk_system_resume(struct device *dev)
> +{
> +	return 0;
> +}

[Severity: High]
Does device_set_wakeup_path() actually keep PD_PIPE powered on RK3568?  The
commit message states:

	genpd then leaves the domain powered, because the Rockchip power
	domain driver sets GENPD_FLAG_ACTIVE_WAKEUP on PD_PIPE, which makes
	genpd check the wakeup path of its consumers during system suspend.

PD_PIPE is registered without active wakeup in
drivers/pmdomain/rockchip/pm-domains.c:

	[RK3568_PD_PIPE]	= DOMAIN_RK3568("pipe", BIT(8), BIT(11), false, false),

The last argument is the wakeup flag, so pd_info->active_wakeup stays false
and GENPD_FLAG_ACTIVE_WAKEUP is never set for this domain.  genpd only
honours the consumer wakeup path when that flag is present:

drivers/pmdomain/core.c:genpd_finish_suspend() {
	if (device_awake_path(dev) && genpd_is_active_wakeup(genpd) &&
	    !device_out_band_wakeup(dev))
		return 0;
}

Without the flag, does genpd fall through to genpd_sync_power_off() and power
PD_PIPE down, which is exactly the situation the probe comment warns about
("The domain must be powered on before any register access, otherwise the SoC
will trigger a synchronous external abort")?

dev_pm_genpd_rpm_always_on() does not appear to cover the system-suspend path
either; genpd_sync_power_off() only consults GENPD_FLAG_ALWAYS_ON:

drivers/pmdomain/core.c:genpd_sync_power_off() {
	if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
		return;
}

If the domain is power-cycled, what restores the hardware on the way back up?
phy_init() and phy_power_on() are called once from xpcs_rk_probe() via
xpcs_rk_serdes_phy_poweron(), xpcs_rk_system_resume() returns 0 without
re-initialising the SerDes or the PCS, and
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c has no dev_pm_ops at all.
Does that leave the combphy and XPCS at power-on-reset defaults (no SerDes
configuration, no SerDes TX clock) after resume while the driver, the xpcs
core and phylink still assume the programmed configuration is live, and MAC
WoL, the stated reason for the mechanism, non-functional?

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In SGMII mode, the RK3568 glue tells phylink it supports
  interface modes that the hardware cannot run and that the glue never…
- [Medium] The commit's explanation of the SGMII in-band vs out-of-band
  problem does not match the code, and the driver does nothing to reject…

Pre-existing issues:
- [Medium] rk_gmac_resume() (dwmac-rk.c:1618-1627) calls
  rk_gmac_powerup() and throws away its return value, then always…
- [Medium] gmac_clk_enable(bsp_priv, true) (dwmac-rk.c:1181-1200) first
  enables the bulk clocks with clk_bulk_prepare_enable() (line 1187).
- [Medium] __stmmac_dvr_probe() calls pm_runtime_get_noresume(device) and
  enables runtime PM (stmmac_main.c:8112-8115) before calling…

--

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

This adds SGMII support for the RK3568 GMACs. It sets GRF bit 7 and
hooks up the Rockchip XPCS through the pcs_init, pcs_exit and select_pcs
callbacks. It also moves the clock enable in rk_gmac_powerup() earlier,
unifies its error unwinding, and for SGMII clears set_clk_tx_rate and
sets default_an_inband.

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

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

[ ... ]

> @@ -1208,6 +1236,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);

[Severity: Medium]
This is a pre-existing issue in the stmmac core, but this patch makes it
much easier to hit. rk_pcs_init() returns -EPROBE_DEFER from
xpcs_rk_create() whenever the XPCS device isn't bound yet. Does each of
those deferrals leak a runtime PM usage count?

__stmmac_dvr_probe() takes the reference before PCS setup:

    pm_runtime_get_noresume(device);
    pm_runtime_set_active(device);
    if (!pm_runtime_enabled(device))
        pm_runtime_enable(device);
    ...
error_pcs_setup:
    stmmac_mdio_unregister(ndev);
error_mdio_register:
    stmmac_napi_del(ndev);

None of the error labels drop that count or disable runtime PM.
pm_runtime_reinit() returns early while runtime PM is enabled, so
usage_count is never reset.

fw_devlink doesn't parse pcs-handle, so the GMAC can probe before the
XPCS. After the probe finally succeeds, the count can't reach zero, so
the MAC would never runtime suspend.

Deferral from stmmac_mdio_register() already hit this path before the
patch, but deferral is now likely on RK3568 SGMII boards.

> +
> +	priv->hw->xpcs = xpcs;
> +	return 0;
> +}
> +

[ ... ]

> +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: Medium]
Does this give phylink interface modes that the RK3568 hardware can't
run?

rk_get_interfaces() still sets RGMII and RMII, because rk3568_ops now
has both set_to_rgmii and set_to_rmii. SGMII comes from the XPCS through
stmmac_phylink_setup():

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

xpcs_rk_create() uses the plain xpcs_create_mdiodev() and never narrows
pcs.supported_interfaces. The set is therefore filled from the generic
synopsys_xpcs_compat[] table: USXGMII, 10GKR, 25GBASER, XLGMII,
50GBASER, LAUI, 100GBASEP, 10GBASER, SGMII, 1000BASEX and 2500BASEX.

The rockchip,rk3568-xpcs.yaml binding in this series says the hardware
supports only 1000BASE-X, SGMII and QSGMII. The dwmac4 MAC caps include
MAC_2500FD, so phylink keeps 2500BASE-X and USXGMII as valid choices.

rk_select_pcs() also ignores its interface argument and returns the XPCS
for every mode. Neither the GRF nor the combphy is reprogrammed when the
interface changes: set_to_sgmii() runs once from the DT phy-mode, and
the combphy is fixed to PHY_TYPE_SGMII.

Now take an SFP cage, or a PHY that switches host interface, such as a
2.5G PHY moving between SGMII and 2500BASE-X. Could phylink pick
2500BASE-X or USXGMII, and xpcs_config() then program a mode the SerDes
and MAC clocking can't carry?

The same set is also copied into lpi_interfaces for EEE.

The in-tree Photonicat board uses a 1G PHY with a fixed SGMII interface,
so it isn't affected.

Could the RK3568 XPCS supported_interfaces be restricted, and
rk_select_pcs() return the PCS only for SGMII?

[ ... ]

> @@ -1379,16 +1449,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 the direct return here
only works if gmac_clk_enable() cleans up after itself. Does it?

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;
    ...
}

If clk_prepare_enable(bsp_priv->clk_phy) fails, the bulk clocks stay
enabled and clk_enabled stays false.

Every later gmac_clk_enable(bsp_priv, false) then returns early on the
clk_enabled check. That includes the new clk_disable label and
rk_gmac_powerdown(). Does this leak the enable counts of the bulk
clocks?

This only applies with an integrated PHY, because clk_phy is NULL
otherwise.

[ ... ]

> @@ -1447,15 +1517,20 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)

[ ... ]

>  	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 +1538,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;
>  }

[Severity: Medium]
This is a pre-existing issue, but rk_gmac_resume() throws away the error
that this path returns:

    if (!device_may_wakeup(dev))
        rk_gmac_powerup(bsp_priv);

    return 0;

On the non-WoL resume path, a failure in rk_phy_powerup() takes the
clk_disable label, gates the GMAC clocks and returns an error. Nothing
sees that error.

stmmac_resume() checks the plat->resume return value, but gets 0 here.
Can it then go on to MDIO and MAC/DMA setup with pclk_mac and aclk_mac
disabled?

[ ... ]

> @@ -1602,6 +1681,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]
Does the commit message's explanation of the in-band vs out-of-band
problem match the code? It says:

    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.

The clk_tx_i comment in include/linux/stmmac.h and
stmmac_set_clk_tx_rate() both give 125/25/2.5 MHz for 1000/100/10.

In stmmac_mac_link_up(), the negotiation mode is passed only to
plat->fix_mac_speed, which dwmac-rk doesn't set. The MAC speed
programming and the now-NULL set_clk_tx_rate call are the same in both
modes. The MAC therefore sees the same fixed 125 MHz clock whether
in-band or out-of-band is used.

The difference between the modes seems to be on the XPCS side: AN in
xpcs_config_aneg_c37_sgmii(), versus the fixed BMCR write in
xpcs_link_up_sgmii_1000basex():

    if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
        return;
    ...
    ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
                     mii_bmcr_encode_fixed(speed, duplex));

The commit message also says that fixed-link nodes, and PHYs without
in-band support, can still end up in out-of-band SGMII. TX then works
but RX fails at 10/100.

xpcs_inband_caps() returns this for SGMII:

    case DW_AN_C37_SGMII:
    case DW_AN_C37_1000BASEX:
        return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;

So phylink will accept out-of-band mode, and the glue neither rejects
nor reports it. Should rk_gmac_probe() refuse, or at least warn about, a
fixed-link or out-of-band SGMII setup, rather than bring the link up
with RX not working?

> +		plat_dat->pcs_init = rk_pcs_init;
> +		plat_dat->pcs_exit = rk_pcs_exit;
> +		plat_dat->select_pcs = rk_select_pcs;
> +	}
> +

[ ... ]

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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message of "arm64: dts: rockchip: rk3568-photonicat:
  enable SGMII LAN port" describes the combphy2, xpcs/xpcs_mii0 and…

--

arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

This commit enables the SGMII LAN port on the Ariaboard Photonicat. It
routes combphy2 to GMAC0, enables the xpcs node, configures gmac0 for
SGMII and adds the YT8521SC PHY on mdio0. It also adds LED nodes to both
the new SGMII PHY and the existing RGMII WAN PHY on mdio1.

> 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

[ ... ]

> @@ -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";
>  };

[Severity: Low]
This isn't a bug, but the commit message describes the combphy2,
xpcs/xpcs_mii0 and mdio0 changes and not this &gmac0 rework. The gmac0
rework is the main functional change in the patch. Could the message
describe it too?

In this hunk, status = "disabled" becomes a full SGMII configuration
that:

  - reparents SCLK_GMAC0_RX_TX to the 125 MHz clk_gmac0_xpcs_mii clock
    through assigned-clock-parents
  - sets managed = "in-band-status". The commit "net: stmmac: dwmac-rk:
    add SGMII support for RK3568" says boards must use it because
    out-of-band SGMII breaks RX at 10/100
  - adds pcs-handle = <&xpcs_mii0>, phy-handle, phy-supply and the
    gmac0_miim pinctrl

The commit message also says:

  "Also add LED configuration for the existing RGMII PHY on mdio1 for
  consistency."

Those LED_FUNCTION_WAN nodes go on rgmii_phy, which drives the separate
WAN port that already works. Could that go in its own patch? As it
stands, reverting or bisecting the SGMII LAN enablement also drops the
WAN LED change.

[ ... ]

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

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

[Coia Prant]

<netdev-bot+sashiko@kernel.org> 于2026年9月25日周五 10:03写道：
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 2 (3) · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] In SGMII mode, the RK3568 glue tells phylink it supports
>   interface modes that the hardware cannot run and that the glue never…
> - [Medium] The commit's explanation of the SGMII in-band vs out-of-band
>   problem does not match the code, and the driver does nothing to reject…
>
> Pre-existing issues:
> - [Medium] rk_gmac_resume() (dwmac-rk.c:1618-1627) calls
>   rk_gmac_powerup() and throws away its return value, then always…
> - [Medium] gmac_clk_enable(bsp_priv, true) (dwmac-rk.c:1181-1200) first
>   enables the bulk clocks with clk_bulk_prepare_enable() (line 1187).
> - [Medium] __stmmac_dvr_probe() calls pm_runtime_get_noresume(device) and
>   enables runtime PM (stmmac_main.c:8112-8115) before calling…
>
> --
>
> net: stmmac: dwmac-rk: add SGMII support for RK3568
>
> This adds SGMII support for the RK3568 GMACs. It sets GRF bit 7 and
> hooks up the Rockchip XPCS through the pcs_init, pcs_exit and select_pcs
> callbacks. It also moves the clock enable in rk_gmac_powerup() earlier,
> unifies its error unwinding, and for SGMII clears set_clk_tx_rate and
> sets default_an_inband.
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > index 8d7042e689261..88f09014e3a69 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>
> [ ... ]
>
> > @@ -1208,6 +1236,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);
>
> [Severity: Medium]
> This is a pre-existing issue in the stmmac core, but this patch makes it
> much easier to hit. rk_pcs_init() returns -EPROBE_DEFER from
> xpcs_rk_create() whenever the XPCS device isn't bound yet. Does each of
> those deferrals leak a runtime PM usage count?
>
> __stmmac_dvr_probe() takes the reference before PCS setup:
>
>     pm_runtime_get_noresume(device);
>     pm_runtime_set_active(device);
>     if (!pm_runtime_enabled(device))
>         pm_runtime_enable(device);
>     ...
> error_pcs_setup:
>     stmmac_mdio_unregister(ndev);
> error_mdio_register:
>     stmmac_napi_del(ndev);
>
> None of the error labels drop that count or disable runtime PM.
> pm_runtime_reinit() returns early while runtime PM is enabled, so
> usage_count is never reset.
>
> fw_devlink doesn't parse pcs-handle, so the GMAC can probe before the
> XPCS. After the probe finally succeeds, the count can't reach zero, so
> the MAC would never runtime suspend.
>
> Deferral from stmmac_mdio_register() already hit this path before the
> patch, but deferral is now likely on RK3568 SGMII boards.

Agreed that this is a real problem, but it's in __stmmac_dvr_probe(),
not in dwmac-rk. The error labels in the core don't drop the count that
pm_runtime_get_noresume() took, and pcs-handle is not a fw_devlink
dependency so the deferral is now easier to hit. That should be fixed
in the stmmac core in a separate series. I'm not going to restructure
the core probe error paths as part of a platform glue patch.

> > +
> > +     priv->hw->xpcs = xpcs;
> > +     return 0;
> > +}
> > +
>
> [ ... ]
>
> > +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: Medium]
> Does this give phylink interface modes that the RK3568 hardware can't
> run?
>
> rk_get_interfaces() still sets RGMII and RMII, because rk3568_ops now
> has both set_to_rgmii and set_to_rmii. SGMII comes from the XPCS through
> stmmac_phylink_setup():
>
>     if (pcs)
>         phy_interface_or(config->supported_interfaces,
>                          config->supported_interfaces,
>                          pcs->supported_interfaces);
>
> xpcs_rk_create() uses the plain xpcs_create_mdiodev() and never narrows
> pcs.supported_interfaces. The set is therefore filled from the generic
> synopsys_xpcs_compat[] table: USXGMII, 10GKR, 25GBASER, XLGMII,
> 50GBASER, LAUI, 100GBASEP, 10GBASER, SGMII, 1000BASEX and 2500BASEX.
>
> The rockchip,rk3568-xpcs.yaml binding in this series says the hardware
> supports only 1000BASE-X, SGMII and QSGMII. The dwmac4 MAC caps include
> MAC_2500FD, so phylink keeps 2500BASE-X and USXGMII as valid choices.
>
> rk_select_pcs() also ignores its interface argument and returns the XPCS
> for every mode. Neither the GRF nor the combphy is reprogrammed when the
> interface changes: set_to_sgmii() runs once from the DT phy-mode, and
> the combphy is fixed to PHY_TYPE_SGMII.
>
> Now take an SFP cage, or a PHY that switches host interface, such as a
> 2.5G PHY moving between SGMII and 2500BASE-X. Could phylink pick
> 2500BASE-X or USXGMII, and xpcs_config() then program a mode the SerDes
> and MAC clocking can't carry?
>
> The same set is also copied into lpi_interfaces for EEE.
>
> The in-tree Photonicat board uses a 1G PHY with a fixed SGMII interface,
> so it isn't affected.
>
> Could the RK3568 XPCS supported_interfaces be restricted, and
> rk_select_pcs() return the PCS only for SGMII?

The glue passes pcs-handle to xpcs_rk_create(), which uses the plain
xpcs_create_mdiodev(). The XPCS core has no platform callback to narrow
supported_interfaces, so the glue cannot filter the list cleanly.

The only way I see to restrict it from the glue is to overwrite
pcs->supported_interfaces (and possibly compat) after
xpcs_create_mdiodev() returns. That reaches into the core's internal
structure and I'm not sure maintainers would accept it, so I've left it
out of this series for now. If there's a preferred way to express
"this platform only supports SGMII" from a glue driver, I'm happy to
use it.

In practice the phy-mode is fixed in DT and the combphy is configured
for PHY_TYPE_SGMII, so phylink will not negotiate into 2500BASE-X or
USXGMII on this platform. The wide set is harmless here.

> > @@ -1379,16 +1449,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 the direct return here
> only works if gmac_clk_enable() cleans up after itself. Does it?
>
> 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;
>     ...
> }
>
> If clk_prepare_enable(bsp_priv->clk_phy) fails, the bulk clocks stay
> enabled and clk_enabled stays false.
>
> Every later gmac_clk_enable(bsp_priv, false) then returns early on the
> clk_enabled check. That includes the new clk_disable label and
> rk_gmac_powerdown(). Does this leak the enable counts of the bulk
> clocks?
>
> This only applies with an integrated PHY, because clk_phy is NULL
> otherwise.

The clk_phy failure path was already fixed by a separate commit.

> >       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 +1538,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;
> >  }
>
> [Severity: Medium]
> This is a pre-existing issue, but rk_gmac_resume() throws away the error
> that this path returns:
>
>     if (!device_may_wakeup(dev))
>         rk_gmac_powerup(bsp_priv);
>
>     return 0;
>
> On the non-WoL resume path, a failure in rk_phy_powerup() takes the
> clk_disable label, gates the GMAC clocks and returns an error. Nothing
> sees that error.
>
> stmmac_resume() checks the plat->resume return value, but gets 0 here.
> Can it then go on to MDIO and MAC/DMA setup with pclk_mac and aclk_mac
> disabled?

Pre-existing. The path that returns an error from rk_gmac_powerup() is
the rk_phy_powerup() failure, which on RK3568 is the external PHY
regulator. It's not related to SGMII or XPCS. I'll leave it for a
separate fix.

> > @@ -1602,6 +1681,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]
> Does the commit message's explanation of the in-band vs out-of-band
> problem match the code? It says:
>
>     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.
>
> The clk_tx_i comment in include/linux/stmmac.h and
> stmmac_set_clk_tx_rate() both give 125/25/2.5 MHz for 1000/100/10.
>
> In stmmac_mac_link_up(), the negotiation mode is passed only to
> plat->fix_mac_speed, which dwmac-rk doesn't set. The MAC speed
> programming and the now-NULL set_clk_tx_rate call are the same in both
> modes. The MAC therefore sees the same fixed 125 MHz clock whether
> in-band or out-of-band is used.
>
> The difference between the modes seems to be on the XPCS side: AN in
> xpcs_config_aneg_c37_sgmii(), versus the fixed BMCR write in
> xpcs_link_up_sgmii_1000basex():
>
>     if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
>         return;
>     ...
>     ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
>                      mii_bmcr_encode_fixed(speed, duplex));
>
> The commit message also says that fixed-link nodes, and PHYs without
> in-band support, can still end up in out-of-band SGMII. TX then works
> but RX fails at 10/100.
>
> xpcs_inband_caps() returns this for SGMII:
>
>     case DW_AN_C37_SGMII:
>     case DW_AN_C37_1000BASEX:
>         return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
>
> So phylink will accept out-of-band mode, and the glue neither rejects
> nor reports it. Should rk_gmac_probe() refuse, or at least warn about, a
> fixed-link or out-of-band SGMII setup, rather than bring the link up
> with RX not working?

The out-of-band SGMII rejection cannot be implemented here: the XPCS
core's xpcs_inband_caps() returns LINK_INBAND_DISABLE |
LINK_INBAND_ENABLE for SGMII, and there is no platform callback for the
glue to narrow that or to veto out-of-band mode. Phylink will therefore
accept out-of-band SGMII and the link will come up with RX broken at
10/100. That needs an XPCS core API, not a dwmac-rk change.

No respin planned for this series.

Coia

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

[Coia Prant]

<netdev-bot+sashiko@kernel.org> 于2026年9月25日周五 10:03写道：
>
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 3 · Low: 2
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] In drivers/net/pcs/pcs-xpcs-rk.c, xpcs_rk_system_suspend()
>   relies solely on device_set_wakeup_path() to keep the PD_PIPE power…
> - [Medium] xpcs_rk_mdio_write_remapping() (drivers/net/pcs/pcs-xpcs-
>   rk.c:137-150) forces writes of DW_VR_MII_AN_CTRL,…
> - [Medium] xpcs_rk_create() (drivers/net/pcs/pcs-xpcs-rk.c:537-546)
>   dereferences the struct device_link * returned by device_link_add()…
> - [Medium] Concern: xpcs_rk_create() validates the supplier with an
>   unlocked read of the driver-core-internal field…
> - [Low] Concern: xpcs_rk_create() is EXPORT_SYMBOL_GPL'ed and published
>   in a new header include/linux/pcs/pcs-xpcs-rk.h with no in-tree caller…
> - [Low] In xpcs_rk_create() (drivers/net/pcs/pcs-xpcs-rk.c:518-558) the
>   comment justifying the device_link handling misstates the API…
>
> --
>
> net: pcs: xpcs: add Rockchip RK3568 platform glue driver
>
> This adds a Rockchip RK3568 glue driver for the DesignWare XPCS that
> exposes the APB3-mapped CSRs through a virtual MDIO bus, splits the
> generic platform glue into its own PCS_XPCS_PLATFORM module, and adds
> power-management handling for the shared PD_PIPE power domain.
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 59, CRU_CLKSEL_CON29)
> 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/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> > new file mode 100644
> > index 0000000000000..35ee980a759e5
> > --- /dev/null
> > +++ b/drivers/net/pcs/pcs-xpcs-rk.c
> > @@ -0,0 +1,619 @@
>
> [ ... ]
>
> > +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;
> > +     }
> > +
> > +     /*
> > +      * Reads are redirected by hardware to the port's read-only mirror;
> > +      * only writes have to be targeted at MII (see the write path).
> > +      */
> > +     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;
> > +     }
> > +
> > +     /*
> > +      * These registers physically live only in MII (the management port).
> > +      * Ports 1-3 expose read-only mirrors of these bits, so writes must
> > +      * always target MII; the read path remaps per address and the
> > +      * hardware redirects to the port's mirror.
> > +      */
> > +     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;
> > +     }
>
> [Severity: Medium]
> Reads and writes of the same (addr, reg) pair resolve to different MMDs for
> addresses 1..3.  xpcs_rk_mdio_write_remapping() sends AN_CTRL, AN_INTR_STS,
> EEE_MCTRL0/1 and DIG_CTRL2 to ROCKCHIP_MMD_MII before the per-address switch
> is reached, while xpcs_rk_mdio_read_remapping() returns ROCKCHIP_MMD_MII1/2/3
> for those same registers.  Since xpcs_rk_addr_format() folds the MMD into the
> APB CSR index, the two accesses land on different physical offsets.
>
> The unmodified core does read-modify-write on exactly these registers, e.g.
> in xpcs_config_aneg_c37_sgmii():
>
>         ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, mask, val);
>         ...
>         ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
>
> and in xpcs_get_state_c37_sgmii():
>
>         an_intr = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
>         if (an_intr & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
>                 an_intr &= ~DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
>                 xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, an_intr);
>
> For port 1..3, is the value read from the port mirror then written back into
> port 0's register, and is the autoneg-complete latch that was observed a
> different one from the latch that gets acknowledged?
>
> Two related questions on the same mapping:
>
> AN_CTRL (PCS mode / TX_CONFIG) and the EEE registers become one shared
> instance for all four ports with no serialization, yet the commit message
> says ports 0..3 can be routed to GMAC0 or GMAC1 independently.  How is
> cross-port clobbering avoided when two MACs configure their PCS?
>
> DW_VR_MII_DIG_CTRL1, which holds VR_RST plus 2G5_EN/MAC_AUTO_SW and is
> modified by the core in xpcs_config_aneg_c37_sgmii() and
> xpcs_config_2500basex(), is not in the list and therefore goes to the
> per-port MMD.  Does that contradict the comment's claim that such vendor
> registers "physically live only in MII"?

For ports 1..3 the vendor registers are read-only mirrors of MII. The
hardware redirects reads to the port's mirror, so reading MII1's
AN_INTR_STS returns the same value as reading MII's, and writing MII
clears the shared instance. The read-modify-write sequence is
therefore equivalent across ports.

On the cross-port clobbering question: ports 0..3 can be routed to
different GMACs, but the vendor registers live in a single shared MII
instance by hardware design. The glue mirrors what the hardware does;
it doesn't add serialization beyond what the core already provides.

DW_VR_MII_DIG_CTRL1 goes to the per-port MMD because the Rockchip
hardware has a per-port copy of that register, unlike the shared
registers in the list. The comment in the write path is about the
specific registers that only exist in MII.

> > +     /*
> > +      * Establish the device link before reading the supplier's drvdata.
> > +      * device_link_add() does not fail on a supplier that is unbinding:
> > +      * it creates the link in DL_STATE_SUPPLIER_UNBIND. Whether the link
> > +      * actually protects the drvdata depends on the supplier's state at
> > +      * creation time.
> > +      *
> > +      * Check link->supplier->links.status right after creation. If the
> > +      * supplier was DL_DEV_DRIVER_BOUND, the link is in
> > +      * DL_STATE_CONSUMER_PROBE and device_links_unbind_consumers() will
> > +      * wait for this probe to finish before unbinding the supplier, so
> > +      * the drvdata stays valid for the rest of the function. Any other
> > +      * state means the supplier is not usable yet; defer and retry.
> > +      *
> > +      * The link is released automatically when the consumer device is
> > +      * destroyed (DL_FLAG_AUTOREMOVE_CONSUMER), so no explicit
> > +      * device_link_remove() is needed on the failure paths.
> > +      */
>
> [Severity: Low]
> Two details in this comment in xpcs_rk_create() look inaccurate.
>
> include/linux/device.h describes the flag as:
>
>         /* Remove the link automatically on consumer driver unbind. */
>
> so is "released automatically when the consumer device is destroyed" the
> right wording?  The core drops such links from __device_links_no_driver() on
> consumer probe failure or driver unbind, not at device destruction.

DL_FLAG_AUTOREMOVE_CONSUMER is dropped on consumer probe failure or
driver unbind. device_del() also purges the link via
device_links_purge(), so the comment isn't wrong, just incomplete.

> The claim that a DL_DEV_DRIVER_BOUND supplier implies DL_STATE_CONSUMER_PROBE
> only holds while the consumer is DL_DEV_PROBING:
>
> drivers/base/core.c:device_link_init_status() {
>         case DL_DEV_DRIVER_BOUND:
>                 switch (consumer->links.status) {
>                 case DL_DEV_PROBING:
>                         link->status = DL_STATE_CONSUMER_PROBE;
>         ...
> }
>
> An already-bound consumer gets DL_STATE_ACTIVE and anything else gets
> DL_STATE_AVAILABLE.  Could the comment (or the kernel-doc of the exported
> helper) state that xpcs_rk_create() must be called from the consumer's probe?

This function is called only during the stmmac probe — as indicated by
subsequent dwmac-rk patches — and has no other users.

> > +     link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > +     if (!link) {
> > +             put_device(&pdev->dev);
> > +             return ERR_PTR(-EPROBE_DEFER);
> > +     }
> > +
> > +     if (READ_ONCE(link->supplier->links.status) != DL_DEV_DRIVER_BOUND) {
> > +             put_device(&pdev->dev);
> > +             return ERR_PTR(-EPROBE_DEFER);
> > +     }
>
> [Severity: Medium]
> Is it valid to dereference the pointer returned by device_link_add() here?
> DL_FLAG_STATELESS is not passed, so this is a managed link, and the
> kernel-doc above device_link_add() says:
>
>  * If that flag is not set, however, the caller of this function is handing the
>  * management of the link over to the driver core entirely and its return value
>  * can only be used to check whether or not the link is present.
>
> No kref is taken for managed links (kref_get() only happens on the stateless
> path), so the caller owns no reference on the link object, which the core can
> free from device_link_drop_managed() -> kref_put(&link->kref,
> __device_link_del) or from device_del() -> device_links_purge().
>
> Since link->supplier is just &pdev->dev, and this function already holds a
> reference on pdev from of_find_device_by_node(), would reading
> pdev->dev.links.status instead give the same result without touching the
> link object?

Agreed in principle: for a managed link the return value is only good
for checking presence, and I hold a reference on pdev from
of_find_device_by_node(), so reading pdev->dev.links.status directly
would give the same value without touching the link object.

That said, this doesn't actually cause a problem today. The link is
created in DL_STATE_CONSUMER_PROBE and the supplier waits for this
probe to finish, so the link object cannot be freed while we're in
xpcs_rk_create(). The dereference is safe under the current caller.

I'll clean this up in a separate patch, together with the same pattern
in mtk-svs, so it's one change rather than two.

> > +     pxpcs = platform_get_drvdata(pdev);
> > +     if (!pxpcs || !pxpcs->bus) {
> > +             put_device(&pdev->dev);
> > +             return ERR_PTR(-EPROBE_DEFER);
> > +     }
> > +
> > +     xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
> [Severity: Medium]
> This is a check on a driver-core-internal field (links.status) followed by
> use of the supplier's devm-owned data (pxpcs->bus, pxpcs->eee_mult_fact) and
> registration of an MDIO device on that bus.
>
> For the in-tree caller the window does look closed: rk_pcs_init() runs from
> stmmac_pcs_setup() in __stmmac_dvr_probe(), so the consumer is DL_DEV_PROBING
> and the new link is DL_STATE_CONSUMER_PROBE, which makes the supplier wait:
>
> drivers/base/core.c:device_links_unbind_consumers() {
>         if (status == DL_STATE_CONSUMER_PROBE) {
>                 device_links_write_unlock();
>
>                 wait_for_device_probe();
>                 goto start;
>         }
> }
>
> But xpcs_rk_create() is exported and does not document that precondition.
> For a caller outside the consumer's probe the link would be
> DL_STATE_AVAILABLE or DL_STATE_ACTIVE, nothing would wait for it, and the
> supplier's devres could be released while pxpcs->bus is in use.  Also note
> that xpcs_rk_create_data() publishes drvdata before bus and eee_mult_fact
> exist:
>
>         pxpcs->pdev = pdev;
>
>         dev_set_drvdata(&pdev->dev, pxpcs);
>
> so the read here relies entirely on the supplier being fully bound.  Would
> it be preferable to document the probe-context requirement, or to avoid
> inspecting links.status from a driver altogether?

Same reasoning. For the in-tree caller the window is closed:
rk_pcs_init() runs from __stmmac_dvr_probe(), the consumer is
DL_DEV_PROBING, and the link is DL_STATE_CONSUMER_PROBE, so the
supplier waits.

> [Severity: Low]
> At this commit xpcs_rk_create() has no in-tree caller, and the new header
> include/linux/pcs/pcs-xpcs-rk.h declares it without a
> !CONFIG_PCS_XPCS_ROCKCHIP stub.  The caller does arrive later in the same
> series (rk_pcs_init() in drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> calls it and is installed as plat_dat->pcs_init for
> PHY_INTERFACE_MODE_SGMII), and DWMAC_ROCKCHIP does "select
> PCS_XPCS_ROCKCHIP", which forces the tristate to at least the selector's
> value, so there is no y/m link mismatch either.  Noting it only because the
> symbol is unused at this point in the series.

The caller arrives in patch 9 of the same series and DWMAC_ROCKCHIP
selects PCS_XPCS_ROCKCHIP, so there's no y/m mismatch. Splitting the
export and header out would just create a forward declaration for one
patch.

> > +static int xpcs_rk_system_suspend(struct device *dev)
> > +{
> > +     /*
> > +      * Keep the PD_PIPE power domain on during system suspend.
> > +      *
> > +      * PD_PIPE is shared with SATA/PCIe and would be powered down by
> > +      * genpd once all its consumers are suspended, killing the SerDes
> > +      * and breaking MAC WoL.  Mark the XPCS as part of the wakeup path
> > +      * so genpd keeps the domain on.  Unconditional because the XPCS
> > +      * core has no callback to convey the MAC WoL state.
> > +      */
> > +     device_set_wakeup_path(dev);
> > +     return 0;
> > +}
> > +
> > +static int xpcs_rk_system_resume(struct device *dev)
> > +{
> > +     return 0;
> > +}
>
> [Severity: High]
> Does device_set_wakeup_path() actually keep PD_PIPE powered on RK3568?  The
> commit message states:
>
>         genpd then leaves the domain powered, because the Rockchip power
>         domain driver sets GENPD_FLAG_ACTIVE_WAKEUP on PD_PIPE, which makes
>         genpd check the wakeup path of its consumers during system suspend.
>
> PD_PIPE is registered without active wakeup in
> drivers/pmdomain/rockchip/pm-domains.c:
>
>         [RK3568_PD_PIPE]        = DOMAIN_RK3568("pipe", BIT(8), BIT(11), false, false),
>
> The last argument is the wakeup flag, so pd_info->active_wakeup stays false
> and GENPD_FLAG_ACTIVE_WAKEUP is never set for this domain.  genpd only
> honours the consumer wakeup path when that flag is present:
>
> drivers/pmdomain/core.c:genpd_finish_suspend() {
>         if (device_awake_path(dev) && genpd_is_active_wakeup(genpd) &&
>             !device_out_band_wakeup(dev))
>                 return 0;
> }
>
> Without the flag, does genpd fall through to genpd_sync_power_off() and power
> PD_PIPE down, which is exactly the situation the probe comment warns about
> ("The domain must be powered on before any register access, otherwise the SoC
> will trigger a synchronous external abort")?
>
> dev_pm_genpd_rpm_always_on() does not appear to cover the system-suspend path
> either; genpd_sync_power_off() only consults GENPD_FLAG_ALWAYS_ON:
>
> drivers/pmdomain/core.c:genpd_sync_power_off() {
>         if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
>                 return;
> }
>
> If the domain is power-cycled, what restores the hardware on the way back up?
> phy_init() and phy_power_on() are called once from xpcs_rk_probe() via
> xpcs_rk_serdes_phy_poweron(), xpcs_rk_system_resume() returns 0 without
> re-initialising the SerDes or the PCS, and
> drivers/phy/rockchip/phy-rockchip-naneng-combphy.c has no dev_pm_ops at all.
> Does that leave the combphy and XPCS at power-on-reset defaults (no SerDes
> configuration, no SerDes TX clock) after resume while the driver, the xpcs
> core and phylink still assume the programmed configuration is live, and MAC
> WoL, the stated reason for the mechanism, non-functional?

You're right. RK3568_PD_PIPE is registered with active_wakeup = false,
so GENPD_FLAG_ACTIVE_WAKEUP is not set and device_set_wakeup_path()
alone does not keep the domain on during system suspend. The commit
message and the comment in xpcs_rk_system_suspend() are wrong about
this.

The separate patch for the Rockchip PD driver to set active_wakeup on
PD_PIPE is here:

https://lore.kernel.org/all/20260925041751.495818-1-coiaprant@gmail.com/

It affects SATA and PCIe as well, which is why it isn't folded into
this PCS glue series. Once it lands, the device_set_wakeup_path() call
here becomes effective.

The runtime path is already covered by dev_pm_genpd_rpm_always_on();
only the system-suspend path was missing.

No respin planned for this series.

Coia