Re: [PATCH net-next] net: stmmac: fix dwmac4 transmit performance regression

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Russell,

On 16/01/2026 01:49, Russell King (Oracle) wrote:
> dwmac4's transmit performance dropped by a factor of four due to an
> incorrect assumption about which definitions are for what. This
> highlights the need for sane register macros.
> 
> Commit 8409495bf6c9 ("net: stmmac: cores: remove many xxx_SHIFT
> definitions") changed the way the txpbl value is merged into the
> register:
> 
>         value = readl(ioaddr + DMA_CHAN_TX_CONTROL(dwmac4_addrs, chan));
> -       value = value | (txpbl << DMA_BUS_MODE_PBL_SHIFT);
> +       value = value | FIELD_PREP(DMA_BUS_MODE_PBL, txpbl);
> 
> With the following in the header file:
> 
>  #define DMA_BUS_MODE_PBL               BIT(16)
> -#define DMA_BUS_MODE_PBL_SHIFT         16
> 
> The assumption here was that DMA_BUS_MODE_PBL was the mask for
> DMA_BUS_MODE_PBL_SHIFT, but this turns out not to be the case.
> 
> The field is actually six bits wide, buts 21:16, and is called
> TXPBL.
> 
> What's even more confusing is, there turns out to be a PBLX8
> single bit in the DMA_CHAN_CONTROL register (0x1100 for channel 0),
> and DMA_BUS_MODE_PBL seems to be used for that. However, this bit
> et.al. was listed under a comment "/* DMA SYS Bus Mode bitmap */"
> which is for register 0x1004.
> 
> Fix this up by adding an appropriately named field definition under
> the DMA_CHAN_TX_CONTROL() register address definition.
> 
> Move the RPBL mask definition under DMA_CHAN_RX_CONTROL(), correctly
> renaming it as well.
> 
> Also move the PBL bit definition under DMA_CHAN_CONTROL(), correctly
> renaming it.
> 
> This removes confusion over the PBL fields.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Good job finding the problem ! However you need a Fixes tag, even though
ths is is for net-next.

It would also have been nice to be in CC, I spent some time on the bisect...

Besides that, problem solved on an imx8mp setup :)

Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 8 ++++----
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h | 7 ++++---
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> index 7036beccfc85..aaa83e9ff4f0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> @@ -52,7 +52,7 @@ static void dwmac4_dma_init_rx_chan(struct stmmac_priv *priv,
>  	u32 rxpbl = dma_cfg->rxpbl ?: dma_cfg->pbl;
>  
>  	value = readl(ioaddr + DMA_CHAN_RX_CONTROL(dwmac4_addrs, chan));
> -	value = value | FIELD_PREP(DMA_BUS_MODE_RPBL_MASK, rxpbl);
> +	value = value | FIELD_PREP(DMA_CHAN_RX_CTRL_RXPBL_MASK, rxpbl);
>  	writel(value, ioaddr + DMA_CHAN_RX_CONTROL(dwmac4_addrs, chan));
>  
>  	if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) && likely(dma_cfg->eame))
> @@ -73,7 +73,7 @@ static void dwmac4_dma_init_tx_chan(struct stmmac_priv *priv,
>  	u32 txpbl = dma_cfg->txpbl ?: dma_cfg->pbl;
>  
>  	value = readl(ioaddr + DMA_CHAN_TX_CONTROL(dwmac4_addrs, chan));
> -	value = value | FIELD_PREP(DMA_BUS_MODE_PBL, txpbl);
> +	value = value | FIELD_PREP(DMA_CHAN_TX_CTRL_TXPBL_MASK, txpbl);
>  
>  	/* Enable OSP to get best performance */
>  	value |= DMA_CONTROL_OSP;
> @@ -98,7 +98,7 @@ static void dwmac4_dma_init_channel(struct stmmac_priv *priv,
>  	/* common channel control register config */
>  	value = readl(ioaddr + DMA_CHAN_CONTROL(dwmac4_addrs, chan));
>  	if (dma_cfg->pblx8)
> -		value = value | DMA_BUS_MODE_PBL;
> +		value = value | DMA_CHAN_CTRL_PBLX8;
>  	writel(value, ioaddr + DMA_CHAN_CONTROL(dwmac4_addrs, chan));
>  
>  	/* Mask interrupts by writing to CSR7 */
> @@ -116,7 +116,7 @@ static void dwmac410_dma_init_channel(struct stmmac_priv *priv,
>  	/* common channel control register config */
>  	value = readl(ioaddr + DMA_CHAN_CONTROL(dwmac4_addrs, chan));
>  	if (dma_cfg->pblx8)
> -		value = value | DMA_BUS_MODE_PBL;
> +		value = value | DMA_CHAN_CTRL_PBLX8;
>  
>  	writel(value, ioaddr + DMA_CHAN_CONTROL(dwmac4_addrs, chan));
>  
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
> index 5f1e2916f099..9d9077a4ac9f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
> @@ -24,8 +24,6 @@
>  
>  #define DMA_SYS_BUS_MODE		0x00001004
>  
> -#define DMA_BUS_MODE_PBL		BIT(16)
> -#define DMA_BUS_MODE_RPBL_MASK		GENMASK(21, 16)
>  #define DMA_BUS_MODE_MB			BIT(14)
>  #define DMA_BUS_MODE_FB			BIT(0)
>  
> @@ -68,19 +66,22 @@ static inline u32 dma_chanx_base_addr(const struct dwmac4_addrs *addrs,
>  
>  #define DMA_CHAN_CONTROL(addrs, x)	dma_chanx_base_addr(addrs, x)
>  
> +#define DMA_CHAN_CTRL_PBLX8		BIT(16)
>  #define DMA_CONTROL_SPH			BIT(24)
>  
>  #define DMA_CHAN_TX_CONTROL(addrs, x)	(dma_chanx_base_addr(addrs, x) + 0x4)
>  
>  #define DMA_CONTROL_EDSE		BIT(28)
> +#define DMA_CHAN_TX_CTRL_TXPBL_MASK	GENMASK(21, 16)
>  #define DMA_CONTROL_TSE			BIT(12)
>  #define DMA_CONTROL_OSP			BIT(4)
>  #define DMA_CONTROL_ST			BIT(0)
>  
>  #define DMA_CHAN_RX_CONTROL(addrs, x)	(dma_chanx_base_addr(addrs, x) + 0x8)
>  
> -#define DMA_CONTROL_SR			BIT(0)
> +#define DMA_CHAN_RX_CTRL_RXPBL_MASK	GENMASK(21, 16)
>  #define DMA_RBSZ_MASK			GENMASK(14, 1)
> +#define DMA_CONTROL_SR			BIT(0)
>  
>  #define DMA_CHAN_TX_BASE_ADDR_HI(addrs, x)	(dma_chanx_base_addr(addrs, x) + 0x10)
>  #define DMA_CHAN_TX_BASE_ADDR(addrs, x)	(dma_chanx_base_addr(addrs, x) + 0x14)