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

[Jitendra Vegiraju]

On Mon, Apr 6, 2026 at 7:09 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> Subject: net: stmmac: Add DW25GMAC support in stmmac core driver
>
> This commit introduces support for the DW25GMAC hardware in the stmmac
> core driver, which uses a new Hyper-DMA (HDMA) architecture. It adds a
> new DMA ops instance mapping virtual DMA channels to physical DMA
> channels and sets up the basic device configurations.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dw25gmac.c b/drivers/net/ethernet/stmicro/stmmac/dw25gmac.c
> > new file mode 100644
> > index 0000000000000..82eb22bff73e0
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dw25gmac.c
>
> [ ... ]
>
> > +static int rd_dma_ch_ind(void __iomem *ioaddr, u8 mode, u32 channel)
> > +{
> > +     u32 reg_val = 0;
> > +
> > +     reg_val |= FIELD_PREP(XXVGMAC_MODE_SELECT, mode);
> > +     reg_val |= FIELD_PREP(XXVGMAC_ADDR_OFFSET, channel);
> > +     reg_val |= XXVGMAC_CMD_TYPE | XXVGMAC_OB;
> > +     writel(reg_val, ioaddr + XXVGMAC_DMA_CH_IND_CONTROL);
> > +     return readl(ioaddr + XXVGMAC_DMA_CH_IND_DATA);
> > +}
>
> Does this readl() need to wait for the hardware to clear the XXVGMAC_OB
> (Operation Busy) bit? Returning immediately might read uninitialized or
> stale data if the hardware hasn't finished the fetch.
>
This is an issue. We will fix it.
> > +
> > +static void wr_dma_ch_ind(void __iomem *ioaddr, u8 mode, u32 channel, u32 val)
> > +{
> > +     u32 reg_val = 0;
> > +
> > +     writel(val, ioaddr + XXVGMAC_DMA_CH_IND_DATA);
> > +     reg_val |= FIELD_PREP(XXVGMAC_MODE_SELECT, mode);
> > +     reg_val |= FIELD_PREP(XXVGMAC_ADDR_OFFSET, channel);
> > +     reg_val |= XGMAC_OB;
>
> This isn't a bug, but should this use the XXVGMAC_OB macro instead of
> XGMAC_OB here for consistency?
>
> > +     writel(reg_val, ioaddr + XXVGMAC_DMA_CH_IND_CONTROL);
> > +}
>
> Similarly, does this function need to poll for the busy bit to clear after
> the writel()? If back-to-back operations occur, the next indirect write
> might overwrite the control and data registers while the hardware is still
> processing this request.
>
Agreed. This is bug, we will fix it.
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > index f02b434bbd505..64c38de294da2 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
>
> [ ... ]
>
> > -int dwxgmac2_setup(struct stmmac_priv *priv)
> > +static void dwxgmac2_common_setup(struct stmmac_priv *priv, char *mac_name)
> >  {
>
> This isn't a bug, but should mac_name be declared as const char * since it
> is passed string literals like "XGMAC2" and "DW25GMAC"? This avoids
> discarded-qualifiers compiler warnings.
>
> >       struct mac_device_info *mac = priv->hw;
> >
> > -     dev_info(priv->device, "\tXGMAC2\n");
> > +     dev_info(priv->device, "\t%s\n", mac_name);
>
Ack
> [ ... ]
> --
> pw-bot: cr

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature