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

quoted

quoted

quoted

quoted

quoted

quoted

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

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?

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?

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.

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.

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