回覆: 回覆: [PATCH net-next v2 1/6] dt-bindings: net: ftgmac100: Add AST2700 compatible

[Jacky Chou]

> > > > > @@ -18,6 +18,7 @@ properties:
> > > > >                - aspeed,ast2400-mac
> > > > >                - aspeed,ast2500-mac
> > > > >                - aspeed,ast2600-mac
> > > > > +              - aspeed,ast2700-mac
> > > > >            - const: faraday,ftgmac100
> > > >
> > > > [Severity: Low]
> > > > With this patch, "aspeed,ast2700-mac", "faraday,ftgmac100" becomes
> > > > a valid compatible pair. However, at this commit,
> > > > ftgmac100_of_match[] in drivers/net/ethernet/faraday/ftgmac100.c
> > > > has no aspeed,ast2700-mac
> > > entry.
> > > > Such a node could only bind through the generic faraday,ftgmac100
> > > > entry (FTGMAC100_FARADAY).
> > > >
> > > > On that path, ftgmac100_probe() would do the following:
> > > >
> > > >   - program the ring-end bits at BIT(15) instead of the Aspeed BIT(30)
> > > >   - leave is_aspeed false, so ftgmac100_setup_clk() never claims
> > > >     MACCLK/RCLK
> > > >   - register the embedded MDIO bus through ftgmac100_setup_mdio()
> > > >   - never set a DMA mask wider than 32 bits
> > > >
> > > > Would an AST2700 node work correctly if it fell back to the
> > > > Faraday match?
> > >
> > > Sounds like no, can the submitter please clarify if the fallback is valid?
> > >
> >
> > Agreed. I think the 'aspeed,ast2700-mac' should not be based on the
> 'faraday,ftgmac100'.
> > Maybe the dt-bindings put ast2700-mac as a const like:
> > ```
> > ...
> > properties:
> >   compatible:
> >     oneOf:
> >       - const: faraday,ftgmac100
> >       - const: aspeed,ast2700-mac             // as a const
> >       - items:
> >           - enum:
> >               - aspeed,ast2400-mac
> >               - aspeed,ast2500-mac
> >               - aspeed,ast2600-mac
> >           - const: faraday,ftgmac100
> 
> Looks good. I am curious whether the existing fallbacks used with other aspeed
> devices are also inaccurate? Could be worth looking into?

Thanks for digging into this further.

I went back to the current driver to confirm the exact bit
(drivers/net/ethernet/faraday/ftgmac100.c, ftgmac100_probe()):

if (priv->mac_id == FTGMAC100_AST2400 ||
    priv->mac_id == FTGMAC100_AST2500 ||
    priv->mac_id == FTGMAC100_AST2600 ||
    priv->mac_id == FTGMAC100_AST2700) {
        priv->rxdes0_edorr_mask = BIT(30);
        priv->txdes0_edotr_mask = BIT(30);
        priv->is_aspeed = true;
} else {
        priv->rxdes0_edorr_mask = BIT(15);
        priv->txdes0_edotr_mask = BIT(15);
}

So it's actually BIT(30) for all Aspeed variants (AST2400/2500/2600/2700), vs
BIT(15) for the plain Faraday IP — not BIT(16). The conclusion still holds either
way: none of the Aspeed compatibles can safely fall back to faraday,ftgmac100 alone,
since the driver would mark the wrong bit as end-of-ring in rxdes0, and the MAC would
walk past the end of the RX descriptor ring instead of wrapping, exactly as you
described.

That said, I don't think we should extend the same fix to AST2400/2500/2600 in this
series. AST2700 is a brand-new compatible with no existing devicetree using it, so
making it a standalone const (compatible = "aspeed,ast2700-mac" only) has zero
migration cost.

AST2400/2500/2600 are different: every upstream devicetree already declares the pair,
e.g. aspeed-g6.dtsi:

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

If we turn aspeed,ast2600-mac (etc.) into a standalone const in the oneOf, the
schema would only accept a single-element compatible array, which would break
make dtbs_check for every existing AST2400/2500/2600 board devicetree. We'd also need
to strip the faraday,ftgmac100 fallback from aspeed-g4.dtsi, aspeed-g5.dtsi,
aspeed-g6.dtsi (and anything inheriting from them) — a much bigger, unrelated change.

It's also not fixing a live bug: ftgmac100_of_match[] only had the generic
faraday,ftgmac100 entry until Feb 2026 (commit 03a2aba50b2c), when the
Aspeed-specific of_device_id rows for AST2400/2500/2600 were added as a refactor.
Before that, the driver always bound through the generic fallback string and used
of_device_is_compatible() internally in probe() to pick the right EDORR bit /
clocks / MDIO. So there's never been a real kernel + devicetree combination where an
AST2400/2500/2600 node bound through the generic entry and got the wrong EDORR bit —
the pairing has been safe in practice.

So I'd suggest leaving AST2400/2500/2600 as-is (paired with the fallback, same as
today) and keeping the standalone-const treatment scoped to AST2700 only, since that's
the one case where the binding and match table genuinely diverge right now.

Thanks,
Jacky