| Message ID | 20210423014741.11858-4-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Multiple improvement to qca8k stability | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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); > } >
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
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 >
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); }
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(-)