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

[Jakub Kicinski <kuba@kernel.org>]

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.

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

[ ... ]

> 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);

[ ... ]
-- 
pw-bot: cr