[03/14] drivers: net: mdio: mdio-ip8064: improve busy wait delay
diff mbox series

Message ID 20210423014741.11858-4-ansuelsmth@gmail.com
State New, archived
Headers show
Series
  • Multiple improvement to qca8k stability
Related show

Commit Message

Ansuel Smith April 23, 2021, 1:47 a.m. UTC
With the use of the qca8k dsa driver, some problem arised related to
port status detection. With a load on a specific port (for example a
simple speed test), the driver starts to bheave in a strange way and
garbage data is produced. To address this, enlarge the sleep delay and
address a bug for the reg offset 31 that require additional delay for
this specific reg.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/mdio/mdio-ipq8064.c | 36 ++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 14 deletions(-)

Comments

Florian Fainelli April 23, 2021, 1:56 a.m. UTC | #1
On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> With the use of the qca8k dsa driver, some problem arised related to
> port status detection. With a load on a specific port (for example a
> simple speed test), the driver starts to bheave in a strange way and

s/bheave/behave/

> garbage data is produced. To address this, enlarge the sleep delay and
> address a bug for the reg offset 31 that require additional delay for
> this specific reg.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/mdio/mdio-ipq8064.c | 36 ++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c
> index 1bd18857e1c5..5bd6d0501642 100644
> --- a/drivers/net/mdio/mdio-ipq8064.c
> +++ b/drivers/net/mdio/mdio-ipq8064.c
> @@ -15,25 +15,26 @@
>  #include <linux/mfd/syscon.h>
>  
>  /* MII address register definitions */
> -#define MII_ADDR_REG_ADDR                       0x10
> -#define MII_BUSY                                BIT(0)
> -#define MII_WRITE                               BIT(1)
> -#define MII_CLKRANGE_60_100M                    (0 << 2)
> -#define MII_CLKRANGE_100_150M                   (1 << 2)
> -#define MII_CLKRANGE_20_35M                     (2 << 2)
> -#define MII_CLKRANGE_35_60M                     (3 << 2)
> -#define MII_CLKRANGE_150_250M                   (4 << 2)
> -#define MII_CLKRANGE_250_300M                   (5 << 2)
> +#define MII_ADDR_REG_ADDR			0x10
> +#define MII_BUSY				BIT(0)
> +#define MII_WRITE				BIT(1)
> +#define MII_CLKRANGE(x)				((x) << 2)
> +#define MII_CLKRANGE_60_100M			MII_CLKRANGE(0)
> +#define MII_CLKRANGE_100_150M			MII_CLKRANGE(1)
> +#define MII_CLKRANGE_20_35M			MII_CLKRANGE(2)
> +#define MII_CLKRANGE_35_60M			MII_CLKRANGE(3)
> +#define MII_CLKRANGE_150_250M			MII_CLKRANGE(4)
> +#define MII_CLKRANGE_250_300M			MII_CLKRANGE(5)
>  #define MII_CLKRANGE_MASK			GENMASK(4, 2)
>  #define MII_REG_SHIFT				6
>  #define MII_REG_MASK				GENMASK(10, 6)
>  #define MII_ADDR_SHIFT				11
>  #define MII_ADDR_MASK				GENMASK(15, 11)
>  
> -#define MII_DATA_REG_ADDR                       0x14
> +#define MII_DATA_REG_ADDR			0x14
>  
> -#define MII_MDIO_DELAY_USEC                     (1000)
> -#define MII_MDIO_RETRY_MSEC                     (10)
> +#define MII_MDIO_DELAY_USEC			(1000)
> +#define MII_MDIO_RETRY_MSEC			(10)

These changes are not related to what you are doing and are just
whitespace cleaning, better not to mix them with functional changes.

>  
>  struct ipq8064_mdio {
>  	struct regmap *base; /* NSS_GMAC0_BASE */
> @@ -65,7 +66,7 @@ ipq8064_mdio_read(struct mii_bus *bus, int phy_addr, int reg_offset)
>  		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
>  
>  	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> -	usleep_range(8, 10);
> +	usleep_range(10, 13);
>  
>  	err = ipq8064_mdio_wait_busy(priv);
>  	if (err)
> @@ -91,7 +92,14 @@ ipq8064_mdio_write(struct mii_bus *bus, int phy_addr, int reg_offset, u16 data)
>  		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
>  
>  	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> -	usleep_range(8, 10);
> +
> +	/* For the specific reg 31 extra time is needed or the next
> +	 * read will produce grabage data.

s/grabage/garbage/

> +	 */
> +	if (reg_offset == 31)
> +		usleep_range(30, 43);
> +	else
> +		usleep_range(10, 13);

This is just super weird, presumably register 31 needs to be conditional
to the PHY, or pseudo-PHY being driven here. Not that it would harm, but
waiting an extra 30 to 43 microseconds with a Marvell PHY or Broadcom
PHY or from another vendor would not be necessary.

>  
>  	return ipq8064_mdio_wait_busy(priv);
>  }
>
Ansuel Smith April 23, 2021, 2:03 a.m. UTC | #2
On Thu, Apr 22, 2021 at 06:56:34PM -0700, Florian Fainelli wrote:
> 
> 
> On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> > With the use of the qca8k dsa driver, some problem arised related to
> > port status detection. With a load on a specific port (for example a
> > simple speed test), the driver starts to bheave in a strange way and
> 
> s/bheave/behave/
> 
> > garbage data is produced. To address this, enlarge the sleep delay and
> > address a bug for the reg offset 31 that require additional delay for
> > this specific reg.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/mdio/mdio-ipq8064.c | 36 ++++++++++++++++++++-------------
> >  1 file changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c
> > index 1bd18857e1c5..5bd6d0501642 100644
> > --- a/drivers/net/mdio/mdio-ipq8064.c
> > +++ b/drivers/net/mdio/mdio-ipq8064.c
> > @@ -15,25 +15,26 @@
> >  #include <linux/mfd/syscon.h>
> >  
> >  /* MII address register definitions */
> > -#define MII_ADDR_REG_ADDR                       0x10
> > -#define MII_BUSY                                BIT(0)
> > -#define MII_WRITE                               BIT(1)
> > -#define MII_CLKRANGE_60_100M                    (0 << 2)
> > -#define MII_CLKRANGE_100_150M                   (1 << 2)
> > -#define MII_CLKRANGE_20_35M                     (2 << 2)
> > -#define MII_CLKRANGE_35_60M                     (3 << 2)
> > -#define MII_CLKRANGE_150_250M                   (4 << 2)
> > -#define MII_CLKRANGE_250_300M                   (5 << 2)
> > +#define MII_ADDR_REG_ADDR			0x10
> > +#define MII_BUSY				BIT(0)
> > +#define MII_WRITE				BIT(1)
> > +#define MII_CLKRANGE(x)				((x) << 2)
> > +#define MII_CLKRANGE_60_100M			MII_CLKRANGE(0)
> > +#define MII_CLKRANGE_100_150M			MII_CLKRANGE(1)
> > +#define MII_CLKRANGE_20_35M			MII_CLKRANGE(2)
> > +#define MII_CLKRANGE_35_60M			MII_CLKRANGE(3)
> > +#define MII_CLKRANGE_150_250M			MII_CLKRANGE(4)
> > +#define MII_CLKRANGE_250_300M			MII_CLKRANGE(5)
> >  #define MII_CLKRANGE_MASK			GENMASK(4, 2)
> >  #define MII_REG_SHIFT				6
> >  #define MII_REG_MASK				GENMASK(10, 6)
> >  #define MII_ADDR_SHIFT				11
> >  #define MII_ADDR_MASK				GENMASK(15, 11)
> >  
> > -#define MII_DATA_REG_ADDR                       0x14
> > +#define MII_DATA_REG_ADDR			0x14
> >  
> > -#define MII_MDIO_DELAY_USEC                     (1000)
> > -#define MII_MDIO_RETRY_MSEC                     (10)
> > +#define MII_MDIO_DELAY_USEC			(1000)
> > +#define MII_MDIO_RETRY_MSEC			(10)
> 
> These changes are not related to what you are doing and are just
> whitespace cleaning, better not to mix them with functional changes.
>

Ok will send them in a different patch.

> >  
> >  struct ipq8064_mdio {
> >  	struct regmap *base; /* NSS_GMAC0_BASE */
> > @@ -65,7 +66,7 @@ ipq8064_mdio_read(struct mii_bus *bus, int phy_addr, int reg_offset)
> >  		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
> >  
> >  	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> > -	usleep_range(8, 10);
> > +	usleep_range(10, 13);
> >  
> >  	err = ipq8064_mdio_wait_busy(priv);
> >  	if (err)
> > @@ -91,7 +92,14 @@ ipq8064_mdio_write(struct mii_bus *bus, int phy_addr, int reg_offset, u16 data)
> >  		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
> >  
> >  	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> > -	usleep_range(8, 10);
> > +
> > +	/* For the specific reg 31 extra time is needed or the next
> > +	 * read will produce grabage data.
> 
> s/grabage/garbage/
> 
> > +	 */
> > +	if (reg_offset == 31)
> > +		usleep_range(30, 43);
> > +	else
> > +		usleep_range(10, 13);
> 
> This is just super weird, presumably register 31 needs to be conditional
> to the PHY, or pseudo-PHY being driven here. Not that it would harm, but
> waiting an extra 30 to 43 microseconds with a Marvell PHY or Broadcom
> PHY or from another vendor would not be necessary.
>

Any idea how to check this? I found this by printing every value wrote
and read to the mdio driver and notice this. With only this reg. By
adding extra delay the problem is solved, without this the very next
read produce bad data. Maybe some type of specific binding can be useful
here? Some type of 'qcom,extra-delay-31' binding? (fell free to suggest
a better name since i'm very bad at them)

> >  
> >  	return ipq8064_mdio_wait_busy(priv);
> >  }
> > 
> 
> -- 
> Florian
Andrew Lunn April 23, 2021, 12:38 p.m. UTC | #3
On Fri, Apr 23, 2021 at 03:47:29AM +0200, Ansuel Smith wrote:
> With the use of the qca8k dsa driver, some problem arised related to
> port status detection. With a load on a specific port (for example a
> simple speed test), the driver starts to bheave in a strange way and
> garbage data is produced. To address this, enlarge the sleep delay and
> address a bug for the reg offset 31 that require additional delay for
> this specific reg.

>  struct ipq8064_mdio {
>  	struct regmap *base; /* NSS_GMAC0_BASE */
> @@ -65,7 +66,7 @@ ipq8064_mdio_read(struct mii_bus *bus, int phy_addr, int reg_offset)
>  		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
>  
>  	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> -	usleep_range(8, 10);
> +	usleep_range(10, 13);
>  
>  	err = ipq8064_mdio_wait_busy(priv);
>  	if (err)
> @@ -91,7 +92,14 @@ ipq8064_mdio_write(struct mii_bus *bus, int phy_addr, int reg_offset, u16 data)
>  		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
>  
>  	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
> -	usleep_range(8, 10);
> +
> +	/* For the specific reg 31 extra time is needed or the next
> +	 * read will produce grabage data.
> +	 */
> +	if (reg_offset == 31)
> +		usleep_range(30, 43);
> +	else
> +		usleep_range(10, 13);
>  
>  	return ipq8064_mdio_wait_busy(priv);

Is there any documentation as to what register 31 does?

Maybe the real problem is in ipq8064_mdio_wait_busy()? Have you looked
at how long you typically end up waiting? If you know the MDIO bus
speed, you can work out how long a transaction should take. If it is
taking less, something is broken.

Are you sure regmap caching is disabled, so that
ipq8064_mdio_wait_busy() really is reading the register, not some
old cached value?

      Andrew





>  }
> -- 
> 2.30.2
>

Patch
diff mbox series

diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c
index 1bd18857e1c5..5bd6d0501642 100644
--- a/drivers/net/mdio/mdio-ipq8064.c
+++ b/drivers/net/mdio/mdio-ipq8064.c
@@ -15,25 +15,26 @@ 
 #include <linux/mfd/syscon.h>
 
 /* MII address register definitions */
-#define MII_ADDR_REG_ADDR                       0x10
-#define MII_BUSY                                BIT(0)
-#define MII_WRITE                               BIT(1)
-#define MII_CLKRANGE_60_100M                    (0 << 2)
-#define MII_CLKRANGE_100_150M                   (1 << 2)
-#define MII_CLKRANGE_20_35M                     (2 << 2)
-#define MII_CLKRANGE_35_60M                     (3 << 2)
-#define MII_CLKRANGE_150_250M                   (4 << 2)
-#define MII_CLKRANGE_250_300M                   (5 << 2)
+#define MII_ADDR_REG_ADDR			0x10
+#define MII_BUSY				BIT(0)
+#define MII_WRITE				BIT(1)
+#define MII_CLKRANGE(x)				((x) << 2)
+#define MII_CLKRANGE_60_100M			MII_CLKRANGE(0)
+#define MII_CLKRANGE_100_150M			MII_CLKRANGE(1)
+#define MII_CLKRANGE_20_35M			MII_CLKRANGE(2)
+#define MII_CLKRANGE_35_60M			MII_CLKRANGE(3)
+#define MII_CLKRANGE_150_250M			MII_CLKRANGE(4)
+#define MII_CLKRANGE_250_300M			MII_CLKRANGE(5)
 #define MII_CLKRANGE_MASK			GENMASK(4, 2)
 #define MII_REG_SHIFT				6
 #define MII_REG_MASK				GENMASK(10, 6)
 #define MII_ADDR_SHIFT				11
 #define MII_ADDR_MASK				GENMASK(15, 11)
 
-#define MII_DATA_REG_ADDR                       0x14
+#define MII_DATA_REG_ADDR			0x14
 
-#define MII_MDIO_DELAY_USEC                     (1000)
-#define MII_MDIO_RETRY_MSEC                     (10)
+#define MII_MDIO_DELAY_USEC			(1000)
+#define MII_MDIO_RETRY_MSEC			(10)
 
 struct ipq8064_mdio {
 	struct regmap *base; /* NSS_GMAC0_BASE */
@@ -65,7 +66,7 @@  ipq8064_mdio_read(struct mii_bus *bus, int phy_addr, int reg_offset)
 		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
 
 	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
-	usleep_range(8, 10);
+	usleep_range(10, 13);
 
 	err = ipq8064_mdio_wait_busy(priv);
 	if (err)
@@ -91,7 +92,14 @@  ipq8064_mdio_write(struct mii_bus *bus, int phy_addr, int reg_offset, u16 data)
 		   ((reg_offset << MII_REG_SHIFT) & MII_REG_MASK);
 
 	regmap_write(priv->base, MII_ADDR_REG_ADDR, miiaddr);
-	usleep_range(8, 10);
+
+	/* For the specific reg 31 extra time is needed or the next
+	 * read will produce grabage data.
+	 */
+	if (reg_offset == 31)
+		usleep_range(30, 43);
+	else
+		usleep_range(10, 13);
 
 	return ipq8064_mdio_wait_busy(priv);
 }