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

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

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.

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.

Thanks,
Jacky