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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

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)

quoted

quoted

quoted

quoted

quoted

  - leave is_aspeed false, so ftgmac100_setup_clk() never claims
    MACCLK/RCLK
  - register the embedded MDIO bus through

ftgmac100_setup_mdio()

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

Agreed. I think the 'aspeed,ast2700-mac' should not be based
on the

'faraday,ftgmac100'.

quoted

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?

quoted

quoted

quoted

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.

quoted

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

I'll prepare a follow-up series for this:

- dt-bindings: net: ftgmac100: make aspeed,ast2400/2500/2600-mac

standalone consts

quoted

  (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