Re: [PATCH net-next v6 1/7] dt-bindings: net: ftgmac100: Make Aspeed compatibles standalone

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] dt-bindings: net: ftgmac100: dropping fallback compatible
  breaks dtbs_check
- [Low] dt-bindings: net: ftgmac100: unwieldy per-compatible
  conditionals in schema
- [Low] dt-bindings: net: ftgmac100: inaccurate DMA mask claim in commit
  message
- [Low] dt-bindings: net: ftgmac100: incomplete clock-names constraint
  for two clocks
--

commit 8d20c693d9736c24e1fec0cfa5f5df1d9636e586
Author: Jacky Chou <jacky_chou@aspeedtech.com>

dt-bindings: net: ftgmac100: Make Aspeed compatibles standalone

This commit updates the ftgmac100 Device Tree binding to make the Aspeed
compatible strings standalone and adds per-compatible constraints for the
clocks and clock-names properties.

> The aspeed,ast2400-mac, aspeed,ast2500-mac, and aspeed,ast2600-mac
> compatibles are currently only valid when paired with the generic
> "faraday,ftgmac100" fallback: if a node ever bound through the
> generic entry alone, ftgmac100_probe() would set
> rxdes0_edorr_mask/txdes0_edotr_mask to BIT(15) instead of the Aspeed
> BIT(30), skip Aspeed clock setup, and leave the DMA mask too narrow.

[Severity: Low]
Is the commit message accurate about ftgmac100_probe() leaving the DMA
mask too narrow for these compatibles?

In drivers/net/ethernet/faraday/ftgmac100.c, ftgmac100_probe() does not
call dma_set_mask() or dma_set_mask_and_coherent() for AST2400, AST2500,
or AST2600.

> diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> index d14410018bcf6..9d352e3686aaf 100644
> --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> @@ -11,14 +11,11 @@ maintainers:
>  
>  properties:
>    compatible:
> -    oneOf:
> -      - const: faraday,ftgmac100
> -      - items:
> -          - enum:
> -              - aspeed,ast2400-mac
> -              - aspeed,ast2500-mac
> -              - aspeed,ast2600-mac
> -          - const: faraday,ftgmac100
> +    enum:
> +      - faraday,ftgmac100
> +      - aspeed,ast2400-mac
> +      - aspeed,ast2500-mac
> +      - aspeed,ast2600-mac

[Severity: Medium]
As reported by Rob Herring on v4 [1], does dropping the two-string
compatible entry break dtbs_check and backwards compatibility for existing
device trees?

All MAC nodes in arch/arm/boot/dts/aspeed/aspeed-g4.dtsi, aspeed-g5.dtsi,
and aspeed-g6.dtsi, as well as existing device trees in the field and
kernels prior to 03a2aba50b2c ("net: ftgmac100: List all compatibles"),
declare the SoC compatible followed by "faraday,ftgmac100".  Retaining
the two-item compatible entry with deprecated: true alongside the
standalone enum would allow the new standalone compatibles without
breaking validation of existing device trees.

> @@ -88,22 +85,76 @@ allOf:
>      else:
>        properties:
>          resets: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2400-mac
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 1
> +        clock-names:
> +          maxItems: 1
> +      required:
> +        - clocks
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - aspeed,ast2500-mac
> +              - aspeed,ast2600-mac
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 1
> +          maxItems: 2
> +        clock-names:
> +          minItems: 1
> +          maxItems: 2
> +      required:
> +        - clocks
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - aspeed,ast2400-mac
> +                - aspeed,ast2500-mac
> +                - aspeed,ast2600-mac
> +    then:
> +      properties:
> +        clocks: false
> +        clock-names: false

[Severity: Low]
This isn't a bug, but now that the Aspeed compatibles are standalone and no
longer fall back to faraday,ftgmac100, their property requirements diverge
enough from plain faraday,ftgmac100 (which forbids resets, clocks, and
clock-names) that piling multiple per-compatible if-then blocks into
faraday,ftgmac100.yaml makes the single schema unwieldy and duplicates the
top-level minItems/maxItems constraints on clocks and clock-names.  Would
splitting the Aspeed MAC binding into its own schema document be cleaner?

> +  - if:
> +      properties:
> +        clocks:
> +          minItems: 2
> +    then:
> +      required:
> +        - clock-names

[Severity: Low]
Does this rule still allow a single-element clock-names array when two
clocks are specified?

Because clock-names permits minItems: 1, requiring clock-names without
also setting properties: clock-names: minItems: 2 in the then branch lets
a node with two clocks and only clock-names = "MACCLK" pass validation,
even though ftgmac100_setup_clk() looks up the second clock by name via
devm_clk_get_optional_enabled(priv->dev, "RCLK").  In addition, since
clocks is not required at the top level, the if branch needs required:
[clocks] so nodes without clocks do not match this condition under JSON
Schema evaluation.

[1] https://lore.kernel.org/r/20261020181441.GA510274-robh@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-support_ast2700-v6-0-238020bf46cf@aspeedtech.com?part=1