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: 6867 bytes --]

On Wed, Oct 14, 2026 at 10:24:47AM +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.
> 
> Fair enough — you're right that "it breaks dtbs_check" isn't a reason not to fix it, and if the 
> EDORR bit mismatch is real for AST2400/2500/2600 too, it should be fixed properly rather 
> than left as a known landmine.
> 
> I'll prepare a follow-up series for this:
> 
> - dt-bindings: net: ftgmac100: make aspeed,ast2400/2500/2600-mac standalone consts
>   (drop the faraday,ftgmac100 fallback requirement, same shape as the AST2700 fix here)
> - ARM: dts: aspeed: drop the faraday,ftgmac100 fallback compatible from
>   aspeed-g4.dtsi / aspeed-g5.dtsi / aspeed-g6.dtsi
> 
> I'd like to keep it as a separate series rather than folding it into this one, since the
> dtsi changes go through the Aspeed/arm-soc tree rather than net-next, and I'd rather not
> block this AST2700 series on cross-tree coordination. I'll send it as a follow-up once
> this series lands (or in parallel, if that's preferred) — let me know if you'd rather
> see it done differently.

I'd fix the binding here, since you're gonna generate conflicts with
yourself otherwise. The dts changes need to be sent separately anyway
(or at least applied separately) since binding changes don't go via soc
maintainers.

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