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

["Nazle Asmade, Muhammad Nazim Amirul" <muhammad.nazim.amirul.nazle.asmade@altera.com>]

On 1/5/2026 8:37 am, Jakub Kicinski 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.
> ---
> 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.
All comments have been addressed and updated in v2

https://lore.kernel.org/all/20260505080311.17405-1-muhammad.nazim.amirul.nazle.asmade@altera.com/

Nazim