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

[netdev-bot+sashiko@kernel.org]

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?

> 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