Messages in this thread |  | | From | "Nazle Asmade, Muhammad Nazim Amirul" <> | | Subject | Re: [PATCH] net: stmmac: Use interrupt mode INTM=1 for per channel irq | | Date | Tue, 5 May 2026 08:05:37 +0000 |
| |
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
|  |