[v5,3/9] net: stmmac: Add Loongson DWGMAC definitions

Message ID	87011adcd39f20250edc09ee5d31bda01ded98b5.1699533745.git.siyanteng@loongson.cn (mailing list archive)
State	Changes Requested
Delegated to:	Netdev Maintainers
Headers	show Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C71A111CA1 for <netdev@vger.kernel.org>; Fri, 10 Nov 2023 09:25:52 +0000 (UTC) From: Yanteng Si <siyanteng@loongson.cn> To: andrew@lunn.ch, hkallweit1@gmail.com, peppe.cavallaro@st.com, alexandre.torgue@foss.st.com, joabreu@synopsys.com Cc: Yanteng Si <siyanteng@loongson.cn>, fancer.lancer@gmail.com, Jose.Abreu@synopsys.com, chenhuacai@loongson.cn, linux@armlinux.org.uk, dongbiao@loongson.cn, guyinggang@loongson.cn, netdev@vger.kernel.org, loongarch@lists.linux.dev, chris.chenfeiyang@gmail.com Subject: [PATCH v5 3/9] net: stmmac: Add Loongson DWGMAC definitions Date: Fri, 10 Nov 2023 17:25:42 +0800 Message-Id: <87011adcd39f20250edc09ee5d31bda01ded98b5.1699533745.git.siyanteng@loongson.cn> In-Reply-To: <cover.1699533745.git.siyanteng@loongson.cn> References: <cover.1699533745.git.siyanteng@loongson.cn> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	stmmac: Add Loongson platform support \| expand [v5,0/9] stmmac: Add Loongson platform support [v5,1/9] net: stmmac: Pass stmmac_priv and chan in some callbacks [v5,2/9] net: stmmac: Allow platforms to set irq_flags [v5,3/9] net: stmmac: Add Loongson DWGMAC definitions [v5,4/9] net: stmmac: dwmac-loongson: Refactor code for loongson_dwmac_probe() [v5,5/9] net: stmmac: dwmac-loongson: Add full PCI support [v5,6/9] net: stmmac: dwmac-loongson: Add MSI support [v5,7/9] net: stmmac: dwmac-loongson: Add GNET support [v5,8/9] net: stmmac: dwmac-loongson: Disable flow control for GMAC [v5,9/9] net: stmmac: Disable coe for some Loongson GNET

Context	Check	Description
netdev/series_format	warning	Target tree name not specified in the subject
netdev/tree_selection	success	Guessed tree name to be net-next
netdev/fixes_present	success	Fixes tag not required for -next series
netdev/header_inline	success	No static functions without inline keyword in header files
netdev/build_32bit	success	Errors and warnings before: 1161 this patch: 1161
netdev/cc_maintainers	warning	6 maintainers not CCed: linux-arm-kernel@lists.infradead.org edumazet@google.com pabeni@redhat.com kuba@kernel.org linux-stm32@st-md-mailman.stormreply.com mcoquelin.stm32@gmail.com
netdev/build_clang	success	Errors and warnings before: 1161 this patch: 1161
netdev/verify_signedoff	success	Signed-off-by tag matches author and committer
netdev/deprecated_api	success	None detected
netdev/check_selftest	success	No net selftest shell script
netdev/verify_fixes	success	No Fixes tag
netdev/build_allmodconfig_warn	success	Errors and warnings before: 1189 this patch: 1189
netdev/checkpatch	warning	CHECK: Alignment should match open parenthesis WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns WARNING: line length of 99 exceeds 80 columns
netdev/build_clang_rust	success	No Rust files in patch. Skipping build
netdev/kdoc	success	Errors and warnings before: 0 this patch: 0
netdev/source_inline	success	Was 0 now: 0

Yanteng Si Nov. 10, 2023, 9:25 a.m. UTC

Loongson platforms use a DWGMAC which supports multi-channel.

There are two types of Loongson DWGMAC. The first type shares the same
register definitions and has similar logic as dwmac1000. The second type
uses several different register definitions.

Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
---
 drivers/net/ethernet/stmicro/stmmac/common.h  |  1 +
 .../ethernet/stmicro/stmmac/dwmac1000_dma.c   | 63 ++++++++++++++++---
 .../net/ethernet/stmicro/stmmac/dwmac_dma.h   | 57 ++++++++++++++++-
 .../net/ethernet/stmicro/stmmac/dwmac_lib.c   | 39 ++++++------
 drivers/net/ethernet/stmicro/stmmac/hwif.c    |  3 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  2 +
 6 files changed, 133 insertions(+), 32 deletions(-)

Andrew Lunn Nov. 11, 2023, 8:07 p.m. UTC | #1

> +#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.

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

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

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

       Andrew

Serge Semin Nov. 13, 2023, 3:05 p.m. UTC | #2

On Fri, Nov 10, 2023 at 05:25:42PM +0800, Yanteng Si wrote:
> Loongson platforms use a DWGMAC which supports multi-channel.
> 
> There are two types of Loongson DWGMAC. The first type shares the same
> register definitions and has similar logic as dwmac1000. The second type
> uses several different register definitions.
> 
> Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> ---
>  drivers/net/ethernet/stmicro/stmmac/common.h  |  1 +
>  .../ethernet/stmicro/stmmac/dwmac1000_dma.c   | 63 ++++++++++++++++---
>  .../net/ethernet/stmicro/stmmac/dwmac_dma.h   | 57 ++++++++++++++++-
>  .../net/ethernet/stmicro/stmmac/dwmac_lib.c   | 39 ++++++------
>  drivers/net/ethernet/stmicro/stmmac/hwif.c    |  3 +-
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  2 +
>  6 files changed, 133 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index e3f650e88f82..e01584fe9efa 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -34,6 +34,7 @@
>  #define DWMAC_CORE_5_00		0x50
>  #define DWMAC_CORE_5_10		0x51
>  #define DWMAC_CORE_5_20		0x52

> +#define DWGMAC_CORE_1_00	0x10

Name is ambiguous. All of the DWMAC_CORE_* macro above also refer to
the DW GMACs. Moreover generic DW MAC IP-core v1 I guess never existed
in a way the driver expects it. So IMO it would be better for you to
drop this and just add a new flag to the plat_stmmacenet_data
structure (extended_desc or has_lson/STMMAC_FLAG_HAS_LSON).

* One more time took a look at the plat_stmmacenet_data structure and
* scared of the mess it has evolved to.

>  #define DWXGMAC_CORE_2_10	0x21
>  #define DWXGMAC_CORE_2_20	0x22
>  #define DWXLGMAC_CORE_2_00	0x20
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> index ce0e6ca6f3a2..234d30c5a836 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> @@ -12,7 +12,8 @@
>    Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>  *******************************************************************************/
>  
> -#include <asm/io.h>
> +#include <linux/io.h>
> +#include "stmmac.h"
>  #include "dwmac1000.h"
>  #include "dwmac_dma.h"
>  
> @@ -111,13 +112,58 @@ static void dwmac1000_dma_init(struct stmmac_priv *priv, void __iomem *ioaddr,
>  	writel(DMA_INTR_DEFAULT_MASK, ioaddr + DMA_INTR_ENA);
>  }
>  

> +static void dwmac1000_dma_init_channel(struct stmmac_priv *priv,
> +				     void __iomem *ioaddr,
> +				     struct stmmac_dma_cfg *dma_cfg, u32 chan)
> +{
> +	u32 value;
> +	int txpbl = dma_cfg->txpbl ?: dma_cfg->pbl;
> +	int rxpbl = dma_cfg->rxpbl ?: dma_cfg->pbl;
> +
> +	if (!(priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN))
> +		return;

Use dma_cfg->multi_msi_en in a way it's done in the DW GMAC v4*
module. Also note this flag isn't something that should block all the
DMA-channel-specific setups. It's intended to activate the per-channel
IRQs instead of raising the generic MAC IRQ for all the events.

> +
> +	/* common channel control register config */
> +	value = readl(ioaddr + DMA_CHAN_BUS_MODE(chan));
> +
> +	/* Set the DMA PBL (Programmable Burst Length) mode.
> +	 *
> +	 * Note: before stmmac core 3.50 this mode bit was 4xPBL, and
> +	 * post 3.5 mode bit acts as 8*PBL.
> +	 */
> +	if (dma_cfg->pblx8)
> +		value |= DMA_BUS_MODE_MAXPBL;
> +	value |= DMA_BUS_MODE_USP;
> +	value &= ~(DMA_BUS_MODE_PBL_MASK | DMA_BUS_MODE_RPBL_MASK);
> +	value |= (txpbl << DMA_BUS_MODE_PBL_SHIFT);
> +	value |= (rxpbl << DMA_BUS_MODE_RPBL_SHIFT);
> +
> +	/* Set the Fixed burst mode */
> +	if (dma_cfg->fixed_burst)
> +		value |= DMA_BUS_MODE_FB;
> +
> +	/* Mixed Burst has no effect when fb is set */
> +	if (dma_cfg->mixed_burst)
> +		value |= DMA_BUS_MODE_MB;
> +
> +	value |= DMA_BUS_MODE_ATDS;
> +
> +	if (dma_cfg->aal)
> +		value |= DMA_BUS_MODE_AAL;
> +
> +	writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan));
> +
> +	/* Mask interrupts by writing to CSR7 */
> +	writel(DMA_INTR_DEFAULT_MASK, ioaddr + DMA_CHAN_INTR_ENA(chan));
> +}

This is just a multi-channel copy of the dwmac1000_dma_init(). Please
factor out all the channel-specific setups to the
dwmac1000_dma_init_channel() method and all the generic setups (if
any) to the  dwmac1000_dma_init() function.

> +
>  static void dwmac1000_dma_init_rx(struct stmmac_priv *priv,
>  				  void __iomem *ioaddr,
>  				  struct stmmac_dma_cfg *dma_cfg,
>  				  dma_addr_t dma_rx_phy, u32 chan)
>  {
>  	/* RX descriptor base address list must be written into DMA CSR3 */
> -	writel(lower_32_bits(dma_rx_phy), ioaddr + DMA_RCV_BASE_ADDR);
> +	writel(lower_32_bits(dma_rx_phy), ioaddr + DMA_CHAN_RCV_BASE_ADDR(chan));
>  }
>  
>  static void dwmac1000_dma_init_tx(struct stmmac_priv *priv,
> @@ -126,7 +172,7 @@ static void dwmac1000_dma_init_tx(struct stmmac_priv *priv,
>  				  dma_addr_t dma_tx_phy, u32 chan)
>  {
>  	/* TX descriptor base address list must be written into DMA CSR4 */
> -	writel(lower_32_bits(dma_tx_phy), ioaddr + DMA_TX_BASE_ADDR);
> +	writel(lower_32_bits(dma_tx_phy), ioaddr + DMA_CHAN_TX_BASE_ADDR(chan));
>  }
>  
>  static u32 dwmac1000_configure_fc(u32 csr6, int rxfifosz)
> @@ -154,7 +200,7 @@ static void dwmac1000_dma_operation_mode_rx(struct stmmac_priv *priv,
>  					    void __iomem *ioaddr, int mode,
>  					    u32 channel, int fifosz, u8 qmode)
>  {
> -	u32 csr6 = readl(ioaddr + DMA_CONTROL);
> +	u32 csr6 = readl(ioaddr + DMA_CHAN_CONTROL(channel));
>  
>  	if (mode == SF_DMA_MODE) {
>  		pr_debug("GMAC: enable RX store and forward mode\n");
> @@ -176,14 +222,14 @@ static void dwmac1000_dma_operation_mode_rx(struct stmmac_priv *priv,
>  	/* Configure flow control based on rx fifo size */
>  	csr6 = dwmac1000_configure_fc(csr6, fifosz);
>  
> -	writel(csr6, ioaddr + DMA_CONTROL);
> +	writel(csr6, ioaddr + DMA_CHAN_CONTROL(channel));
>  }
>  
>  static void dwmac1000_dma_operation_mode_tx(struct stmmac_priv *priv,
>  					    void __iomem *ioaddr, int mode,
>  					    u32 channel, int fifosz, u8 qmode)
>  {
> -	u32 csr6 = readl(ioaddr + DMA_CONTROL);
> +	u32 csr6 = readl(ioaddr + DMA_CHAN_CONTROL(channel));
>  
>  	if (mode == SF_DMA_MODE) {
>  		pr_debug("GMAC: enable TX store and forward mode\n");
> @@ -210,7 +256,7 @@ static void dwmac1000_dma_operation_mode_tx(struct stmmac_priv *priv,
>  			csr6 |= DMA_CONTROL_TTC_256;
>  	}
>  
> -	writel(csr6, ioaddr + DMA_CONTROL);
> +	writel(csr6, ioaddr + DMA_CHAN_CONTROL(channel));
>  }
>  
>  static void dwmac1000_dump_dma_regs(struct stmmac_priv *priv,
> @@ -273,12 +319,13 @@ static int dwmac1000_get_hw_feature(struct stmmac_priv *priv,
>  static void dwmac1000_rx_watchdog(struct stmmac_priv *priv,
>  				  void __iomem *ioaddr, u32 riwt, u32 queue)
>  {
> -	writel(riwt, ioaddr + DMA_RX_WATCHDOG);
> +	writel(riwt, ioaddr + DMA_CHAN_RX_WATCHDOG(queue));
>  }
>  
>  const struct stmmac_dma_ops dwmac1000_dma_ops = {
>  	.reset = dwmac_dma_reset,
>  	.init = dwmac1000_dma_init,
> +	.init_chan = dwmac1000_dma_init_channel,
>  	.init_rx_chan = dwmac1000_dma_init_rx,
>  	.init_tx_chan = dwmac1000_dma_init_tx,
>  	.axi = dwmac1000_dma_axi,
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> index 77141391bd2f..90464e1c9649 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
> @@ -76,8 +76,15 @@
>  #define DMA_INTR_ENA_RIE 0x00000040	/* Receive Interrupt */
>  #define DMA_INTR_ENA_ERE 0x00004000	/* Early Receive */
>  

> +#define DMA_INTR_ENA_NIE_LOONGSON 0x00060000	/* Loongson Normal Summary */
> +
> +#ifdef	CONFIG_DWMAC_LOONGSON
> +#define DMA_INTR_NORMAL	(DMA_INTR_ENA_NIE_LOONGSON | DMA_INTR_ENA_NIE | \

Emm, I don't understand this:
DMA_INTR_ENA_NIE          = 0x00010000
DMA_INTR_ENA_NIE_LOONGSON = 0x00060000
What are these additional NIE flags and why do you call them also NIE?

> +			DMA_INTR_ENA_RIE | DMA_INTR_ENA_TIE)
> +#else
>  #define DMA_INTR_NORMAL	(DMA_INTR_ENA_NIE | DMA_INTR_ENA_RIE | \
>  			DMA_INTR_ENA_TIE)
> +#endif
>  
>  /* DMA Abnormal interrupt */
>  #define DMA_INTR_ENA_AIE 0x00008000	/* Abnormal Summary */
> @@ -91,8 +98,15 @@
>  #define DMA_INTR_ENA_TJE 0x00000008	/* Transmit Jabber */
>  #define DMA_INTR_ENA_TSE 0x00000002	/* Transmit Stopped */
>  

> +#define DMA_INTR_ENA_AIE_LOONGSON 0x00018000	/* Loongson Abnormal Summary */
> +
> +#ifdef	CONFIG_DWMAC_LOONGSON
> +#define DMA_INTR_ABNORMAL	(DMA_INTR_ENA_AIE_LOONGSON | DMA_INTR_ENA_AIE | \

Similarly I don't understand this.
DMA_INTR_ENA_AIE          = 0x00008000
DMA_INTR_ENA_AIE_LOONGSON = 0x00018000
so it's Loongson-specific needs normal DMA_INTR_ENA_AIE and some
additional flag enabled. What is the meaning of that flag?

> +				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
>  
>  /* DMA default interrupt mask */
>  #define DMA_INTR_DEFAULT_MASK	(DMA_INTR_NORMAL | DMA_INTR_ABNORMAL)
> @@ -128,9 +142,29 @@
>  #define DMA_STATUS_TI	0x00000001	/* Transmit Interrupt */
>  #define DMA_CONTROL_FTF		0x00100000	/* Flush transmit FIFO */
>  
> -#define DMA_STATUS_MSK_COMMON		(DMA_STATUS_NIS | \
> -					 DMA_STATUS_AIS | \
> -					 DMA_STATUS_FBI)

> +#define DMA_STATUS_TX_NIS_LOONGSON		0x00040000	/* Normal Tx Interrupt Summary */
> +#define DMA_STATUS_RX_NIS_LOONGSON		0x00020000	/* Normal Rx Interrupt Summary */
> +#define DMA_STATUS_TX_AIS_LOONGSON		0x00010000	/* Abnormal Tx Interrupt Summary */
> +#define DMA_STATUS_RX_AIS_LOONGSON		0x00008000	/* Abnormal Rx Interrupt Summary */
> +#define DMA_STATUS_TX_FBI_LOONGSON		0x00002000	/* Fatal Tx Bus Error Interrupt */
> +#define DMA_STATUS_RX_FBI_LOONGSON		0x00001000	/* Fatal Rx Bus Error Interrupt */
> +
> +#ifdef	CONFIG_DWMAC_LOONGSON
> +#define DMA_NOR_INTR_STATUS	    (DMA_STATUS_TX_NIS_LOONGSON | DMA_STATUS_RX_NIS_LOONGSON)
> +#define DMA_ABNOR_INTR_STATUS	    (DMA_STATUS_TX_AIS_LOONGSON | DMA_STATUS_RX_AIS_LOONGSON)
> +#define DMA_FB_INTR_STATUS	    (DMA_STATUS_TX_FBI_LOONGSON | DMA_STATUS_RX_FBI_LOONGSON)
> +#else
> +#define DMA_NOR_INTR_STATUS	    DMA_STATUS_NIS
> +#define DMA_ABNOR_INTR_STATUS	    DMA_STATUS_AIS
> +#define DMA_FB_INTR_STATUS	    DMA_STATUS_FBI
> +#endif

In addition to what said Andrew, this looks incorrect since the
Loongson flags intersect the generic status flags. So it seems like
at least the statistics handling or the
show_rx_process_state()/show_tx_process_state() won't work correctly.

-Serge(y)

> +
> +#define DMA_INTR_STATUS		    (DMA_STATUS_GPI | \
> +					 DMA_STATUS_GMI | \
> +					 DMA_STATUS_GLI)
> +#define DMA_STATUS_MSK_COMMON		(DMA_NOR_INTR_STATUS | \
> +					 DMA_ABNOR_INTR_STATUS | \
> +					 DMA_FB_INTR_STATUS)
>  
>  #define DMA_STATUS_MSK_RX		(DMA_STATUS_ERI | \
>  					 DMA_STATUS_RWT | \
> @@ -148,6 +182,9 @@
>  					 DMA_STATUS_TI | \
>  					 DMA_STATUS_MSK_COMMON)
>  
> +/* Following DMA defines are chanels oriented */
> +#define DMA_CHAN_OFFSET			0x100
> +
>  #define NUM_DWMAC100_DMA_REGS	9
>  #define NUM_DWMAC1000_DMA_REGS	23
>  #define NUM_DWMAC4_DMA_REGS	27
> @@ -170,4 +207,18 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
>  			struct stmmac_extra_stats *x, u32 chan, u32 dir);
>  int dwmac_dma_reset(struct stmmac_priv *priv, void __iomem *ioaddr);
>  
> +static inline u32 dma_chan_base_addr(u32 base, u32 chan)
> +{
> +	return base + chan * DMA_CHAN_OFFSET;
> +}
> +
> +#define DMA_CHAN_XMT_POLL_DEMAND(chan)	dma_chan_base_addr(DMA_XMT_POLL_DEMAND, chan)
> +#define DMA_CHAN_INTR_ENA(chan)		dma_chan_base_addr(DMA_INTR_ENA, chan)
> +#define DMA_CHAN_CONTROL(chan)		dma_chan_base_addr(DMA_CONTROL, chan)
> +#define DMA_CHAN_STATUS(chan)		dma_chan_base_addr(DMA_STATUS, chan)
> +#define DMA_CHAN_BUS_MODE(chan)		dma_chan_base_addr(DMA_BUS_MODE, chan)
> +#define DMA_CHAN_RCV_BASE_ADDR(chan)	dma_chan_base_addr(DMA_RCV_BASE_ADDR, chan)
> +#define DMA_CHAN_TX_BASE_ADDR(chan)	dma_chan_base_addr(DMA_TX_BASE_ADDR, chan)
> +#define DMA_CHAN_RX_WATCHDOG(chan)	dma_chan_base_addr(DMA_RX_WATCHDOG, chan)
> +
>  #endif /* __DWMAC_DMA_H__ */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c b/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
> index 0cb337ffb7ac..c36aec97bbb5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
> @@ -31,63 +31,63 @@ int dwmac_dma_reset(struct stmmac_priv *priv, void __iomem *ioaddr)
>  void dwmac_enable_dma_transmission(struct stmmac_priv *priv,
>  				   void __iomem *ioaddr, u32 chan)
>  {
> -	writel(1, ioaddr + DMA_XMT_POLL_DEMAND);
> +	writel(1, ioaddr + DMA_CHAN_XMT_POLL_DEMAND(chan));
>  }
>  
>  void dwmac_enable_dma_irq(struct stmmac_priv *priv, void __iomem *ioaddr,
>  			  u32 chan, bool rx, bool tx)
>  {
> -	u32 value = readl(ioaddr + DMA_INTR_ENA);
> +	u32 value = readl(ioaddr + DMA_CHAN_INTR_ENA(chan));
>  
>  	if (rx)
>  		value |= DMA_INTR_DEFAULT_RX;
>  	if (tx)
>  		value |= DMA_INTR_DEFAULT_TX;
>  
> -	writel(value, ioaddr + DMA_INTR_ENA);
> +	writel(value, ioaddr + DMA_CHAN_INTR_ENA(chan));
>  }
>  
>  void dwmac_disable_dma_irq(struct stmmac_priv *priv, void __iomem *ioaddr,
>  			   u32 chan, bool rx, bool tx)
>  {
> -	u32 value = readl(ioaddr + DMA_INTR_ENA);
> +	u32 value = readl(ioaddr + DMA_CHAN_INTR_ENA(chan));
>  
>  	if (rx)
>  		value &= ~DMA_INTR_DEFAULT_RX;
>  	if (tx)
>  		value &= ~DMA_INTR_DEFAULT_TX;
>  
> -	writel(value, ioaddr + DMA_INTR_ENA);
> +	writel(value, ioaddr + DMA_CHAN_INTR_ENA(chan));
>  }
>  
>  void dwmac_dma_start_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
>  			u32 chan)
>  {
> -	u32 value = readl(ioaddr + DMA_CONTROL);
> +	u32 value = readl(ioaddr + DMA_CHAN_CONTROL(chan));
>  	value |= DMA_CONTROL_ST;
> -	writel(value, ioaddr + DMA_CONTROL);
> +	writel(value, ioaddr + DMA_CHAN_CONTROL(chan));
>  }
>  
>  void dwmac_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr, u32 chan)
>  {
> -	u32 value = readl(ioaddr + DMA_CONTROL);
> +	u32 value = readl(ioaddr + DMA_CHAN_CONTROL(chan));
>  	value &= ~DMA_CONTROL_ST;
> -	writel(value, ioaddr + DMA_CONTROL);
> +	writel(value, ioaddr + DMA_CHAN_CONTROL(chan));
>  }
>  
>  void dwmac_dma_start_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
>  			u32 chan)
>  {
> -	u32 value = readl(ioaddr + DMA_CONTROL);
> +	u32 value = readl(ioaddr + DMA_CHAN_CONTROL(chan));
>  	value |= DMA_CONTROL_SR;
> -	writel(value, ioaddr + DMA_CONTROL);
> +	writel(value, ioaddr + DMA_CHAN_CONTROL(chan));
>  }
>  
>  void dwmac_dma_stop_rx(struct stmmac_priv *priv, void __iomem *ioaddr, u32 chan)
>  {
> -	u32 value = readl(ioaddr + DMA_CONTROL);
> +	u32 value = readl(ioaddr + DMA_CHAN_CONTROL(chan));
>  	value &= ~DMA_CONTROL_SR;
> -	writel(value, ioaddr + DMA_CONTROL);
> +	writel(value, ioaddr + DMA_CHAN_CONTROL(chan));
>  }
>  
>  #ifdef DWMAC_DMA_DEBUG
> @@ -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));
>  
>  #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)) {
>  		if (unlikely(intr_status & DMA_STATUS_UNF)) {
>  			ret = tx_hard_error_bump_tc;
>  			x->tx_undeflow_irq++;
> @@ -205,13 +205,13 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
>  			x->tx_process_stopped_irq++;
>  			ret = tx_hard_error;
>  		}
> -		if (unlikely(intr_status & DMA_STATUS_FBI)) {
> +		if (unlikely(intr_status & DMA_FB_INTR_STATUS)) {
>  			x->fatal_bus_error_irq++;
>  			ret = tx_hard_error;
>  		}
>  	}
>  	/* TX/RX NORMAL interrupts */
> -	if (likely(intr_status & DMA_STATUS_NIS)) {
> +	if (likely(intr_status & DMA_NOR_INTR_STATUS)) {
>  		if (likely(intr_status & DMA_STATUS_RI)) {
>  			u32 value = readl(ioaddr + DMA_INTR_ENA);
>  			/* to schedule NAPI on real RIE event. */
> @@ -232,12 +232,11 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
>  			x->rx_early_irq++;
>  	}
>  	/* Optional hardware blocks, interrupts should be disabled */
> -	if (unlikely(intr_status &
> -		     (DMA_STATUS_GPI | DMA_STATUS_GMI | DMA_STATUS_GLI)))
> +	if (unlikely(intr_status & DMA_INTR_STATUS))
>  		pr_warn("%s: unexpected status %08x\n", __func__, intr_status);
>  
>  	/* Clear the interrupt by writing a logic 1 to the CSR5[15-0] */
> -	writel((intr_status & 0x1ffff), ioaddr + DMA_STATUS);
> +	writel((intr_status & 0x7ffff), ioaddr + DMA_CHAN_STATUS(chan));
>  
>  	return ret;
>  }
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> index 93cead5613e3..e5e7ac03459d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> @@ -58,7 +58,8 @@ static int stmmac_dwmac1_quirks(struct stmmac_priv *priv)
>  		dev_info(priv->device, "Enhanced/Alternate descriptors\n");
>  
>  		/* GMAC older than 3.50 has no extended descriptors */
> -		if (priv->synopsys_id >= DWMAC_CORE_3_50) {
> +		if (priv->synopsys_id >= DWMAC_CORE_3_50 ||
> +		    priv->synopsys_id == DWGMAC_CORE_1_00) {
>  			dev_info(priv->device, "Enabled extended descriptors\n");
>  			priv->extend_desc = 1;
>  		} else {
> 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;
>  
>  	/* Initialize HW Interface */
> @@ -7142,6 +7143,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>  	 * riwt_off field from the platform.
>  	 */
>  	if (((priv->synopsys_id >= DWMAC_CORE_3_50) ||
> +		(priv->synopsys_id == DWGMAC_CORE_1_00) ||
>  	    (priv->plat->has_xgmac)) && (!priv->plat->riwt_off)) {
>  		priv->use_riwt = 1;
>  		dev_info(priv->device,
> -- 
> 2.31.4
>

Yanteng Si Nov. 21, 2023, 9:55 a.m. UTC | #3

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

Andrew Lunn Nov. 22, 2023, 3:39 a.m. UTC | #4

On Tue, Nov 21, 2023 at 05:55:24PM +0800, Yanteng Si wrote:
> 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.

What does DMA_INTR_ENA_AIE_LOONGSON do? This seems like an interrupt
mask, and this is enabling an interrupt source? However, i don't see
this bit being tested in any interrupt status register? Or is it
hiding in one of the other patches?

This is where lots of small patches, with good descriptions helps.

     Andrew

Xi Ruoyao Nov. 22, 2023, 4:02 a.m. UTC | #5

On Wed, 2023-11-22 at 04:39 +0100, Andrew Lunn wrote:
> On Tue, Nov 21, 2023 at 05:55:24PM +0800, Yanteng Si wrote:
> > 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.

You need to determine to use Loongson register or general dwmac1000
register definition at *runtime*, not *compile time*.

Or when people enable CONFIG_DWMAC_LOONGSON (for a release build of
kernel in a mainstream distro the maintainers often enable as many
drivers as possible) they'll suddenly find their dwmac1000's are broken
and we'll get many bug reports.

Yanteng Si Nov. 24, 2023, 1:14 p.m. UTC | #6

在 2023/11/22 11:39, Andrew Lunn 写道:
> On Tue, Nov 21, 2023 at 05:55:24PM +0800, Yanteng Si wrote:
>> 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.
> What does DMA_INTR_ENA_AIE_LOONGSON do? This seems like an interrupt
> mask, and this is enabling an interrupt source? However, i don't see
> this bit being tested in any interrupt status register? Or is it
> hiding in one of the other patches?

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

DMA_INTR_ENA_AIE = DMA_INTR_ENA_AIE_LOONGSON= DMA_INTR_ENA_TX_AIE + 
DMA_INTR_ENA_RX_AIE

DMA_STATUS_NIS = DMA_STATUS_TX_NIS_LOONGSON + DMA_STATUS_RX_NIS_LOONGSON

DMA_STATUS_AIS = DMA_STATUS_TX_AIS_LOONGSON + DMA_STATUS_RX_AIS_LOONGSON

DMA_STATUS_FBI = DMA_STATUS_TX_FBI_LOONGSON + DMA_STATUS_RX_FBI_LOONGSON


>
> This is where lots of small patches, with good descriptions helps.

Ok, thanks for your advice, I will try to split it in the next version.


Thanks,

Yanteng

>
>       Andrew

Andrew Lunn Nov. 24, 2023, 2:51 p.m. UTC | #7

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

	Andrew

Serge Semin Nov. 24, 2023, 4:44 p.m. UTC | #8

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.

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.

-Serge(y)

> 
> 	Andrew

Yanteng Si Nov. 26, 2023, 12:25 p.m. UTC | #9

在 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 review!


Thanks,

Yanteng

>
> -Serge(y)
>
>> 	Andrew

Serge Semin Nov. 27, 2023, 11:32 a.m. UTC | #10

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
>

Yanteng Si Nov. 29, 2023, 10:29 a.m. UTC | #11

在 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

[v5,3/9] net: stmmac: Add Loongson DWGMAC definitions

Checks

Commit Message

Comments

Patch