Re: [PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-14 20:25:17
Also in:
linux-arm-kernel, linux-devicetree, linux-renesas-soc, linux-rockchip, lkml, netdev
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.
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 +
[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.
quoted hunk ↗ jump to hunk
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 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy