Re: [PATCH net-next v9 1/4] net: stmmac: Add DW25GMAC support in stmmac core driver

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Apr 07, 2026 at 03:09:32PM +0100, Russell King (Oracle) wrote:
> Not withstanding my comment about the other Synopsys xlgmac driver that
> we have in the kernel...
> 
> On Thu, Apr 02, 2026 at 02:36:26PM -0700, Jitendra Vegiraju wrote:
> > From: Jitendra Vegiraju <jitendra.vegiraju@broadcom.com>
> > 
> > The DW25GMAC introduced a new DMA architecture called Hyper-DMA (HDMA) for
> > virtualization scalability. This is realized by decoupling physical DMA
> > channels(PDMA) from potentially large number of virtual DMA channels(VDMA).
> > The VDMAs provide software abstraction to driver that map to PDMAs for
> > frame transmission and reception.
> > Since 25GMAC is a derivative of XGMAC, majority of IP is common to both.
> > 
> > To add support for the HDMA in 25GMAC, a new instance of dma_ops,
> > dw25gmac400_dma_ops is introduced.
> > To support the current needs, a simple one-to-one mapping of dw25gmac's
> > logical VDMA (channel) to TC to PDMAs is used. Most of the other dma
> > operation functions in existing dwxgamc2_dma.c file are reused whereever
> 
> Typo: dwxgmac2_dma.c
> 
> > applicable.
> > Added setup function for DW25GMAC's stmmac_hwif_entry in stmmac core.
> 
> In a previous review, I questioned the use of DWMAC_CORE_25GMAC and
> asked about its version numberspace. I believe you indicated that the
> version numberspace is the same as the existing XGMAC core.
> 
> I'm going to question the value of adding DWMAC_CORE_25GMAC.
> 
> 1. What is the value of splitting DWMAC_CORE_25GMAC from
>    DWMAC_CORE_XGMAC given that it's in the same versioning numberspace
>    as XGMAC, and most tests (via dwmac_is_xgmac()) test for XGMAC or
>    25GMAC?
> 
> 2. Have you reviewed all the places that explicitly test for
>    DWMAC_CORE_XGMAC, looking at their "false" paths (for non-XGMAC
>    cores) to determine whether they are suitable? For example:
> 
>         if (priv->plat->core_type == DWMAC_CORE_XGMAC)
>                 ndev->max_mtu = XGMAC_JUMBO_LEN;
>         else if (priv->plat->enh_desc || priv->synopsys_id >= DWMAC_CORE_4_00)
>                 ndev->max_mtu = JUMBO_LEN;
>         else
>                 ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);
> 
>    XGMAC can handle a max MTU of 16368, but with your code using
>    DWMAC_CORE_25GMAC, we fall back to the next test, which tests the
>    IP version against 0x40, and uses a max MTU of 9000. Given that
>    DWMAC_CORE_4_00 is a different "version number space" this seems
>    wrong.

This is actually wrong - for DWMAC_CORE_25GMAC with a synopsys_id
less than 0x40, this will fall back to SKB_MAX_HEAD(NET_SKB_PAD +
NET_IP_ALIGN) which is as good as "pluck a number out of the air
and watch the driver fall over if you increase the MTU to maximum".
max_mtu ends up being dependent on the system page size, not on any
hardware limitation, which is garbage.

> 3. Looking at the MDIO code, this looks very wrong if you're
>    introducing DWMAC_CORE_25GMAC. Have you tested MDIO accesses?
> 
>    dwxgmac2_setup() is called for DWMAC_CORE_XGMAC core-type. In
>    stmmac_mdio_register(), DWMAC_CORE_XGMAC uses different functions
>    for MDIO bus access for C22 and C45 from other cores - it uses the
>    stmmac_xgmac2_mdio_* functions.
> 
>    These use stmmac_xgmac2_c45_format() and stmmac_xgmac2_c22_format()
>    to format the register values which do not depend on mii.*_mask, but
>    do use mii.address and mii.data for the register offsets. Thus, is
>    there any point to setting mii.addr_mask and mii.reg_mask ?
> 
>    For non-DWMAC_CORE_XGMAC cores, we fall back to the stmmac_mdio_*()
>    functions, which for non-DWMAC_CORE_GMAC4 will only support Clause
>    22 access, not Clause 45 - which would be very strange for a 25G
>    core.
> 
> 4. What about the feature printing in
>    stmmac_main.c::stmmac_dma_cap_show() ?
> 
> 5. What about similar tests in stmmac_est.c and stmmac_ethtool.c ?

Another issue that needs to be looked into is all the tests that
check priv->synopsys_id without checking the core_type. I'm already
concerned that many of these are wrong.

I have some patches now that rename synopsys_id to snpsver (it's really
the Synopsys IP version field which is BCD of the major version and
first digit of the minor version, not some random ID that identifies
the core.)

From what I've ascertained so far:

    GMAC100 cores do not have a readable snpsver number, thus this
    will be zero.

    GMAC cores generally have a snpsver number less than 0x40.

    GMAC4 cores may have a version number that overlaps GMAC cores
    (see first entry for DWMAC_CORE_GMAC4).

    XGMAC and XLGMAC cores each have an entirely separate IP version
    number space from GMAC and GMAC4, which are distinguished by their
    respective userver.

For example:

        /* Only DWMAC core version 5.20 onwards supports HW descriptor prefetch.
         */
        if (priv->snpsver < DWMAC_CORE_5_20)
                priv->plat->dma_cfg->dche = false;

This will match your 25GMAC cores and the XGMAC cores because they have
a Synopsys IP version number less than 0x52. What saves us there is
that dche is only read by dwmac4_dma_init(), used by dwmac4_dma_ops and
dwmac410_dma_ops. These are all used by hwif entries that require
priv->plat->core_type to be DWMAC_CORE_GMAC4. Thus, I'm changing this
one to:

        /* Only DWMAC4 core version 5.20 onwards support HW descriptor prefetch.
         */
        if (priv->plat->core_type != DWMAC_CORE_GMAC4 ||
            priv->snpsver < DWMAC_CORE_5_20)
                priv->plat->dma_cfg->dche = false;

although I'm wondering if that should really be == && to avoid writing
to it for non-GMAC4 cores (it really doesn't matter.)

Then there this:

                case HWTSTAMP_FILTER_PTP_V2_EVENT:
                        /* PTP v2/802.AS1 any layer, any kind of event packet */
                        config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
                        ptp_v2 = PTP_TCR_TSVER2ENA;
                        snap_type_sel = PTP_TCR_SNAPTYPSEL_1;
                        if (priv->snpsver < DWMAC_CORE_4_10)
                                ts_event_en = PTP_TCR_TSEVNTENA;

Is PTP_TCR_TSEVNTENA needed for this PTP filter type for XGMAC and
25GMAC cores - because they'll get it because they have snpsver
smaller than 0x41.

Similar here:

        if (priv->plat->rx_coe) {
                priv->hw->rx_csum = priv->plat->rx_coe;
                dev_info(priv->device, "RX Checksum Offload Engine supported\n")+;
                if (priv->snpsver < DWMAC_CORE_4_00)
                        dev_info(priv->device, "COE Type %d\n", priv->hw->rx_csu+m);
        }

In stmmac_ethtool.c, stmmac_mac_debug() won't be called for XGMAC or
25GMAC cores, because:

                if (priv->snpsver >= DWMAC_CORE_3_50)
                        stmmac_mac_debug(priv, priv->ioaddr,
                                        (void *)&priv->xstats,
                                        rx_queues_count, tx_queues_count);

snpsver (synopsys_id) will be smaller than 0x35. Is this correct?

The good news is that stmmac_mdio.c at gates all its checks on the
Synopsys IP version against the core type.

I'll post my RFC patches tidying up some of the version mess in the
next day or so.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!