net: stmmac: enable MAC after MTL configuring

Commit Message

[e-mail] Oct. 14, 2024, 5:44 a.m. UTC

From: "hailong.fan" <[e-mail]>

DMA maybe block while ETH is opening,
Adjust the enable sequence, put the MAC enable last

Signed-off-by: hailong.fan <[e-mail]>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c   |  8 --------
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c | 12 ------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  6 +++---
 3 files changed, 3 insertions(+), 23 deletions(-)

Comments

Simon Horman Oct. 17, 2024, 10:18 a.m. UTC | #1

On Mon, Oct 14, 2024 at 01:44:03PM +0800, [e-mail] wrote:
> From: "hailong.fan" <[e-mail]>
> 
> DMA maybe block while ETH is opening,
> Adjust the enable sequence, put the MAC enable last
> 
> Signed-off-by: hailong.fan <[e-mail]>

Hi,

I think that some more explanation of this is required.
Including if a problem has been observed, and if so under what
conditions. Or, if not, some background information on why
this adjustment is correct.

I also think some explanation is required of the relationship
between the changes this patch makes to setup, and the
changes it makes to start and stop.

...

Fan Hailong/范海龙 Oct. 18, 2024, 1:15 a.m. UTC | #2

Hi

For example, ETH is directly connected to the switch, which never power down and sends broadcast packets at regular intervals. 
During the process of opening ETH, data may flow into the MTL FIFO, once MAC RX is enabled.
and then, MTL will be set, such as FIFO size. 
Once enable DMA, There is a certain probability that DMA will read incorrect data from MTL FIFO, causing DMA to hang up. 
By read DMA_Debug_Status, you can be observed that the RPS remains at a certain value forever. 
The correct process should be to configure MAC/MTL/DMA before enabling DMA/MAC

stmmac_start_rx/tx and stmmac_stop_rx/tx should only handle the DMA module to avoid confusion



-----邮件原件-----
发件人: Simon Horman <[e-mail]> 
发送时间: 2024年10月17日 18:19
收件人: [e-mail]
抄送: [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; Fan Hailong/范海龙 <[e-mail]>
主题: Re: [PATCH] net: stmmac: enable MAC after MTL configuring

On Mon, Oct 14, 2024 at 01:44:03PM +0800, [e-mail] wrote:
> From: "hailong.fan" <[e-mail]>
> 
> DMA maybe block while ETH is opening,
> Adjust the enable sequence, put the MAC enable last
> 
> Signed-off-by: hailong.fan <[e-mail]>

Hi,

I think that some more explanation of this is required.
Including if a problem has been observed, and if so under what conditions. Or, if not, some background information on why this adjustment is correct.

I also think some explanation is required of the relationship between the changes this patch makes to setup, and the changes it makes to start and stop.

...

Andrew Lunn Oct. 18, 2024, 5:57 p.m. UTC | #3

On Fri, Oct 18, 2024 at 01:15:30AM +0000, Fan Hailong/范海龙 wrote:
> Hi
> 
> For example, ETH is directly connected to the switch, which never power down and sends broadcast packets at regular intervals. 
> During the process of opening ETH, data may flow into the MTL FIFO, once MAC RX is enabled.
> and then, MTL will be set, such as FIFO size. 
> Once enable DMA, There is a certain probability that DMA will read incorrect data from MTL FIFO, causing DMA to hang up. 
> By read DMA_Debug_Status, you can be observed that the RPS remains at a certain value forever. 
> The correct process should be to configure MAC/MTL/DMA before enabling DMA/MAC

What Simon is asking for is that this is part of the commit message.

Please also don't top post.

    Andrew

---
pw-bot: cr

Fan Hailong/范海龙 Oct. 20, 2024, 1:45 a.m. UTC | #4

Hi 

Please find new patch in attachments, thanks.


-----邮件原件-----
发件人: Andrew Lunn <[e-mail]> 
发送时间: 2024年10月19日 1:57
收件人: Fan Hailong/范海龙 <[e-mail]>
抄送: Simon Horman <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]
主题: Re: 回复: [PATCH] net: stmmac: enable MAC after MTL configuring

On Fri, Oct 18, 2024 at 01:15:30AM +0000, Fan Hailong/范海龙 wrote:
> Hi
> 
> For example, ETH is directly connected to the switch, which never power down and sends broadcast packets at regular intervals. 
> During the process of opening ETH, data may flow into the MTL FIFO, once MAC RX is enabled.
> and then, MTL will be set, such as FIFO size. 
> Once enable DMA, There is a certain probability that DMA will read incorrect data from MTL FIFO, causing DMA to hang up. 
> By read DMA_Debug_Status, you can be observed that the RPS remains at a certain value forever. 
> The correct process should be to configure MAC/MTL/DMA before enabling DMA/MAC

What Simon is asking for is that this is part of the commit message.

Please also don't top post.

    Andrew

---
pw-bot: cr

Andrew Lunn Oct. 20, 2024, 2:49 p.m. UTC | #5

On Sun, Oct 20, 2024 at 01:45:41AM +0000, Fan Hailong/范海龙 wrote:
> Hi 
> 
> Please find new patch in attachments, thanks.

Please read

https://docs.kernel.org/process/submitting-patches.html

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Attachments are not accepted.

	Andrew

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
index 0d185e54e..92448d858 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_lib.c
@@ -50,10 +50,6 @@  void dwmac4_dma_start_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
 
 	value |= DMA_CONTROL_ST;
 	writel(value, ioaddr + DMA_CHAN_TX_CONTROL(dwmac4_addrs, chan));
-
-	value = readl(ioaddr + GMAC_CONFIG);
-	value |= GMAC_CONFIG_TE;
-	writel(value, ioaddr + GMAC_CONFIG);
 }
 
 void dwmac4_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
@@ -77,10 +73,6 @@  void dwmac4_dma_start_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
 	value |= DMA_CONTROL_SR;
 
 	writel(value, ioaddr + DMA_CHAN_RX_CONTROL(dwmac4_addrs, chan));
-
-	value = readl(ioaddr + GMAC_CONFIG);
-	value |= GMAC_CONFIG_RE;
-	writel(value, ioaddr + GMAC_CONFIG);
 }
 
 void dwmac4_dma_stop_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
index 7840bc403..cba12edc1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
@@ -288,10 +288,6 @@  static void dwxgmac2_dma_start_tx(struct stmmac_priv *priv,
 	value = readl(ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
 	value |= XGMAC_TXST;
 	writel(value, ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
-
-	value = readl(ioaddr + XGMAC_TX_CONFIG);
-	value |= XGMAC_CONFIG_TE;
-	writel(value, ioaddr + XGMAC_TX_CONFIG);
 }
 
 static void dwxgmac2_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
@@ -302,10 +298,6 @@  static void dwxgmac2_dma_stop_tx(struct stmmac_priv *priv, void __iomem *ioaddr,
 	value = readl(ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
 	value &= ~XGMAC_TXST;
 	writel(value, ioaddr + XGMAC_DMA_CH_TX_CONTROL(chan));
-
-	value = readl(ioaddr + XGMAC_TX_CONFIG);
-	value &= ~XGMAC_CONFIG_TE;
-	writel(value, ioaddr + XGMAC_TX_CONFIG);
 }
 
 static void dwxgmac2_dma_start_rx(struct stmmac_priv *priv,
@@ -316,10 +308,6 @@  static void dwxgmac2_dma_start_rx(struct stmmac_priv *priv,
 	value = readl(ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
 	value |= XGMAC_RXST;
 	writel(value, ioaddr + XGMAC_DMA_CH_RX_CONTROL(chan));
-
-	value = readl(ioaddr + XGMAC_RX_CONFIG);
-	value |= XGMAC_CONFIG_RE;
-	writel(value, ioaddr + XGMAC_RX_CONFIG);
 }
 
 static void dwxgmac2_dma_stop_rx(struct stmmac_priv *priv, void __iomem *ioaddr,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e21404822..c19ca62a4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3437,9 +3437,6 @@  static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
 		priv->hw->rx_csum = 0;
 	}
 
-	/* Enable the MAC Rx/Tx */
-	stmmac_mac_set(priv, priv->ioaddr, true);
-
 	/* Set the HW DMA mode and the COE */
 	stmmac_dma_operation_mode(priv);
 
@@ -3523,6 +3520,9 @@  static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
 	/* Start the ball rolling... */
 	stmmac_start_all_dma(priv);
 
+	/* Enable the MAC Rx/Tx */
+	stmmac_mac_set(priv, priv->ioaddr, true);
+
 	stmmac_set_hw_vlan_mode(priv, priv->hw);
 
 	return 0;