Re: [PATCH v6 2/7] dt-bindings: net: amlogic,meson-dwmac: add amlogic,t7-dwmac

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] dt-bindings: net: amlogic,meson-dwmac: unwieldy per-compatible
  conditionals
--

commit bc12f1675c9788e6cacf4400aec8cfab4ff91981
Author: Lucas Tanure <tanure@linux.com>

dt-bindings: net: amlogic,meson-dwmac: add amlogic,t7-dwmac

This commit adds the amlogic,t7-dwmac compatible string and its associated
clock and delay constraints to the Amlogic Meson DWMAC device tree binding
schema.

> diff --git a/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml b/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml
> index e769c9bb70d84..ff5e50f651144 100644
> --- a/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml

[ ... ]

> @@ -84,6 +85,49 @@ allOf:
>          rx-internal-delay-ps:
>            default: 0
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: amlogic,t7-dwmac
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: GMAC main clock
> +            - description: First parent clock of the internal mux
> +            - description: Second parent clock of the internal mux
> +            - description:
> +                clock of the pipeline stage inserted in the bus path between
> +                the controller and the DRAM. Without it, the controller cannot
> +                complete DMA transfers.
> +            - description:
> +                clock of the register block holding PRG_ETH0 and PRG_ETH1.
> +                Without it, the block reads as zero and ignores writes.
> +
> +        clock-names:
> +          items:
> +            - const: stmmaceth
> +            - const: clkin0
> +            - const: clkin1
> +            - const: pipeline
> +            - const: pclk
> +
> +        # The PHY adds the 2ns RGMII delays, see ethernet-controller.yaml.
> +        # The RX delay line of this glue is fine tuning in 200ps steps,
> +        # added on top of what phy-mode describes. The TX side has no fine
> +        # tuning: its only step is a quarter of the clock period, the full
> +        # 2ns at 1Gbit/s, so the TX property is not accepted.
> +        rx-internal-delay-ps:
> +          default: 0
> +
> +        tx-internal-delay-ps: false
> +
> +        # Deprecated: new SoCs use the standard *-internal-delay-ps
> +        # properties instead.
> +        amlogic,tx-delay-ns: false
> +        amlogic,rx-delay-ns: false
> +
>    - if:
>        properties:
>          compatible:
> @@ -106,6 +150,7 @@ allOf:
>            contains:
>              enum:
>                - amlogic,meson-g12a-dwmac
> +              - amlogic,t7-dwmac
>      then:
>        properties:
>          rx-internal-delay-ps:
> @@ -142,6 +187,10 @@ properties:
>        - items:
>            - const: amlogic,meson6-dwmac
>            - const: snps,dwmac
> +      - items:
> +          - const: amlogic,t7-dwmac
> +          - const: snps,dwmac-5.10a
> +          - const: snps,dwmac

[Severity: Low]
This isn't a bug, but as reported by Sashiko on v5 [1], the amlogic,t7-dwmac
variant differs substantially from the existing Meson6 and Meson8b-through-G12A
variants in Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml: it
uses a different Synopsys core fallback (snps,dwmac-5.10a instead of
snps,dwmac-3.70a or snps,dwmac), requires a different five-clock list
(stmmaceth, clkin0, clkin1, pipeline, pclk without timing-adjustment), and
disallows tx-internal-delay-ps as well as the legacy amlogic,tx-delay-ns and
amlogic,rx-delay-ns properties.

Rather than piling another large per-compatible if-then conditional block into
allOf and splitting the rx-internal-delay-ps constraints for amlogic,t7-dwmac
across two separate if-then blocks in allOf (while also setting
amlogic,tx-delay-ns and amlogic,rx-delay-ns to false even though those vendor
properties are only defined inside the mutually exclusive Meson8b-through-G12A
conditional branch and are already rejected by unevaluatedProperties: false),
would it be cleaner to split the divergent T7 binding into a separate schema
document?

[1] https://lore.kernel.org/r/20261103163715.385394-3-tanure@linux.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105100322.37816-1-tanure@linux.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic