diff mbox series

[v2,net] net: stmmac: dwmac4: extend timeout for VLAN Tag register busy bit check

Message ID 20240918193452.417115-1-shenwei.wang@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v2,net] net: stmmac: dwmac4: extend timeout for VLAN Tag register busy bit check | expand

Commit Message

Shenwei Wang Sept. 18, 2024, 7:34 p.m. UTC 
Increase the timeout for checking the busy bit of the VLAN Tag register
from 10µs to 500ms. This change is necessary to accommodate scenarios
where Energy Efficient Ethernet (EEE) is enabled.

Overnight testing revealed that when EEE is active, the busy bit can
remain set for up to approximately 300ms. The new 500ms timeout provides
a safety margin.

Fixes: ed64639bc1e0 ("net: stmmac: Add support for VLAN Rx filtering")
Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
Changes in v2:
 - replace the udelay with readl_poll_timeout per Simon's review.

---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

--
2.34.1

Comments

Andrew Lunn Sept. 18, 2024, 11:57 p.m. UTC | #1 
On Wed, Sep 18, 2024 at 02:34:52PM -0500, Shenwei Wang wrote:
> Increase the timeout for checking the busy bit of the VLAN Tag register
> from 10µs to 500ms. This change is necessary to accommodate scenarios
> where Energy Efficient Ethernet (EEE) is enabled.
> 
> Overnight testing revealed that when EEE is active, the busy bit can
> remain set for up to approximately 300ms. The new 500ms timeout provides
> a safety margin.

Do you know what EEE has to do with VLAN filtering?

Could there be other registers which suffer from the same problem?

      Andrew
Shenwei Wang Sept. 20, 2024, 2:24 p.m. UTC | #2 
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, September 18, 2024 6:58 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> horms@kernel.org; Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose
> Abreu <joabreu@synopsys.com>; Ong Boon Leong <boon.leong.ong@intel.com>;
> Wong Vee Khee <vee.khee.wong@intel.com>; Chuah Kim Tatt
> <kim.tatt.chuah@intel.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v2 net] net: stmmac: dwmac4: extend timeout for
> VLAN Tag register busy bit check
> > Overnight testing revealed that when EEE is active, the busy bit can
> > remain set for up to approximately 300ms. The new 500ms timeout
> > provides a safety margin.
> 
> Do you know what EEE has to do with VLAN filtering?
> 

The exact design details are not available to me, but my understanding is that the Busy Bit is synchronized to the RX clock 
supplied by the PHY. When EEE is active and the PHY enters LPI state, the RX clock is gated, preventing updates to the Busy Bit.

The question is the significant delay observed, especially considering that the PHY transitions between active and LPI states 
multiple times during this period. There should have a lot of chances to update the Busy Bit sooner when it is in the active state.

> Could there be other registers which suffer from the same problem?
> 

So far I think it only impact the VLAN status register because those bits are driven by another clock instead of CSR clock.
Based on current observations, it appears that this issue primarily affects the VLAN status register. The reason for this 
is that the bits in the VLAN status register are driven by a clock source distinct from the CSR clock.

Regards,
Shenwei

>       Andrew
Andrew Lunn Sept. 20, 2024, 6:58 p.m. UTC | #3 
> > Could there be other registers which suffer from the same problem?
> > 
> 
> So far I think it only impact the VLAN status register because those bits are driven by another clock instead of CSR clock.
> Based on current observations, it appears that this issue primarily affects the VLAN status register. The reason for this 
> is that the bits in the VLAN status register are driven by a clock source distinct from the CSR clock.

Thanks for the explanation.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Serge Semin Sept. 22, 2024, 10:17 p.m. UTC | #4 
On Wed, Sep 18, 2024 at 02:34:52PM -0500, Shenwei Wang wrote:
> Increase the timeout for checking the busy bit of the VLAN Tag register
> from 10µs to 500ms. This change is necessary to accommodate scenarios
> where Energy Efficient Ethernet (EEE) is enabled.
> 
> Overnight testing revealed that when EEE is active, the busy bit can
> remain set for up to approximately 300ms. The new 500ms timeout provides
> a safety margin.
> 
> Fixes: ed64639bc1e0 ("net: stmmac: Add support for VLAN Rx filtering")
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
> Changes in v2:
>  - replace the udelay with readl_poll_timeout per Simon's review.
> 
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> index a1858f083eef..a0cfa2eaebb4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> @@ -14,6 +14,7 @@
>  #include <linux/slab.h>
>  #include <linux/ethtool.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include "stmmac.h"
>  #include "stmmac_pcs.h"
>  #include "dwmac4.h"
> @@ -471,7 +472,7 @@ static int dwmac4_write_vlan_filter(struct net_device *dev,
>  				    u8 index, u32 data)
>  {
>  	void __iomem *ioaddr = (void __iomem *)dev->base_addr;
> -	int i, timeout = 10;
> +	int ret, timeout = 500000; //500ms
>  	u32 val;
> 
>  	if (index >= hw->num_vlan)
> @@ -487,12 +488,11 @@ static int dwmac4_write_vlan_filter(struct net_device *dev,
> 
>  	writel(val, ioaddr + GMAC_VLAN_TAG);
> 
> -	for (i = 0; i < timeout; i++) {
> -		val = readl(ioaddr + GMAC_VLAN_TAG);
> -		if (!(val & GMAC_VLAN_TAG_CTRL_OB))
> -			return 0;
> -		udelay(1);
> -	}

> +	ret = readl_poll_timeout(ioaddr + GMAC_VLAN_TAG, val,
> +				 !(val & GMAC_VLAN_TAG_CTRL_OB),
> +				 1000, timeout);
> +	if (!ret)
> +		return 0;
> 
>  	netdev_err(dev, "Timeout accessing MAC_VLAN_Tag_Filter\n");

1. Just drop the timeout local variable and use the 500000 literal
directly. There is no point in such parametrization especially you
have already added the delay as is.

2. A more traditional, readable and maintainable pattern is the
error-check statement after the call. So seeing you are changing this
part of the method anyway, let's convert it to:

+	ret = readl_poll_timeout(ioaddr + GMAC_VLAN_TAG, val,
+				 !(val & GMAC_VLAN_TAG_CTRL_OB),
+				 1000, timeout);
+	if (ret) {
+		netdev_err(dev, "Timeout accessing MAC_VLAN_Tag_Filter\n");
+		return ret;
+	}
+
+	return 0;

-Serge(y)

> 
> --
> 2.34.1
> 
>
Shenwei Wang Sept. 23, 2024, 4:18 p.m. UTC | #5 
> -----Original Message-----
> From: Serge Semin <fancer.lancer@gmail.com>
> Sent: Sunday, September 22, 2024 5:18 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> horms@kernel.org; Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose
> Abreu <joabreu@synopsys.com>; Ong Boon Leong <boon.leong.ong@intel.com>;
> Wong Vee Khee <vee.khee.wong@intel.com>; Chuah Kim Tatt
> <kim.tatt.chuah@intel.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH v2 net] net: stmmac: dwmac4: extend timeout for
> VLAN Tag register busy bit check
> >
> > -     for (i = 0; i < timeout; i++) {
> > -             val = readl(ioaddr + GMAC_VLAN_TAG);
> > -             if (!(val & GMAC_VLAN_TAG_CTRL_OB))
> > -                     return 0;
> > -             udelay(1);
> > -     }
> 
> > +     ret = readl_poll_timeout(ioaddr + GMAC_VLAN_TAG, val,
> > +                              !(val & GMAC_VLAN_TAG_CTRL_OB),
> > +                              1000, timeout);
> > +     if (!ret)
> > +             return 0;
> >
> >       netdev_err(dev, "Timeout accessing MAC_VLAN_Tag_Filter\n");
> 
> 1. Just drop the timeout local variable and use the 500000 literal directly. There is
> no point in such parametrization especially you have already added the delay as
> is.
> 
> 2. A more traditional, readable and maintainable pattern is the error-check
> statement after the call. So seeing you are changing this part of the method
> anyway, let's convert it to:
> 
> +       ret = readl_poll_timeout(ioaddr + GMAC_VLAN_TAG, val,
> +                                !(val & GMAC_VLAN_TAG_CTRL_OB),
> +                                1000, timeout);
> +       if (ret) {
> +               netdev_err(dev, "Timeout accessing MAC_VLAN_Tag_Filter\n");
> +               return ret;
> +       }
> +
> +       return 0;
> 

Hi Serge,

Good suggestions! Will send out a new version.

Thanks,
Shenwei

> -Serge(y)
> 
> >
> > --
> > 2.34.1
> >
> >
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index a1858f083eef..a0cfa2eaebb4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -14,6 +14,7 @@ 
 #include <linux/slab.h>
 #include <linux/ethtool.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include "stmmac.h"
 #include "stmmac_pcs.h"
 #include "dwmac4.h"
@@ -471,7 +472,7 @@  static int dwmac4_write_vlan_filter(struct net_device *dev,
 				    u8 index, u32 data)
 {
 	void __iomem *ioaddr = (void __iomem *)dev->base_addr;
-	int i, timeout = 10;
+	int ret, timeout = 500000; //500ms
 	u32 val;

 	if (index >= hw->num_vlan)
@@ -487,12 +488,11 @@  static int dwmac4_write_vlan_filter(struct net_device *dev,

 	writel(val, ioaddr + GMAC_VLAN_TAG);

-	for (i = 0; i < timeout; i++) {
-		val = readl(ioaddr + GMAC_VLAN_TAG);
-		if (!(val & GMAC_VLAN_TAG_CTRL_OB))
-			return 0;
-		udelay(1);
-	}
+	ret = readl_poll_timeout(ioaddr + GMAC_VLAN_TAG, val,
+				 !(val & GMAC_VLAN_TAG_CTRL_OB),
+				 1000, timeout);
+	if (!ret)
+		return 0;

 	netdev_err(dev, "Timeout accessing MAC_VLAN_Tag_Filter\n");