Date: Mon, 27 Nov 2023 14:32:31 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: Andrew Lunn <andrew@...n.ch>, hkallweit1@...il.com, 
	peppe.cavallaro@...com, alexandre.torgue@...s.st.com, joabreu@...opsys.com, 
	Jose.Abreu@...opsys.com, chenhuacai@...ngson.cn, linux@...linux.org.uk, 
	dongbiao@...ngson.cn, guyinggang@...ngson.cn, netdev@...r.kernel.org, 
	loongarch@...ts.linux.dev, chris.chenfeiyang@...il.com
Subject: Re: [PATCH v5 3/9] net: stmmac: Add Loongson DWGMAC definitions

Yanteng

On Sun, Nov 26, 2023 at 08:25:13PM +0800, Yanteng Si wrote:
> 
> 在 2023/11/25 00:44, Serge Semin 写道:
> > On Fri, Nov 24, 2023 at 03:51:08PM +0100, Andrew Lunn wrote:
> > > > In general, we split one into two.
> > > > 
> > > > the details are as follows：
> > > > 
> > > > DMA_INTR_ENA_NIE = DMA_INTR_ENA_NIE_LOONGSON= DMA_INTR_ENA_TX_NIE +
> > > > DMA_INTR_ENA_RX_NIE
> > > What does the documentation from Synopsys say about the bit you have
> > > used for DMA_INTR_ENA_NIE_LOONGSON? Is it marked as being usable by IP
> > > integrators for whatever they want, or is it marked as reserved?
> > > 
> > > I'm just wondering if we are heading towards a problem when the next
> > > version of this IP assigns the bit to mean something else.
> > That's what I started to figure out in my initial message:
> > Link: https://lore.kernel.org/netdev/gxods3yclaqkrud6jxhvcjm67vfp5zmuoxjlr6llcddny7zwsr@473g74uk36xn/
> > but Yanteng for some reason ignored all my comments.
> 
> Sorry, I always keep your review comments in mind, and even this version of
> the patch is
> 
> largely based on your previous comments. Please give me some more time and I
> promise
> 
> to answer your comments before the next patch is made.
> 
> > 
> > Anyway AFAICS this Loongson GMAC has NIE and AIE flags defined differently:
> > DW GMAC: NIE - BIT(16) - all non-fatal Tx and Rx errors,
> >           AIE - BIT(15) - all fatal Tx, Rx and Bus errors.
> > Loongson GMAC: NIE - BIT(18) | BIT(17) - one flag for Tx and another for Rx errors.
> >                 AIE - BIT(16) | BIT(15) - Tx, Rx and don't know about the Bus errors.
> > So the Loongson GMAC has not only NIE/AIE flags re-defined, but
> > also get to occupy two reserved in the generic DW GMAC bits: BIT(18) and BIT(17).
> > 
> > Moreover Yanteng in his patch defines DMA_INTR_NORMAL as a combination
> > of both _generic_ DW and Loongson-specific NIE flags and
> > DMA_INTR_ABNORMAL as a combination of both _generic_ DW and
> > Loongson-specific AIE flags. At the very least it doesn't look
> > correct, since _generic_ DW GMAC NIE flag BIT(16) is defined as a part
> > of the Loongson GMAC AIE flags set.
> 
> We will consider this seriously, please give us some more time, and of
> course, we
> 
> are looking forward to your opinions on this problem.
> 
> 
> I hope you can accept my apologies, Please allow me to say sorry again.

Thanks for your response. No worries. I keep following this thread and
sending my comments because the changes here deeply concerns our
hardware. Besides the driver already looks unreasonably
overcomplicated and weakly structured in a lot of aspects. I bet
nobody here wants it to be having even more clumsy parts. That's why
all the "irritating" comments and nitpicks.

Anyway regarding the Loongson Multi-channels GMAC IRQs based on all
your info in the previous replies I guess what could be an acceptable
solution (with the subsystem/driver maintainers blessing) is something
like this:

1. Add new platform feature flag:
include/linux/stmmac.h:
+#define STMMAC_FLAG_HAS_LSMC			BIT(13)

2. Update the stmmac_dma_ops.init() callback prototype to accepting
a pointer to the stmmac_priv instance:
drivers/net/ethernet/stmicro/stmmac/hwif.h:
	void (*init)(struct stmmac_priv *priv, void __iomem *ioaddr,
		     struct stmmac_dma_cfg *dma_cfg, int atds);
+drivers/net/ethernet/stmicro/stmmac/dwmac100_dma.c ...
+drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c ...
+drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c ...
+drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c ...
!!!+drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c ...

3. Add the IRQs macros specific to the LoongSon Multi-channels GMAC:
drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h:
+#define DMA_INTR_ENA_LS_NIE 0x00060000	/* Normal Loongson Tx/Rx Summary */
#define DMA_INTR_ENA_NIE 0x00010000	/* Normal Summary */
...

+#define DMA_INTR_LS_NORMAL	(DMA_INTR_ENA_LS_NIE | DMA_INTR_ENA_RIE | \
				DMA_INTR_ENA_TIE)
#define DMA_INTR_NORMAL		(DMA_INTR_ENA_NIE | DMA_INTR_ENA_RIE | \
				DMA_INTR_ENA_TIE)
...
+#define DMA_INTR_ENA_LS_AIE 0x00018000	/* Abnormal Loongson Tx/Rx Summary */
#define DMA_INTR_ENA_AIE 0x00008000	/* Abnormal Summary */
...
define DMA_INTR_LS_ABNORMAL	(DMA_INTR_ENA_LS_AIE | DMA_INTR_ENA_FBE | \
				DMA_INTR_ENA_UNE)
#define DMA_INTR_ABNORMAL	(DMA_INTR_ENA_AIE | DMA_INTR_ENA_FBE | \
				DMA_INTR_ENA_UNE)
...
#define DMA_INTR_LS_DEFAULT_MASK	(DMA_INTR_LS_NORMAL | DMA_INTR_LS_ABNORMAL)
#define DMA_INTR_DEFAULT_MASK		(DMA_INTR_NORMAL | DMA_INTR_ABNORMAL)

etc for the DMA_STATUS_TX_*, DMA_STATUS_RX_* macros.

4. Update the DW GMAC DMA init() method to be activating all the
Normal and Abnormal Loongson-specific IRQs:
drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c:
	if (priv->plat->flags & STMMAC_FLAG_HAS_LSMC)
		writel(DMA_INTR_LS_DEFAULT_MASK, ioaddr + DMA_INTR_ENA);
	else
		writel(DMA_INTR_DEFAULT_MASK, ioaddr + DMA_INTR_ENA);

5. Make sure your low-level driver sets the STMMAC_FLAG_HAS_LSMC flag
in the plat_stmmacenet_data structure instance.

Note 1. For the sake of consistency a similar update can be provided
for the dwmac_enable_dma_irq()/dwmac_disable_dma_irq() methods and
the DMA_INTR_DEFAULT_RX/DMA_INTR_DEFAULT_TX macros seeing your device
is able to disable/enable the xfer-specific summary IRQs. But it
doesn't look like required seeing the driver won't raise the summary
IRQs if no respective xfer IRQ is enabled. Most importantly this
update would add additional code to the Tx/Rx paths, which in a tiny
bit measure may affect the other platforms perf. So it's better to
avoid it if possible.

Note 2. The STMMAC_FLAG_HAS_LSMC flag might be utilized to tweak up
the other generic parts of the STMMAC driver.

Noet 3. An alternative to the step 3 above could be to define two
additional plat_stmmacenet_data fields like: dma_def_intr_mask,
dma_nor_stat, dma_abnor_stat, which would be initialized in the
stmmac_probe() method with the currently defined
DMA_INTR_DEFAULT_MASK, DMA_NOR_INTR_STATUS and DMA_ABNOR_INTR_STATUS
macros if they haven't been initialized by the low-level drivers.
These fields could be then used in the respective DMA IRQ init and
handler methods. I don't know which solution is better. At the first
glance this one might be even better than what is described in step 3.

Note 4. The solution above will also cover the Andrew's note of having
the kernel which runs on all possible variants.

-Serge(y)

> 
> 
> Thanks for your review!
> 
> 
> Thanks,
> 
> Yanteng
> 
> > 
> > -Serge(y)
> > 
> > > 	Andrew
> 

Powered by blists - more mailing lists