Re: [PATCH] net: stmmac: Use interrupt mode INTM=1 for per channel irq

[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.
---
net: stmmac: Use interrupt mode INTM=1 for per channel irq

This commit adds programming of the XGMAC DMA_MODE.INTM=1 bit on xgmac2
to deliver per-channel TX/RX interrupts via shared peripheral interrupts
(SPI), mirroring the approach of the earlier MSI-oriented commit
6ccf12ae111e. New fields are added to stmmac_dma_cfg and
plat_stmmacenet_data to drive the new path.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index 51943705a2b0..94cbf24b3118 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -320,6 +320,9 @@
>  /* DMA Registers */
>  #define XGMAC_DMA_MODE			0x00003000
>  #define XGMAC_SWR			BIT(0)
> +#define DMA_MODE_INTM_MASK		GENMASK(13, 12)
> +#define DMA_MODE_INTM_SHIFT		12
> +#define DMA_MODE_INTM_MODE1		0x1

Should these three macros carry the XGMAC_ prefix used by every other
symbol in dwxgmac2.h (XGMAC_DMA_MODE, XGMAC_SWR, XGMAC_DMA_SYSBUS_MODE,
and so on)?  The dwmac4 sibling header defines analogous symbols as
DMA_BUS_MODE_INTM_MASK / DMA_BUS_MODE_INTM_MODE1, but within dwxgmac2.h
the established convention appears to be XGMAC_*.

The referenced precedent commit 6ccf12ae111e uses FIELD_PREP() /
u32_replace_bits() rather than a hand-rolled SHIFT constant.  Would it
be cleaner to drop DMA_MODE_INTM_SHIFT and use FIELD_PREP() here too?

>  #define XGMAC_DMA_SYSBUS_MODE		0x00003004
>  #define XGMAC_WR_OSR_LMT		GENMASK(29, 24)
>  #define XGMAC_RD_OSR_LMT		GENMASK(21, 16)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> index 03437f1cf3df..3789b62cd2e3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> @@ -31,6 +31,13 @@ static void dwxgmac2_dma_init(void __iomem *ioaddr,
>  		value |= XGMAC_EAME;
>  
>  	writel(value, ioaddr + XGMAC_DMA_SYSBUS_MODE);
> +
> +	if (dma_cfg->multi_irq_en) {
> +		value = readl(ioaddr + XGMAC_DMA_MODE);
> +		value &= ~DMA_MODE_INTM_MASK;
> +		value |= (DMA_MODE_INTM_MODE1 << DMA_MODE_INTM_SHIFT);
> +		writel(value, ioaddr + XGMAC_DMA_MODE);
> +	}
>  }

Is this new block reachable on any platform?

A tree-wide search for multi_irq_en returns only three references: this
read site and the two struct declarations added below.  Nothing assigns
dma_cfg->multi_irq_en anywhere, so the condition appears to always be
false and INTM=1 would never be programmed.

For comparison, the analogous multi_msi_en is set up in stmmac_main.c:

    priv->plat->dma_cfg->multi_msi_en =
            (priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN);

and is consumed from glue drivers such as dwmac-intel.c, dwmac-loongson.c,
and dwmac-s32.c.  Should an equivalent assignment and a corresponding
plat flag (or the new plat->multi_irq_en field) be wired up in this
patch so the feature actually takes effect?

The commit message also states:

    The TX/RX NORMAL interrupt check is decoupled since the NIS bit is
    not asserted for TI/RI events when INTM=1.

but the diff touches only dwxgmac2_dma_init() and does not modify
dwxgmac2_dma_interrupt() or any other ISR/interrupt-status path.  The
existing dwxgmac2_dma_interrupt() already tests XGMAC_RI and
XGMAC_TI | XGMAC_TBU independently of XGMAC_NIS:

    if (likely(intr_status & XGMAC_RI)) {
            ...
    }
    if (likely(intr_status & (XGMAC_TI | XGMAC_TBU))) {
            ...
    }

Is this paragraph carried over from the earlier dwmac4 commit
6ccf12ae111e, or is a companion hunk missing from this patch?

>  static void dwxgmac2_dma_init_chan(struct stmmac_priv *priv,
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index 4430b967abde..5deaa1a797a5 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -122,6 +122,7 @@ struct stmmac_dma_cfg {
>  	bool eame;
>  	/* multi_msi_en: stmmac core internal */
>  	bool multi_msi_en;
> +	bool multi_irq_en;
>  	/* atds: stmmac core internal */
>  	bool atds;
>  };
> @@ -349,6 +350,11 @@ struct plat_stmmacenet_data {
>  	u8 vlan_fail_q;
>  	bool provide_bus_info;
>  	int int_snapshot_num;
> +	int ext_snapshot_num;
> +	bool int_snapshot_en;
> +	bool ext_snapshot_en;
> +	bool multi_msi_en;
> +	bool multi_irq_en;
>  	int msi_mac_vec;
>  	int msi_wol_vec;
>  	int msi_sfty_ce_vec;

Are these five new plat_stmmacenet_data fields used by anything?

A tree-wide search finds no reader or writer for ext_snapshot_num,
int_snapshot_en, ext_snapshot_en, plat->multi_msi_en, or
plat->multi_irq_en, and they are not mentioned in the commit message.
The commit message scope is limited to enabling INTM=1 per-channel SPI
IRQs, which would only require stmmac_dma_cfg::multi_irq_en.

The same concepts already appear to be represented via plat->flags bits
(STMMAC_FLAG_MULTI_MSI_EN, STMMAC_FLAG_INT_SNAPSHOT_EN,
STMMAC_FLAG_EXT_SNAPSHOT_EN) and the existing int_snapshot_num member.
Could the four unrelated fields be dropped from this patch and, if
still needed, be introduced in a separate series that actually wires
them up?  Having both a flags bit and a bool for the same concept
risks future drift about which representation is authoritative.
-- 
pw-bot: cr