Re: [PATCH net-next v6 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-17 18:38:41
Also in:
linux-arm-kernel, linux-devicetree, linux-renesas-soc, linux-rockchip, lkml, netdev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 2 - [Low] Commit message describes code that does not exist at this commit: the message states the Rockchip dwmac glue "now supports SGMII on… - [Low] The new `allOf` conditional in Documentation/devicetree/bindings/net/rockchip-dwmac.yaml keys only on… -- dt-bindings: net: rockchip-dwmac: document pcs-handle This adds a pcs-handle property to the Rockchip GMAC binding and a new allOf conditional making it required whenever phy-mode is "sgmii". Two observations below, one about the commit message wording and one about the scope of the new conditional.
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".
[Severity: Low]
This isn't a bug, but the wording is in the past tense for code that is not
present yet at this commit. At fc2e19066d08,
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c has no rk_pcs_init(), no
"pcs-handle" parsing, and no pcs_init/pcs_exit/select_pcs callbacks or
SGMII support.
Checking the end of the series, those all arrive in the following patch
("net: stmmac: dwmac-rk: add SGMII support for RK3568"), which adds
rk_pcs_init() with:
pcs_node = of_parse_phandle(np, "pcs-handle", 0);
if (!pcs_node)
return -ENODEV;
and installs the callbacks in rk_gmac_probe() for
PHY_INTERFACE_MODE_SGMII.
Putting the binding before the driver is the usual convention, so would it
be worth rewording to say the property will be parsed by rk_pcs_init() in
the following patch, rather than that it already is?
quoted hunk ↗ jump to hunk
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 +
[Severity: Low]
Should this conditional also key on compatible? As written it keys only on
phy-mode, so required: [pcs-handle] applies to all sixteen compatibles this
schema selects (px30, rk3128, rk3228, rk3288, rk3308, rk3328, rk3366,
rk3368, rk3399, rk3506, rk3528, rk3568, rk3576, rk3588, rv1108, rv1126).
At the end of the series only rk3568_ops provides .set_to_sgmii, and
dwmac-rk.c derives the capability from it:
bsp_priv->supports_sgmii = ops->supports_sgmii || !!ops->set_to_sgmii;
so the PCS requirement looks specific to rockchip,rk3568-gmac.
The reverse direction is open too: pcs-handle is added unconditionally, so a
GMAC node on a Rockchip SoC with no XPCS can carry a pcs-handle and still
pass dt_binding_check. The rest of this file already gates SoC-specific
constraints on compatible with a negative else branch, for example:
- if:
properties:
compatible:
contains:
enum:
- rockchip,rk3576-gmac
- rockchip,rk3588-gmac
then:
required:
- rockchip,php-grf
else:
properties:
rockchip,php-grf: false
Documentation/devicetree/bindings/net/renesas,rzv2h-gbeth.yaml uses
"pcs-handle: false" in the else branch for the same purpose. Would nesting
the new block under a check for rockchip,rk3568-gmac, plus
"pcs-handle: false" for the variants without a PCS, describe the hardware
more accurately?
No in-tree DT is affected today: the only Rockchip GMAC node with
phy-mode = "sgmii" is in rk3568-photonicat.dts, and it already provides
pcs-handle = <&xpcs_mii0>.
unevaluatedProperties: false examples:
-- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915123802.1561724-1-coiaprant%40gmail.com -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy