Re: [PATCH net-next v5 03/10] dt-bindings: net: mediatek-dwmac: add support for MT8189 SoC

[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 new compatible `mediatek,mt8189-gmac` is added to the
  binding, but the descriptions of `mediatek,tx-delay-ps` and…

--

dt-bindings: net: mediatek-dwmac: add support for MT8189 SoC

This patch adds the mediatek,mt8189-gmac compatible to the MediaTek DWMAC
binding and lowers the clocks/clock-names minItems to 4. It also adds
allOf conditionals that keep 5 clocks for MT2712/MT8188/MT8195, allow at
most 4 for MT8189, and disallow mediatek,rmii-clk-from-mac on MT8189.

> diff --git a/Documentation/devicetree/bindings/net/mediatek-dwmac.yaml b/Documentation/devicetree/bindings/net/mediatek-dwmac.yaml
> index 9562406166387..0fc2eed066464 100644
> --- a/Documentation/devicetree/bindings/net/mediatek-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/mediatek-dwmac.yaml

[ ... ]

> @@ -153,6 +152,39 @@ required:
>    - phy-mode
>    - mediatek,pericfg
>  
> +allOf:
> +  - $ref: snps,dwmac.yaml#

[ ... ]

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - mediatek,mt8189-gmac
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 4
> +        clock-names:
> +          maxItems: 4
> +
> +        mediatek,rmii-clk-from-mac: false

[Severity: Low]

This block only disallows mediatek,rmii-clk-from-mac for MT8189, so
mediatek,tx-delay-ps and mediatek,rx-delay-ps are still allowed there.
Should their descriptions also cover the new compatible?

At the moment they only describe MT2712 and MT8188/MT8195:

  mediatek,tx-delay-ps:
    ...
      For MT8188/MT8195 RGMII/RMII/MII interface, Allowed value need to be a multiple of 290,
      or will round down. Range 0~31*290.

The mediatek,rx-delay-ps description has the same text.

The later patch in this series, "net: stmmac: mediatek: add support for
MT8189 SoC", uses a different step size and maximum for MT8189:

  #define MT8189_MII_DLY_STAGE_DIV	180  /* 180ps per stage */
  #define MT8189_MII_DELAY_MAX		5760 /* 32 x 180ps */

For MII/RMII, these vendor properties are the only way to set the internal
delays. mediatek_dwmac_config_dt() reads the standard *-internal-delay-ps
properties only for RGMII.

Suppose a value is picked using the documented 290 ps step. The driver will
then program value / 180 stages instead of value / 290. Any value of 5760 ps
or more (for example 20 * 290 = 5800) is rejected with -EINVAL:

  of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps);
  if (tx_delay_ps >= plat->variant->mii_delay_max) {

and probe fails.

Setting mediatek,tx-delay-ps: false for MT8189 would not be the right fix,
because it would remove the only way to configure MII/RMII delays on MT8189.

Could both descriptions get an MT8189 line with the 180 ps step and the
0~31*180 range instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-dwmac-mediatek-mt8189-v5-0-7d99d6292962%40collabora.com