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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5150 bytes --]

On Wed, Oct 14, 2026 at 02:49:37AM +0000, Jacky Chou wrote:
> > > > > > @@ -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.

It's wrong, and there's no pain involved in fixing it (just have to send
patches), so I think it should be fixed. Breaking dtbs_check isn't
an excuse to not fix things, we "break" it all the time adding new
devices etc.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]