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

[Jacky Chou]

> > > > > > > > >                - 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.

Got it. I will add change in this patch in next version.

Thanks,
Jacky