Date: Wed, 29 Nov 2023 18:29:59 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>
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


在 2023/11/27 19:32, Serge Semin 写道:
> 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.
I get it.
>
> 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:

Okay, thank you, I will try them one by one and report the results in time.


Thanks for your advice！


Thanks,

Yanteng

>
> 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