[RFC PATCH 00/10] net-next: add basic support for RK3568 XPCS

[RFC PATCH 00/10] net-next: 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:
- Fixing the XPCS driver's SGMII AN state handling
- Refactoring stmmac PCS lifetime management to allow platform drivers
  full control over PCS creation/destruction
- 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
           (introduces pcs_init/pcs_exit callbacks)

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: improve SGMII AN state handling (fixes link-down recovery)
  Patch 7: implement the Rockchip XPCS platform glue driver
  Patch 8: wire up SGMII support in dwmac-rk
  Patch 10: update MAINTAINERS

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

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.
- I also noticed that the PHY (YT8521) reports 100Mbps/Half in out-band
  tesing while the PCS reports 100Mbps/Full if using in-band.
  This looks like a separate PHY driver bug.
  I will address/report it independently after this series lands
  (or if a maintainer points me to the right list).

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

Questions
=========
1. Patch 6 (SGMII AN state handling) touches generic pcs-xpcs code and
   may affect Wangxun NICs.  I don't have Wangxun hardware to test.
   The original Wangxun-specific path is kept unchanged, so I believe
   there is no regression risk.

2. Would Heiko Stuebner be willing to be listed as a co-maintainer
   for the Rockchip XPCS driver?  I've added myself in MAINTAINERS,
   but having a more experienced Rockchip maintainer on board would
   be ideal.

Related discussion
==================
Previous attempt at SGMII support on RK3568 by others:
https://lore.kernel.org/all/20221129072714.22880-2-amadeus@jmu.edu.cn/

Also related (runtime PM fix for xpcs-plat, sent separately):
https://lore.kernel.org/all/20260704214808.1566710-1-coiaprant@gmail.com/

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

This is my first kernel driver series.  I've spent many nights
debugging the hardware quirks on this board.  I hope this can finally
replace the out-of-tree OEM code with a clean upstream solution.
Any guidance during review is greatly appreciated.

Thanks in advance,
Coia Prant
---
Coia Prant (10):
  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 binding
  arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes
  net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568
  net: pcs: xpcs: add Rockchip RK3568 platform glue driver
  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

 .../bindings/net/pcs/rockchip-dwxpcs.yaml     | 126 +++++
 .../phy/phy-rockchip-naneng-combphy.yaml      |   7 +
 MAINTAINERS                                   |   9 +
 .../boot/dts/rockchip/rk3568-photonicat.dts   |  77 ++-
 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    |  87 ++-
 .../net/ethernet/stmicro/stmmac/dwmac-rzn1.c  |   7 +-
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |   7 +-
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c |  37 +-
 drivers/net/pcs/Kconfig                       |  22 +
 drivers/net/pcs/Makefile                      |   7 +-
 drivers/net/pcs/pcs-xpcs-rk.c                 | 526 ++++++++++++++++++
 drivers/net/pcs/pcs-xpcs.c                    |  31 +-
 .../rockchip/phy-rockchip-naneng-combphy.c    |   8 +
 include/linux/pcs/pcs-xpcs-rk.h               |  11 +
 18 files changed, 1008 insertions(+), 51 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
 create mode 100644 drivers/net/pcs/pcs-xpcs-rk.c
 create mode 100644 include/linux/pcs/pcs-xpcs-rk.h

-- 
2.47.3


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 01/10] net: stmmac: move XPCS lifetime management to platform drivers

[Coia Prant]

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

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

Second, and more critically, when stmmac_pcs_setup() fails to create
an XPCS (either because no pcs-handle is present and no pcs_mask is
configured), it falls through to the else branch and leaves
priv->hw->xpcs as NULL. This will silently override any XPCS that a
platform driver may have already set up during its own initialization,
for example in a pcs_init() callback or during probe. The platform
driver has no way to prevent this override because the common code
runs unconditionally 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 introducing pcs_init() and pcs_exit()
callbacks in plat_stmmacenet_data. These callbacks give platform
drivers full control over when and how the XPCS is created, configured,
and destroyed. The common stmmac_pcs_setup() and stmmac_pcs_clean()
functions are simplified to just call these callbacks, 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.

Existing platform drivers (intel, rzn1, socfpga) are updated to use
the new callbacks by moving their XPCS creation and cleanup logic into
pcs_init() and pcs_exit(). In their pcs_exit() implementations, the
pointer to the destroyed PCS is explicitly set to NULL to avoid
dangling pointer references.

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 | 37 +++-------------
 5 files changed, 61 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index b8d467ba6d72d..081323c32bcc1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -572,13 +572,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 = NULL;
+	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);
 }
 
@@ -702,6 +736,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..d2f77f0c223a7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -426,36 +426,15 @@ 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 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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 02/10] 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), with 0 being
the default.

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          | 7 +++++++
 1 file changed, 7 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..6173192e31ab2 100644
--- a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
+++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
@@ -80,6 +80,13 @@ 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: 0
+    description:
+      Select gmac0 or gmac1 to be used as SGMII controller.
+
   "#phy-cells":
     const: 1
 
-- 
2.47.3


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 03/10] 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.

When the property is set to a non-zero value, GMAC1 is selected;
otherwise GMAC0 remains the default. (HW Reset Value: GMAC1)

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

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

diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index 2b0f152f54709..ff290bc18589a 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 = 0;
+	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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs binding

[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 binding describes:
- Required properties: compatible, reg, clocks, clock-names
- Optional properties: phys, phy-names, power-domains
- pcs-mii sub-nodes for each MII port (reg 0..3)

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

diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
new file mode 100644
index 0000000000000..14fadf67c793a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
@@ -0,0 +1,126 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/rockchip-dwxpcs.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 (pcs-mii@0..3) 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
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  reg:
+    description: |
+      Base address and size of the XPCS register space mapped over the
+      APB3 bus.
+    maxItems: 1
+
+  clocks:
+    description: |
+      Clock sources for the XPCS:
+      - csr: APB3 bus interface clock (clk_csr_i), required for register
+        access.
+      - eee: EEE clock (clk_eee_i), required for Energy Efficient
+        Ethernet (EEE) operation.
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: csr
+      - const: eee
+
+  phys:
+    description: |
+      The phandle of SerDes PHY (Naneng Combo PHY) that provides
+      the serial lanes for 1000BASE-X / SGMII / QSGMII.
+      The SerDes must be powered on and initialised before any XPCS
+      register access.
+    maxItems: 1
+
+  phy-names:
+    const: serdes
+
+  power-domains:
+    description: |
+      Power domain for the XPCS.
+      On RK3568 this is typically the PD_PIPE power domain, which also
+      supplies the SerDes PHY.
+    maxItems: 1
+
+patternProperties:
+  "^pcs-mii@[0-3]$":
+    type: object
+    description: |
+      One of the four MII ports of the XPCS.
+      The port number is specified by the reg property (0..3).
+      The port is linked to an Ethernet MAC controller via the
+      pcs-handle property in the MAC's device tree node.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 3
+        description: |
+          MII port number of PCS.
+
+      status: true
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3568-cru.h>
+    #include <dt-bindings/power/rk3568-power.h>
+
+    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";
+      phys = <&combphy2 PHY_TYPE_SGMII>;
+      phy-names = "serdes";
+      power-domains = <&power RK3568_PD_PIPE>;
+
+      pcs-mii@0 {
+        reg = <0>;
+      };
+    };
-- 
2.47.3


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

[Coia Prant]

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

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

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

All nodes are left disabled by default and must be enabled at the
board level when 1000BASE-X/SGMII/QSGMII is in use.  The XPCS node
also requires a reference to the appropriate Naneng Combo PHY via
the phys property at the board level.

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

diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
index 3bc653f027f1f..989e164c0eb39 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
@@ -110,6 +110,51 @@ sata0: sata@fc000000 {
 		status = "disabled";
 	};
 
+	xpcs: 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: pcs-mii@0 {
+			reg = <0>;
+			status = "disabled";
+		};
+
+		xpcs_mii1: pcs-mii@1 {
+			reg = <1>;
+			status = "disabled";
+		};
+
+		xpcs_mii2: pcs-mii@2 {
+			reg = <2>;
+			status = "disabled";
+		};
+
+		xpcs_mii3: pcs-mii@3 {
+			reg = <3>;
+			status = "disabled";
+		};
+	};
+
+	xpcs_gmac0_clk: xpcs-gmac0-clock {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "clk_gmac0_xpcs_mii";
+		#clock-cells = <0>;
+	};
+
+	xpcs_gmac1_clk: xpcs-gmac1-clock {
+		compatible = "fixed-clock";
+		clock-frequency = <125000000>;
+		clock-output-names = "clk_gmac1_xpcs_mii";
+		#clock-cells = <0>;
+	};
+
 	pipe_phy_grf0: syscon@fdc70000 {
 		compatible = "rockchip,rk3568-pipe-phy-grf", "syscon";
 		reg = <0x0 0xfdc70000 0x0 0x1000>;
-- 
2.47.3


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

[Coia Prant]

Commit 2a22b7ae2fa3 ("net: pcs: xpcs: adapt Wangxun NICs for SGMII mode")
added support for reading CL37_ANCMPLT_INTR and then reading BMCR for
speed/duplex.  This may work on Wangxun hardware but not on RK3568.

On RK3568, reading BMCR returns a fixed value (HW Reset Value or Write
Manual) instead of the negotiated result, so the correct speed/duplex
must be read from CL37_ANSGM_STS.  Also, when the link is down,
CL37_ANCMPLT_INTR stays set and the PCS does not restart AN automatically
when the PHY link returns, so an explicit AN restart via BMCR_ANRESTART
is needed.

Modify xpcs_get_state_c37_sgmii() to check CL37_ANSGM_STS for link
status first.  If the link is up, report the state.  If AN is complete
(CL37_ANCMPLT_INTR set), clear the interrupt and restart AN for
non-Wangxun platforms.  The original Wangxun-specific path is kept
unchanged unless we confirm it's a bug not a feature.

Also clear CL37 AN complete status in xpcs_config_aneg_c37_sgmii()
before starting AN to ensure a clean initial state.

Fixes: 2a22b7ae2fa3 ("net: pcs: xpcs: adapt Wangxun NICs for SGMII mode")
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 drivers/net/pcs/pcs-xpcs.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index e69fa2f0a0e8d..cf370ba247cac 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -816,6 +816,11 @@ 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);
@@ -884,7 +889,7 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
 
 	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);
 		if (ret < 0)
 			return ret;
 	}
@@ -1058,6 +1063,7 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
 
 	/* Reset link_state */
 	state->link = false;
+	state->an_complete = false;
 	state->speed = SPEED_UNKNOWN;
 	state->duplex = DUPLEX_UNKNOWN;
 	state->pause = 0;
@@ -1069,6 +1075,8 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
 	if (ret < 0)
 		return ret;
 
+	state->an_complete = ret & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
+
 	if (ret & DW_VR_MII_C37_ANSGM_SP_LNKSTS) {
 		int speed_value;
 
@@ -1086,7 +1094,24 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
 			state->duplex = DUPLEX_FULL;
 		else
 			state->duplex = DUPLEX_HALF;
-	} else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
+
+		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 (xpcs->info.pma != WX_TXGBE_XPCS_PMA_10G_ID) {
+		/* If the link down, restart Auto-Negotiation */
+		if (state->an_complete)
+			xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
+				    BMCR_ANRESTART);
+
+		return 0;
+	}
+
+	if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
 		int speed, duplex;
 
 		state->link = true;
@@ -1112,7 +1137,7 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
 		else if (duplex & ADVERTISE_1000XHALF)
 			state->duplex = DUPLEX_HALF;
 
-		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+		return 0;
 	}
 
 	return 0;
-- 
2.47.3


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[Coia Prant]

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

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

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

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 drivers/net/pcs/Kconfig         |  22 ++
 drivers/net/pcs/Makefile        |   7 +-
 drivers/net/pcs/pcs-xpcs-rk.c   | 526 ++++++++++++++++++++++++++++++++
 include/linux/pcs/pcs-xpcs-rk.h |  11 +
 4 files changed, 562 insertions(+), 4 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..3286bc93e7026 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -12,6 +12,28 @@ 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"
+	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
+	help
+	  Support for XPCS controller on Rockchip RK356x SoC.
+
+	  If you have a Rockchip SoC with this interface, say Y or M here.
+
+endif # PCS_XPCS
+
 config PCS_LYNX
 	tristate
 	help
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b900..c809b7f942a51 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,10 +1,9 @@
 # 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
-
-obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
+obj-$(CONFIG_PCS_XPCS) += pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.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..ed969a38d544d
--- /dev/null
+++ b/drivers/net/pcs/pcs-xpcs-rk.c
@@ -0,0 +1,526 @@
+// 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/mdio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pcs/pcs-xpcs-rk.h>
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/sizes.h>
+
+#include "pcs-xpcs.h"
+
+struct dw_xpcs_rk {
+	struct platform_device *pdev;
+	struct mii_bus *bus;
+	void __iomem *reg_base;
+	struct phy *serdes_phy;
+	struct clk *csr_clk;
+	struct clk *eee_clk;
+};
+
+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;
+	}
+
+	switch (addr) {
+	case 0:
+		return ROCKCHIP_MMD_MII;
+	case 1:
+		return ROCKCHIP_MMD_MII1;
+	case 2:
+		return ROCKCHIP_MMD_MII2;
+	case 3:
+		return ROCKCHIP_MMD_MII3;
+	default:
+		return -ENODEV;
+	}
+}
+
+static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
+{
+	switch (dev) {
+	case MDIO_MMD_PMAPMD:
+		return ROCKCHIP_MMD_PMAPMD;
+	case MDIO_MMD_VEND2:
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	/* Writable only on MII */
+	switch (reg) {
+	case DW_VR_MII_AN_CTRL:
+	case DW_VR_MII_AN_INTR_STS:
+	case DW_VR_MII_EEE_MCTRL0:
+	case DW_VR_MII_EEE_MCTRL1:
+	case DW_VR_MII_DIG_CTRL2:
+		return ROCKCHIP_MMD_MII;
+	default:
+		break;
+	}
+
+	switch (addr) {
+	case 0:
+		return ROCKCHIP_MMD_MII;
+	case 1:
+		return ROCKCHIP_MMD_MII1;
+	case 2:
+		return ROCKCHIP_MMD_MII2;
+	case 3:
+		return ROCKCHIP_MMD_MII3;
+	default:
+		return -ENODEV;
+	}
+}
+
+static int xpcs_rk_read_c22(struct mii_bus *bus, int addr, int reg)
+{
+	struct dw_xpcs_rk *pxpcs = bus->priv;
+	int dev;
+
+	if (!xpcs_rk_mdio_addr_validate(addr))
+		return -ENODEV;
+
+	dev = xpcs_rk_mdio_read_remapping(addr, MDIO_MMD_VEND2, reg);
+	if (dev < 0)
+		return 0xffff;
+
+	return xpcs_rk_read_reg(pxpcs, dev, reg);
+}
+
+static int xpcs_rk_write_c22(struct mii_bus *bus, int addr, int reg, u16 val)
+{
+	struct dw_xpcs_rk *pxpcs = bus->priv;
+	int dev;
+
+	if (!xpcs_rk_mdio_addr_validate(addr))
+		return -ENODEV;
+
+	dev = xpcs_rk_mdio_write_remapping(addr, MDIO_MMD_VEND2, reg);
+	if (dev < 0)
+		return 0;
+
+	return xpcs_rk_write_reg(pxpcs, dev, reg, val);
+}
+
+static int xpcs_rk_read_c45(struct mii_bus *bus, int addr, int dev, int reg)
+{
+	struct dw_xpcs_rk *pxpcs = bus->priv;
+
+	if (!xpcs_rk_mdio_addr_validate(addr))
+		return -ENODEV;
+
+	dev = xpcs_rk_mdio_read_remapping(addr, dev, reg);
+	if (dev < 0)
+		return 0xffff;
+
+	return xpcs_rk_read_reg(pxpcs, dev, reg);
+}
+
+static int xpcs_rk_write_c45(struct mii_bus *bus, int addr, int dev, int reg, u16 val)
+{
+	struct dw_xpcs_rk *pxpcs = bus->priv;
+
+	if (!xpcs_rk_mdio_addr_validate(addr))
+		return -ENODEV;
+
+	dev = xpcs_rk_mdio_write_remapping(addr, dev, reg);
+	if (dev < 0)
+		return 0;
+
+	return xpcs_rk_write_reg(pxpcs, dev, reg, val);
+}
+
+static struct dw_xpcs_rk *xpcs_rk_create_data(struct platform_device *pdev)
+{
+	struct dw_xpcs_rk *pxpcs;
+
+	pxpcs = devm_kzalloc(&pdev->dev, sizeof(*pxpcs), GFP_KERNEL);
+	if (!pxpcs)
+		return ERR_PTR(-ENOMEM);
+
+	pxpcs->pdev = pdev;
+
+	dev_set_drvdata(&pdev->dev, pxpcs);
+
+	return pxpcs;
+}
+
+static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
+{
+	struct device *dev = &pxpcs->pdev->dev;
+
+	pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
+	if (IS_ERR(pxpcs->serdes_phy))
+		return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
+					"Failed to get SerDes PHY\n");
+
+	return 0;
+}
+
+static void xpcs_rk_serdes_phy_poweroff(void *data)
+{
+	struct dw_xpcs_rk *pxpcs = data;
+	struct device *dev = &pxpcs->pdev->dev;
+
+	phy_power_off(pxpcs->serdes_phy);
+	phy_exit(pxpcs->serdes_phy);
+
+	dev_pm_genpd_rpm_always_on(dev, false);
+}
+
+static int xpcs_rk_serdes_phy_poweron(struct dw_xpcs_rk *pxpcs)
+{
+	struct device *dev = &pxpcs->pdev->dev;
+	int ret;
+
+	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;
+
+	clk_disable_unprepare(pxpcs->eee_clk);
+}
+
+static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
+{
+	struct device *dev = &pxpcs->pdev->dev;
+	int ret;
+
+	pxpcs->csr_clk = devm_clk_get(dev, "csr");
+	if (IS_ERR(pxpcs->csr_clk))
+		return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
+					 "Failed to get CSR clock\n");
+
+	pm_runtime_set_suspended(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret) {
+		dev_err(dev, "Failed to enable runtime-PM\n");
+		return ret;
+	}
+
+	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;
+	}
+
+	ret = devm_add_action_or_reset(dev, xpcs_rk_exit_clk, pxpcs);
+	if (ret) {
+		dev_err(dev, "Failed to register devm for EEE clock: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int xpcs_rk_init_bus(struct dw_xpcs_rk *pxpcs)
+{
+	struct device *dev = &pxpcs->pdev->dev;
+	static atomic_t id = ATOMIC_INIT(-1);
+	struct mii_bus *bus;
+	int ret;
+
+	bus = devm_mdiobus_alloc_size(dev, 0);
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "Rockchip DW XPCS MCI/APB3";
+	bus->read = xpcs_rk_read_c22;
+	bus->write = xpcs_rk_write_c22;
+	bus->read_c45 = xpcs_rk_read_c45;
+	bus->write_c45 = xpcs_rk_write_c45;
+	bus->phy_mask = ~0;
+	bus->parent = dev;
+	bus->priv = pxpcs;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE,
+		 "rockchip_dwxpcs-%x", atomic_inc_return(&id));
+
+	/*
+	 * MDIO-bus here serves as just a back-end engine abstracting out
+	 * the MDIO and MCI/APB3 IO interfaces utilized for the Rockchip DWXPCS CSRs
+	 * access.
+	 */
+	ret = devm_mdiobus_register(dev, bus);
+	if (ret) {
+		dev_err(dev, "Failed to create MDIO bus\n");
+		return ret;
+	}
+
+	pxpcs->bus = bus;
+	return 0;
+}
+
+static int xpcs_rk_probe(struct platform_device *pdev)
+{
+	struct dw_xpcs_rk *pxpcs;
+	int ret;
+
+	pxpcs = xpcs_rk_create_data(pdev);
+	if (IS_ERR(pxpcs))
+		return PTR_ERR(pxpcs);
+
+	/*
+	 * The XPCS may be attached to a power domain (e.g. PD_PIPE). The domain
+	 * must be powered on before any register access, otherwise the SoC will
+	 * trigger a synchronous external abort (SError).
+	 *
+	 * Accessing the XPCS registers also requires a TX clock from the SerDes,
+	 * which is needed for the soft reset.
+	 */
+	ret = xpcs_rk_serdes_phy_init(pxpcs);
+	if (ret)
+		return ret;
+
+	ret = xpcs_rk_serdes_phy_poweron(pxpcs);
+	if (ret)
+		return ret;
+
+	ret = xpcs_rk_init_res(pxpcs);
+	if (ret)
+		return ret;
+
+	ret = xpcs_rk_init_clk(pxpcs);
+	if (ret)
+		return ret;
+
+	ret = xpcs_rk_init_bus(pxpcs);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct device_node *pcs_np;
+	struct dw_xpcs_rk *pxpcs;
+	struct dw_xpcs *xpcs;
+	u32 port;
+
+	if (!of_device_is_available(np))
+		return ERR_PTR(-ENODEV);
+
+	if (of_property_read_u32(np, "reg", &port))
+		return ERR_PTR(-EINVAL);
+
+	if (!xpcs_rk_mdio_addr_validate((int)port))
+		return ERR_PTR(-EINVAL);
+
+	/* The XPCS pdev is attached to the parent node */
+	pcs_np = of_get_parent(np);
+	if (!pcs_np)
+		return ERR_PTR(-ENODEV);
+
+	if (!of_device_is_available(pcs_np)) {
+		of_node_put(pcs_np);
+		return ERR_PTR(-ENODEV);
+	}
+
+	pdev = of_find_device_by_node(pcs_np);
+	of_node_put(pcs_np);
+	if (!pdev)
+		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;
+	}
+
+	device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
+	put_device(&pdev->dev);
+	return xpcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_rk_create);
+
+static int __maybe_unused 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 __maybe_unused 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 const struct dev_pm_ops xpcs_rk_pm_ops = {
+	SET_RUNTIME_PM_OPS(xpcs_rk_pm_runtime_suspend,
+			   xpcs_rk_pm_runtime_resume,
+			   NULL)
+};
+
+static const struct of_device_id xpcs_rk_of_ids[] = {
+	{ .compatible = "rockchip,rk3568-xpcs" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, xpcs_rk_of_ids);
+
+static struct platform_driver xpcs_rk_driver = {
+	.probe = xpcs_rk_probe,
+	.driver = {
+		.name = "rk_xpcs-dwxpcs",
+		.pm = &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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 08/10] 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).  Also add a supports_sgmii flag to indicate SGMII
capability.

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 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.  Without
this, the stmmac core would attempt to change the clock rate on speed
changes, causing TX to work but RX to fail.

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

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..eca482b4b6bfc 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)
@@ -851,18 +857,32 @@ 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_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,
+	.supports_sgmii = true,
 
 	.regs_valid = true,
 	.regs = {
@@ -1208,6 +1228,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 +1387,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 +1419,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,6 +1441,9 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 	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;
@@ -1416,7 +1481,8 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 		}
 	}
 
-	/*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,6 +1513,11 @@ 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");
 	}
@@ -1486,6 +1557,9 @@ static void rk_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
 
 	if (rk->supports_rmii)
 		__set_bit(PHY_INTERFACE_MODE_RMII, interfaces);
+
+	if (rk->supports_sgmii)
+		__set_bit(PHY_INTERFACE_MODE_SGMII, interfaces);
 }
 
 static int rk_set_clk_tx_rate(void *bsp_priv_, struct clk *clk_tx_i,
@@ -1602,6 +1676,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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 09/10] 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.

Enable combphy2 with rockchip,sgmii-mac-sel = <0> to route the SGMII
interface to GMAC0.  Enable the xpcs node and its port 0 sub-node,
referencing combphy2 as the SerDes PHY.

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

Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 .../boot/dts/rockchip/rk3568-photonicat.dts   | 77 ++++++++++++++++++-
 1 file changed, 75 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..91c17b624fd17 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"
@@ -242,6 +243,7 @@ &combphy1 {
 
 &combphy2 {
 	status = "okay";
+	rockchip,sgmii-mac-sel = <0>;
 };
 
 &cpu0 {
@@ -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 = <&xpcs_gmac0_clk>;
+	pcs-handle = <&xpcs_mii0>;
+	managed = "in-band-status";
+	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,39 @@ &i2s0_8ch {
 	status = "okay";
 };
 
+&mdio0 {
+	sgmii_phy: ethernet-phy@3 {
+		compatible = "ethernet-phy-id0000.011a";
+		reg = <0x3>;
+		max-speed = <1000>;
+		eee-broken-10gt;
+		eee-broken-10gkx4;
+		eee-broken-10gkr;
+		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 +394,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 +649,13 @@ &xin32k {
 	pinctrl-names = "default";
 	pinctrl-0 = <&clk32k_out1>;
 };
+
+&xpcs {
+	status = "okay";
+	phys = <&combphy2 PHY_TYPE_SGMII>;
+	phy-names = "serdes";
+};
+
+&xpcs_mii0 {
+	status = "okay";
+};
-- 
2.47.3


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RFC PATCH 10/10] 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 806bd2d80d153..8cba6a0a2dd03 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23463,6 +23463,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-dwxpcs.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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

[Andrew Lunn]

On Wed, Jul 15, 2026 at 03:08:34AM +0800, Coia Prant wrote:
> Commit 2a22b7ae2fa3 ("net: pcs: xpcs: adapt Wangxun NICs for SGMII mode")

You do not appear to Cc: the Wangxun NIC people. It would be good to
have there comments on this change.

> Fixes: 2a22b7ae2fa3 ("net: pcs: xpcs: adapt Wangxun NICs for SGMII mode")
> Signed-off-by: Coia Prant <coiaprant@gmail.com>

Please don't mix fixed and new code. Is this a real fix? Should it be
back ported to stable?

     Andrew

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

[Coia Prant]

Andrew Lunn <andrew@lunn.ch> 于2026年7月15日周三 06:44写道：
>
> On Wed, Jul 15, 2026 at 03:08:34AM +0800, Coia Prant wrote:
> > Commit 2a22b7ae2fa3 ("net: pcs: xpcs: adapt Wangxun NICs for SGMII mode")
>
> You do not appear to Cc: the Wangxun NIC people. It would be good to
> have there comments on this change.

I apologize; the output from get_maintainer.pl is very long. I’ve
heard that having too many recipients can cause the PATCH to be
rejected by the LKML mail server.

I have added Wangxun maintainer (Jiawen Wu <jiawenwu@trustnetic.com>
and Mengyuan Lou <mengyuanlou@net-swift.com>) to the CC list.

> > Fixes: 2a22b7ae2fa3 ("net: pcs: xpcs: adapt Wangxun NICs for SGMII mode")
> > Signed-off-by: Coia Prant <coiaprant@gmail.com>
>
> Please don't mix fixed and new code. Is this a real fix? Should it be
> back ported to stable?

I am not sure if this is a specific characteristic of Wangxun NICs, as
I do not have any available for testing.

The behavior of the Rockchip DW XPCS IP core matches what is described
in the commit message (even though phylink brings the link to down
based on the phydev link status).

This appears to be a bug (at least on Rockchip platforms) or Wangxun
NICs features.

However, I cannot confirm whether Wangxun NICs behave the same way.
Therefore, I have kept their code as is for now.

Could a Wangxun NICs maintainer provide some feedback based on testing?

If we can confirm that this is indeed a bug, I can submit a separate fix.

I would greatly appreciate it.

Thanks.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 01/10] net: stmmac: move XPCS lifetime management to platform drivers

[Maxime Chevallier]

Hi,

+Christian

On 7/14/26 21:08, Coia Prant wrote:
> The current XPCS creation logic in stmmac_pcs_setup() is problematic
> for several reasons.
> 
> First, if a device tree specifies a "pcs-handle" but no select_pcs()
> callback is provided by the platform driver, the created XPCS is never
> used. The phylink framework requires select_pcs() to actually return
> the PCS to the core, so the pcs-handle property becomes effectively
> useless without the matching callback. This is confusing for developers
> who expect that specifying a pcs-handle in their device tree should be
> sufficient to enable the PCS.

I think Christian's work on fwnode PCS would help a lot with that PCS
handling in stmmac:

https://lore.kernel.org/netdev/20260618125752.1223-1-ansuelsmth@gmail.com/

I don't know when Christian plans to iterate, it could be worth using
that new fwnode mechanism here ?

Maxime

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[Maxime Chevallier]

Hi,

On 7/14/26 21:08, Coia Prant wrote:
> The RK3568 SoC integrates a Synopsys DesignWare XPCS that is accessed
> via APB3 memory-mapped registers.
> This driver provides the glue logic to make the XPCS accessible to
> the generic pcs-xpcs core.
> 
> The XPCS block contains four MII ports (0..3), each of which can be
> routed to GMAC0 or GMAC1 via the pcs-handle property in the MAC node.
> The hardware maps these ports to different MMDs:
>   - port 0: MMD 7 (ROCKCHIP_MMD_MII)
>   - port 1: MMD 2 (ROCKCHIP_MMD_MII1)
>   - port 2: MMD 3 (ROCKCHIP_MMD_MII2)
>   - port 3: MMD 4 (ROCKCHIP_MMD_MII3)
> 
> This driver creates a virtual MDIO bus that translates MDIO operations
> to APB3 register accesses, with proper address remapping for each port.
> The generic xpcs driver then creates a phylink_pcs instance on top of
> this bus, allowing the MAC to use the PCS via the standard phylink API.
> 
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)
> Signed-off-by: Coia Prant <coiaprant@gmail.com>

[...]

> +static int xpcs_rk_probe(struct platform_device *pdev)
> +{
> +	struct dw_xpcs_rk *pxpcs;
> +	int ret;
> +
> +	pxpcs = xpcs_rk_create_data(pdev);
> +	if (IS_ERR(pxpcs))
> +		return PTR_ERR(pxpcs);
> +
> +	/*
> +	 * The XPCS may be attached to a power domain (e.g. PD_PIPE). The domain
> +	 * must be powered on before any register access, otherwise the SoC will
> +	 * trigger a synchronous external abort (SError).
> +	 *
> +	 * Accessing the XPCS registers also requires a TX clock from the SerDes,
> +	 * which is needed for the soft reset.
> +	 */
> +	ret = xpcs_rk_serdes_phy_init(pxpcs);
> +	if (ret)
> +		return ret;
> +
> +	ret = xpcs_rk_serdes_phy_poweron(pxpcs);
> +	if (ret)
> +		return ret;
>
There are 2 unusual things here.

The first one is that you manage the serdes phy from the PCS driver, usually it's
the MAC driver doing so. In stmmac, we have the serdes_poweron and serdes_poweroff
callbacks to hook into for Serdes control.

The second thing is that you're setting the serdes ON at probe time, there's no
dynamic control of it. Usually we try to only power this on at admin-up time.

Can you explain the rationale behind controlling the serdes here directly, and not
from the MAC driver ? I don't see any mention of that in the commit log, and I'm not
convinced this is the correct approach.

Maxime

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[Coia Prant]

Maxime Chevallier <maxime.chevallier@bootlin.com> 于2026年7月15日周三 15:43写道：
>
> Hi,
>
> On 7/14/26 21:08, Coia Prant wrote:
> > The RK3568 SoC integrates a Synopsys DesignWare XPCS that is accessed
> > via APB3 memory-mapped registers.
> > This driver provides the glue logic to make the XPCS accessible to
> > the generic pcs-xpcs core.
> >
> > The XPCS block contains four MII ports (0..3), each of which can be
> > routed to GMAC0 or GMAC1 via the pcs-handle property in the MAC node.
> > The hardware maps these ports to different MMDs:
> >   - port 0: MMD 7 (ROCKCHIP_MMD_MII)
> >   - port 1: MMD 2 (ROCKCHIP_MMD_MII1)
> >   - port 2: MMD 3 (ROCKCHIP_MMD_MII2)
> >   - port 3: MMD 4 (ROCKCHIP_MMD_MII3)
> >
> > This driver creates a virtual MDIO bus that translates MDIO operations
> > to APB3 register accesses, with proper address remapping for each port.
> > The generic xpcs driver then creates a phylink_pcs instance on top of
> > this bus, allowing the MAC to use the PCS via the standard phylink API.
> >
> > Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)
> > Signed-off-by: Coia Prant <coiaprant@gmail.com>
>
> [...]
>
> > +static int xpcs_rk_probe(struct platform_device *pdev)
> > +{
> > +     struct dw_xpcs_rk *pxpcs;
> > +     int ret;
> > +
> > +     pxpcs = xpcs_rk_create_data(pdev);
> > +     if (IS_ERR(pxpcs))
> > +             return PTR_ERR(pxpcs);
> > +
> > +     /*
> > +      * The XPCS may be attached to a power domain (e.g. PD_PIPE). The domain
> > +      * must be powered on before any register access, otherwise the SoC will
> > +      * trigger a synchronous external abort (SError).
> > +      *
> > +      * Accessing the XPCS registers also requires a TX clock from the SerDes,
> > +      * which is needed for the soft reset.
> > +      */
> > +     ret = xpcs_rk_serdes_phy_init(pxpcs);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = xpcs_rk_serdes_phy_poweron(pxpcs);
> > +     if (ret)
> > +             return ret;
> >
> There are 2 unusual things here.
>
> The first one is that you manage the serdes phy from the PCS driver, usually it's
> the MAC driver doing so. In stmmac, we have the serdes_poweron and serdes_poweroff
> callbacks to hook into for Serdes control.
>
> The second thing is that you're setting the serdes ON at probe time, there's no
> dynamic control of it. Usually we try to only power this on at admin-up time.
>
> Can you explain the rationale behind controlling the serdes here directly, and not
> from the MAC driver ? I don't see any mention of that in the commit log, and I'm not
> convinced this is the correct approach.
>
> Maxime

Hi Maxime,

Thanks for the review.

This is intentional. The SerDes is attached to the XPCS node because
on RK3568, a single SerDes serves all four XPCS MII ports in QSGMII
mode. If we managed it from dwmac-rk, we would need complex
refcounting across multiple MAC instances. Keeping it at the XPCS
level simplifies the design and prepares for future QSGMII support.

The probe-time power-on is a one-time initialization (mode selection),
not a runtime power toggle. The actual power is controlled by the
PD_PIPE domain via PM runtime. Also, phy_power_on() on this PHY is
effectively a no-op; only phy_init() is required.

I considered the stmmac serdes_poweron/serdes_poweroff callbacks, but
they do not handle shared SerDes well. I am happy to rework if the
community prefers otherwise, but this approach seems cleaner for
multi-port setups.

Thanks,
Coia

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 01/10] net: stmmac: move XPCS lifetime management to platform drivers

[Coia Prant]

Maxime Chevallier <maxime.chevallier@bootlin.com> 于2026年7月15日周三 15:31写道：
>
> Hi,
>
> +Christian
>
> On 7/14/26 21:08, Coia Prant wrote:
> > The current XPCS creation logic in stmmac_pcs_setup() is problematic
> > for several reasons.
> >
> > First, if a device tree specifies a "pcs-handle" but no select_pcs()
> > callback is provided by the platform driver, the created XPCS is never
> > used. The phylink framework requires select_pcs() to actually return
> > the PCS to the core, so the pcs-handle property becomes effectively
> > useless without the matching callback. This is confusing for developers
> > who expect that specifying a pcs-handle in their device tree should be
> > sufficient to enable the PCS.
>
> I think Christian's work on fwnode PCS would help a lot with that PCS
> handling in stmmac:
>
> https://lore.kernel.org/netdev/20260618125752.1223-1-ansuelsmth@gmail.com/
>
> I don't know when Christian plans to iterate, it could be worth using
> that new fwnode mechanism here ?
>
> Maxime

Hi Maxime,

Thanks for pointing me to Christian's work. This looks like a
much-needed improvement.

I actually spent all night debugging call traces caused by the current
stmmac PCS lifetime management, and it was not a pleasant experience.
The code feels like accumulated technical debt that should be cleaned
up.

Regarding timeline: since Christian's series is still in RFC with an
uncertain merge date, I'd prefer to keep this series as-is for now, as
it solves the problem for Rockchip and has already started receiving
review feedback. Once Christian's fwnode PCS work lands in net-next,
I'm happy to rebase and convert the Rockchip glue driver to the new
interface.

One thing I'd really like to see: the ability to specify the logical
MII port instance via something like:

    pcs-handle = <&pcs MII_PortX>;

That would make the DT binding much cleaner and more flexible for
multi-port configurations.

Thanks,
Coia

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 01/10] net: stmmac: move XPCS lifetime management to platform drivers

[Christian Marangi]

On Wed, Jul 15, 2026 at 04:17:50PM +0800, Coia Prant wrote:
> Maxime Chevallier <maxime.chevallier@bootlin.com> 于2026年7月15日周三 15:31写道：
> >
> > Hi,
> >
> > +Christian
> >
> > On 7/14/26 21:08, Coia Prant wrote:
> > > The current XPCS creation logic in stmmac_pcs_setup() is problematic
> > > for several reasons.
> > >
> > > First, if a device tree specifies a "pcs-handle" but no select_pcs()
> > > callback is provided by the platform driver, the created XPCS is never
> > > used. The phylink framework requires select_pcs() to actually return
> > > the PCS to the core, so the pcs-handle property becomes effectively
> > > useless without the matching callback. This is confusing for developers
> > > who expect that specifying a pcs-handle in their device tree should be
> > > sufficient to enable the PCS.
> >
> > I think Christian's work on fwnode PCS would help a lot with that PCS
> > handling in stmmac:
> >
> > https://lore.kernel.org/netdev/20260618125752.1223-1-ansuelsmth@gmail.com/
> >
> > I don't know when Christian plans to iterate, it could be worth using
> > that new fwnode mechanism here ?
> >
> > Maxime
> 
> Hi Maxime,
> 
> Thanks for pointing me to Christian's work. This looks like a
> much-needed improvement.
> 
> I actually spent all night debugging call traces caused by the current
> stmmac PCS lifetime management, and it was not a pleasant experience.
> The code feels like accumulated technical debt that should be cleaned
> up.
>

Yes we also got a similar situation with an ipq50xx SoC where the
standalone PCS feature was implemented (I can add reference to OpenWrt
code) and we also had some ""magic"" code to implement PCS as it does use
the DWMAC plat. It seems for DWMAC PCS is very abstracted and have at least
3 different implementation aside from the common "select_pcs" one.
 
> Regarding timeline: since Christian's series is still in RFC with an
> uncertain merge date, I'd prefer to keep this series as-is for now, as
> it solves the problem for Rockchip and has already started receiving
> review feedback. Once Christian's fwnode PCS work lands in net-next,
> I'm happy to rebase and convert the Rockchip glue driver to the new
> interface.

The series in RFC just because i posted while net-next was closed but it's
not in RFC state. (sashiko is starting to hallucinate problems) I plan to
post v10 today but still low review aside from ""lovely"" bot.

> 
> One thing I'd really like to see: the ability to specify the logical
> MII port instance via something like:
> 
>     pcs-handle = <&pcs MII_PortX>;
> 
> That would make the DT binding much cleaner and more flexible for
> multi-port configurations.

That is exactly one of the main feature of this new implementation as is
already used downstream by Airoha SoC where a PCIe PCS expose 2 PCS from a
single provider. (the code use the simple consumer/provider pattern and the
driver have complete freedom of applying whatever logic is needed when
returning the correct cell)

I think the idea of Maxime is to test that series on most Scenario as
possible to verify for fragility or regression on it.

(but just for Maxime the feature is getting actively used on OpenWrt by 3
different SoC and no complain for now)

-- 
	Ansuel

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 01/10] net: stmmac: move XPCS lifetime management to platform drivers

[Maxime Chevallier]

Hi Christian,

> I think the idea of Maxime is to test that series on most Scenario as
> possible to verify for fragility or regression on it.

I've tested your RFC on a few boards that use the .select_pcs() callback,
and didn't find any regressions :)
> (but just for Maxime the feature is getting actively used on OpenWrt by 3
> different SoC and no complain for now)

I have no doubt that the fwnode PCS registration works, however there's no
user of the .fill_available_pcs() path in this series. You may not have access
to HW to test this though, but this is my main concern that we discover issues
when using "internal" PCS with the new API.

I _may_ have time to play around with is on mvpp2, hard to say when though, but
feel free to send another iteration for review :)

Maxime

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 01/10] net: stmmac: move XPCS lifetime management to platform drivers

[Andrew Lunn]

> I actually spent all night debugging call traces caused by the current
> stmmac PCS lifetime management, and it was not a pleasant experience.
> The code feels like accumulated technical debt that should be cleaned
> up.

Russell King will strongly agree with you, if he was around. The
phylink integration into stmmac causes a lot of pain, and he wanted to
rip it all out and start again. But doing that without breaking stuff
is going to be interesting.

If you feel up to it, have a go. But take a good look around first,
make sure you understand the different variant, internal vs external
PCS.

	Andrew

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[Andrew Lunn]

> This is intentional. The SerDes is attached to the XPCS node because
> on RK3568, a single SerDes serves all four XPCS MII ports in QSGMII
> mode.

This is not the only device supporting QSGMII, and so the issues you
are addressing should be common to many QSGMII implementations.

Please take a step back. What would a generic solution look like?

Can the reference counting be placed into the core somewhere? The
common clock framework allows a clock to be enabled and disabled by
multiple consumers, and the core clock code does the reference
counting, only calling into the clock driver when state change is
actually needed. Maybe the generic PHY core needs something similar?

	Andrew

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Logical contradiction between the property's documented default value and the commit message's explicit configuration requirement.
- [Low] The `rockchip,sgmii-mac-sel` property is defined globally and not conditionally constrained to the RK3568 SoC.
--

--- Patch [2]: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property ---
commit 0d950b4055dd144fecf5059f16535f2c8d94b5d3
Author: Coia Prant <coiaprant@gmail.com>

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

This commit adds the "rockchip,sgmii-mac-sel" property to the Rockchip
Naneng combphy device tree binding. It allows selecting between GMAC0
and GMAC1 for the SGMII interface on the RK3568 SoC.

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

[Severity: Medium]
Is it actually necessary to explicitly configure the property to 0 for
GMAC0? 

The schema defines the default as 0, which implies that omitting the property
would naturally default the hardware to GMAC0. If explicit configuration is
required, does this mean the hardware or bootloader leaves the register at 1,
making the `default: 0` documented in the schema incorrect?

> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> index 379b08bd9e97a..6173192e31ab2 100644
> --- a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
[ ... ]
> @@ -80,6 +80,13 @@ properties:
>      description:
>        Some additional pipe settings are accessed through GRF regs.
>  
> +  rockchip,sgmii-mac-sel:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

[Severity: Low]
Should this property be conditionally restricted to the RK3568 compatible
string? 

Because it is defined in the top-level properties block, it will pass
validation for all other hardware variants (like RK3528 or RK3588) that do
not actually support SGMII. Could an `if-then` block in the `allOf` section
be used to set this property to false for compatibles other than
`rockchip,rk3568-naneng-combphy`?

> +    default: 0
> +    description:
> +      Select gmac0 or gmac1 to be used as SGMII controller.
> +
>    "#phy-cells":
>      const: 1
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch breaks device tree backward compatibility by forcing the SGMII MAC selection to GMAC0 when the optional `rockchip,sgmii-mac-sel` property is absent, overriding the hardware default.
--

--- Patch [3]: [RFC PATCH 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568 ---
commit 605f86936fd231092ab8489f15df507702255912
Author: Coia Prant <coiaprant@gmail.com>

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

This commit adds support for routing the SGMII interface to either GMAC0
or GMAC1 on RK3568 by introducing the rockchip,sgmii-mac-sel DT property.

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

> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c

[ ... ]

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

[ ... ]

> @@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
>  		break;
>  
>  	case PHY_TYPE_SGMII:
> +		rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
> +					priv->sgmii_mac_sel > 0);

[Severity: High]
Does this break device tree backward compatibility?

When the rockchip,sgmii-mac-sel property is missing from an older device
tree, priv->sgmii_mac_sel defaults to 0 in rockchip_combphy_parse_dt().

By unconditionally calling rockchip_combphy_param_write() here, the driver
now actively clears the MAC selection bit to 0 (GMAC0).

Because the hardware reset value is GMAC1, this actively overrides the default
hardware and bootloader state that older device trees were relying on.

Could the code check if the property was actually present before writing
to the register, to ensure the hardware default is preserved for existing
setups?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs binding

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The device tree example uses a 64-bit `reg` format that fails `dt_binding_check` validation due to a mismatch with the default `#address-cells`.
- [Low] The device tree example fails to include the header for `PHY_TYPE_SGMII`, causing a compilation error during `dt_binding_check`.
--

--- Patch [4]: [RFC PATCH 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs binding ---
commit 2d8d4cd3ced5ab201d6610a9b6b870ceb6ceaed5
Author: Coia Prant <coiaprant@gmail.com>

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

This commit adds device tree binding documentation for the Synopsys DesignWare
XPCS integrated on the Rockchip RK3568 SoC, supporting 1000BASE-X, SGMII,
and QSGMII modes with four MII ports.

> diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
> new file mode 100644
> index 0000000000000..14fadf67c793a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/clock/rk3568-cru.h>
> +    #include <dt-bindings/power/rk3568-power.h>
> +
> +    pcs@fda00000 {
> +      compatible = "rockchip,rk3568-xpcs";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      reg = <0x0 0xfda00000 0x0 0x200000>;

[Severity: Low]
Does this 64-bit register format fail dt_binding_check validation?

The example is extracted into a dummy root node with default
#address-cells = <1> and #size-cells = <1>. This means the 4 cells here will
be parsed as two items, which violates the maxItems: 1 constraint defined
in the schema for the reg property.

> +      clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
> +      clock-names = "csr", "eee";
> +      phys = <&combphy2 PHY_TYPE_SGMII>;

[Severity: Low]
Will this fail to compile during dt_binding_check?

The example uses the PHY_TYPE_SGMII macro, but the required
<dt-bindings/phy/phy.h> header isn't included in the example.

> +      phy-names = "serdes";
> +      power-domains = <&power RK3568_PD_PIPE>;
> +
> +      pcs-mii@0 {
> +        reg = <0>;
> +      };
> +    };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=4

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Devicetree anti-pattern: dummy fixed-clock nodes used for internal SoC clocks and left unassigned.
- [Low] State mismatch: fixed-clock nodes are enabled by default despite commit message claims.
--

--- Patch [5]: [RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes ---
commit b47e1fcebd43374d6431fc20ffa64a0397513d68
Author: Coia Prant <coiaprant@gmail.com>

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

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.

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

[Severity: Low]
The commit message states these clocks are used as assigned-clock-parents
for the respective GMAC nodes, but the patch doesn't appear to add the
assigned-clock-parents properties to the gmac0 or gmac1 nodes. Are these
assignments missing?

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> index 3bc653f027f1f..989e164c0eb39 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi

[ ... ]

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

[Severity: Low]
Should this internal SoC clock be modeled as a clock provider from the
generating XPCS IP node using #clock-cells, rather than using a dummy
fixed-clock?

Additionally, the commit message states "All nodes are left disabled by
default", but this node is missing the status = "disabled" property. Will
this leave the clock unconditionally enabled?

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

[Severity: Low]
Similarly, does this node also need a status = "disabled" property to align
with the behavior described in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=5

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The AN complete status is cleared before reading MII_BMCR in the Wangxun-specific path, which can cause the link to stall permanently if a subsequent MDIO read fails.
- [Medium] The patch modifies the 1000BASE-X configuration path to assert BMCR_ANRESTART without any mention or justification in the commit message.
--

--- Patch [6]: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568 ---
commit 8c1976e4f52a5e7d8b4bab114f6cf214fe50a249
Author: Coia Prant <coiaprant@gmail.com>

net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

This patch modifies SGMII AN state handling for Rockchip RK3568 by checking
CL37_ANSGM_STS for link status. It clears the AN complete interrupt and
restarts AN for non-Wangxun platforms if complete, and clears CL37 AN
complete status before starting AN to ensure a clean initial state.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index e69fa2f0a0e8d..cf370ba247cac 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c

[ ... ]

> @@ -884,7 +889,7 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
>  
>  	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);

[Severity: Medium]
The commit message mentions improving SGMII AN state handling, but this
adds BMCR_ANRESTART to the 1000BASE-X configuration path in
xpcs_config_aneg_c37_1000basex(). Was this unintentional, or should the
commit message be updated to mention this behavior change for 1000BASE-X?

>  		if (ret < 0)
>  			return ret;
>  	}

[ ... ]

> @@ -1086,7 +1094,24 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
>  			state->duplex = DUPLEX_FULL;
>  		else
>  			state->duplex = DUPLEX_HALF;
> -	} else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> +
> +		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);

[Severity: High]
Can clearing the interrupt here lead to a permanently stalled link on
Wangxun platforms if the subsequent xpcs_read() fails?

The original code cleared DW_VR_MII_AN_INTR_STS at the very end of the
DW_VR_MII_AN_STS_C37_ANCMPLT_INTR block, after successfully reading
MII_BMCR and MII_ADVERTISE.

By moving the interrupt clear here:

    if (state->an_complete)
        xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);

If a transient MDIO error occurs during the subsequent MII_BMCR read:

    speed = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
    if (speed < 0)
        return speed;

The function will exit with an error and the link will remain down, but the
AN complete status has already been cleared. Will this prevent the Wangxun
initialization block from running on the next poll and leave the link
stuck?

> +
> +	if (xpcs->info.pma != WX_TXGBE_XPCS_PMA_10G_ID) {
> +		/* If the link down, restart Auto-Negotiation */
> +		if (state->an_complete)
> +			xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> +				    BMCR_ANRESTART);
> +
> +		return 0;
> +	}
> +
> +	if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
>  		int speed, duplex;
>  
>  		state->link = true;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=6

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Asymmetric MDIO read/write remapping for VEND2 control registers corrupts port 0 and breaks configuration for ports 1-3.
- [High] Accessing the supplier device's `drvdata` without synchronization before establishing the device link causes a Use-After-Free race condition.
- [Medium] Clock `csr_clk` leaks if the device is removed while its runtime PM state is active.
- [Medium] `xpcs_rk_create` extracts driver data from an arbitrary device node without validating its compatible string or bound driver, leading to type confusion.
- [Low] The Makefile modifications incorrectly split the composite `pcs_xpcs` module into multiple independent modules, causing `modpost` build failures.
- [Low] The `dev_pm_ops` structure is assigned unconditionally to the driver's `.pm` field, causing dead code when `CONFIG_PM` is disabled.
--

--- Patch [7]: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver ---
commit c0159daf5cb4d2e6a0fbbe7a38525a8e230e6d93
Author: Coia Prant <coiaprant@gmail.com>

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

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

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

> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 4f7920618b900..c809b7f942a51 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -1,10 +1,9 @@
>  # 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
> -
> -obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
> +obj-$(CONFIG_PCS_XPCS) += pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o

[Severity: Low]
Does this change cause build failures when CONFIG_PCS_XPCS=m?

Replacing the composite module assignment (pcs_xpcs-y) with individual
object assignments causes Kbuild to build separate modules instead of one
composite module, which can lead to undefined references during modpost.

> +obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
> +obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.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..ed969a38d544d
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
[ ... ]
> +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;
> +	}
> +
> +	switch (addr) {
> +	case 0:
> +		return ROCKCHIP_MMD_MII;
> +	case 1:
> +		return ROCKCHIP_MMD_MII1;
[ ... ]
> +static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
> +{
> +	switch (dev) {
> +	case MDIO_MMD_PMAPMD:
> +		return ROCKCHIP_MMD_PMAPMD;
> +	case MDIO_MMD_VEND2:
> +		break;
> +	default:
> +		return -ENXIO;
> +	}
> +
> +	/* Writable only on MII */
> +	switch (reg) {
> +	case DW_VR_MII_AN_CTRL:
> +	case DW_VR_MII_AN_INTR_STS:
> +	case DW_VR_MII_EEE_MCTRL0:
> +	case DW_VR_MII_EEE_MCTRL1:
> +	case DW_VR_MII_DIG_CTRL2:
> +		return ROCKCHIP_MMD_MII;
> +	default:
> +		break;
> +	}

[Severity: High]
Can this asymmetric remapping between reads and writes cause configuration
corruption on port 0?

In xpcs_rk_mdio_write_remapping(), writes to these VEND2 registers are
unconditionally mapped to ROCKCHIP_MMD_MII (port 0). However,
xpcs_rk_mdio_read_remapping() lacks an equivalent redirect for reads.

Because the generic XPCS driver uses read-modify-write operations on VEND2
control registers for ports 1-3, this will result in reading from the target
port's MMD but overwriting port 0's configuration.

> +
> +	switch (addr) {
> +	case 0:

[ ... ]

> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +	int ret;
> +
> +	pxpcs->csr_clk = devm_clk_get(dev, "csr");
> +	if (IS_ERR(pxpcs->csr_clk))
> +		return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
> +					 "Failed to get CSR clock\n");
> +
> +	pm_runtime_set_suspended(dev);
> +	ret = devm_pm_runtime_enable(dev);

[Severity: Medium]
Will this leak the csr_clk if the device is removed while active?

The generic devm_pm_runtime_enable() runs pm_runtime_disable() on cleanup,
which cancels pending suspends but does not synchronously suspend an active
device.

Since this driver lacks a .remove callback to manually suspend the device,
xpcs_rk_pm_runtime_suspend() might never be invoked on unbind, leaving the
clock prepare and enable counts unbalanced.

> +	if (ret) {
> +		dev_err(dev, "Failed to enable runtime-PM\n");
> +		return ret;
> +	}

[ ... ]

> +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> +{
[ ... ]
> +	pdev = of_find_device_by_node(pcs_np);
> +	of_node_put(pcs_np);
> +	if (!pdev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	pxpcs = platform_get_drvdata(pdev);

[Severity: Medium]
Could this blind cast lead to type confusion?

The code extracts driver data from the platform device associated with
pcs_np without validating if the device is actually compatible with
rockchip,rk3568-xpcs or bound to this driver.

If a misconfigured device tree points to a different type of device (like
an I2C controller), casting its drvdata to struct dw_xpcs_rk * and
dereferencing it could crash the system.

> +	if (!pxpcs || !pxpcs->bus) {

[Severity: High]
Is there a use-after-free race condition when accessing drvdata here?

This checks and dereferences pxpcs->bus before establishing a device link
to lock the supplier.

If a user concurrently unbinds the XPCS supplier driver (for example, via
sysfs), devres will free the dw_xpcs_rk structure and clear the drvdata.
The consumer driver might then dereference the freed pxpcs->bus before
device_link_add() is called.

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

[ ... ]

> +static struct platform_driver xpcs_rk_driver = {
> +	.probe = xpcs_rk_probe,
> +	.driver = {
> +		.name = "rk_xpcs-dwxpcs",
> +		.pm = &xpcs_rk_pm_ops,

[Severity: Low]
Will this assignment cause dead code when CONFIG_PM is disabled?

Assigning the dev_pm_ops structure unconditionally to the .pm field without
the pm_ptr() wrapper results in dead code and possible compiler warnings
when CONFIG_PM is not set, which violates the PM subsystem guidelines.

> +		.of_match_table = xpcs_rk_of_ids,
> +	},
> +};
> +module_platform_driver(xpcs_rk_driver);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The SGMII initialization path inadvertently skips enabling the MAC clocks, leading to hardware hangs or faults when MAC registers are accessed.
--

--- Patch [8]: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568 ---
commit 7be50db2ac8c40c8451407f345aaaaf2d0538799
Author: Coia Prant <coiaprant@gmail.com>

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

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.

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..eca482b4b6bfc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
[ ... ]
> @@ -1379,6 +1441,9 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  	if (ret)
>  		return ret;
>  
> +	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> +		goto set_mode;
> +

[Severity: High]
Does this goto skip enabling the MAC clocks? By jumping directly to set_mode,
we bypass the call to gmac_clk_enable() further down in this function.

Can this cause a synchronous external abort when the MAC registers are accessed
subsequently while their clock domain remains disabled?

>  	ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
>  	if (ret < 0)
>  		return ret;
[ ... ]
> -	/*rmii or rgmii*/
> +set_mode:
> +	/* rmii, rgmii, sgmii */
>  	switch (bsp_priv->phy_iface) {
>  	case PHY_INTERFACE_MODE_RGMII:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=8

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property

[Coia Prant]

Thanks for the review. I'll address both issues in v2.

> --- Patch [2]: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property ---
> commit 0d950b4055dd144fecf5059f16535f2c8d94b5d3
> Author: Coia Prant <coiaprant@gmail.com>
>
> dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
>
> This commit adds the "rockchip,sgmii-mac-sel" property to the Rockchip
> Naneng combphy device tree binding. It allows selecting between GMAC0
> and GMAC1 for the SGMII interface on the RK3568 SoC.
>
> > This is necessary for boards such as the Ariaboard Photonicat, where
> > the SGMII interface is connected to GMAC0 and needs to be explicitly
> > configured.
>
> [Severity: Medium]
> Is it actually necessary to explicitly configure the property to 0 for
> GMAC0?
>
> The schema defines the default as 0, which implies that omitting the property
> would naturally default the hardware to GMAC0. If explicit configuration is
> required, does this mean the hardware or bootloader leaves the register at 1,
> making the `default: 0` documented in the schema incorrect?

Regarding the default value: the hardware reset value for the
`pipe_sgmii_mac_sel` bit is GMAC1 (1). I have updated the binding
to set `default: 1` and revised the commit message accordingly,
removing the contradictory "default 0" statement.

The default now matches the hardware reset value.

> [Severity: Low]
> Should this property be conditionally restricted to the RK3568 compatible
> string?
>
> Because it is defined in the top-level properties block, it will pass
> validation for all other hardware variants (like RK3528 or RK3588) that do
> not actually support SGMII. Could an `if-then` block in the `allOf` section
> be used to set this property to false for compatibles other than
> `rockchip,rk3568-naneng-combphy`?

Regarding the property scope: you are correct that this property
only makes sense for RK3568. I have added an `if-then` conditional
in the `allOf` section to restrict this property to
`rockchip,rk3568-naneng-combphy` only.

Thanks,
Coia

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568

[Coia Prant]

> [Severity: High]
> Does this break device tree backward compatibility?
>
> When the rockchip,sgmii-mac-sel property is missing from an older device
> tree, priv->sgmii_mac_sel defaults to 0 in rockchip_combphy_parse_dt().
>
> By unconditionally calling rockchip_combphy_param_write() here, the driver
> now actively clears the MAC selection bit to 0 (GMAC0).
>
> Because the hardware reset value is GMAC1, this actively overrides the default
> hardware and bootloader state that older device trees were relying on.
>
> Could the code check if the property was actually present before writing
> to the register, to ensure the hardware default is preserved for existing
> setups?

Thanks for the detailed review. Regarding the backward compatibility
concern for Patch 3:

First, the driver code in the RFC patch set has already been revised
to align with the hardware default: `priv->sgmii_mac_sel = 1` (GMAC1).
So when the property is omitted, the hardware default is preserved.

Second, and more importantly, there is no backward compatibility
issue in practice: SGMII support for RK3568 has never been functional
in mainline before this series. There are no existing device trees
that rely on this path, because the PHY driver simply did not support
SGMII mode at all. Older device trees either didn't enable SGMII,
or were never operational.

So this change only affects new users who explicitly enable SGMII
and set the property as needed. It does not break anything that
was previously working.

Thanks,
Coia

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs binding

[Coia Prant]

> [ ... ]
>
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/rk3568-cru.h>
> > +    #include <dt-bindings/power/rk3568-power.h>
> > +
> > +    pcs@fda00000 {
> > +      compatible = "rockchip,rk3568-xpcs";
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      reg = <0x0 0xfda00000 0x0 0x200000>;
>
> [Severity: Low]
> Does this 64-bit register format fail dt_binding_check validation?
>
> The example is extracted into a dummy root node with default
> #address-cells = <1> and #size-cells = <1>. This means the 4 cells here will
> be parsed as two items, which violates the maxItems: 1 constraint defined
> in the schema for the reg property.

I will update the example to use
the correct format that passes `dt_binding_check`. The proper way to
represent this is:

    reg = <0xfda00000 0x200000>;

or with explicit address/size cells:

    reg = <0x0 0xfda00000 0x0 0x200000>;

I will adjust the example to match what `dt_binding_check` expects.


> [Severity: Low]
> Will this fail to compile during dt_binding_check?
>
> The example uses the PHY_TYPE_SGMII macro, but the required
> <dt-bindings/phy/phy.h> header isn't included in the example.

Yes, you are correct. I will add the missing include:

    #include <dt-bindings/phy/phy.h>

to the example section.

Thanks for the review. I'll address both issues in v2.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes

[Coia Prant]

Thanks for the review.

> [Severity: Low]
> The commit message states these clocks are used as assigned-clock-parents
> for the respective GMAC nodes, but the patch doesn't appear to add the
> assigned-clock-parents properties to the gmac0 or gmac1 nodes. Are these
> assignments missing?

The `assigned-clock-parents` assignments are not added in the SoC-level
`rk3568.dtsi` because doing so would break existing boards that use
RGMII mode. The clock parent for GMAC0/1 is currently hardcoded to the
RGMII clock path in the mainline device tree.

Instead, the `assigned-clock-parents` are applied at the board level.
This is already done in Patch 9 of this series for the Photonicat board:

    &gmac0 {
        assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
        assigned-clock-parents = <&xpcs_gmac0_clk>;
        ...
    };

So the assignments exist, but in the board DTS rather than the SoC DTSI.

> [Severity: Low]
> Should this internal SoC clock be modeled as a clock provider from the
> generating XPCS IP node using #clock-cells, rather than using a dummy
> fixed-clock?

The XPCS block does not have a clock controller interface. It simply
outputs a fixed 125MHz clock for the GMAC when operating in SGMII mode.
There is no register interface to control or configure this clock — it
is a fixed-rate output from the PCS hardware.

Using `fixed-clock` is therefore the correct and accurate representation
of the hardware. The XPCS is not a clock controller, so it should not
be modeled as one.

> Additionally, the commit message states "All nodes are left disabled by
> default", but this node is missing the status = "disabled" property. Will
> this leave the clock unconditionally enabled?
> [Severity: Low]
>
> Similarly, does this node also need a status = "disabled" property to align
> with the behavior described in the commit message?

The fixed-clock nodes are always present and do not have a `status`
property. This is intentional: they are part of the SoC's fixed clock
infrastructure, similar to other fixed-clock nodes in the SoC DTSI.
They are not "devices" that need to be enabled/disabled, but rather
static clock providers.

The "all nodes are left disabled by default" statement in the commit
message refers to the XPCS node and its pcs-mii sub-nodes, not the
fixed-clock nodes. I will revise the commit message to clarify this.

Thanks,
Coia

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

[Coia Prant]

Thanks for the review.

> [ ... ]
>
> > @@ -884,7 +889,7 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
> >
> >       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);
>
> [Severity: Medium]
> The commit message mentions improving SGMII AN state handling, but this
> adds BMCR_ANRESTART to the 1000BASE-X configuration path in
> xpcs_config_aneg_c37_1000basex(). Was this unintentional, or should the
> commit message be updated to mention this behavior change for 1000BASE-X?

You are correct. This was unintentional. The `BMCR_ANRESTART` change in
`xpcs_config_aneg_c37_1000basex()` is a mistake that was introduced
during development and should not be there. I will remove it in v2.

The SGMII path (`xpcs_config_aneg_c37_sgmii()`) is the one that actually
needs this change, and that is what I have tested on the Photonicat board.
The 1000BASE-X path was not tested and the change was not intended.

> [ ... ]
>
> > @@ -1086,7 +1094,24 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
> >                       state->duplex = DUPLEX_FULL;
> >               else
> >                       state->duplex = DUPLEX_HALF;
> > -     } else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> > +
> > +             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);
>
> [Severity: High]
> Can clearing the interrupt here lead to a permanently stalled link on
> Wangxun platforms if the subsequent xpcs_read() fails?
>
> The original code cleared DW_VR_MII_AN_INTR_STS at the very end of the
> DW_VR_MII_AN_STS_C37_ANCMPLT_INTR block, after successfully reading
> MII_BMCR and MII_ADVERTISE.
>
> By moving the interrupt clear here:
>
>     if (state->an_complete)
>         xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
>
> If a transient MDIO error occurs during the subsequent MII_BMCR read:
>
>     speed = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
>     if (speed < 0)
>         return speed;
>
> The function will exit with an error and the link will remain down, but the
> AN complete status has already been cleared. Will this prevent the Wangxun
> initialization block from running on the next poll and leave the link
> stuck?

No, this is safe. The `ret` value used to determine whether to enter the
Wangxun-specific path is read from the register before the clear operation.
The clear only affects the hardware register, not the stack variable. So
Wangxun's `if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR)` block will still
be reached correctly.

If the subsequent MDIO read fails, the function will return the error and
the link state will remain down. Since the hardware interrupt status has
already been cleared, the next poll will not see `state->an_complete` set
again, so the Wangxun block will not be entered until a new AN event
occurs. This is actually the intended behavior — the interrupt is only
valid for the current AN event and should not be processed more than once.

Therefore, the change does not introduce a regression on Wangxun platforms.

I will remove the 1000BASE-X change in v2 and keep only the SGMII-related
fixes.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568

[Coia Prant]

> [ ... ]
> > @@ -1379,6 +1441,9 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
> >       if (ret)
> >               return ret;
> >
> > +     if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> > +             goto set_mode;
> > +
>
> [Severity: High]
> Does this goto skip enabling the MAC clocks? By jumping directly to set_mode,
> we bypass the call to gmac_clk_enable() further down in this function.
>
> Can this cause a synchronous external abort when the MAC registers are accessed
> subsequently while their clock domain remains disabled?

You are correct. The original RFC patch had this issue: the SGMII path
bypassed `gmac_clk_enable()` via the `goto set_mode`, which could lead
to hardware faults when accessing MAC registers with the clock disabled.

This has been fixed in the updated version of the patch. The clock enable
is now moved before the SGMII check:

    ret = gmac_clk_enable(bsp_priv, true);
    if (ret)
        return ret;

    if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
        goto set_mode;

So the clocks are always enabled before any register access, regardless of
the interface mode.

The error handling has also been cleaned up with a unified `clk_disable`
label, and the default (unhandled interface) case now properly returns an
error and disables the clocks.

Regarding stable backport: this issue does not exist in the 6.18
kernel that I am using for backport testing, as the code structure in
`rk_gmac_powerup()` is significantly different there. The fix only
affects the mainline version of the patch.

Thanks,
Coia

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy