* [PATCH net-next v5 01/11] net: stmmac: move XPCS lifetime management to platform drivers
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 02/11] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property Coia Prant
` (9 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
The current XPCS creation logic in stmmac_pcs_setup() is problematic
for several reasons.
First, if a device tree specifies a "pcs-handle" but no select_pcs()
callback is provided by the platform driver, the created XPCS is never
used. The phylink framework requires select_pcs() to actually return
the PCS to the core, so the pcs-handle property becomes effectively
useless without the matching callback. This is confusing for developers
who expect that specifying a pcs-handle in their device tree should be
sufficient to enable the PCS.
Second, and more critically, when a platform driver sets pcs_init()
and creates an XPCS inside that callback, the common code afterwards
still runs unconditionally and overwrites priv->hw->xpcs with the
local xpcs variable, which stays NULL. The platform driver has no way
to prevent this override because the common code runs after the
platform-specific initialization.
After commit 93f84152e4ae ("net: stmmac: clean up
stmmac_mac_select_pcs()"), the common code no longer falls back to
priv->hw->phylink_pcs if select_pcs() is not set. This change
reinforces that each platform must manage its own PCS life cycle
explicitly, but the XPCS creation code in stmmac_pcs_setup() was not
updated to match this new expectation, leaving a gap where platform
drivers have no clean way to take control of XPCS creation.
Address all of these issues by simplifying the existing pcs_init() and
pcs_exit() dispatch in stmmac_pcs_setup() and stmmac_pcs_clean(). The
common stmmac_pcs_setup() now calls plat->pcs_init() if present, and
stmmac_pcs_clean() calls plat->pcs_exit() if present, removing the
confusing and error-prone XPCS creation logic from the common code.
Platforms that do not need an XPCS simply leave the callbacks as NULL
and no change in behavior occurs. Platforms that do need an XPCS can
now create it with the exact configuration they require, including
wrapping it with custom phylink_pcs_ops when necessary.
The Intel mGbE glue is updated to create its XPCS inside its own
pcs_init() implementation, and the renesas-gbeth, rzn1 and socfpga
pcs_exit() callbacks now explicitly clear priv->hw->phylink_pcs after
destroying the PCS to avoid dangling pointer references.
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 44 +++++++++++++++++--
.../stmicro/stmmac/dwmac-renesas-gbeth.c | 7 ++-
.../net/ethernet/stmicro/stmmac/dwmac-rzn1.c | 7 ++-
.../ethernet/stmicro/stmmac/dwmac-socfpga.c | 7 ++-
.../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 39 +++-------------
5 files changed, 62 insertions(+), 42 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index f5f9fa67ecd77..4308dccbf2570 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -603,13 +603,47 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
plat->mdio_bus_data->needs_reset = true;
}
+static int intel_mgbe_pcs_init(struct stmmac_priv *priv)
+{
+ struct fwnode_handle *devnode, *pcsnode;
+ struct dw_xpcs *xpcs;
+ int addr;
+
+ devnode = dev_fwnode(priv->device);
+
+ if (fwnode_property_present(devnode, "pcs-handle")) {
+ pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
+ xpcs = xpcs_create_fwnode(pcsnode);
+ fwnode_handle_put(pcsnode);
+ } else {
+ addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
+ xpcs = xpcs_create_mdiodev(priv->mii, addr);
+ }
+
+ if (IS_ERR(xpcs))
+ return PTR_ERR(xpcs);
+
+ xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
+
+ priv->hw->xpcs = xpcs;
+ return 0;
+}
+
+static void intel_mgbe_pcs_exit(struct stmmac_priv *priv)
+{
+ if (!priv->hw->xpcs)
+ return;
+
+ xpcs_destroy(priv->hw->xpcs);
+ priv->hw->xpcs = NULL;
+}
+
static struct phylink_pcs *intel_mgbe_select_pcs(struct stmmac_priv *priv,
phy_interface_t interface)
{
- /* plat->mdio_bus_data->has_xpcs has been set true, so there
- * should always be an XPCS. The original code would always
- * return this if present.
- */
+ if (!priv->hw->xpcs)
+ return NULL;
+
return xpcs_to_phylink_pcs(priv->hw->xpcs);
}
@@ -733,6 +767,8 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
plat->phy_interface == PHY_INTERFACE_MODE_1000BASEX) {
plat->mdio_bus_data->pcs_mask = BIT_U32(INTEL_MGBE_XPCS_ADDR);
plat->default_an_inband = true;
+ plat->pcs_init = intel_mgbe_pcs_init;
+ plat->pcs_exit = intel_mgbe_pcs_exit;
plat->select_pcs = intel_mgbe_select_pcs;
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
index 19f34e18bfef2..9af32c26f9c14 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
@@ -81,8 +81,11 @@ static int renesas_gmac_pcs_init(struct stmmac_priv *priv)
static void renesas_gmac_pcs_exit(struct stmmac_priv *priv)
{
- if (priv->hw->phylink_pcs)
- miic_destroy(priv->hw->phylink_pcs);
+ if (!priv->hw->phylink_pcs)
+ return;
+
+ miic_destroy(priv->hw->phylink_pcs);
+ priv->hw->phylink_pcs = NULL;
}
static struct phylink_pcs *renesas_gmac_select_pcs(struct stmmac_priv *priv,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
index 13634965bc19a..01df4776edb3f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
@@ -35,8 +35,11 @@ static int rzn1_dwmac_pcs_init(struct stmmac_priv *priv)
static void rzn1_dwmac_pcs_exit(struct stmmac_priv *priv)
{
- if (priv->hw->phylink_pcs)
- miic_destroy(priv->hw->phylink_pcs);
+ if (!priv->hw->phylink_pcs)
+ return;
+
+ miic_destroy(priv->hw->phylink_pcs);
+ priv->hw->phylink_pcs = NULL;
}
static struct phylink_pcs *rzn1_dwmac_select_pcs(struct stmmac_priv *priv,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 1d7f0a57d2889..6d4bc1fe8f751 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -539,8 +539,11 @@ static int socfpga_dwmac_pcs_init(struct stmmac_priv *priv)
static void socfpga_dwmac_pcs_exit(struct stmmac_priv *priv)
{
- if (priv->hw->phylink_pcs)
- lynx_pcs_destroy(priv->hw->phylink_pcs);
+ if (!priv->hw->phylink_pcs)
+ return;
+
+ lynx_pcs_destroy(priv->hw->phylink_pcs);
+ priv->hw->phylink_pcs = NULL;
}
static struct phylink_pcs *socfpga_dwmac_select_pcs(struct stmmac_priv *priv,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index afe98ff5bdcb0..7396b68899c66 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -426,35 +426,14 @@ int stmmac_mdio_reset(struct mii_bus *bus)
int stmmac_pcs_setup(struct net_device *ndev)
{
struct stmmac_priv *priv = netdev_priv(ndev);
- struct fwnode_handle *devnode, *pcsnode;
- struct dw_xpcs *xpcs = NULL;
- int addr, ret;
-
- devnode = dev_fwnode(priv->device);
-
- if (priv->plat->pcs_init) {
- ret = priv->plat->pcs_init(priv);
- } else if (fwnode_property_present(devnode, "pcs-handle")) {
- pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
- xpcs = xpcs_create_fwnode(pcsnode);
- fwnode_handle_put(pcsnode);
- ret = PTR_ERR_OR_ZERO(xpcs);
- } else if (priv->plat->mdio_bus_data &&
- priv->plat->mdio_bus_data->pcs_mask) {
- addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
- xpcs = xpcs_create_mdiodev(priv->mii, addr);
- ret = PTR_ERR_OR_ZERO(xpcs);
- } else {
+ int ret;
+
+ if (!priv->plat->pcs_init)
return 0;
- }
+ ret = priv->plat->pcs_init(priv);
if (ret)
- return dev_err_probe(priv->device, ret, "No xPCS found\n");
-
- if (xpcs)
- xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
-
- priv->hw->xpcs = xpcs;
+ return dev_err_probe(priv->device, ret, "Failed to initialize PCS\n");
return 0;
}
@@ -463,14 +442,10 @@ void stmmac_pcs_clean(struct net_device *ndev)
{
struct stmmac_priv *priv = netdev_priv(ndev);
- if (priv->plat->pcs_exit)
- priv->plat->pcs_exit(priv);
-
- if (!priv->hw->xpcs)
+ if (!priv->plat->pcs_exit)
return;
- xpcs_destroy(priv->hw->xpcs);
- priv->hw->xpcs = NULL;
+ priv->plat->pcs_exit(priv);
}
struct stmmac_clk_rate {
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 01/11] net: stmmac: move XPCS lifetime management to platform drivers
2026-09-13 18:58 ` [PATCH net-next v5 01/11] net: stmmac: move XPCS lifetime management to platform drivers Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 1
- [Medium] Commit message mischaracterizes the removal of the generic
XPCS creation as removing dead code.
- [Medium] DT binding vs implementation mismatch: after this patch
nothing in the stmmac core reads the generic 'pcs-handle' property,…
- [Low] The commit message describes two defects, one framed as "more
critically" (the common code overwriting priv->hw->xpcs after a…
--
net: stmmac: move XPCS lifetime management to platform drivers
The generic XPCS creation in stmmac_pcs_setup() (pcs-handle lookup and
mdio_bus_data->pcs_mask based xpcs_create_mdiodev()) is removed, leaving
only the plat->pcs_init() / plat->pcs_exit() dispatch. The Intel mGbE glue
grows its own pcs_init()/pcs_exit() that create and destroy the XPCS, and
the renesas-gbeth, rzn1 and socfpga pcs_exit() callbacks now clear
priv->hw->phylink_pcs after destroying the PCS.
A few questions on the commit message and on the removal of the generic
path.
> First, if a device tree specifies a "pcs-handle" but no select_pcs()
> callback is provided by the platform driver, the created XPCS is never
> used. The phylink framework requires select_pcs() to actually return
> the PCS to the core, so the pcs-handle property becomes effectively
> useless without the matching callback.
[Severity: Medium]
Is "the created XPCS is never used" accurate? An XPCS stored in
priv->hw->xpcs has two consumers that do not go through select_pcs().
stmmac_phylink_setup() in stmmac_main.c uses it to widen the phylink
interface sets:
/* If we have an xpcs, it defines which PHY interfaces are supported. */
if (priv->hw->xpcs)
pcs = xpcs_to_phylink_pcs(priv->hw->xpcs);
else
pcs = priv->hw->phylink_pcs;
if (pcs)
phy_interface_or(config->supported_interfaces, ...
and stmmac_init_phy() uses it to decide whether to attach a PHY at all:
if (priv->hw->xpcs &&
xpcs_get_an_mode(priv->hw->xpcs, mode) == DW_AN_C73)
return 0;
So for a dwmac instance with a pcs-handle but no plat->pcs_init, does this
change narrow config->supported_interfaces (and config->lpi_interfaces) and
alter the PHY-attach decision, rather than just dropping something unused?
Would it be worth spelling that behavioural change out in the commit
message?
> Second, and more critically, when a platform driver sets pcs_init()
> and creates an XPCS inside that callback, the common code afterwards
> still runs unconditionally and overwrites priv->hw->xpcs with the
> local xpcs variable, which stays NULL. The platform driver has no way
> to prevent this override because the common code runs after the
> platform-specific initialization.
[Severity: Low]
This isn't a bug, but could this paragraph say that no in-tree platform is
affected today?
All three pre-existing pcs_init() implementations store their PCS in
priv->hw->phylink_pcs, not priv->hw->xpcs:
renesas_gmac_pcs_init() -> priv->hw->phylink_pcs = miic_create(...)
rzn1_dwmac_pcs_init() -> priv->hw->phylink_pcs = miic_create(...)
socfpga_dwmac_pcs_init() -> priv->hw->phylink_pcs = pcs
so the old "priv->hw->xpcs = xpcs;" on the pcs_init branch overwrote NULL
with NULL. intel_mgbe_pcs_init() added here is the first in-tree pcs_init()
that stores an XPCS, which makes this a refactor that removes a latent trap
rather than a fix for observable breakage.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index f5f9fa67ecd77..4308dccbf2570 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -603,13 +603,47 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
> plat->mdio_bus_data->needs_reset = true;
> }
>
> +static int intel_mgbe_pcs_init(struct stmmac_priv *priv)
> +{
> + struct fwnode_handle *devnode, *pcsnode;
> + struct dw_xpcs *xpcs;
> + int addr;
> +
> + devnode = dev_fwnode(priv->device);
> +
> + if (fwnode_property_present(devnode, "pcs-handle")) {
> + pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
> + xpcs = xpcs_create_fwnode(pcsnode);
> + fwnode_handle_put(pcsnode);
> + } else {
> + addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
> + xpcs = xpcs_create_mdiodev(priv->mii, addr);
> + }
> +
> + if (IS_ERR(xpcs))
> + return PTR_ERR(xpcs);
> +
> + xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
> +
> + priv->hw->xpcs = xpcs;
> + return 0;
> +}
> +
[ ... ]
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index afe98ff5bdcb0..7396b68899c66 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -426,35 +426,14 @@ int stmmac_mdio_reset(struct mii_bus *bus)
> int stmmac_pcs_setup(struct net_device *ndev)
> {
> struct stmmac_priv *priv = netdev_priv(ndev);
> - struct fwnode_handle *devnode, *pcsnode;
> - struct dw_xpcs *xpcs = NULL;
> - int addr, ret;
> -
> - devnode = dev_fwnode(priv->device);
> -
> - if (priv->plat->pcs_init) {
> - ret = priv->plat->pcs_init(priv);
> - } else if (fwnode_property_present(devnode, "pcs-handle")) {
> - pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
> - xpcs = xpcs_create_fwnode(pcsnode);
> - fwnode_handle_put(pcsnode);
> - ret = PTR_ERR_OR_ZERO(xpcs);
> - } else if (priv->plat->mdio_bus_data &&
> - priv->plat->mdio_bus_data->pcs_mask) {
> - addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
> - xpcs = xpcs_create_mdiodev(priv->mii, addr);
> - ret = PTR_ERR_OR_ZERO(xpcs);
> - } else {
> + int ret;
> +
> + if (!priv->plat->pcs_init)
> return 0;
> - }
>
> + ret = priv->plat->pcs_init(priv);
> if (ret)
> - return dev_err_probe(priv->device, ret, "No xPCS found\n");
> -
> - if (xpcs)
> - xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
> -
> - priv->hw->xpcs = xpcs;
> + return dev_err_probe(priv->device, ret, "Failed to initialize PCS\n");
>
> return 0;
> }
[Severity: Medium]
After this change, does anything in the stmmac core still read the generic
"pcs-handle" property?
The property is still documented generically in
Documentation/devicetree/bindings/net/ethernet-controller.yaml:
pcs-handle:
$ref: /schemas/types.yaml#/definitions/phandle-array
items:
maxItems: 1
description:
Specifies a reference to a node representing a PCS PHY device on a MDIO
bus to link with an external PHY (phy-handle) if exists.
and snps,dwmac.yaml $refs ethernet-controller.yaml and ends with
"additionalProperties: true", so pcs-handle stays schema-valid on every
dwmac node regardless of which glue is bound.
On a glue that does not set plat->pcs_init, stmmac_pcs_setup() now returns 0
without looking at the property, priv->hw->xpcs stays NULL, and nothing is
logged - whereas the removed code would at least fail probe with
"No xPCS found" for a bad phandle. Should the binding be updated to say
pcs-handle is only honoured by specific glue drivers, or should the core
keep a generic path (or emit a warning) so the two agree?
The same question applies to struct stmmac_mdio_bus_data.pcs_mask.
stmmac_mdio_register() still treats it as "a PCS lives at these MDIO
addresses" and masks them out of the bus scan:
new_bus->phy_mask = mdio_bus_data->phy_mask | mdio_bus_data->pcs_mask;
but no core code creates a PCS for those addresses any more, only
dwmac-intel's private intel_mgbe_pcs_init(). Does a glue that sets
pcs_mask without pcs_init now end up with that address neither scanned as a
PHY nor instantiated as a PCS?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 02/11] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
2026-09-13 18:58 ` [PATCH net-next v5 01/11] net: stmmac: move XPCS lifetime management to platform drivers Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568 Coia Prant
` (8 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
On RK3568, the SGMII interface can be routed to either GMAC0 or
GMAC1 via the pipe_sgmii_mac_sel bit in the pipe GRF registers.
Add the optional "rockchip,sgmii-mac-sel" property to allow the
device tree to select which GMAC controller is used for SGMII.
The property takes a value of 0 (GMAC0) or 1 (GMAC1). The hardware
reset value is 1 (GMAC1), but this can be overridden by setting the
property to 0 for boards where SGMII is connected to GMAC0.
This is necessary for boards such as the Ariaboard Photonicat, where
the SGMII interface is connected to GMAC0 and needs to be explicitly
configured.
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
.../bindings/phy/phy-rockchip-naneng-combphy.yaml | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
index 379b08bd9e97a..8e898bce9af73 100644
--- a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
+++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
@@ -80,6 +80,15 @@ properties:
description:
Some additional pipe settings are accessed through GRF regs.
+ rockchip,sgmii-mac-sel:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1]
+ default: 1
+ description:
+ Select gmac0 or gmac1 to be used as SGMII controller.
+ The hardware reset value is GMAC1 (1). Set this to 0 to route
+ SGMII to GMAC0.
+
"#phy-cells":
const: 1
@@ -105,6 +114,10 @@ allOf:
maxItems: 1
reset-names:
maxItems: 1
+ rockchip,sgmii-mac-sel: true
+ else:
+ properties:
+ rockchip,sgmii-mac-sel: false
- if:
properties:
compatible:
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 02/11] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
2026-09-13 18:58 ` [PATCH net-next v5 02/11] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 1
- [Low] Concern (validated as NOT a defect in the series context): the
binding adds `rockchip,sgmii-mac-sel` while, at this commit alone, no…
--
dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
This adds an optional rockchip,sgmii-mac-sel property to the naneng-combphy
binding so a board can select whether the RK3568 SGMII interface is routed
to GMAC0 or GMAC1. The property is restricted to the RK3568 compatible via
the existing allOf conditional and defaults to 1 (GMAC1).
> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> index 379b08bd9e97a..8e898bce9af73 100644
> --- a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> @@ -80,6 +80,15 @@ properties:
> description:
> Some additional pipe settings are accessed through GRF regs.
>
> + rockchip,sgmii-mac-sel:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1]
> + default: 1
> + description:
> + Select gmac0 or gmac1 to be used as SGMII controller.
> + The hardware reset value is GMAC1 (1). Set this to 0 to route
> + SGMII to GMAC0.
> +
[Severity: Low]
At this commit alone, is rockchip,sgmii-mac-sel read by anything?
rockchip_combphy_parse_dt() in
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c does not look it up,
struct rockchip_combphy_grfcfg has no pipe_sgmii_mac_sel descriptor, and
no in-tree DTS sets it, so a kernel built from exactly this commit would
silently ignore the property.
This resolves itself later in the same series: "phy: rockchip:
naneng-combphy: add SGMII MAC selection for RK3568" adds the parsing
priv->sgmii_mac_sel = 1;
device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
along with the RK3568 pipe-GRF descriptor
.pipe_sgmii_mac_sel = { 0x0040, 1, 1, 0x00, 0x01 },
and the write in the PHY_TYPE_SGMII path of rk3568_combphy_cfg(), and
"arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port" adds the
first user:
&combphy2 {
rockchip,sgmii-mac-sel = <0>;
status = "okay";
};
So this is noted only for the record, since the binding and its consumer
being split across consecutive patches is the normal series layout.
The remaining point is a design question rather than a bug: is a raw
uint32 MAC index the interface preferred here, or would a phandle to the
GMAC controller node be a better fit for describing the routing?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
2026-09-13 18:58 ` [PATCH net-next v5 01/11] net: stmmac: move XPCS lifetime management to platform drivers Coia Prant
2026-09-13 18:58 ` [PATCH net-next v5 02/11] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 04/11] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support Coia Prant
` (7 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
On RK3568, the SGMII interface can be routed to either GMAC0 or
GMAC1 via the GRF register pipe_sgmii_mac_sel.
Add support for this selection by introducing
the "rockchip,sgmii-mac-sel" DT property.
From the RK3568 TRM (Part1, Page 229), the PIPE_GRF_XPCS_CON0
bit 1 (pipe_sgmii_mac_sel) is defined as:
0: SGMII routed to GMAC0
1: SGMII routed to GMAC1
The hardware reset value is 1 (GMAC1). If the property is set to 0,
the driver routes SGMII to GMAC0; if set to 1 (or omitted), it
remains at GMAC1.
This is necessary for boards such as the Ariaboard Photonicat, which
uses the SGMII interface connected to GMAC0.
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index 7843356a4dd47..7b867e7520064 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
@@ -186,6 +186,7 @@ struct rockchip_combphy_grfcfg {
struct combphy_reg pipe_xpcs_phy_ready;
struct combphy_reg pipe_pcie1l0_sel;
struct combphy_reg pipe_pcie1l1_sel;
+ struct combphy_reg pipe_sgmii_mac_sel;
struct combphy_reg u3otg0_port_en;
struct combphy_reg u3otg1_port_en;
};
@@ -212,6 +213,7 @@ struct rockchip_combphy_priv {
bool enable_ssc;
bool ext_refclk;
struct clk *refclk;
+ u32 sgmii_mac_sel;
};
static void rockchip_combphy_updatel(struct rockchip_combphy_priv *priv,
@@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
+ priv->sgmii_mac_sel = 1;
+ device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
+
priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
/* fallback to old behaviour */
if (PTR_ERR(priv->phy_rst) == -ENOENT)
@@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
break;
case PHY_TYPE_SGMII:
+ rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
+ priv->sgmii_mac_sel > 0);
rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);
@@ -984,6 +991,7 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
.con3_for_sata = { 0x000c, 15, 0, 0x00, 0x4407 },
/* pipe-grf */
.pipe_con0_for_sata = { 0x0000, 15, 0, 0x00, 0x2220 },
+ .pipe_sgmii_mac_sel = { 0x0040, 1, 1, 0x00, 0x01 },
.pipe_xpcs_phy_ready = { 0x0040, 2, 2, 0x00, 0x01 },
.u3otg0_port_en = { 0x0104, 15, 0, 0x0181, 0x1100 },
.u3otg1_port_en = { 0x0144, 15, 0, 0x0181, 0x1100 },
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
2026-09-13 18:58 ` [PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568 Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 1
- [Low] In rk3568_combphy_cfg() (drivers/phy/rockchip/phy-rockchip-
naneng-combphy.c:881-882) the newly added write of pipe_sgmii_mac_sel…
--
phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
This adds a "rockchip,sgmii-mac-sel" DT property to pick whether the RK3568
SGMII interface is routed to GMAC0 or GMAC1 via PIPE_GRF_XPCS_CON0 bit 1.
The value is read in rockchip_combphy_parse_dt() and programmed from the
PHY_TYPE_SGMII branch of rk3568_combphy_cfg().
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)
One question about the commit message wording:
The hardware reset value is 1 (GMAC1). If the property is set to 0,
the driver routes SGMII to GMAC0; if set to 1 (or omitted), it
remains at GMAC1.
Does "or omitted ... it remains at GMAC1" match the code? The write in
rk3568_combphy_cfg() is unconditional, so in the omitted case the driver
does not leave the bit alone, it actively programs it to 1. See the
inline comment below.
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 7843356a4dd47..7b867e7520064 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
[ ... ]
> @@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
>
> priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
>
> + priv->sgmii_mac_sel = 1;
> + device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
> +
[Severity: Low]
Since device_property_read_u32() leaves the output untouched on failure,
rockchip_combphy_parse_dt() ends up with priv->sgmii_mac_sel == 1 both when
the property says 1 and when the property is absent. Is there a way for the
driver to express "leave the bit as firmware left it"?
Before this patch the kernel never touched PIPE_GRF_XPCS_CON0 bit 1, so a
board whose bootloader routed SGMII to GMAC0 kept that routing. With this
change, a DT that instantiates the combphy as PHY_TYPE_SGMII but does not
carry the new property has the bit re-programmed to GMAC1 on every
phy_init(). Can that break such a board's SGMII link?
> priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
> /* fallback to old behaviour */
> if (PTR_ERR(priv->phy_rst) == -ENOENT)
> @@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
> break;
>
> case PHY_TYPE_SGMII:
> + rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
> + priv->sgmii_mac_sel > 0);
[Severity: Low]
This write always happens, including in the "property omitted" case, and
rockchip_combphy_param_write() sets the write-enable mask along with the
value:
tmp = en ? reg->enable : reg->disable;
mask = GENMASK(reg->bitend, reg->bitstart);
val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);
return regmap_write(base, reg->offset, val);
so with priv->sgmii_mac_sel defaulting to 1 the register gets
(0x1 << 1) | (GENMASK(1, 1) << 16), i.e. GMAC1 is forced. Would it be
better to only issue this write when the property is actually present?
A second question: bit 1 of PIPE_GRF_XPCS_CON0 is a single SoC-wide mux,
while the property sits on a per-PHY node. If two combphy instances that
share the same rockchip,pipe-grf syscon are both brought up as
PHY_TYPE_SGMII with different property values, the last phy_init() wins and
nothing is reported. Should conflicting values be rejected or at least
warned about?
> rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
> rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
> rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);
> @@ -984,6 +991,7 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
> .con3_for_sata = { 0x000c, 15, 0, 0x00, 0x4407 },
> /* pipe-grf */
> .pipe_con0_for_sata = { 0x0000, 15, 0, 0x00, 0x2220 },
> + .pipe_sgmii_mac_sel = { 0x0040, 1, 1, 0x00, 0x01 },
> .pipe_xpcs_phy_ready = { 0x0040, 2, 2, 0x00, 0x01 },
For what it is worth, the only in-tree SGMII consumer at the end of this
series, rk3568-photonicat.dts, sets rockchip,sgmii-mac-sel = <0>
explicitly, so no upstream DT changes behaviour today; the concern is about
out-of-tree or ported device trees and about the commit message wording.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 04/11] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
` (2 preceding siblings ...)
2026-09-13 18:58 ` [PATCH net-next v5 03/11] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568 Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 05/11] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes Coia Prant
` (6 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
Add device tree binding documentation for the Synopsys DesignWare
XPCS integrated on the Rockchip RK3568 SoC.
The XPCS is accessed over the APB3 bus and internally connected to
a Naneng Combo SerDes PHY. It supports 1000BASE-X, SGMII, and
QSGMII modes, with four MII ports.
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
.../net/pcs/rockchip,rk3568-xpcs.yaml | 110 ++++++++++++++++++
1 file changed, 110 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
new file mode 100644
index 0000000000000..703fcff0e3f70
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/rockchip,rk3568-xpcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip RK3568 Synopsys DesignWare Ethernet PCS
+
+maintainers:
+ - Coia Prant <coiaprant@gmail.com>
+
+description: |
+ Rockchip RK3568 SoC integrates a Synopsys DesignWare Ethernet Physical
+ Coding Sublayer (XPCS).
+ The PCS provides an interface between the Media Access Control (MAC)
+ and the Physical Medium Attachment (PMA) sublayer through a Media
+ Independent Interface (GMII).
+
+ The XPCS is accessed over the APB3 bus and internally connected to a
+ Naneng Combo SerDes PHY.
+ It supports 1000BASE-X, SGMII and QSGMII modes.
+
+ The block contains four MII ports that can be individually enabled and
+ routed to one of the Ethernet GMAC controllers via the pcs-handle
+ property in the MAC device tree node.
+
+properties:
+ compatible:
+ const: rockchip,rk3568-xpcs
+
+ reg:
+ maxItems: 1
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ clocks:
+ items:
+ - description: APB3 bus interface clock (clk_csr_i), required for register access
+ - description: EEE clock (clk_eee_i), required for Energy Efficient Ethernet operation
+
+ clock-names:
+ items:
+ - const: csr
+ - const: eee
+
+ phys:
+ maxItems: 1
+
+ phy-names:
+ const: serdes
+
+ power-domains:
+ maxItems: 1
+
+patternProperties:
+ "^ethernet-pcs-mii@[0-3]$":
+ type: object
+ description:
+ One of the four MII ports of the XPCS. The port is linked to an
+ Ethernet MAC controller via the pcs-handle property in the MAC's
+ device tree node.
+
+ properties:
+ reg:
+ description: MII port number.
+ enum: [0, 1, 2, 3]
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+ - clocks
+ - clock-names
+ - phys
+ - phy-names
+ - power-domains
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/rk3568-cru.h>
+ #include <dt-bindings/power/rk3568-power.h>
+ #include <dt-bindings/phy/phy.h>
+
+ ethernet-pcs@fda00000 {
+ compatible = "rockchip,rk3568-xpcs";
+ reg = <0xfda00000 0x200000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
+ clock-names = "csr", "eee";
+ phys = <&combphy2 PHY_TYPE_SGMII>;
+ phy-names = "serdes";
+ power-domains = <&power RK3568_PD_PIPE>;
+
+ ethernet-pcs-mii@0 {
+ reg = <0>;
+ };
+ };
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 04/11] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support
2026-09-13 18:58 ` [PATCH net-next v5 04/11] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 5
- [Low] Concern (validated as a false positive): the new schema was said
to fork the DW XPCS description in two, to lack the conventional…
- [Low] Concern (validated as a false positive): the four `ethernet-pcs-
mii@[0-3]` child nodes were said to have no in-tree implementation,…
- [Low] Concern (validated as a false positive): the binding omits
`resets`/`reset-names` for the RK3568 XPCS soft resets (SRST_XPCS,…
- [Low] Concern (validated as a false positive): the mandatory `eee`
clock is a documented property that no in-tree XPCS code requests…
- [Low] Concern (validated as a false positive): the binding omits `reg-
names` (direct/indirect) and `reg-io-width`, so the CSR mapping style…
--
dt-bindings: net: pcs: add rockchip,rk3568-xpcs support
This adds a new YAML binding for the Synopsys DesignWare XPCS as
integrated on the RK3568, described as an APB3-mapped block with a
Naneng Combo SerDes PHY and four MII child ports. All the observations
below are documentation-level questions only; the accompanying driver
patches in the series do implement what the schema describes.
> diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> new file mode 100644
> index 0000000000000..703fcff0e3f70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
> @@ -0,0 +1,110 @@
[ ... ]
> +properties:
> + compatible:
> + const: rockchip,rk3568-xpcs
[Severity: Low]
This isn't a bug, but would it be worth saying in the commit message why
this gets its own schema and a single compatible, rather than the usual
"rockchip,rk3568-xpcs", "snps,dw-xpcs" fallback?
The glue driver added later in the series maps registers differently from
the generic one:
drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_addr_format() {
return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);
}
and remaps MDIO_MMD_PMAPMD/MDIO_MMD_VEND2 per port in
xpcs_rk_mdio_read_remapping(), so a snps,dw-xpcs fallback would let
pcs-xpcs-plat bind and touch the wrong MMDs. Recording that reasoning in
the changelog would save the next reader the same investigation.
> + reg:
> + maxItems: 1
[Severity: Low]
The existing snps,dw-xpcs binding describes reg-names (direct/indirect)
and reg-io-width; neither appears here. Is the intent that the compatible
alone implies a direct-mapped 2 MiB window with a fixed 4-byte stride?
That is what the driver assumes:
drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_init_res() {
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
...
if (resource_size(res) < SZ_2M) {
drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_read_reg() {
ret = readl(pxpcs->reg_base + (csr << 2)) & 0xffff;
Would it help to state the 2 MiB size and the 4-byte register stride in
the description so the omission of reg-names and reg-io-width is clearly
deliberate?
> + clocks:
> + items:
> + - description: APB3 bus interface clock (clk_csr_i), required for register access
> + - description: EEE clock (clk_eee_i), required for Energy Efficient Ethernet operation
> +
> + clock-names:
> + items:
> + - const: csr
> + - const: eee
[Severity: Low]
The fixed [csr, eee] list differs from the names the generic XPCS code
looks up ("core" and "pad" in pcs-xpcs.c, "csr" in pcs-xpcs-plat.c). Is
the exclusion of core/pad intentional here?
Both documented clocks are used by the Rockchip glue:
drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_init_clk() {
pxpcs->csr_clk = devm_clk_get(dev, "csr");
...
pxpcs->eee_clk = devm_clk_get(dev, "eee");
...
pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
1000000000UL) - 1;
so the naming looks consistent with the implementation; a sentence in the
changelog noting that this integration does not use the core/pad clocks
would make the difference from snps,dw-xpcs obvious.
[ ... ]
> +patternProperties:
> + "^ethernet-pcs-mii@[0-3]$":
> + type: object
> + description:
> + One of the four MII ports of the XPCS. The port is linked to an
> + Ethernet MAC controller via the pcs-handle property in the MAC's
> + device tree node.
[Severity: Low]
This isn't a bug, but the changelog doesn't mention the two patches that
consume these child nodes, which makes the new ABI look unimplemented when
the binding patch is read on its own.
The consumers are:
drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_create() {
if (of_property_read_u32(np, "reg", &port))
return ERR_PTR(-EINVAL);
...
xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:rk_pcs_init() {
pcs_node = of_parse_phandle(np, "pcs-handle", 0);
...
xpcs = xpcs_rk_create(priv->device, pcs_node);
Could the commit message reference them?
[ ... ]
> +required:
> + - compatible
> + - reg
> + - "#address-cells"
> + - "#size-cells"
> + - clocks
> + - clock-names
> + - phys
> + - phy-names
> + - power-domains
> +
> +additionalProperties: false
[Severity: Low]
The RK3568 CRU defines four XPCS soft resets:
include/dt-bindings/clock/rk3568-cru.h:
#define SRST_XPCS 150
#define SRST_XPCS_TX_DIV10 151
#define SRST_XPCS_RX_DIV10 152
#define SRST_XPCS_XGXS_RX 153
but the schema has no resets/reset-names, so a device tree cannot express
them. Is that because bring-up relies only on the power domain, the
SerDes PHY and the XPCS register-level soft reset?
drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_probe() {
ret = dev_pm_genpd_rpm_always_on(dev, true);
...
ret = phy_init(pxpcs->serdes_phy);
...
ret = phy_power_on(pxpcs->serdes_phy);
A grep at the end of the series shows SRST_XPCS* matching only the
dt-bindings header, with no driver or DTS user, so this may well be
intentional; a note confirming the resets are left at their SoC defaults
would settle it.
Documenting them now as optional properties would also stay backwards
compatible if they turn out to be needed later.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 05/11] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
` (3 preceding siblings ...)
2026-09-13 18:58 ` [PATCH net-next v5 04/11] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery Coia Prant
` (5 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
The RK3568 SoC integrates a Synopsys DesignWare XPCS that provides
the Physical Coding Sublayer for 1000BASE-X, SGMII, and QSGMII
interfaces via its four MII ports. Add the XPCS device node and
its pcs-mii sub-nodes to the SoC device tree.
The XPCS device is accessed via the APB3 bus at 0xfda00000 and
requires the CSR clock (PCLK_XPCS) for register access and the EEE
clock (CLK_XPCS_EEE) for Energy Efficient Ethernet operation. The
PD_PIPE power domain must be enabled before any register access.
Also add two fixed-clock nodes (xpcs_gmac0_clk and xpcs_gmac1_clk)
providing the 125 MHz reference clock for the GMACs when operating
with XPCS. These clocks are used as the assigned-clock-parents
for the respective GMAC nodes in board-level device trees.
The XPCS node and its pcs-mii sub-nodes are disabled by default and
must be enabled at the board level when 1000BASE-X/SGMII/QSGMII is
in use. The fixed-clock nodes are always present and do not have a
status property, as they are static clock sources.
The XPCS node also requires a reference to the appropriate Naneng
Combo PHY via the phys property at the board level.
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
arch/arm64/boot/dts/rockchip/rk3568.dtsi | 45 ++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
index 3bc653f027f1f..227d03e336043 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
@@ -110,6 +110,51 @@ sata0: sata@fc000000 {
status = "disabled";
};
+ xpcs: ethernet-pcs@fda00000 {
+ compatible = "rockchip,rk3568-xpcs";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x0 0xfda00000 0x0 0x200000>;
+ clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
+ clock-names = "csr", "eee";
+ power-domains = <&power RK3568_PD_PIPE>;
+ status = "disabled";
+
+ xpcs_mii0: ethernet-pcs-mii@0 {
+ reg = <0>;
+ status = "disabled";
+ };
+
+ xpcs_mii1: ethernet-pcs-mii@1 {
+ reg = <1>;
+ status = "disabled";
+ };
+
+ xpcs_mii2: ethernet-pcs-mii@2 {
+ reg = <2>;
+ status = "disabled";
+ };
+
+ xpcs_mii3: ethernet-pcs-mii@3 {
+ reg = <3>;
+ status = "disabled";
+ };
+ };
+
+ clk_gmac0_xpcs_mii: clock-xpcs-gmac0 {
+ compatible = "fixed-clock";
+ clock-frequency = <125000000>;
+ clock-output-names = "clk_gmac0_xpcs_mii";
+ #clock-cells = <0>;
+ };
+
+ clk_gmac1_xpcs_mii: clock-xpcs-gmac1 {
+ compatible = "fixed-clock";
+ clock-frequency = <125000000>;
+ clock-output-names = "clk_gmac1_xpcs_mii";
+ #clock-cells = <0>;
+ };
+
pipe_phy_grf0: syscon@fdc70000 {
compatible = "rockchip,rk3568-pipe-phy-grf", "syscon";
reg = <0x0 0xfdc70000 0x0 0x1000>;
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 05/11] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes
2026-09-13 18:58 ` [PATCH net-next v5 05/11] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 1
- [Medium] DT/binding mismatch: the new `xpcs: ethernet-pcs@fda00000`
node in `arch/arm64/boot/dts/rockchip/rk3568.dtsi` (line 113) omits…
- [Low] Claimed missing driver/consumer for the new DT nodes: no driver
matches `rockchip,rk3568-xpcs`, the `ethernet-pcs-mii@N` children are…
--
arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes
Adds the RK3568 XPCS node at 0xfda00000 with its four ethernet-pcs-mii
child nodes, all disabled by default, plus two 125 MHz fixed-clock nodes
intended as assigned-clock-parents for gmac0 and gmac1.
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> index 3bc653f027f1f..227d03e336043 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> @@ -110,6 +110,51 @@ sata0: sata@fc000000 {
> status = "disabled";
> };
>
> + xpcs: ethernet-pcs@fda00000 {
> + compatible = "rockchip,rk3568-xpcs";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x0 0xfda00000 0x0 0x200000>;
> + clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
> + clock-names = "csr", "eee";
> + power-domains = <&power RK3568_PD_PIPE>;
> + status = "disabled";
[Severity: Medium]
Does this node validate against the binding added earlier in the series?
Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml lists
phys and phy-names in its top-level required array, unconditionally, and
sets additionalProperties: false:
required:
- compatible
- reg
- "#address-cells"
- "#size-cells"
- clocks
- clock-names
- phys
- phy-names
- power-domains
The node here supplies clocks and power-domains but not phys/phy-names.
dtschema validates nodes irrespective of status = "disabled", so does
make dtbs_check now report "'phys' is a required property" and
"'phy-names' is a required property" for every rk3568 board DTB? At the
end of the series only rk3568-photonicat.dts fills them in:
&xpcs {
phys = <&combphy2 PHY_TYPE_SGMII>;
phy-names = "serdes";
status = "okay";
};
The other 21 device trees that include rk3568.dtsi (rk3568-bpi-r2-pro.dts,
rk3568-rock-3a.dts, rk3568-odroid-m1.dts, rk3568-nanopi-r5s.dtsi,
rk3568-qnap-ts433.dts, ...) do not.
The commit message says "The XPCS node also requires a reference to the
appropriate Naneng Combo PHY via the phys property at the board level."
If that is the intended model, should phys/phy-names be dropped from the
binding's required list (or gated), rather than left mandatory?
Alternatively the SoC dtsi could carry a default phandle for boards to
override.
> +
> + xpcs_mii0: ethernet-pcs-mii@0 {
> + reg = <0>;
> + status = "disabled";
> + };
[ ... ]
> + clk_gmac0_xpcs_mii: clock-xpcs-gmac0 {
> + compatible = "fixed-clock";
> + clock-frequency = <125000000>;
> + clock-output-names = "clk_gmac0_xpcs_mii";
> + #clock-cells = <0>;
> + };
[Severity: Low]
One point that was checked and turned out not to be an issue, noted here
only for completeness: the compatible string, the ethernet-pcs-mii@N
children and these two fixed clocks all gain in-tree consumers later in
the same series, so there is nothing to change here.
drivers/net/pcs/pcs-xpcs-rk.c matches the compatible:
static const struct of_device_id xpcs_rk_of_ids[] = {
{ .compatible = "rockchip,rk3568-xpcs" },
{ /* sentinel */ },
};
and rk3568-photonicat.dts consumes both the child node and the clock:
assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
assigned-clock-parents = <&clk_gmac0_xpcs_mii>;
managed = "in-band-status";
pcs-handle = <&xpcs_mii0>;
Until the driver patch lands the nodes are disabled and inert, which is
the usual bindings-then-dtsi-then-driver ordering.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
` (4 preceding siblings ...)
2026-09-13 18:58 ` [PATCH net-next v5 05/11] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver Coia Prant
` (4 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc,
Jiawen Wu
On some hardware using the DesignWare XPCS IP (e.g., RK3568 MAC side
SGMII), the PCS does not automatically restart auto-negotiation when the
link goes down and comes back up. Without an explicit ANRESTART, the link
stays down forever.
Add BMCR_ANRESTART in two places:
1. In xpcs_config_aneg_c37_sgmii(), when starting AN, set ANRESTART
alongside ANENABLE to initiate a fresh negotiation.
2. In xpcs_get_state_c37_sgmii(), when link is down and AN completion is
detected, clear the interrupt and trigger ANRESTART to restart the
negotiation process. Propagate the return value of the restart so
errors are not silently ignored.
Update the comment in xpcs_config_aneg_c37_sgmii() to note that although
the DesignWare databook says AN restart is not needed for MAC side SGMII,
some implementations (e.g. Rockchip RK3568) require it to recover the
link after a disconnect.
This is not a fix for an existing mainline platform: the affected
platform (RK3568 XPCS) is introduced later in the same series.
Tested-by: Jiawen Wu <jiawenwu@trustnetic.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
drivers/net/pcs/pcs-xpcs.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 0337e2bcc0125..8c3875b6985b9 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -761,7 +761,9 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
* DW xPCS used with DW EQoS MAC is always MAC side SGMII.
* 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic
* speed/duplex mode change by HW after SGMII AN complete)
- * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN)
+ * 5) VR_MII_AN_INTR_STS = 0x0 (Clear CL37 AN complete status)
+ * 6) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN)
+ * VR_MII_MMD_CTRL Bit(9) [AN_RESTART] = 1b (Restart SGMII AN)
*
* Note that VR_MII_MMD_CTRL is MII_BMCR.
*
@@ -769,7 +771,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
* SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
* PHY about the link state change after C28 AN is completed
* between PHY and Link Partner. There is also no need to
- * trigger AN restart for MAC-side SGMII.
+ * trigger AN restart for MAC-side SGMII on most devices.
+ *
+ * Note: While the DesignWare databook states that AN restart is
+ * not needed for MAC side SGMII, some implementations (e.g.
+ * Rockchip RK3568) exhibit a timing quirk when integrated with
+ * phylink and do not restart AN automatically when the link
+ * comes back up. An explicit AN restart is required on those
+ * parts to recover the link after a disconnect.
*/
mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
if (mdio_ctrl < 0)
@@ -816,9 +825,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
if (ret < 0)
return ret;
+ /* Clear CL37 AN complete status */
+ ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+ if (ret < 0)
+ return ret;
+
if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
- mdio_ctrl | BMCR_ANENABLE);
+ mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
return ret;
}
@@ -1093,9 +1107,18 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
return 0;
}
- /* Clear AN complete status or interrupt */
- if (state->an_complete)
- xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+ if (state->an_complete) {
+ /* Clear AN complete status or interrupt */
+ ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+ if (ret < 0)
+ return ret;
+
+ /* Initiate the next round of AN */
+ ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
+ BMCR_ANRESTART);
+ if (ret < 0)
+ return ret;
+ }
return 0;
}
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery
2026-09-13 18:58 ` [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 4 · Low: 0
- [High] In xpcs_get_state_c37_sgmii() the combination
'DW_VR_MII_AN_STS_C37_ANCMPLT_INTR set + DW_VR_MII_C37_ANSGM_SP_LNKSTS…
- [Medium] Undisclosed functional change: besides the two BMCR_ANRESTART
additions the commit message advertises, the patch adds a new…
- [Medium] Device-specific quirk applied unconditionally: the commit
message and the new comment at drivers/net/pcs/pcs-xpcs.c:776-781…
- [Medium] Wrong ordering in xpcs_get_state_c37_sgmii(): the latched AN-
complete indication is consumed (xpcs_write(DW_VR_MII_AN_INTR_STS, 0),…
- [Medium] The patch turns the read-only-by-contract pcs_get_state()
callback into a hardware-state-changing operation (BMCR read-modify-…
--
net: pcs: xpcs: add ANRESTART support for SGMII link recovery
The patch sets BMCR_ANRESTART together with BMCR_ANENABLE when
xpcs_config_aneg_c37_sgmii() enables in-band AN, and makes
xpcs_get_state_c37_sgmii() clear DW_VR_MII_AN_INTR_STS and assert
BMCR_ANRESTART when the link is down with AN completion latched. A new
comment explains that the DesignWare databook says AN restart is not
needed for MAC side SGMII, but that some parts (Rockchip RK3568) need it.
A few questions below.
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 0337e2bcc0125..8c3875b6985b9 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -769,7 +771,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
> * SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
> * PHY about the link state change after C28 AN is completed
> * between PHY and Link Partner. There is also no need to
> - * trigger AN restart for MAC-side SGMII.
> + * trigger AN restart for MAC-side SGMII on most devices.
> + *
> + * Note: While the DesignWare databook states that AN restart is
> + * not needed for MAC side SGMII, some implementations (e.g.
> + * Rockchip RK3568) exhibit a timing quirk when integrated with
> + * phylink and do not restart AN automatically when the link
> + * comes back up. An explicit AN restart is required on those
> + * parts to recover the link after a disconnect.
> */
[Severity: Medium]
The comment says the restart is only "required on those parts" and that
there is "no need to trigger AN restart for MAC-side SGMII on most
devices", and the commit message says "This is not a fix for an existing
mainline platform". The code, however, applies the restart on every
device: no gate on xpcs->info.pma or xpcs->desc, and struct dw_xpcs gains
no quirk flag.
The same function already uses that gating idiom a few lines below:
if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
mask |= DW_VR_MII_AN_CTRL_8BIT;
val |= DW_VR_MII_AN_CTRL_8BIT;
/* Hardware requires it to be PHY side SGMII */
tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
Note that this TXGBE case is configured as PHY-side SGMII, which the
MAC-side/RK3568 rationale in the comment does not cover.
Should the new restart be gated on the affected PMA (or on a new dw_xpcs
quirk flag) so the code matches what the comment and the commit message
describe?
Also, phylink already has a path for this: pcs_config() may return a
positive value to ask phylink to call .pcs_an_restart, and
xpcs_an_restart() in this same driver performs exactly the same register
write:
xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
BMCR_ANRESTART);
Would using that framework path be preferable to open-coding the restart
in the config and get_state callbacks?
> @@ -816,9 +825,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
> if (ret < 0)
> return ret;
>
> + /* Clear CL37 AN complete status */
> + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> + if (ret < 0)
> + return ret;
> +
[Severity: Medium]
The commit message describes the change as "Add BMCR_ANRESTART in two
places" plus a comment update, but this hunk adds a third functional
change that is only visible as item "5)" in the in-code numbered list.
This write is unconditional: it runs for every xpcs_config() ->
xpcs_do_config() -> xpcs_config_aneg_c37_sgmii() call and for every
neg_mode, including PHYLINK_PCS_NEG_OUTBAND where no AN is subsequently
enabled. It clears the register that xpcs_get_state_c37_sgmii() uses as
its only source of link/speed/duplex and of state->an_complete:
ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
It also changes the value returned when neg_mode is not
PHYLINK_PCS_NEG_INBAND_ENABLED: the function now returns the result of
this AN_INTR_STS write instead of the DIG_CTRL1 modify.
Could the commit message mention this status clear and the return value
change? For reference, xpcs_config_aneg_c37_1000basex() already performs
the identical clear, so the write itself matches existing driver practice.
> if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> - mdio_ctrl | BMCR_ANENABLE);
> + mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
>
> return ret;
> }
> @@ -1093,9 +1107,18 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
> return 0;
> }
>
> - /* Clear AN complete status or interrupt */
> - if (state->an_complete)
> - xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> + if (state->an_complete) {
> + /* Clear AN complete status or interrupt */
> + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> + if (ret < 0)
> + return ret;
> +
> + /* Initiate the next round of AN */
> + ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> + BMCR_ANRESTART);
> + if (ret < 0)
> + return ret;
> + }
[Severity: High]
Is the trigger condition here really "stuck AN"? The state reached at
this point is ANCMPLT latched with LNKSTS clear, taken from the same
register read:
state->an_complete = ret & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
if (ret & DW_VR_MII_C37_ANSGM_SP_LNKSTS) {
...
return 0;
}
For C37/SGMII, isn't that the normal steady state whenever the partner
PHY's media side is down? The PHY keeps sending config words, the SGMII
AN exchange completes and latches ANCMPLT (bit 0), and the media link
state is carried separately in the received config word via LNKSTS
(bit 4), per pcs-xpcs.h:
#define DW_VR_MII_AN_STS_C37_ANCMPLT_INTR BIT(0)
...
#define DW_VR_MII_C37_ANSGM_SP_LNKSTS BIT(4)
If so, then on an unplugged port each restart re-completes within the AN
link timer with LNKSTS still 0 and re-latches ANCMPLT, so the condition
re-arms itself and a fresh AN restart is issued on every poll,
indefinitely. phylink polls pcs_get_state once per second and
xpcs->pcs.poll is true for everything except WX_TXGBE and MP_FBNIC, so
this affects the existing DW_AN_C37_SGMII users (stmmac/Intel mGbE, NXP
SJA1105/SJA1110, Wangxun), not only RK3568.
Two further consequences: with MAC_AUTO_SW armed, the hardware-selected
speed/duplex is dropped on each cycle, and because the whole status word
is cleared by writing 0, a restart that lands on a just-latched LNKSTS=1
word destroys that link-up indication and costs an extra poll interval.
For the interrupt-driven case there may also be a faster loop. The SGMII
AN_CTRL modify in xpcs_config_aneg_c37_sgmii() does not include
DW_VR_MII_AN_INTR_EN in its mask, while the 1000BASE-X path sets it:
if (!xpcs->pcs.poll) {
mask |= DW_VR_MII_AN_INTR_EN;
val |= DW_VR_MII_AN_INTR_EN;
}
so an enable programmed by a previous 1000BASE-X configuration persists
into SGMII mode. On txgbe the AN event is routed to the link IRQ:
drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c:txgbe_misc_irq_handle() {
if (eicr & (TXGBE_PX_MISC_ETH_LK | TXGBE_PX_MISC_ETH_LKDN |
TXGBE_PX_MISC_ETH_AN)) {
sub_irq = irq_find_mapping(txgbe->misc.domain, TXGBE_IRQ_LINK);
...
}
which ends in phylink_pcs_change() -> pcs_get_state(), so the restart can
be re-triggered at hardware rate rather than once per second.
Would gating this on the affected PMA/quirk, or on an explicit "AN is not
active" hardware condition rather than on the latched AN-complete bit,
address this?
[Severity: Medium]
Is the ordering here recoverable if the second access fails? The latched
AN-complete indication is consumed by the xpcs_write(DW_VR_MII_AN_INTR_STS,
0) before the fallible read-modify-write that actually starts the next
negotiation.
If the write succeeds and the xpcs_modify() of MII_BMCR fails on the
register back-end, the only indication that AN completed is gone and no
new negotiation was started. The block that would retry is itself gated
on state->an_complete, which is now read back as 0, so the restart is
never attempted again. The error only reaches xpcs_get_state(), which
logs it from a void callback:
ret = xpcs_get_state_c37_sgmii(xpcs, state);
if (ret)
dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
"xpcs_get_state_c37_sgmii", ERR_PTR(ret));
and nothing records a pending restart. This matters most on the hardware
this patch targets, where AN is not restarted automatically, so the port
would stay down until a fresh pcs_config(). The failure is plausible for
these vendor-MMD accesses: the RK3568 back-end added later in this series
returns errors from pm_runtime_resume_and_get() inside
xpcs_rk_write_reg()/xpcs_rk_read_reg().
Would asserting BMCR_ANRESTART before clearing the status word, or
retrying independently of the latch, close this window?
[Severity: Medium]
This makes pcs_get_state() change hardware state rather than only read it,
and phylink calls that callback from contexts with no common lock.
phylink_resolve() holds pl->state_mutex (and the phy locks) but not RTNL,
while these two callers hold only RTNL:
drivers/net/phy/phylink.c:phylink_ethtool_ksettings_get() {
...
phylink_mac_pcs_get_state(pl, &link_state);
...
}
drivers/net/phy/phylink.c:phylink_mii_read() {
...
phylink_mac_pcs_get_state(pl, &state);
...
}
The lockset intersection is empty, and the driver holds no private lock,
so two CPUs can run this block at once and both assert BMCR_ANRESTART,
aborting each other's negotiation. It can also interleave with the
multi-transaction BMCR sequence in xpcs_config_aneg_c37_sgmii(), which
reads BMCR, writes it with ANENABLE cleared, programs AN_CTRL/DIG_CTRL1,
clears AN_INTR_STS, and finally writes the cached value back:
mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
...
ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
mdio_ctrl & ~BMCR_ANENABLE);
Each xpcs_read/xpcs_write is a separate bus transaction, so an AN restart
can land while AN is disabled and the AN configuration is only half
applied.
Both RTNL-only paths need the PCS-only case (no phydev), which is
reachable for inband SGMII without a PHY, and SIOCGMIIREG and
ETHTOOL_GLINKSETTINGS are unprivileged. Before this patch these paths
only cleared the AN status word; should the restart be moved to a context
that is serialized with pcs_config(), such as the .pcs_an_restart op?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
` (5 preceding siblings ...)
2026-09-13 18:58 ` [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle Coia Prant
` (3 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
The RK3568 SoC integrates a Synopsys DesignWare XPCS that is accessed
via APB3 memory-mapped registers.
This driver provides the glue logic to make the XPCS accessible to
the generic pcs-xpcs core.
The XPCS block contains four MII ports (0..3), each of which can be
routed to GMAC0 or GMAC1 via the pcs-handle property in the MAC node.
The hardware maps these ports to different MMDs:
- port 0: MMD 7 (ROCKCHIP_MMD_MII)
- port 1: MMD 2 (ROCKCHIP_MMD_MII1)
- port 2: MMD 3 (ROCKCHIP_MMD_MII2)
- port 3: MMD 4 (ROCKCHIP_MMD_MII3)
This driver creates a virtual MDIO bus that translates MDIO operations
to APB3 register accesses, with proper address remapping for each port.
The generic xpcs driver then creates a phylink_pcs instance on top of
this bus, allowing the MAC to use the PCS via the standard phylink API.
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
drivers/net/pcs/Kconfig | 25 ++
drivers/net/pcs/Makefile | 5 +-
drivers/net/pcs/pcs-xpcs-rk.c | 559 ++++++++++++++++++++++++++++++++
include/linux/pcs/pcs-xpcs-rk.h | 11 +
4 files changed, 598 insertions(+), 2 deletions(-)
create mode 100644 drivers/net/pcs/pcs-xpcs-rk.c
create mode 100644 include/linux/pcs/pcs-xpcs-rk.h
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index e417fd66f660a..080538f193d59 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -12,6 +12,31 @@ config PCS_XPCS
This module provides a driver and helper functions for Synopsys
DesignWare XPCS controllers.
+if PCS_XPCS
+
+config PCS_XPCS_PLATFORM
+ tristate "Generic XPCS controller support"
+ default PCS_XPCS
+ help
+ Generic DWXPCS driver for platforms that don't require any
+ platform specific code to function or is using platform
+ data for setup.
+
+ If you have a controller with this interface, say Y or M here.
+
+config PCS_XPCS_ROCKCHIP
+ tristate "Rockchip XPCS controller support"
+ default ARCH_ROCKCHIP
+ depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
+ depends on GENERIC_PHY || COMPILE_TEST
+ depends on PM_GENERIC_DOMAINS || COMPILE_TEST
+ help
+ Support for XPCS controller on Rockchip RK356x SoC.
+
+ If you have a Rockchip SoC with this interface, say Y or M here.
+
+endif # PCS_XPCS
+
config PCS_LYNX
tristate
help
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b900..f9f6cf2578d72 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,10 +1,11 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for Linux PCS drivers
-pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
- pcs-xpcs-nxp.o pcs-xpcs-wx.o
+pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o
obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o
+obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
+obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.o
obj-$(CONFIG_PCS_LYNX) += pcs-lynx.o
obj-$(CONFIG_PCS_MTK_LYNXI) += pcs-mtk-lynxi.o
obj-$(CONFIG_PCS_RZN1_MIIC) += pcs-rzn1-miic.o
diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
new file mode 100644
index 0000000000000..7fd1408c044cc
--- /dev/null
+++ b/drivers/net/pcs/pcs-xpcs-rk.c
@@ -0,0 +1,559 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Rockchip XPCS platform device driver
+ *
+ * Based on the Synopsys DesignWare XPCS platform driver.
+ * Copyright (C) 2024 Serge Semin
+ *
+ * Adapted for Rockchip SoCs, with reference to the Rockchip OEM driver.
+ * Copyright (C) 2026 Coia Prant
+ */
+
+#include <linux/atomic.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/math.h>
+#include <linux/mdio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pcs/pcs-xpcs-rk.h>
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/sizes.h>
+
+#include "pcs-xpcs.h"
+
+struct dw_xpcs_rk {
+ struct platform_device *pdev;
+ struct mii_bus *bus;
+ void __iomem *reg_base;
+ struct phy *serdes_phy;
+ struct clk *csr_clk;
+ struct clk *eee_clk;
+ u8 eee_mult_fact;
+};
+
+static ptrdiff_t xpcs_rk_addr_format(int dev, int reg)
+{
+ return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);
+}
+
+static int xpcs_rk_read_reg(struct dw_xpcs_rk *pxpcs, int dev, int reg)
+{
+ ptrdiff_t csr;
+ int ret;
+
+ csr = xpcs_rk_addr_format(dev, reg);
+
+ ret = pm_runtime_resume_and_get(&pxpcs->pdev->dev);
+ if (ret)
+ return ret;
+
+ ret = readl(pxpcs->reg_base + (csr << 2)) & 0xffff;
+
+ pm_runtime_put(&pxpcs->pdev->dev);
+ return ret;
+}
+
+static int xpcs_rk_write_reg(struct dw_xpcs_rk *pxpcs, int dev, int reg, u16 val)
+{
+ ptrdiff_t csr;
+ int ret;
+
+ csr = xpcs_rk_addr_format(dev, reg);
+
+ ret = pm_runtime_resume_and_get(&pxpcs->pdev->dev);
+ if (ret)
+ return ret;
+
+ writel(val, pxpcs->reg_base + (csr << 2));
+
+ pm_runtime_put(&pxpcs->pdev->dev);
+ return 0;
+}
+
+#define ROCKCHIP_MMD_MII1 2
+#define ROCKCHIP_MMD_MII2 3
+#define ROCKCHIP_MMD_MII3 4
+#define ROCKCHIP_MMD_PMAPMD 6
+#define ROCKCHIP_MMD_MII 7
+
+static bool xpcs_rk_mdio_addr_validate(int addr)
+{
+ return !(addr < 0 || addr > 3);
+}
+
+static int xpcs_rk_mdio_read_remapping(int addr, int dev, int reg)
+{
+ switch (dev) {
+ case MDIO_MMD_PMAPMD:
+ return ROCKCHIP_MMD_PMAPMD;
+ case MDIO_MMD_VEND2:
+ break;
+ default:
+ return -ENXIO;
+ }
+
+ /* read remapping to MII is performed by HW */
+ switch (addr) {
+ case 0:
+ return ROCKCHIP_MMD_MII;
+ case 1:
+ return ROCKCHIP_MMD_MII1;
+ case 2:
+ return ROCKCHIP_MMD_MII2;
+ case 3:
+ return ROCKCHIP_MMD_MII3;
+ default:
+ return -ENODEV;
+ }
+}
+
+static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
+{
+ switch (dev) {
+ case MDIO_MMD_PMAPMD:
+ return ROCKCHIP_MMD_PMAPMD;
+ case MDIO_MMD_VEND2:
+ break;
+ default:
+ return -ENXIO;
+ }
+
+ /* Writable only on MII */
+ switch (reg) {
+ case DW_VR_MII_AN_CTRL:
+ case DW_VR_MII_AN_INTR_STS:
+ case DW_VR_MII_EEE_MCTRL0:
+ case DW_VR_MII_EEE_MCTRL1:
+ case DW_VR_MII_DIG_CTRL2:
+ return ROCKCHIP_MMD_MII;
+ default:
+ break;
+ }
+
+ switch (addr) {
+ case 0:
+ return ROCKCHIP_MMD_MII;
+ case 1:
+ return ROCKCHIP_MMD_MII1;
+ case 2:
+ return ROCKCHIP_MMD_MII2;
+ case 3:
+ return ROCKCHIP_MMD_MII3;
+ default:
+ return -ENODEV;
+ }
+}
+
+static int xpcs_rk_read_c22(struct mii_bus *bus, int addr, int reg)
+{
+ struct dw_xpcs_rk *pxpcs = bus->priv;
+ int dev;
+
+ if (!xpcs_rk_mdio_addr_validate(addr))
+ return -ENODEV;
+
+ dev = xpcs_rk_mdio_read_remapping(addr, MDIO_MMD_VEND2, reg);
+ if (dev < 0)
+ return 0xffff;
+
+ return xpcs_rk_read_reg(pxpcs, dev, reg);
+}
+
+static int xpcs_rk_write_c22(struct mii_bus *bus, int addr, int reg, u16 val)
+{
+ struct dw_xpcs_rk *pxpcs = bus->priv;
+ int dev;
+
+ if (!xpcs_rk_mdio_addr_validate(addr))
+ return -ENODEV;
+
+ dev = xpcs_rk_mdio_write_remapping(addr, MDIO_MMD_VEND2, reg);
+ if (dev < 0)
+ return 0;
+
+ return xpcs_rk_write_reg(pxpcs, dev, reg, val);
+}
+
+static int xpcs_rk_read_c45(struct mii_bus *bus, int addr, int dev, int reg)
+{
+ struct dw_xpcs_rk *pxpcs = bus->priv;
+
+ if (!xpcs_rk_mdio_addr_validate(addr))
+ return -ENODEV;
+
+ dev = xpcs_rk_mdio_read_remapping(addr, dev, reg);
+ if (dev < 0)
+ return 0xffff;
+
+ return xpcs_rk_read_reg(pxpcs, dev, reg);
+}
+
+static int xpcs_rk_write_c45(struct mii_bus *bus, int addr, int dev, int reg, u16 val)
+{
+ struct dw_xpcs_rk *pxpcs = bus->priv;
+
+ if (!xpcs_rk_mdio_addr_validate(addr))
+ return -ENODEV;
+
+ dev = xpcs_rk_mdio_write_remapping(addr, dev, reg);
+ if (dev < 0)
+ return 0;
+
+ return xpcs_rk_write_reg(pxpcs, dev, reg, val);
+}
+
+static struct dw_xpcs_rk *xpcs_rk_create_data(struct platform_device *pdev)
+{
+ struct dw_xpcs_rk *pxpcs;
+
+ pxpcs = devm_kzalloc(&pdev->dev, sizeof(*pxpcs), GFP_KERNEL);
+ if (!pxpcs)
+ return ERR_PTR(-ENOMEM);
+
+ pxpcs->pdev = pdev;
+
+ dev_set_drvdata(&pdev->dev, pxpcs);
+
+ return pxpcs;
+}
+
+static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
+{
+ struct device *dev = &pxpcs->pdev->dev;
+
+ pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
+ if (IS_ERR(pxpcs->serdes_phy))
+ return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
+ "Failed to get SerDes PHY\n");
+
+ return 0;
+}
+
+static void xpcs_rk_serdes_phy_poweroff(void *data)
+{
+ struct dw_xpcs_rk *pxpcs = data;
+ struct device *dev = &pxpcs->pdev->dev;
+
+ phy_power_off(pxpcs->serdes_phy);
+ phy_exit(pxpcs->serdes_phy);
+
+ dev_pm_genpd_rpm_always_on(dev, false);
+}
+
+static int xpcs_rk_serdes_phy_poweron(struct dw_xpcs_rk *pxpcs)
+{
+ struct device *dev = &pxpcs->pdev->dev;
+ int ret;
+
+ /*
+ * The power domain is required and must be enabled, which allows us to
+ * dynamically turn the CSR clock on/off using PM while keeping the PCS
+ * powered on.
+ */
+ ret = dev_pm_genpd_rpm_always_on(dev, true);
+ if (ret) {
+ dev_err(dev, "Failed to power on power-domains\n");
+ return ret;
+ }
+
+ ret = phy_init(pxpcs->serdes_phy);
+ if (ret) {
+ dev_err(dev, "Failed to init SerDes PHY\n");
+ goto pm_domain;
+ }
+
+ ret = phy_power_on(pxpcs->serdes_phy);
+ if (ret) {
+ dev_err(dev, "Failed to power on SerDes PHY\n");
+ goto serdes_phy;
+ }
+
+ ret = devm_add_action_or_reset(dev, xpcs_rk_serdes_phy_poweroff, pxpcs);
+ if (ret) {
+ dev_err(dev, "Failed to register devm for SerDes PHY: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+
+serdes_phy:
+ phy_exit(pxpcs->serdes_phy);
+pm_domain:
+ dev_pm_genpd_rpm_always_on(dev, false);
+ return ret;
+}
+
+static int xpcs_rk_init_res(struct dw_xpcs_rk *pxpcs)
+{
+ struct platform_device *pdev = pxpcs->pdev;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(dev, "No reg-space found\n");
+ return -EINVAL;
+ }
+
+ if (resource_size(res) < SZ_2M) {
+ dev_err(dev, "Invalid reg-space size\n");
+ return -EINVAL;
+ }
+
+ pxpcs->reg_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pxpcs->reg_base)) {
+ dev_err(dev, "Failed to map reg-space\n");
+ return PTR_ERR(pxpcs->reg_base);
+ }
+
+ return 0;
+}
+
+static void xpcs_rk_exit_clk(void *data)
+{
+ struct dw_xpcs_rk *pxpcs = data;
+ struct device *dev = &pxpcs->pdev->dev;
+
+ pm_runtime_force_suspend(dev);
+ clk_disable_unprepare(pxpcs->eee_clk);
+}
+
+static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
+{
+ struct device *dev = &pxpcs->pdev->dev;
+ int ret;
+
+ pxpcs->csr_clk = devm_clk_get(dev, "csr");
+ if (IS_ERR(pxpcs->csr_clk))
+ return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
+ "Failed to get CSR clock\n");
+
+ pxpcs->eee_clk = devm_clk_get(dev, "eee");
+ if (IS_ERR(pxpcs->eee_clk))
+ return dev_err_probe(dev, PTR_ERR(pxpcs->eee_clk),
+ "Failed to get EEE clock\n");
+
+ ret = clk_prepare_enable(pxpcs->eee_clk);
+ if (ret) {
+ dev_err(dev, "Failed to enable EEE clock\n");
+ return ret;
+ }
+
+ /*
+ * Compute the multiplier for the EEE clock so that
+ * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
+ *
+ * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
+ * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
+ * The reset value is 0 (200 MHz), but derive the value at runtime to
+ * stay correct if the mux is changed by a board.
+ */
+ pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
+ 1000000000UL) - 1;
+
+ pm_runtime_set_suspended(dev);
+ pm_runtime_enable(dev);
+
+ ret = devm_add_action_or_reset(dev, xpcs_rk_exit_clk, pxpcs);
+ if (ret) {
+ dev_err(dev, "Failed to register devm for EEE clock: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int xpcs_rk_init_bus(struct dw_xpcs_rk *pxpcs)
+{
+ struct device *dev = &pxpcs->pdev->dev;
+ static atomic_t id = ATOMIC_INIT(-1);
+ struct mii_bus *bus;
+ int ret;
+
+ bus = devm_mdiobus_alloc_size(dev, 0);
+ if (!bus)
+ return -ENOMEM;
+
+ bus->name = "Rockchip DW XPCS MCI/APB3";
+ bus->read = xpcs_rk_read_c22;
+ bus->write = xpcs_rk_write_c22;
+ bus->read_c45 = xpcs_rk_read_c45;
+ bus->write_c45 = xpcs_rk_write_c45;
+ bus->phy_mask = ~0;
+ bus->parent = dev;
+ bus->priv = pxpcs;
+
+ snprintf(bus->id, MII_BUS_ID_SIZE,
+ "rockchip_dwxpcs-%x", atomic_inc_return(&id));
+
+ /*
+ * MDIO-bus here serves as just a back-end engine abstracting out
+ * the MDIO and MCI/APB3 IO interfaces utilized for the Rockchip DWXPCS CSRs
+ * access.
+ */
+ ret = devm_mdiobus_register(dev, bus);
+ if (ret) {
+ dev_err(dev, "Failed to create MDIO bus\n");
+ return ret;
+ }
+
+ pxpcs->bus = bus;
+ return 0;
+}
+
+static int xpcs_rk_probe(struct platform_device *pdev)
+{
+ struct dw_xpcs_rk *pxpcs;
+ int ret;
+
+ pxpcs = xpcs_rk_create_data(pdev);
+ if (IS_ERR(pxpcs))
+ return PTR_ERR(pxpcs);
+
+ /*
+ * The XPCS may be attached to a power domain (e.g. PD_PIPE). The domain
+ * must be powered on before any register access, otherwise the SoC will
+ * trigger a synchronous external abort (SError).
+ *
+ * Accessing the XPCS registers also requires a TX clock from the SerDes,
+ * which is needed for the soft reset.
+ */
+ ret = xpcs_rk_serdes_phy_init(pxpcs);
+ if (ret)
+ return ret;
+
+ ret = xpcs_rk_serdes_phy_poweron(pxpcs);
+ if (ret)
+ return ret;
+
+ ret = xpcs_rk_init_res(pxpcs);
+ if (ret)
+ return ret;
+
+ ret = xpcs_rk_init_clk(pxpcs);
+ if (ret)
+ return ret;
+
+ ret = xpcs_rk_init_bus(pxpcs);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct of_device_id xpcs_rk_of_ids[] = {
+ { .compatible = "rockchip,rk3568-xpcs" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, xpcs_rk_of_ids);
+
+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
+{
+ struct platform_device *pdev;
+ struct device_node *pcs_np;
+ struct dw_xpcs_rk *pxpcs;
+ struct dw_xpcs *xpcs;
+ u32 port;
+
+ if (!of_device_is_available(np))
+ return ERR_PTR(-ENODEV);
+
+ if (of_property_read_u32(np, "reg", &port))
+ return ERR_PTR(-EINVAL);
+
+ if (!xpcs_rk_mdio_addr_validate((int)port))
+ return ERR_PTR(-EINVAL);
+
+ /* The XPCS pdev is attached to the parent node */
+ pcs_np = of_get_parent(np);
+ if (!pcs_np)
+ return ERR_PTR(-ENODEV);
+
+ if (!of_device_is_available(pcs_np)) {
+ of_node_put(pcs_np);
+ return ERR_PTR(-ENODEV);
+ }
+
+ if (!of_match_node(xpcs_rk_of_ids, pcs_np)) {
+ of_node_put(pcs_np);
+ return ERR_PTR(-EINVAL);
+ }
+
+ pdev = of_find_device_by_node(pcs_np);
+ of_node_put(pcs_np);
+ if (!pdev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ device_lock(&pdev->dev);
+ pxpcs = platform_get_drvdata(pdev);
+ if (!pxpcs || !pxpcs->bus) {
+ device_unlock(&pdev->dev);
+ put_device(&pdev->dev);
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
+ if (IS_ERR(xpcs)) {
+ device_unlock(&pdev->dev);
+ put_device(&pdev->dev);
+ return xpcs;
+ }
+
+ if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
+ xpcs_destroy(xpcs);
+ device_unlock(&pdev->dev);
+ put_device(&pdev->dev);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);
+ device_unlock(&pdev->dev);
+ put_device(&pdev->dev);
+ return xpcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_rk_create);
+
+static int xpcs_rk_pm_runtime_suspend(struct device *dev)
+{
+ struct dw_xpcs_rk *pxpcs = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(pxpcs->csr_clk);
+
+ return 0;
+}
+
+static int xpcs_rk_pm_runtime_resume(struct device *dev)
+{
+ struct dw_xpcs_rk *pxpcs = dev_get_drvdata(dev);
+
+ return clk_prepare_enable(pxpcs->csr_clk);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(xpcs_rk_pm_ops,
+ xpcs_rk_pm_runtime_suspend,
+ xpcs_rk_pm_runtime_resume,
+ NULL);
+
+static struct platform_driver xpcs_rk_driver = {
+ .probe = xpcs_rk_probe,
+ .driver = {
+ .name = "rk_xpcs-dwxpcs",
+ .pm = pm_ptr(&xpcs_rk_pm_ops),
+ .of_match_table = xpcs_rk_of_ids,
+ },
+};
+module_platform_driver(xpcs_rk_driver);
+
+MODULE_DESCRIPTION("Rockchip XPCS platform device driver");
+MODULE_AUTHOR("Coia Prant <coiaprant@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/pcs/pcs-xpcs-rk.h b/include/linux/pcs/pcs-xpcs-rk.h
new file mode 100644
index 0000000000000..28723d5bd75cc
--- /dev/null
+++ b/include/linux/pcs/pcs-xpcs-rk.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_PCS_XPCS_ROCKCHIP_H
+#define __LINUX_PCS_XPCS_ROCKCHIP_H
+
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/pcs/pcs-xpcs.h>
+
+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np);
+
+#endif /* __LINUX_PCS_XPCS_ROCKCHIP_H */
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver
2026-09-13 18:58 ` [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 3 · Low: 2
- [High] Incorrect MMD write remapping for MII vendor registers breaks
configuration for ports 1, 2, and 3.
- [Medium] The patch silently re-packages the pre-existing generic
Synopsys XPCS platform driver.
- [Medium] xpcs_rk_init_clk() (drivers/net/pcs/pcs-xpcs-rk.c:361-362)
computes `pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 *…
- [Medium] Acquiring `device_lock()` of another device from within a
probe path causes a lockdep warning (recursive locking).
- [Low] Concern that the newly exported xpcs_rk_create()
(include/linux/pcs/pcs-xpcs-rk.h) has no in-tree caller and no Kconfig…
- [Low] The rockchip,rk3568-xpcs binding lists `phys` and `phy-names` in
its `required:` list…
--
net: pcs: xpcs: add Rockchip RK3568 platform glue driver
Adds a platform glue driver for the DesignWare XPCS block found on RK3568,
exposing the APB3-mapped XPCS registers as a virtual MDIO bus with per-port
MMD remapping so the generic pcs-xpcs core can create a phylink_pcs. Also
adds new Kconfig symbols for the generic and Rockchip XPCS glue and an
exported xpcs_rk_create() helper for MAC drivers.
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index e417fd66f660a..080538f193d59 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -12,6 +12,31 @@ config PCS_XPCS
> This module provides a driver and helper functions for Synopsys
> DesignWare XPCS controllers.
>
> +if PCS_XPCS
> +
> +config PCS_XPCS_PLATFORM
> + tristate "Generic XPCS controller support"
> + default PCS_XPCS
> + help
> + Generic DWXPCS driver for platforms that don't require any
> + platform specific code to function or is using platform
> + data for setup.
> +
> + If you have a controller with this interface, say Y or M here.
> +
> +config PCS_XPCS_ROCKCHIP
> + tristate "Rockchip XPCS controller support"
> + default ARCH_ROCKCHIP
> + depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
> + depends on GENERIC_PHY || COMPILE_TEST
> + depends on PM_GENERIC_DOMAINS || COMPILE_TEST
> + help
> + Support for XPCS controller on Rockchip RK356x SoC.
> +
> + If you have a Rockchip SoC with this interface, say Y or M here.
> +
> +endif # PCS_XPCS
> +
> config PCS_LYNX
> tristate
> help
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 4f7920618b900..f9f6cf2578d72 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -1,10 +1,11 @@
> # SPDX-License-Identifier: GPL-2.0
> # Makefile for Linux PCS drivers
>
> -pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
> - pcs-xpcs-nxp.o pcs-xpcs-wx.o
> +pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o
>
> obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o
> +obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
> +obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.o
[Severity: Medium]
Should this repackaging of the pre-existing generic platform glue be a
separate patch, or at least be described in the commit message?
pcs-xpcs-plat.o is moved out of the pcs_xpcs composite object and built as
its own module, and the new PCS_XPCS_PLATFORM tristate makes the snps,dw-xpcs
platform glue user-selectable where it was previously unconditional under
PCS_XPCS. So the snps,dw-xpcs platform driver ships in pcs-xpcs-plat.ko
instead of pcs_xpcs.ko after this patch.
The commit message only describes adding the Rockchip glue driver and does
not mention this change to existing functionality. Could the Makefile and
Kconfig refactor be split out with its own rationale?
> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..7fd1408c044cc
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
> @@ -0,0 +1,559 @@
[ ... ]
> +static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
> +{
> + struct device *dev = &pxpcs->pdev->dev;
> +
> + pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
> + if (IS_ERR(pxpcs->serdes_phy))
> + return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
> + "Failed to get SerDes PHY\n");
> +
> + return 0;
> +}
[Severity: Low]
xpcs_rk_serdes_phy_init() treats the "serdes" phy as mandatory and fails
probe when it cannot be resolved, and the binding added later in this series
(Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml) lists
phys and phy-names under required:
required:
- compatible
- reg
...
- phys
- phy-names
- power-domains
The SoC node added in arch/arm64/boot/dts/rockchip/rk3568.dtsi only has:
xpcs: ethernet-pcs@fda00000 {
compatible = "rockchip,rk3568-xpcs";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x0 0xfda00000 0x0 0x200000>;
clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
clock-names = "csr", "eee";
power-domains = <&power RK3568_PD_PIPE>;
status = "disabled";
CHECK_DTBS validates nodes regardless of status, so does every RK3568 DTB
now report missing phys/phy-names for this node? Only
rk3568-photonicat.dts adds them, so a board that just flips this node to
status = "okay" would also fail probe with "Failed to get SerDes PHY".
Should the SoC node carry phys/phy-names, or should the binding not mark
them required at the SoC level?
[ ... ]
> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
[ ... ]
> + /*
> + * Compute the multiplier for the EEE clock so that
> + * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
> + *
> + * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
> + * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
> + * The reset value is 0 (200 MHz), but derive the value at runtime to
> + * stay correct if the mux is changed by a board.
> + */
> + pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
> + 1000000000UL) - 1;
[Severity: Medium]
Can this value overflow the 4-bit register field it ends up in?
For the reset configuration named in the comment (200 MHz, 5 ns) this
computes 100 * 200000000 / 1000000000 - 1 = 19. That is passed on
unchecked:
xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);
and the core programs it in xpcs_config_eee():
drivers/net/pcs/pcs-xpcs.c:
FIELD_PREP(DW_VR_MII_EEE_MULT_FACT_100NS,
xpcs->eee_mult_fact);
with
drivers/net/pcs/pcs-xpcs.h:
#define DW_VR_MII_EEE_MULT_FACT_100NS GENMASK(11, 8)
FIELD_PREP on a runtime value does no range check, so 19 (0x13) becomes 3.
The hardware is then told the 100 ns reference is 4 EEE clock periods
(20 ns) rather than the 80..120 ns the comment and the kernel-doc of
xpcs_config_eee_mult_fact() require, which affects all derived LPI timers
once EEE is enabled. Since the field caps the count at 16 periods, is 15
the only representable value for a 5 ns clock, and should this be clamped?
Two more things about the same expression:
- 100 * clk_get_rate() is evaluated in unsigned long, so on 32-bit builds
100 * 200000000 wraps to 2820130816 and yields mult_fact 2. Would
mul_u64_u32_div() or a u64 intermediate be better here?
- If clk_get_rate() returns 0, 0 - 1 stored in a u8 becomes 255, giving
field value 15. Is a zero rate worth rejecting?
[ ... ]
> @@ -0,0 +1,559 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip XPCS platform device driver
> + *
> + * Based on the Synopsys DesignWare XPCS platform driver.
> + * Copyright (C) 2024 Serge Semin
> + *
> + * Adapted for Rockchip SoCs, with reference to the Rockchip OEM driver.
> + * Copyright (C) 2026 Coia Prant
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/math.h>
> +#include <linux/mdio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pcs/pcs-xpcs-rk.h>
> +#include <linux/phy.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/sizes.h>
> +
> +#include "pcs-xpcs.h"
> +
> +struct dw_xpcs_rk {
> + struct platform_device *pdev;
> + struct mii_bus *bus;
> + void __iomem *reg_base;
> + struct phy *serdes_phy;
> + struct clk *csr_clk;
> + struct clk *eee_clk;
> + u8 eee_mult_fact;
> +};
> +
> +static ptrdiff_t xpcs_rk_addr_format(int dev, int reg)
> +{
> + return FIELD_PREP(0x70000, dev) | FIELD_PREP(0xffff, reg);
> +}
> +
> +static int xpcs_rk_read_reg(struct dw_xpcs_rk *pxpcs, int dev, int reg)
> +{
> + ptrdiff_t csr;
> + int ret;
> +
> + csr = xpcs_rk_addr_format(dev, reg);
> +
> + ret = pm_runtime_resume_and_get(&pxpcs->pdev->dev);
> + if (ret)
> + return ret;
> +
> + ret = readl(pxpcs->reg_base + (csr << 2)) & 0xffff;
> +
> + pm_runtime_put(&pxpcs->pdev->dev);
> + return ret;
> +}
> +
> +static int xpcs_rk_write_reg(struct dw_xpcs_rk *pxpcs, int dev, int reg, u16 val)
> +{
> + ptrdiff_t csr;
> + int ret;
> +
> + csr = xpcs_rk_addr_format(dev, reg);
> +
> + ret = pm_runtime_resume_and_get(&pxpcs->pdev->dev);
> + if (ret)
> + return ret;
> +
> + writel(val, pxpcs->reg_base + (csr << 2));
> +
> + pm_runtime_put(&pxpcs->pdev->dev);
> + return 0;
> +}
> +
> +#define ROCKCHIP_MMD_MII1 2
> +#define ROCKCHIP_MMD_MII2 3
> +#define ROCKCHIP_MMD_MII3 4
> +#define ROCKCHIP_MMD_PMAPMD 6
> +#define ROCKCHIP_MMD_MII 7
> +
> +static bool xpcs_rk_mdio_addr_validate(int addr)
> +{
> + return !(addr < 0 || addr > 3);
> +}
> +
> +static int xpcs_rk_mdio_read_remapping(int addr, int dev, int reg)
> +{
> + switch (dev) {
> + case MDIO_MMD_PMAPMD:
> + return ROCKCHIP_MMD_PMAPMD;
> + case MDIO_MMD_VEND2:
> + break;
> + default:
> + return -ENXIO;
> + }
> +
> + /* read remapping to MII is performed by HW */
> + switch (addr) {
> + case 0:
> + return ROCKCHIP_MMD_MII;
> + case 1:
> + return ROCKCHIP_MMD_MII1;
> + case 2:
> + return ROCKCHIP_MMD_MII2;
> + case 3:
> + return ROCKCHIP_MMD_MII3;
> + default:
> + return -ENODEV;
> + }
> +}
> +
> +static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
> +{
> + switch (dev) {
> + case MDIO_MMD_PMAPMD:
> + return ROCKCHIP_MMD_PMAPMD;
> + case MDIO_MMD_VEND2:
> + break;
> + default:
> + return -ENXIO;
> + }
> +
> + /* Writable only on MII */
> + switch (reg) {
> + case DW_VR_MII_AN_CTRL:
> + case DW_VR_MII_AN_INTR_STS:
> + case DW_VR_MII_EEE_MCTRL0:
> + case DW_VR_MII_EEE_MCTRL1:
> + case DW_VR_MII_DIG_CTRL2:
> + return ROCKCHIP_MMD_MII;
> + default:
> + break;
> + }
> +
> + switch (addr) {
> + case 0:
> + return ROCKCHIP_MMD_MII;
> + case 1:
> + return ROCKCHIP_MMD_MII1;
> + case 2:
> + return ROCKCHIP_MMD_MII2;
> + case 3:
> + return ROCKCHIP_MMD_MII3;
> + default:
> + return -ENODEV;
> + }
> +}
> +
> +static int xpcs_rk_read_c22(struct mii_bus *bus, int addr, int reg)
> +{
> + struct dw_xpcs_rk *pxpcs = bus->priv;
> + int dev;
> +
> + if (!xpcs_rk_mdio_addr_validate(addr))
> + return -ENODEV;
> +
> + dev = xpcs_rk_mdio_read_remapping(addr, MDIO_MMD_VEND2, reg);
> + if (dev < 0)
> + return 0xffff;
> +
> + return xpcs_rk_read_reg(pxpcs, dev, reg);
> +}
> +
> +static int xpcs_rk_write_c22(struct mii_bus *bus, int addr, int reg, u16 val)
> +{
> + struct dw_xpcs_rk *pxpcs = bus->priv;
> + int dev;
> +
> + if (!xpcs_rk_mdio_addr_validate(addr))
> + return -ENODEV;
> +
> + dev = xpcs_rk_mdio_write_remapping(addr, MDIO_MMD_VEND2, reg);
> + if (dev < 0)
> + return 0;
> +
> + return xpcs_rk_write_reg(pxpcs, dev, reg, val);
> +}
> +
> +static int xpcs_rk_read_c45(struct mii_bus *bus, int addr, int dev, int reg)
> +{
> + struct dw_xpcs_rk *pxpcs = bus->priv;
> +
> + if (!xpcs_rk_mdio_addr_validate(addr))
> + return -ENODEV;
> +
> + dev = xpcs_rk_mdio_read_remapping(addr, dev, reg);
> + if (dev < 0)
> + return 0xffff;
> +
> + return xpcs_rk_read_reg(pxpcs, dev, reg);
> +}
> +
> +static int xpcs_rk_write_c45(struct mii_bus *bus, int addr, int dev, int reg, u16 val)
> +{
> + struct dw_xpcs_rk *pxpcs = bus->priv;
> +
> + if (!xpcs_rk_mdio_addr_validate(addr))
> + return -ENODEV;
> +
> + dev = xpcs_rk_mdio_write_remapping(addr, dev, reg);
> + if (dev < 0)
> + return 0;
> +
> + return xpcs_rk_write_reg(pxpcs, dev, reg, val);
> +}
> +
> +static struct dw_xpcs_rk *xpcs_rk_create_data(struct platform_device *pdev)
> +{
> + struct dw_xpcs_rk *pxpcs;
> +
> + pxpcs = devm_kzalloc(&pdev->dev, sizeof(*pxpcs), GFP_KERNEL);
> + if (!pxpcs)
> + return ERR_PTR(-ENOMEM);
> +
> + pxpcs->pdev = pdev;
> +
> + dev_set_drvdata(&pdev->dev, pxpcs);
> +
> + return pxpcs;
> +}
> +
> +static int xpcs_rk_serdes_phy_init(struct dw_xpcs_rk *pxpcs)
> +{
> + struct device *dev = &pxpcs->pdev->dev;
> +
> + pxpcs->serdes_phy = devm_phy_get(dev, "serdes");
> + if (IS_ERR(pxpcs->serdes_phy))
> + return dev_err_probe(dev, PTR_ERR(pxpcs->serdes_phy),
> + "Failed to get SerDes PHY\n");
> +
> + return 0;
> +}
> +
> +static void xpcs_rk_serdes_phy_poweroff(void *data)
> +{
> + struct dw_xpcs_rk *pxpcs = data;
> + struct device *dev = &pxpcs->pdev->dev;
> +
> + phy_power_off(pxpcs->serdes_phy);
> + phy_exit(pxpcs->serdes_phy);
> +
> + dev_pm_genpd_rpm_always_on(dev, false);
> +}
> +
> +static int xpcs_rk_serdes_phy_poweron(struct dw_xpcs_rk *pxpcs)
> +{
> + struct device *dev = &pxpcs->pdev->dev;
> + int ret;
> +
> + /*
> + * The power domain is required and must be enabled, which allows us to
> + * dynamically turn the CSR clock on/off using PM while keeping the PCS
> + * powered on.
> + */
> + ret = dev_pm_genpd_rpm_always_on(dev, true);
> + if (ret) {
> + dev_err(dev, "Failed to power on power-domains\n");
> + return ret;
> + }
> +
> + ret = phy_init(pxpcs->serdes_phy);
> + if (ret) {
> + dev_err(dev, "Failed to init SerDes PHY\n");
> + goto pm_domain;
> + }
> +
> + ret = phy_power_on(pxpcs->serdes_phy);
> + if (ret) {
> + dev_err(dev, "Failed to power on SerDes PHY\n");
> + goto serdes_phy;
> + }
> +
> + ret = devm_add_action_or_reset(dev, xpcs_rk_serdes_phy_poweroff, pxpcs);
> + if (ret) {
> + dev_err(dev, "Failed to register devm for SerDes PHY: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +
> +serdes_phy:
> + phy_exit(pxpcs->serdes_phy);
> +pm_domain:
> + dev_pm_genpd_rpm_always_on(dev, false);
> + return ret;
> +}
> +
> +static int xpcs_rk_init_res(struct dw_xpcs_rk *pxpcs)
> +{
> + struct platform_device *pdev = pxpcs->pdev;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(dev, "No reg-space found\n");
> + return -EINVAL;
> + }
> +
> + if (resource_size(res) < SZ_2M) {
> + dev_err(dev, "Invalid reg-space size\n");
> + return -EINVAL;
> + }
> +
> + pxpcs->reg_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(pxpcs->reg_base)) {
> + dev_err(dev, "Failed to map reg-space\n");
> + return PTR_ERR(pxpcs->reg_base);
> + }
> +
> + return 0;
> +}
> +
> +static void xpcs_rk_exit_clk(void *data)
> +{
> + struct dw_xpcs_rk *pxpcs = data;
> + struct device *dev = &pxpcs->pdev->dev;
> +
> + pm_runtime_force_suspend(dev);
> + clk_disable_unprepare(pxpcs->eee_clk);
> +}
> +
> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
> + struct device *dev = &pxpcs->pdev->dev;
> + int ret;
> +
> + pxpcs->csr_clk = devm_clk_get(dev, "csr");
> + if (IS_ERR(pxpcs->csr_clk))
> + return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
> + "Failed to get CSR clock\n");
> +
> + pxpcs->eee_clk = devm_clk_get(dev, "eee");
> + if (IS_ERR(pxpcs->eee_clk))
> + return dev_err_probe(dev, PTR_ERR(pxpcs->eee_clk),
> + "Failed to get EEE clock\n");
> +
> + ret = clk_prepare_enable(pxpcs->eee_clk);
> + if (ret) {
> + dev_err(dev, "Failed to enable EEE clock\n");
> + return ret;
> + }
> +
> + /*
> + * Compute the multiplier for the EEE clock so that
> + * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
> + *
> + * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
> + * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
> + * The reset value is 0 (200 MHz), but derive the value at runtime to
> + * stay correct if the mux is changed by a board.
> + */
> + pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
> + 1000000000UL) - 1;
> +
> + pm_runtime_set_suspended(dev);
> + pm_runtime_enable(dev);
> +
> + ret = devm_add_action_or_reset(dev, xpcs_rk_exit_clk, pxpcs);
> + if (ret) {
> + dev_err(dev, "Failed to register devm for EEE clock: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int xpcs_rk_init_bus(struct dw_xpcs_rk *pxpcs)
> +{
> + struct device *dev = &pxpcs->pdev->dev;
> + static atomic_t id = ATOMIC_INIT(-1);
> + struct mii_bus *bus;
> + int ret;
> +
> + bus = devm_mdiobus_alloc_size(dev, 0);
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "Rockchip DW XPCS MCI/APB3";
> + bus->read = xpcs_rk_read_c22;
> + bus->write = xpcs_rk_write_c22;
> + bus->read_c45 = xpcs_rk_read_c45;
> + bus->write_c45 = xpcs_rk_write_c45;
> + bus->phy_mask = ~0;
> + bus->parent = dev;
> + bus->priv = pxpcs;
> +
> + snprintf(bus->id, MII_BUS_ID_SIZE,
> + "rockchip_dwxpcs-%x", atomic_inc_return(&id));
> +
> + /*
> + * MDIO-bus here serves as just a back-end engine abstracting out
> + * the MDIO and MCI/APB3 IO interfaces utilized for the Rockchip DWXPCS CSRs
> + * access.
> + */
> + ret = devm_mdiobus_register(dev, bus);
> + if (ret) {
> + dev_err(dev, "Failed to create MDIO bus\n");
> + return ret;
> + }
> +
> + pxpcs->bus = bus;
> + return 0;
> +}
> +
> +static int xpcs_rk_probe(struct platform_device *pdev)
> +{
> + struct dw_xpcs_rk *pxpcs;
> + int ret;
> +
> + pxpcs = xpcs_rk_create_data(pdev);
> + if (IS_ERR(pxpcs))
> + return PTR_ERR(pxpcs);
> +
> + /*
> + * The XPCS may be attached to a power domain (e.g. PD_PIPE). The domain
> + * must be powered on before any register access, otherwise the SoC will
> + * trigger a synchronous external abort (SError).
> + *
> + * Accessing the XPCS registers also requires a TX clock from the SerDes,
> + * which is needed for the soft reset.
> + */
> + ret = xpcs_rk_serdes_phy_init(pxpcs);
> + if (ret)
> + return ret;
> +
> + ret = xpcs_rk_serdes_phy_poweron(pxpcs);
> + if (ret)
> + return ret;
> +
> + ret = xpcs_rk_init_res(pxpcs);
> + if (ret)
> + return ret;
> +
> + ret = xpcs_rk_init_clk(pxpcs);
> + if (ret)
> + return ret;
> +
> + ret = xpcs_rk_init_bus(pxpcs);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id xpcs_rk_of_ids[] = {
> + { .compatible = "rockchip,rk3568-xpcs" },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, xpcs_rk_of_ids);
> +
> +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> +{
> + struct platform_device *pdev;
> + struct device_node *pcs_np;
> + struct dw_xpcs_rk *pxpcs;
> + struct dw_xpcs *xpcs;
> + u32 port;
> +
> + if (!of_device_is_available(np))
> + return ERR_PTR(-ENODEV);
> +
> + if (of_property_read_u32(np, "reg", &port))
> + return ERR_PTR(-EINVAL);
> +
> + if (!xpcs_rk_mdio_addr_validate((int)port))
> + return ERR_PTR(-EINVAL);
> +
> + /* The XPCS pdev is attached to the parent node */
> + pcs_np = of_get_parent(np);
> + if (!pcs_np)
> + return ERR_PTR(-ENODEV);
> +
> + if (!of_device_is_available(pcs_np)) {
> + of_node_put(pcs_np);
> + return ERR_PTR(-ENODEV);
> + }
> +
> + if (!of_match_node(xpcs_rk_of_ids, pcs_np)) {
> + of_node_put(pcs_np);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + pdev = of_find_device_by_node(pcs_np);
> + of_node_put(pcs_np);
> + if (!pdev)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + device_lock(&pdev->dev);
[Severity: Medium]
xpcs_rk_create() is called from a MAC driver's probe path, which is
already running under device_lock() of the MAC device taken by
really_probe(). Taking device_lock(&pdev->dev) here nests a second
struct device mutex, and since all device mutexes share one lockdep
class unless annotated, lockdep will report "possible recursive
locking detected" on the first probe. That is a splat in the logs on
any CONFIG_PROVE_LOCKING build, and a panic with panic_on_warn set.
Holding the XPCS device's lock across xpcs_create_mdiodev() and
device_link_add() is also unusual - device_link_add() does its own
locking and the driver core normally expects supplier device locks not
to be held by consumers.
What race is the lock actually protecting against? pxpcs and
pxpcs->bus are published by xpcs_rk_probe() via dev_set_drvdata() and
xpcs_rk_init_bus(), and the !pxpcs || !pxpcs->bus case already returns
-EPROBE_DEFER, so the consumer retries later; the device link then
keeps the supplier around. Can the device_lock()/device_unlock() pairs
simply be dropped, or is something else needed here (e.g. checking that
the supplier is bound) to close the window you had in mind?
> + pxpcs = platform_get_drvdata(pdev);
> + if (!pxpcs || !pxpcs->bus) {
> + device_unlock(&pdev->dev);
> + put_device(&pdev->dev);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
> + if (IS_ERR(xpcs)) {
> + device_unlock(&pdev->dev);
> + put_device(&pdev->dev);
> + return xpcs;
> + }
> +
> + if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
> + xpcs_destroy(xpcs);
> + device_unlock(&pdev->dev);
> + put_device(&pdev->dev);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + xpcs_config_eee_mult_fact(xpcs, pxpcs->eee_mult_fact);
> + device_unlock(&pdev->dev);
> + put_device(&pdev->dev);
> + return xpcs;
> +}
> +EXPORT_SYMBOL_GPL(xpcs_rk_create);
> +
> +static int xpcs_rk_pm_runtime_suspend(struct device *dev)
> +{
> + struct dw_xpcs_rk *pxpcs = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(pxpcs->csr_clk);
> +
> + return 0;
> +}
> +
> +static int xpcs_rk_pm_runtime_resume(struct device *dev)
> +{
> + struct dw_xpcs_rk *pxpcs = dev_get_drvdata(dev);
> +
> + return clk_prepare_enable(pxpcs->csr_clk);
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(xpcs_rk_pm_ops,
> + xpcs_rk_pm_runtime_suspend,
> + xpcs_rk_pm_runtime_resume,
> + NULL);
> +
> +static struct platform_driver xpcs_rk_driver = {
> + .probe = xpcs_rk_probe,
> + .driver = {
> + .name = "rk_xpcs-dwxpcs",
> + .pm = pm_ptr(&xpcs_rk_pm_ops),
> + .of_match_table = xpcs_rk_of_ids,
> + },
> +};
> +module_platform_driver(xpcs_rk_driver);
> +
> +MODULE_DESCRIPTION("Rockchip XPCS platform device driver");
> +MODULE_AUTHOR("Coia Prant <coiaprant@gmail.com>");
> +MODULE_LICENSE("GPL");
[Severity: High]
This block in xpcs_rk_mdio_write_remapping() ignores addr entirely:
any write to DW_VR_MII_AN_CTRL, DW_VR_MII_AN_INTR_STS,
DW_VR_MII_EEE_MCTRL0, DW_VR_MII_EEE_MCTRL1 or DW_VR_MII_DIG_CTRL2 is
routed to ROCKCHIP_MMD_MII, which per the commit message is port 0's
MMD (7), regardless of which of the four ports the MDIO address
selects.
So when the core configures a PCS instance created with addr 1, 2 or 3
- xpcs_config_aneg_c37_sgmii() writing DW_VR_MII_AN_CTRL and
DW_VR_MII_DIG_CTRL2, xpcs_config_eee() writing DW_VR_MII_EEE_MCTRL0/1,
and the interrupt-status clear on DW_VR_MII_AN_INTR_STS - the write
lands on port 0 instead. That has two effects: the intended port is
never configured (so its link stays broken), and port 0's
auto-negotiation/EEE state is silently clobbered by an unrelated
interface, including its pending AN interrupt status.
The read path is not symmetric either: xpcs_rk_mdio_read_remapping()
remaps per addr with the comment "read remapping to MII is performed by
HW", so a write to these registers on port 2 followed by a read back
would not return what was written. Is that asymmetry deliberate?
Does the TRM say these vendor registers physically exist only in MMD 7
and are shared by all four MII ports? If so, redirecting writes
silently still leaves per-port configuration impossible, and the driver
probably needs to say so explicitly (reject the access, or document how
the shared block is meant to be programmed) rather than aliasing ports
1..3 onto port 0. If instead each port has its own copy of these
registers, this switch should be dropped so the addr-based mapping
below applies. Which is it?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
` (6 preceding siblings ...)
2026-09-13 18:58 ` [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568 Coia Prant
` (2 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
The Rockchip dwmac glue driver now supports SGMII on RK3568 and
requires a PCS, referenced through the "pcs-handle" property. The
property is parsed by rk_pcs_init() and a missing phandle fails the
probe, so describe it in the binding and require it when phy-mode
is "sgmii".
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
.../devicetree/bindings/net/rockchip-dwmac.yaml | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
index 80c252845349c..67b21ea77ec96 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
@@ -120,6 +120,12 @@ properties:
maximum: 0x7F
default: 0x10
+ pcs-handle:
+ description:
+ Specifies a reference to a node representing the PCS device
+ connected to this GMAC. Required when phy-mode is "sgmii".
+ maxItems: 1
+
phy-supply:
description: PHY regulator
@@ -159,6 +165,15 @@ allOf:
clocks:
minItems: 5
+ - if:
+ properties:
+ phy-mode:
+ contains:
+ const: sgmii
+ then:
+ required:
+ - pcs-handle
+
unevaluatedProperties: false
examples:
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle
2026-09-13 18:58 ` [PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 3
- [Low] Concern (verified FALSE POSITIVE, retained for policy): the
binding makes `pcs-handle` required for `phy-mode = "sgmii"` and the…
- [Low] Concern (verified FALSE POSITIVE, retained for policy): the new
`pcs-handle` property and the `if phy-mode contains sgmii -> require…
- [Low] Concern (verified FALSE POSITIVE, retained for policy): the new
conditional's `if` clause tests the value of `phy-mode` without…
--
dt-bindings: net: rockchip-dwmac: document pcs-handle
This adds a pcs-handle property to the Rockchip dwmac binding and adds an
allOf conditional that makes pcs-handle a required property whenever
phy-mode contains "sgmii". There are no other changes to the schema.
No Link: tags are present in the commit header.
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> index 80c252845349c..67b21ea77ec96 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> @@ -120,6 +120,12 @@ properties:
> maximum: 0x7F
> default: 0x10
>
> + pcs-handle:
> + description:
> + Specifies a reference to a node representing the PCS device
> + connected to this GMAC. Required when phy-mode is "sgmii".
> + maxItems: 1
> +
[Severity: Low]
Should pcs-handle be gated on compatible? As written the property is
accepted for every Rockchip GMAC compatible selected by this binding,
even though only rockchip,rk3568-gmac integrates the XPCS plus Naneng
combo SerDes. Other SoC-specific properties in this file use the
else-branch form, for example:
else:
properties:
rockchip,php-grf: false
Checking this further, the un-gated form does not appear to cause a
problem in practice. At the end of the series only rk3568_ops provides
.set_to_sgmii, and rk_gmac_check_ops() in
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c has:
case PHY_INTERFACE_MODE_SGMII:
if (!bsp_priv->supports_sgmii)
return -EINVAL;
so an sgmii device tree on a Rockchip SoC without SGMII support fails
probe rather than misbehaving. This binding has also never restricted
phy-mode per compatible, since it inherits the full phy-connection-type
enum through snps,dwmac.yaml, so accepting phy-mode = "sgmii" on other
Rockchip parts predates this patch. The same construct appears un-gated
in Documentation/devicetree/bindings/net/renesas,rzn1-gmac.yaml, which is
also a snps,dwmac child binding.
> phy-supply:
> description: PHY regulator
>
> @@ -159,6 +165,15 @@ allOf:
> clocks:
> minItems: 5
>
> + - if:
> + properties:
> + phy-mode:
> + contains:
> + const: sgmii
> + then:
> + required:
> + - pcs-handle
> +
[Severity: Low]
This conditional makes pcs-handle mandatory, and the commit message
describes its consumer in the present tense:
The property is parsed by rk_pcs_init() and a missing phandle fails
the probe
At this revision there is no rk_pcs_init() in
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c, so is the wording ahead
of the tree here?
Looking at the rest of the series answers this: the following patch,
"net: stmmac: dwmac-rk: add SGMII support for RK3568", adds
rk_pcs_init() with exactly the described behaviour:
pcs_node = of_parse_phandle(np, "pcs-handle", 0);
if (!pcs_node)
return -ENODEV;
and rk_gmac_probe() installs plat_dat->pcs_init = rk_pcs_init for SGMII.
Sending the binding before the driver change is the expected ordering, so
this is only visible when the patch is read on its own.
[Severity: Low]
Separately, the if clause tests the value of phy-mode without listing it
in required, for example:
- if:
properties:
phy-mode:
contains:
const: sgmii
required:
- phy-mode
Without that, JSON Schema also matches nodes that omit phy-mode entirely,
which nominally makes template nodes such as gmac0 in
arch/arm64/boot/dts/rockchip/rk3568.dtsi and gmac1 in rk356x-base.dtsi
require pcs-handle. Would adding required: [phy-mode] be worthwhile for
clarity?
Following this through, it does not change validation results. This file
has allOf: - $ref: snps,dwmac.yaml#, and snps,dwmac.yaml already lists:
required:
- compatible
- reg
- interrupts
- interrupt-names
- phy-mode
so a node without phy-mode already fails validation on the inherited
required list, and the new conditional only adds a second, redundant
message. The two dtsi nodes above already omit the pre-existing
mandatory phy-mode, so nothing that previously validated is newly
rejected. The same un-guarded pattern is used in
Documentation/devicetree/bindings/net/ethernet-controller.yaml.
> unevaluatedProperties: false
>
> examples:
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
` (7 preceding siblings ...)
2026-09-13 18:58 ` [PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-14 20:24 ` netdev-bot+sashiko
2026-09-13 18:58 ` [PATCH net-next v5 10/11] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port Coia Prant
2026-09-13 18:58 ` [PATCH net-next v5 11/11] MAINTAINERS: add entry for Rockchip XPCS driver Coia Prant
10 siblings, 1 reply; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
The RK3568 SoC integrates a Synopsys DesignWare XPCS that can be
connected to GMAC0 or GMAC1 in SGMII mode. Add the necessary glue
logic to support this configuration.
The current dwmac-rk driver does not support SGMII mode. SGMII
requires a PCS to handle auto-negotiation and link state reporting,
but the existing driver only supports RGMII and RMII.
Add a set_to_sgmii() callback to configure the GMAC GRF register for
SGMII mode (bit 7 set, interface selection bits 4:6 ignored when set).
Also add a set_to_rmii() callback for rk3568 to explicitly clear bit 7,
since the new SGMII path leaves it set and the RMII branch previously
relied on the SoC reset value.
Provide pcs_init/pcs_exit callbacks to create/destroy the XPCS via
xpcs_rk_create() from the Rockchip XPCS platform driver, and a
select_pcs callback to return the XPCS to phylink.
Reorder rk_gmac_powerup() so that gmac_clk_enable() is called before
the SGMII check. The SGMII path skips rk_get_phy_intf_sel(), so the
clock must be enabled earlier to cover all register accesses in that
path. While at it, unify the error unwinding into a single
clk_disable label and add error handling for the default (unhandled
interface) case.
SGMII In-band vs Out-of-band
============================
On RK3568, the MAC clock is fixed at 125 MHz and cannot be dynamically
changed by the stmmac core's set_clk_tx_rate callback. In-band mode
works because the PCS handles rate adaptation internally. Out-of-band
mode does not work because the MAC would need to change the clock rate
to 125/12.5/1.25 MHz for 1000/100/10 Mbps respectively, and the clock
is fixed.
Enable default_an_inband for SGMII and disable the generic stmmac
set_clk_tx_rate callback. This forces phylink to use in-band mode,
where the PCS is responsible for speed/duplex negotiation.
Note that default_an_inband can be overridden by a fixed-link node,
and phylink may also fall back to out-of-band if the PHY does not
support in-band signalling. Out-of-band SGMII is not supported by
this driver: the MAC clock would stay at 125 MHz for 10/100 Mbps,
giving working TX but failing RX. Boards must use in-band mode
(managed = "in-band-status" or an in-band capable PHY).
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/Kconfig | 1 +
.../net/ethernet/stmicro/stmmac/dwmac-rk.c | 134 +++++++++++++++---
2 files changed, 115 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index e3dd5adda5aca..5088acc06982e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -170,6 +170,7 @@ config DWMAC_ROCKCHIP
default ARCH_ROCKCHIP
depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
select MFD_SYSCON
+ select PCS_XPCS_ROCKCHIP
help
Support for Ethernet controller on Rockchip RK3288 SoC.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 8d7042e689261..5d501ed8d1429 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -20,6 +20,7 @@
#include <linux/delay.h>
#include <linux/mfd/syscon.h>
#include <linux/regmap.h>
+#include <linux/pcs/pcs-xpcs-rk.h>
#include <linux/pm_runtime.h>
#include "stmmac_platform.h"
@@ -47,6 +48,7 @@ struct rk_gmac_ops {
void (*set_to_rgmii)(struct rk_priv_data *bsp_priv,
int tx_delay, int rx_delay);
void (*set_to_rmii)(struct rk_priv_data *bsp_priv);
+ void (*set_to_sgmii)(struct rk_priv_data *bsp_priv);
int (*set_speed)(struct rk_priv_data *bsp_priv,
phy_interface_t interface, int speed);
void (*integrated_phy_powerup)(struct rk_priv_data *bsp_priv);
@@ -63,6 +65,7 @@ struct rk_gmac_ops {
bool clock_grf_reg_in_php;
bool supports_rgmii;
bool supports_rmii;
+ bool supports_sgmii;
bool php_grf_required;
bool regs_valid;
u32 regs[];
@@ -98,6 +101,7 @@ struct rk_priv_data {
bool integrated_phy;
bool supports_rgmii;
bool supports_rmii;
+ bool supports_sgmii;
struct clk_bulk_data *clks;
int num_clks;
@@ -809,6 +813,8 @@ static const struct rk_gmac_ops rk3528_ops = {
#define RK3568_GRF_GMAC1_CON1 0x038c
/* RK3568_GRF_GMAC0_CON1 && RK3568_GRF_GMAC1_CON1 */
+#define RK3568_GMAC_MODE_RMII_RGMII GRF_CLR_BIT(7)
+#define RK3568_GMAC_MODE_SGMII_QSGMII GRF_BIT(7)
#define RK3568_GMAC_FLOW_CTRL GRF_BIT(3)
#define RK3568_GMAC_FLOW_CTRL_CLR GRF_CLR_BIT(3)
#define RK3568_GMAC_RXCLK_DLY_ENABLE GRF_BIT(1)
@@ -836,6 +842,17 @@ static int rk3568_init(struct rk_priv_data *bsp_priv)
}
}
+static void rk3568_set_to_rmii(struct rk_priv_data *bsp_priv)
+{
+ u32 con1;
+
+ con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
+ RK3568_GRF_GMAC0_CON1;
+
+ regmap_write(bsp_priv->grf, con1,
+ RK3568_GMAC_MODE_RMII_RGMII);
+}
+
static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
int tx_delay, int rx_delay)
{
@@ -851,19 +868,31 @@ static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
regmap_write(bsp_priv->grf, con1,
+ RK3568_GMAC_MODE_RMII_RGMII |
RK3568_GMAC_RXCLK_DLY_ENABLE |
RK3568_GMAC_TXCLK_DLY_ENABLE);
}
+static void rk3568_set_to_sgmii(struct rk_priv_data *bsp_priv)
+{
+ u32 con1;
+
+ con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
+ RK3568_GRF_GMAC0_CON1;
+
+ regmap_write(bsp_priv->grf, con1, RK3568_GMAC_MODE_SGMII_QSGMII);
+}
+
static const struct rk_gmac_ops rk3568_ops = {
.init = rk3568_init,
+ .set_to_rmii = rk3568_set_to_rmii,
.set_to_rgmii = rk3568_set_to_rgmii,
+ .set_to_sgmii = rk3568_set_to_sgmii,
+
.set_speed = rk_set_clk_mac_speed,
.gmac_phy_intf_sel_mask = GENMASK_U16(6, 4),
- .supports_rmii = true,
-
.regs_valid = true,
.regs = {
0xfe2a0000, /* gmac0 */
@@ -1208,6 +1237,43 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
dev_err(bsp_priv->dev, "fail to disable phy-supply\n");
}
+static int rk_pcs_init(struct stmmac_priv *priv)
+{
+ struct device_node *np = priv->device->of_node;
+ struct device_node *pcs_node;
+ struct dw_xpcs *xpcs;
+
+ pcs_node = of_parse_phandle(np, "pcs-handle", 0);
+ if (!pcs_node)
+ return -ENODEV;
+
+ xpcs = xpcs_rk_create(priv->device, pcs_node);
+ of_node_put(pcs_node);
+ if (IS_ERR(xpcs))
+ return PTR_ERR(xpcs);
+
+ priv->hw->xpcs = xpcs;
+ return 0;
+}
+
+static void rk_pcs_exit(struct stmmac_priv *priv)
+{
+ if (!priv->hw->xpcs)
+ return;
+
+ xpcs_destroy(priv->hw->xpcs);
+ priv->hw->xpcs = NULL;
+}
+
+static struct phylink_pcs *rk_select_pcs(struct stmmac_priv *priv,
+ phy_interface_t interface)
+{
+ if (!priv->hw->xpcs)
+ return NULL;
+
+ return xpcs_to_phylink_pcs(priv->hw->xpcs);
+}
+
static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
struct plat_stmmacenet_data *plat,
const struct rk_gmac_ops *ops)
@@ -1330,6 +1396,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
bsp_priv->supports_rgmii = ops->supports_rgmii || !!ops->set_to_rgmii;
bsp_priv->supports_rmii = ops->supports_rmii || !!ops->set_to_rmii;
+ bsp_priv->supports_sgmii = ops->supports_sgmii || !!ops->set_to_sgmii;
if (ops->init) {
ret = ops->init(bsp_priv);
@@ -1361,6 +1428,10 @@ static int rk_gmac_check_ops(struct rk_priv_data *bsp_priv)
if (!bsp_priv->supports_rmii)
return -EINVAL;
break;
+ case PHY_INTERFACE_MODE_SGMII:
+ if (!bsp_priv->supports_sgmii)
+ return -EINVAL;
+ break;
default:
dev_err(bsp_priv->dev,
"unsupported interface %d", bsp_priv->phy_iface);
@@ -1379,16 +1450,19 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
if (ret)
return ret;
+ ret = gmac_clk_enable(bsp_priv, true);
+ if (ret)
+ return ret;
+
+ if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
+ goto set_mode;
+
ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
if (ret < 0)
- return ret;
+ goto clk_disable;
intf = ret;
- ret = gmac_clk_enable(bsp_priv, true);
- if (ret)
- return ret;
-
if (bsp_priv->gmac_phy_intf_sel_mask ||
bsp_priv->gmac_rmii_mode_mask) {
/* If defined, encode the phy_intf_sel value */
@@ -1399,10 +1473,8 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
bsp_priv->gmac_rmii_mode_mask);
ret = rk_write_gmac_grf_reg(bsp_priv, val);
- if (ret < 0) {
- gmac_clk_enable(bsp_priv, false);
- return ret;
- }
+ if (ret < 0)
+ goto clk_disable;
}
if (bsp_priv->clock.rmii_mode_mask) {
@@ -1410,13 +1482,12 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
bsp_priv->clock.rmii_mode_mask);
ret = rk_write_clock_grf_reg(bsp_priv, val);
- if (ret < 0) {
- gmac_clk_enable(bsp_priv, false);
- return ret;
- }
+ if (ret < 0)
+ goto clk_disable;
}
- /*rmii or rgmii*/
+set_mode:
+ /* rmii, rgmii, sgmii */
switch (bsp_priv->phy_iface) {
case PHY_INTERFACE_MODE_RGMII:
dev_info(dev, "init for RGMII\n");
@@ -1447,15 +1518,20 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
if (bsp_priv->ops->set_to_rmii)
bsp_priv->ops->set_to_rmii(bsp_priv);
break;
+ case PHY_INTERFACE_MODE_SGMII:
+ dev_info(dev, "init for SGMII\n");
+ if (bsp_priv->ops->set_to_sgmii)
+ bsp_priv->ops->set_to_sgmii(bsp_priv);
+ break;
default:
dev_err(dev, "NO interface defined!\n");
+ ret = -EINVAL;
+ goto clk_disable;
}
ret = rk_phy_powerup(bsp_priv);
- if (ret) {
- gmac_clk_enable(bsp_priv, false);
- return ret;
- }
+ if (ret)
+ goto clk_disable;
pm_runtime_get_sync(dev);
@@ -1463,6 +1539,10 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
bsp_priv->ops->integrated_phy_powerup(bsp_priv);
return 0;
+
+clk_disable:
+ gmac_clk_enable(bsp_priv, false);
+ return ret;
}
static void rk_gmac_powerdown(struct rk_priv_data *gmac)
@@ -1486,6 +1566,9 @@ static void rk_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
if (rk->supports_rmii)
__set_bit(PHY_INTERFACE_MODE_RMII, interfaces);
+
+ if (rk->supports_sgmii)
+ __set_bit(PHY_INTERFACE_MODE_SGMII, interfaces);
}
static int rk_set_clk_tx_rate(void *bsp_priv_, struct clk *clk_tx_i,
@@ -1602,6 +1685,17 @@ static int rk_gmac_probe(struct platform_device *pdev)
plat_dat->suspend = rk_gmac_suspend;
plat_dat->resume = rk_gmac_resume;
+ if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII) {
+ /* SGMII clock always runs at 125 MHz */
+ plat_dat->set_clk_tx_rate = NULL;
+
+ /* SGMII requires a PCS */
+ plat_dat->default_an_inband = true;
+ plat_dat->pcs_init = rk_pcs_init;
+ plat_dat->pcs_exit = rk_pcs_exit;
+ plat_dat->select_pcs = rk_select_pcs;
+ }
+
plat_dat->bsp_priv = rk_gmac_setup(pdev, plat_dat, data);
if (IS_ERR(plat_dat->bsp_priv))
return PTR_ERR(plat_dat->bsp_priv);
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net-next v5 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568
2026-09-13 18:58 ` [PATCH net-next v5 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568 Coia Prant
@ 2026-09-14 20:24 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-09-14 20:24 UTC (permalink / raw)
To: coiaprant
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, heiko, vkoul, maxime.chevallier, mcoquelin.stm32,
alexandre.torgue, prabhakar.mahadev-lad.rj, romain.gantois,
hkallweit1, neil.armstrong, linux, shawn.lin, david, netdev,
linux-rockchip, devicetree, linux-arm-kernel, linux-kernel,
linux-phy, linux-stm32, linux-renesas-soc
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 (1) · Low: 3
- [Medium] DWMAC_ROCKCHIP gains an unconditional `select
PCS_XPCS_ROCKCHIP` without inheriting the selected symbol's…
- [Medium] For SGMII the driver removes every speed-dependent clock
programming path (rk_gmac_probe() sets plat_dat->set_clk_tx_rate =…
- [Low] rk_get_interfaces() (dwmac-rk.c:1570-1571) now sets
PHY_INTERFACE_MODE_SGMII in phylink's supported_interfaces for every…
- [Low] The RK3568 XPCS SoC node added by the series ('arm64: dts:
rockchip: rk3568: add XPCS and fixed-clock nodes') omits properties…
- [Low] rk_select_pcs() (dwmac-rk.c:1268-1275) ignores its `interface`
argument and returns the SGMII XPCS for every interface mode,…
Pre-existing issues:
- [Medium] DISCARDED (valid but pre-existing and below the reporting
threshold for pre-existing defects).
--
net: stmmac: dwmac-rk: add SGMII support for RK3568
Adds a set_to_sgmii()/set_to_rmii() pair for rk3568 to program GRF bit 7,
wires up pcs_init/pcs_exit/select_pcs so the Rockchip XPCS is used as the
PCS, and reorders rk_gmac_powerup() so the clocks are enabled before the
interface-specific configuration.
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
A few questions below.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index ab3c4cf964239..c6559e749c0ce 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -170,6 +170,7 @@ config DWMAC_ROCKCHIP
> default ARCH_ROCKCHIP
> depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
> select MFD_SYSCON
> + select PCS_XPCS_ROCKCHIP
[Severity: Medium]
Does this select produce unmet dependency warnings? PCS_XPCS_ROCKCHIP in
drivers/net/pcs/Kconfig carries three dependency lines:
config PCS_XPCS_ROCKCHIP
tristate "Rockchip XPCS controller support"
default ARCH_ROCKCHIP
depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
depends on GENERIC_PHY || COMPILE_TEST
depends on PM_GENERIC_DOMAINS || COMPILE_TEST
while DWMAC_ROCKCHIP only has "depends on OF && (ARCH_ROCKCHIP ||
COMPILE_TEST)". ARCH_ROCKCHIP in arch/arm64/Kconfig.platforms selects PM
but neither GENERIC_PHY nor PM_GENERIC_DOMAINS, so a config with
ARCH_ROCKCHIP=y, COMPILE_TEST=n and GENERIC_PHY=n (or PM_GENERIC_DOMAINS=n)
force-enables PCS_XPCS_ROCKCHIP with its own depends unsatisfied:
WARNING: unmet direct dependencies detected for PCS_XPCS_ROCKCHIP
pcs-xpcs-rk.o is then built against the phy stubs where devm_phy_get()
returns ERR_PTR(-ENOSYS), so the probe cannot succeed.
The select is also unconditional, so every Rockchip dwmac build
(RK3288/RK3328/RK3399/RK3588 ...) force-enables the RK356x-only XPCS
driver. Would inheriting the constraints, or making the selection
conditional, be preferable here? Note that simply adding the two depends
lines to DWMAC_ROCKCHIP would turn off Ethernet on existing RK3288/RK3399
configurations, so the choice probably needs to be explicit.
The Kconfig change is also not mentioned anywhere in the commit message.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 8d7042e689261..5d501ed8d1429 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
[ ... ]
> @@ -1208,6 +1237,43 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
> +static struct phylink_pcs *rk_select_pcs(struct stmmac_priv *priv,
> + phy_interface_t interface)
> +{
> + if (!priv->hw->xpcs)
> + return NULL;
> +
> + return xpcs_to_phylink_pcs(priv->hw->xpcs);
> +}
[Severity: Low]
Should rk_select_pcs() look at its interface argument? The mac_select_pcs
contract in include/linux/phylink.h is to return the PCS for the specified
interface, or NULL when none is required, but this returns the SGMII XPCS
for any mode.
On an RK3568 port probed as SGMII, rk_get_interfaces() also advertises the
RGMII variants and RMII, and xpcs->pcs.supported_interfaces is non-empty
(filled in by xpcs_get_interfaces()). When phylink validates one of those
non-SGMII interfaces, phylink_validate_mac_and_pcs() hits:
if (!phy_interface_empty(pcs->supported_interfaces) &&
!test_bit(state->interface, pcs->supported_interfaces)) {
phylink_err(pl, "MAC returned PCS which does not support %s\n",
phy_modes(state->interface));
return -EINVAL;
}
Would returning the XPCS only for PHY_INTERFACE_MODE_SGMII and NULL
otherwise avoid that error path?
> @@ -1379,16 +1450,19 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
> if (ret)
> return ret;
>
> + ret = gmac_clk_enable(bsp_priv, true);
> + if (ret)
> + return ret;
> +
[Severity: Medium]
This isn't a bug introduced by this patch, but now that gmac_clk_enable()
is the first resource acquisition in rk_gmac_powerup(), its partial-failure
path becomes more visible. Do the bulk clocks leak when the clk_phy enable
fails?
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:gmac_clk_enable() {
...
ret = clk_bulk_prepare_enable(bsp_priv->num_clks,
bsp_priv->clks);
if (ret)
return ret;
ret = clk_prepare_enable(bsp_priv->clk_phy);
if (ret)
return ret;
...
}
The second failure returns without calling clk_bulk_disable_unprepare() and
without setting bsp_priv->clk_enabled, so the later gmac_clk_enable(bsp_priv,
false) in rk_gmac_powerdown() and at the new clk_disable label is a no-op
because it is gated on clk_enabled. This only triggers for integrated-PHY
setups where clk_phy is non-NULL.
> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> + goto set_mode;
> +
> ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
> if (ret < 0)
> - return ret;
> + goto clk_disable;
[ ... ]
> @@ -1486,6 +1566,9 @@ static void rk_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
>
> if (rk->supports_rmii)
> __set_bit(PHY_INTERFACE_MODE_RMII, interfaces);
> +
> + if (rk->supports_sgmii)
> + __set_bit(PHY_INTERFACE_MODE_SGMII, interfaces);
> }
[Severity: Low]
Is SGMII safe to advertise on a port that was not probed as SGMII?
rk3568_ops populates set_to_sgmii unconditionally, and rk_gmac_setup()
derives supports_sgmii from it, so every RK3568 port now advertises SGMII
in phylink's supported_interfaces (stmmac_phy_setup() copies the
get_interfaces() output verbatim into config->supported_interfaces).
All the SGMII setup, though, is keyed off the probe-time DT phy-mode:
- rk_gmac_powerup() writes RK3568_GMAC_MODE_SGMII_QSGMII only in the
"case PHY_INTERFACE_MODE_SGMII:" branch, which runs once at init
- rk_gmac_probe() installs pcs_init/pcs_exit/select_pcs,
default_an_inband and clears set_clk_tx_rate only under
"if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII)"
So on a port probed as rgmii*/rmii, if phylink later selects SGMII (SFP
cage, or a PHY reporting SGMII as its host interface), GRF bit 7 stays
cleared by rk3568_set_to_rgmii()/rk3568_set_to_rmii(), plat->select_pcs is
NULL so stmmac_mac_select_pcs() returns NULL and there is no PCS to run
autonegotiation. Before this patch SGMII was not in supported_interfaces
and such a configuration was rejected during validation. Would gating the
__set_bit() on the probe-time interface be closer to what the hardware can
actually do?
> @@ -1602,6 +1685,17 @@ static int rk_gmac_probe(struct platform_device *pdev)
> plat_dat->suspend = rk_gmac_suspend;
> plat_dat->resume = rk_gmac_resume;
>
> + if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> + /* SGMII clock always runs at 125 MHz */
> + plat_dat->set_clk_tx_rate = NULL;
> +
> + /* SGMII requires a PCS */
> + plat_dat->default_an_inband = true;
[Severity: Medium]
The commit message says:
"Enable default_an_inband for SGMII and disable the generic stmmac
set_clk_tx_rate callback. This forces phylink to use in-band mode,
where the PCS is responsible for speed/duplex negotiation."
Is "forces" accurate? default_an_inband is only a default, and the next
paragraph of the message says so itself. In phylink_parse_mode():
if (pl->config->default_an_inband)
pl->cfg_link_an_mode = MLO_AN_INBAND;
dn = fwnode_get_named_child_node(fwnode, "fixed-link");
if (dn || fwnode_property_present(fwnode, "fixed-link"))
pl->cfg_link_an_mode = MLO_AN_FIXED;
and xpcs_inband_caps() advertises out-of-band as acceptable:
case DW_AN_C37_SGMII:
case DW_AN_C37_1000BASEX:
return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
so phylink_pcs_neg_mode() can settle on PHYLINK_PCS_NEG_OUTBAND for a
fixed-link node or a PHY without in-band support. With set_clk_tx_rate set
to NULL, rk3568_ops.set_speed = rk_set_clk_mac_speed also becomes
unreachable, so a 10/100 Mbps link is brought up with the clocking left at
the 1 Gbps setting and nothing warns or fails. Would rejecting a
non-inband neg_mode (for example in mac_config or pcs_validate), or failing
probe when a fixed-link node is present for SGMII, match what the message
describes?
One more observation on the series rather than on this patch: the new
binding Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
lists phys and phy-names in its unconditional required list, but the
ethernet-pcs@fda00000 node added to arch/arm64/boot/dts/rockchip/rk3568.dtsi
supplies only reg, clocks, clock-names and power-domains. Only
rk3568-photonicat.dts adds them:
&xpcs {
phys = <&combphy2 PHY_TYPE_SGMII>;
phy-names = "serdes";
status = "okay";
};
[Severity: Low]
Does dtbs_check report "phys is a required property" for the other RK3568
board DTBs? dtschema validates nodes regardless of status = "disabled".
The neighbouring sata0 node in the same dtsi does carry phys/phy-names at
SoC level, so would moving them into rk3568.dtsi be an option?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913185849.907479-1-coiaprant%40gmail.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v5 10/11] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
` (8 preceding siblings ...)
2026-09-13 18:58 ` [PATCH net-next v5 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568 Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
2026-09-13 18:58 ` [PATCH net-next v5 11/11] MAINTAINERS: add entry for Rockchip XPCS driver Coia Prant
10 siblings, 0 replies; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
The Ariaboard Photonicat has a Motorcomm YT8521SC Gigabit Ethernet PHY
connected to GMAC0 via XPCS SGMII. Enable the necessary nodes to make
this port functional.
Add rockchip,sgmii-mac-sel = <0> to the already enabled combphy2,
to route the SGMII interface to GMAC0.
Enable the xpcs node and its port 0 sub-node, referencing combphy2
as the SerDes PHY.
Add the mdio0 node with the YT8521SC PHY at address 3, including its
reset GPIO and LED configuration. Also add LED configuration for the
existing RGMII PHY on mdio1 for consistency.
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
.../boot/dts/rockchip/rk3568-photonicat.dts | 74 ++++++++++++++++++-
1 file changed, 72 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
index 58c1052ba8ef3..fdaa4a2a4328b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
@@ -3,6 +3,7 @@
/dts-v1/;
#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/leds/common.h>
#include <dt-bindings/pinctrl/rockchip.h>
#include <dt-bindings/soc/rockchip,vop2.h>
#include "rk3568.dtsi"
@@ -241,6 +242,7 @@ &combphy1 {
};
&combphy2 {
+ rockchip,sgmii-mac-sel = <0>;
status = "okay";
};
@@ -260,9 +262,18 @@ &cpu3 {
cpu-supply = <&vdd_cpu>;
};
-/* Motorcomm YT8521SC LAN port (require SGMII) */
+/* Motorcomm YT8521SC LAN port */
&gmac0 {
- status = "disabled";
+ assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
+ assigned-clock-parents = <&clk_gmac0_xpcs_mii>;
+ managed = "in-band-status";
+ pcs-handle = <&xpcs_mii0>;
+ phy-handle = <&sgmii_phy>;
+ phy-mode = "sgmii";
+ phy-supply = <&vcc_3v3>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&gmac0_miim>;
+ status = "okay";
};
/* Motorcomm YT8521SC WAN port */
@@ -341,6 +352,36 @@ &i2s0_8ch {
status = "okay";
};
+&mdio0 {
+ sgmii_phy: ethernet-phy@3 {
+ compatible = "ethernet-phy-ieee802.3-c22";
+ reg = <0x3>;
+ max-speed = <1000>;
+ reset-assert-us = <20000>;
+ reset-deassert-us = <100000>;
+ reset-gpios = <&gpio3 RK_PC6 GPIO_ACTIVE_LOW>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_LAN;
+ default-state = "keep";
+ };
+
+ led@2 {
+ reg = <2>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ default-state = "keep";
+ };
+ };
+ };
+};
+
&mdio1 {
rgmii_phy: ethernet-phy@3 {
compatible = "ethernet-phy-ieee802.3-c22";
@@ -350,6 +391,25 @@ rgmii_phy: ethernet-phy@3 {
reset-gpios = <&gpio4 RK_PC0 GPIO_ACTIVE_LOW>;
rx-internal-delay-ps = <1500>;
tx-internal-delay-ps = <1500>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_WAN;
+ default-state = "keep";
+ };
+
+ led@2 {
+ reg = <2>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_WAN;
+ default-state = "keep";
+ };
+ };
};
};
@@ -586,3 +646,13 @@ &xin32k {
pinctrl-names = "default";
pinctrl-0 = <&clk32k_out1>;
};
+
+&xpcs {
+ phys = <&combphy2 PHY_TYPE_SGMII>;
+ phy-names = "serdes";
+ status = "okay";
+};
+
+&xpcs_mii0 {
+ status = "okay";
+};
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH net-next v5 11/11] MAINTAINERS: add entry for Rockchip XPCS driver
2026-09-13 18:58 [PATCH net-next v5 00/11] net: pcs: add basic support for RK3568 XPCS Coia Prant
` (9 preceding siblings ...)
2026-09-13 18:58 ` [PATCH net-next v5 10/11] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port Coia Prant
@ 2026-09-13 18:58 ` Coia Prant
10 siblings, 0 replies; 21+ messages in thread
From: Coia Prant @ 2026-09-13 18:58 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Vinod Koul, Maxime Chevallier, Maxime Coquelin,
Alexandre Torgue, Lad Prabhakar, Romain Gantois, Heiner Kallweit,
Coia Prant
Cc: Neil Armstrong, Russell King, Shawn Lin, David Heidelberg,
netdev, linux-rockchip, devicetree, linux-arm-kernel,
linux-kernel, linux-phy, linux-stm32, linux-renesas-soc
Add a MAINTAINERS entry for the Rockchip RK3568 XPCS platform driver
and its device tree binding.
Include the relevant mailing lists (netdev and linux-rockchip) so that
future patches are properly distributed.
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index c2414447892c2..960a53afa8357 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23739,6 +23739,15 @@ S: Maintained
F: Documentation/devicetree/bindings/sound/rockchip,rk3576-sai.yaml
F: sound/soc/rockchip/rockchip_sai.*
+ROCKCHIP XPCS DRIVER
+M: Coia Prant <coiaprant@gmail.com>
+L: netdev@vger.kernel.org
+L: linux-rockchip@lists.infradead.org
+S: Maintained
+F: Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
+F: drivers/net/pcs/pcs-xpcs-rk.c
+F: include/linux/pcs/pcs-xpcs-rk.h
+
ROCKER DRIVER
M: Jiri Pirko <jiri@resnulli.us>
L: netdev@vger.kernel.org
--
2.47.3
^ permalink raw reply [flat|nested] 21+ messages in thread