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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In this binding patch, the new AST2700 rule `if compatible
  contains aspeed,ast2700-mac then required: [phy-mode]`…
- [Medium] This patch makes `aspeed,ast2700-mac` a standalone compatible,
  "consistent with the other Aspeed compatibles".
- [Low] Concern (baseline: not a valid finding): the commit message says
  the AST2700 phy-mode requirement matches "the driver's probe-time…
- [Low] Concern (baseline: not a valid finding): the binding documents
  `aspeed,ast2700-mac` as standalone, but ftgmac100_of_match[] has no…
- [Low] Concern (baseline: not a valid finding): for AST2700 the binding
  only allows `resets`, as for AST2600, instead of requiring it.

--

dt-bindings: net: ftgmac100: Add AST2700 compatible

This adds aspeed,ast2700-mac as a standalone compatible to the ftgmac100
binding. It allows the resets property for it, as is already done for
AST2600. It also adds a conditional block that requires phy-mode whenever
the AST2700 compatible is used.

> 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

[Severity: Medium]
This builds on the previous patch in the series, commit c64ed4df5e64
("dt-bindings: net: ftgmac100: Make Aspeed compatibles standalone"). That
patch turned the compatible into a oneOf of single strings. Do the existing
in-tree Aspeed MAC nodes still pass validation with that?

aspeed-g4.dtsi, aspeed-g5.dtsi and aspeed-g6.dtsi all use the two-string
form. For example, in aspeed-g6.dtsi:

    compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";

A compatible with two entries matches neither oneOf branch. So dtbs_check
would now fail for the MAC nodes on every AST2400/2500/2600 board. The
series doesn't update those dtsi files, and neither commit message
mentions this.

The reason given in the previous patch also looks doubtful. In
__of_device_is_compatible() the score depends on the position in the
compatible list:

    if (of_compat_cmp(cp, compat, strlen(compat)) == 0) {
            score = INT_MAX/2 - (index << 2);
            break;
    }

So the more specific "aspeed,ast2x00-mac" entry always wins over
"faraday,ftgmac100". A two-string node can't bind through the generic
entry alone. The bare "faraday,ftgmac100" alternative is also still
accepted, so the hazard described there is still present.

Could the two-string fallback form stay as an accepted alternative, or
could the dtsi files be updated as part of this series?

[Severity: Low]
At this commit, ftgmac100_of_match[] has no entry for aspeed,ast2700-mac
and there is no FTGMAC100_AST2700 mac_id. Would a node that follows this
binding fail to bind at this point?

A later patch in the series, "net: ftgmac100: Add AST2700 compatible
support", seems to handle this. It adds:

    { .compatible = "aspeed,ast2700-mac",
      .data = &ftgmac100_match_data_ast2700 },

It also adds FTGMAC100_AST2700 to the is_aspeed / BIT(30) selection in
ftgmac100_probe(). So this only affects the intermediate state of the
series, and putting the binding before the driver is the usual DT order.

[ ... ]

> @@ -80,12 +81,21 @@ allOf:
>            contains:
>              enum:
>                - aspeed,ast2600-mac
> +              - aspeed,ast2700-mac
>      then:
>        properties:
>          resets: true
>      else:
>        properties:
>          resets: false

[Severity: Low]
This isn't a bug, but for AST2700 resets is only allowed here, not
required. ftgmac100_probe() gets the reset with
devm_reset_control_get_optional_exclusive(). If resets is missing, the
RMII SCU reset in ftgmac100_reset_and_config_mac() silently does nothing.

This matches the AST2600 handling and the commit message. AST2400/2500 run
the same is_aspeed && RMII reset path while their binding forbids resets,
so this is probably intended.

Does AST2700 hardware need the reset in a way AST2600 does not, so that
resets should be required?

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2700-mac
> +    then:
> +      required:
> +        - phy-mode

[Severity: Medium]
Should NC-SI nodes be exempt from this rule? There is no use-ncsi
exception, so an AST2700 node with use-ncsi and no phy-mode now fails
dtbs_check.

That is the same shape as the first example in this binding, which uses
aspeed,ast2500-mac. The use-ncsi description also says "Currently assumes
rmii (100bT)".

The driver doesn't read phy-mode in NC-SI mode, even at the end of the
series. ftgmac100_probe_dt() returns early:

    if (of_get_property(np, "use-ncsi", NULL))
            return ftgmac100_probe_ncsi(netdev, priv, pdev);

and ftgmac100_probe_ncsi() hardcodes the interface:

    err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
                             PHY_INTERFACE_MODE_RMII);

The phy-mode check that "net: ftgmac100: Require phy-mode for AST2700"
adds to ftgmac100_mii_probe() also looks unreachable on AST2700. Only
ftgmac100_setup_mdio() sets priv->mii_bus, and it is called only for
FARADAY/AST2400/AST2500. So ftgmac100_mii_probe() returns -ENODEV with
"No MDIO bus available" before it gets to the phy-mode test.

That leaves the generic requirement in of_phy_get_and_connect() on the
phy-handle/fixed-link path. AST2600 goes through the same path, but this
binding does not require phy-mode for AST2600.

Given this, is "matching the driver's probe-time requirement" accurate?
Could the rule require phy-mode only when use-ncsi is absent?

[Severity: Low]
On the same topic, the commit message says:

    Require "phy-mode" to be present in the device tree when the AST2700
    compatible is used, matching the driver's probe-time requirement.

At this commit the driver has no AST2700 support yet, and a missing
phy-mode falls back to RGMII. Is the driver requirement referred to here
only the one added later in the series?

After "net: ftgmac100: Add AST2700 compatible support" and "net:
ftgmac100: Require phy-mode for AST2700", the AST2700
phy-handle/fixed-link path does require phy-mode, through
of_phy_get_and_connect():

    ret = of_get_phy_mode(np, &iface);
    if (ret)
            return NULL;

The RGMII default no longer applies to AST2700, because
ftgmac100_mii_probe() returns -ENODEV before reaching it. The remaining
gap is the NC-SI case above.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-support_ast2700-v4-0-4d36b40939b3%40aspeedtech.com