[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <df17d5e9-2c61-47e3-ba04-64b7110a7ba6@loongson.cn>
Date: Tue, 21 Nov 2023 17:55:24 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Andrew Lunn <andrew@...n.ch>
Cc: hkallweit1@...il.com, peppe.cavallaro@...com,
alexandre.torgue@...s.st.com, joabreu@...opsys.com, fancer.lancer@...il.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
Hi Andrew,
在 2023/11/12 04:07, Andrew Lunn 写道:
>> +#ifdef CONFIG_DWMAC_LOONGSON
>> +#define DMA_INTR_ABNORMAL (DMA_INTR_ENA_AIE_LOONGSON | DMA_INTR_ENA_AIE | \
>> + DMA_INTR_ENA_FBE | DMA_INTR_ENA_UNE)
>> +#else
>> #define DMA_INTR_ABNORMAL (DMA_INTR_ENA_AIE | DMA_INTR_ENA_FBE | \
>> DMA_INTR_ENA_UNE)
>> +#endif
> The aim is to produce one kernel which runs on all possible
> variants. So we don't like to see this sort of #ifdef. Please try to
> remove them.
We now run into a tricky problem: we only have a few register
definitions(DMA_XXX_LOONGSON)
that are not the same as the dwmac1000 register definition.
In this case, if we use these "#ifdef", it will reuse most of the
dwmac1000 code to
reduce maintenance stress, at the cost of sacrificing a little
readability; If we created
a new xxx_dma.h, it would add a new set of code similar to dwmac1000,
which is exactly
what the v4 patch did, which was great for readability, but it also made
the code more
maintenance stress, and we got reviews complaining in v4. That is, we
need to find a
balance between readability and maintainability.
however, we haven't yet come up with a way to do both, so it would be
great if you
could give us some advice on this.
v4:<https://lore.kernel.org/loongarch/cover.1692696115.git.chenfeiyang@loongson.cn/>
>
>> @@ -167,7 +167,7 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
>> struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[chan];
>> int ret = 0;
>> /* read the status register (CSR5) */
>> - u32 intr_status = readl(ioaddr + DMA_STATUS);
>> + u32 intr_status = readl(ioaddr + DMA_CHAN_STATUS(chan));
>>
> Please break this patch up. Changes like the above are simple to
> understand. So have one patch which just adds these macros and makes
> use of them.
OK!
>
>> #ifdef DWMAC_DMA_DEBUG
>> /* Enable it to monitor DMA rx/tx status in case of critical problems */
>> @@ -182,7 +182,7 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
>> intr_status &= DMA_STATUS_MSK_TX;
>>
>> /* ABNORMAL interrupts */
>> - if (unlikely(intr_status & DMA_STATUS_AIS)) {
>> + if (unlikely(intr_status & DMA_ABNOR_INTR_STATUS)) {
> However, this is not obviously correct. You need to explain in the
> commit message why this change is needed.
>
> Lots of small patches make the understanding easier.
Because Loongson has two AIS, so:
#ifdef CONFIG_DWMAC_LOONGSON
...
#define DMA_ABNOR_INTR_STATUS (DMA_STATUS_TX_AIS_LOONGSON |
DMA_STATUS_RX_AIS_LOONGSON)
...
#else
...
#define DMA_ABNOR_INTR_STATUS DMA_STATUS_AIS
...
#endif
>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 7371713c116d..aafc75fa14a0 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -7062,6 +7062,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>> /* dwmac-sun8i only work in chain mode */
>> if (priv->plat->flags & STMMAC_FLAG_HAS_SUN8I)
>> chain_mode = 1;
>> +
>> priv->chain_mode = chain_mode;
> Please avoid white space changes.
OK!
Thanks for your review!
Thanks,
Yanteng
>
> Andrew
Powered by blists - more mailing lists