Re: [PATCH net-next v5 08/11] dt-bindings: net: rockchip-dwmac: document pcs-handle

[netdev-bot+sashiko]

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