回覆: 回覆: 回覆: [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.
> 
> 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.

Thanks,
Jacky