Re: [PATCH net-next v5 2/7] dt-bindings: net: ftgmac100: Add AST2700 compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: net: ftgmac100: requiring phy-mode on AST2700 breaks use-ncsi
- [Low] dt-bindings: net: ftgmac100: split Aspeed binding instead of adding if-thens
--

commit 761317ab12f9bb253e49b616ba9665479d666ab1
Author: Jacky Chou <jacky_chou@aspeedtech.com>

dt-bindings: net: ftgmac100: Add AST2700 compatible

This commit adds the aspeed,ast2700-mac compatible to the FTGMAC100 Device
Tree binding schema, allowing the reset line and requiring the phy-mode
property for AST2700.

> diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> index c870dd0bda7b2..0a376b849a916 100644
> --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> @@ -17,6 +17,7 @@ properties:
>            - aspeed,ast2400-mac
>            - aspeed,ast2500-mac
>            - aspeed,ast2600-mac
> +          - aspeed,ast2700-mac
>  
>    reg:
>      maxItems: 1
> @@ -28,7 +29,7 @@ properties:
>      minItems: 1
>      items:
>        - description: MAC IP clock
> -      - description: RMII RCLK gate for AST2500/2600
> +      - description: RMII RCLK gate for AST2500/2600/2700
>  
>    resets:
>      maxItems: 1
> @@ -80,12 +81,21 @@ allOf:
>            contains:
>              enum:
>                - aspeed,ast2600-mac
> +              - aspeed,ast2700-mac
>      then:
>        properties:
>          resets: true
>      else:
>        properties:
>          resets: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2700-mac
> +    then:
> +      required:
> +        - phy-mode

[Severity: Medium]
Does unconditionally requiring phy-mode for aspeed,ast2700-mac break schema
validation for nodes configured with use-ncsi?

In Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml, use-ncsi is
documented as using the NC-SI stack over RMII without an MDIO PHY, and the
first example in the binding specifies use-ncsi without phy-mode.

In drivers/net/ethernet/faraday/ftgmac100.c, ftgmac100_probe_dt() checks for
use-ncsi before reading phy-mode:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_probe_dt() {
	if (of_get_property(np, "use-ncsi", NULL))
		return ftgmac100_probe_ncsi(netdev, priv, pdev);
}

ftgmac100_probe_ncsi() connects a fixed PHY with PHY_INTERFACE_MODE_RMII
directly without consulting phy-mode.

Also, the commit message mentions matching the driver's probe-time
requirement, which in commit e655d4e27cbf2 ("net: ftgmac100: Require phy-mode
for AST2700") was added to ftgmac100_mii_probe(). However,
ftgmac100_mii_probe() is only reachable when priv->mii_bus is allocated by
ftgmac100_setup_mdio(), which is only called for FARADAY, AST2400, and
AST2500, and not for AST2600 or AST2700. When phy-handle or fixed-link is
used, of_phy_get_and_connect() already requires phy-mode across all SoCs.

Should phy-mode not be unconditionally required when use-ncsi is present, or
is this AST2700-specific rule unnecessary?

[Severity: Low]
This isn't a bug, but since the Aspeed MAC compatibles (aspeed,ast2400-mac,
aspeed,ast2500-mac, aspeed,ast2600-mac, and aspeed,ast2700-mac) are standalone
compatibles that no longer fall back to faraday,ftgmac100 and diverge in
clocks, resets, use-ncsi, embedded MDIO child nodes, and per-compatible
allOf conditionals, would it be cleaner to split the Aspeed MAC binding into a
separate schema document rather than adding more per-compatible if-then blocks
to faraday,ftgmac100.yaml?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-support_ast2700-v5-0-44057f4dde75@aspeedtech.com?part=2