Re: [PATCH net-next v11 4/6] dt-bindings: net: rockchip-dwmac: document pcs-handle

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds an allOf entry to
  Documentation/devicetree/bindings/net/rockchip-dwmac.yaml (lines…
- [Low] At this commit, rockchip-dwmac.yaml documents `pcs-handle` and
  requires it for rk3568 when phy-mode is "sgmii", but nothing in dwmac-…

--

dt-bindings: net: rockchip-dwmac: document pcs-handle

This patch documents a pcs-handle property in the Rockchip GMAC binding.
It makes the property required for rockchip,rk3568-gmac when phy-mode is
"sgmii". The SGMII driver support that uses the property arrives later in
the series.

> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> index 80c252845349c..c434450fd07ff 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml

[ ... ]

> @@ -159,6 +165,27 @@ allOf:
>          clocks:
>            minItems: 5
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: rockchip,rk3568-gmac
> +    then:
> +      required:
> +        - phy-mode
> +

[Severity: Low]
Is this block needed? The commit message doesn't mention it.

rockchip-dwmac.yaml already includes snps,dwmac.yaml through its allOf:

  - $ref: snps,dwmac.yaml#

That schema already requires phy-mode for every node, with no condition:

Documentation/devicetree/bindings/net/snps,dwmac.yaml:
required:
  - compatible
  - reg
  - interrupts
  - interrupt-names
  - phy-mode

So phy-mode is already required for all Rockchip GMAC compatibles,
including rk3568, and dt_binding_check / dtbs_check give the same results
with or without this block.

Repeating the requirement for rk3568 only could also suggest that phy-mode
is optional on the other Rockchip SoCs.

If the goal is to keep the sgmii if clause below from matching when
phy-mode is absent, the inherited requirement already does that. Could
this block be dropped? Alternatively, a local required: [phy-mode] could
go inside the sgmii if clause.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: rockchip,rk3568-gmac
> +        phy-mode:
> +          contains:
> +            const: sgmii
> +    then:
> +      required:
> +        - pcs-handle
> +

[Severity: Low]
At this commit, nothing in dwmac-rk.c reads pcs-handle yet, and the driver
doesn't accept SGMII. Doesn't that leave the binding and the driver out of
sync if someone bisects to this commit or applies only part of the series?

The consumer arrives in the next patch, "net: stmmac: dwmac-rk: add SGMII
support for RK3568". That patch adds rk_pcs_init():

drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:rk_pcs_init() {
    ...
	pcs_node = of_parse_phandle(node, "pcs-handle", 0);
	if (!pcs_node)
		return -ENODEV;
    ...
}

It also adds the SGMII pcs_init/pcs_exit/select_pcs handling to
rk_gmac_probe(). By the end of the series the binding and driver match,
and putting the binding before its user is the usual DT submission order.
I'm only noting it here for completeness.

>  unevaluatedProperties: false
>  
>  examples:

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008175225.3375274-1-coiaprant%40gmail.com