[PATCH 00/14] Multiple improvement to qca8k stability

[PATCH 00/14] Multiple improvement to qca8k stability

[Ansuel Smith]

Currently qca8337 switch are widely used on ipq8064 based router.
On these particular router it was notice a very unstable switch with
port not link detected as link with unknown speed, port dropping
randomly and general unreliability. Lots of testing and comparison
between this dsa driver and the original qsdk driver showed lack of some
additional delay and values. A main difference arised from the original
driver and the dsa one. The original driver didn't use MASTER regs to
read phy status and the dedicated mdio driver worked correctly. Now that
the dsa driver actually use these regs, it was found that these special
read/write operation required mutual exclusion to normal
qca8k_read/write operation. The add of mutex for these operation fixed
the random port dropping and now only the actual linked port randomly
dropped. Adding additional delay for set_page operation and fixing a bug
in the mdio dedicated driver fixed also this problem. The current driver
requires also more time to apply vlan switch. All of these changes and
tweak permit a now very stable and reliable dsa driver and 0 port
dropping. This series is currently tested by at least 5 user with
different routers and all reports positive results and no problems.

Ansuel Smith (14):
  drivers: net: dsa: qca8k: handle error with set_page
  drivers: net: dsa: qca8k: tweak internal delay to oem spec
  drivers: net: mdio: mdio-ip8064: improve busy wait delay
  drivers: net: dsa: qca8k: apply suggested packet priority
  drivers: net: dsa: qca8k: add support for qca8327 switch
  devicetree: net: dsa: qca8k: Document new compatible qca8327
  drivers: net: dsa: qca8k: limit priority tweak to qca8337 switch
  drivers: net: dsa: qca8k: add GLOBAL_FC settings needed for qca8327
  drivers: net: dsa: qca8k: add support for switch rev
  drivers: net: dsa: qca8k: add support for specific QCA access function
  drivers: net: dsa: qca8k: apply switch revision fix
  drivers: net: dsa: qca8k: clear MASTER_EN after phy read/write
  drivers: net: dsa: qca8k: protect MASTER busy_wait with mdio mutex
  drivers: net: dsa: qca8k: enlarge mdio delay and timeout

 .../devicetree/bindings/net/dsa/qca8k.txt     |   1 +
 drivers/net/dsa/qca8k.c                       | 256 ++++++++++++++++--
 drivers/net/dsa/qca8k.h                       |  54 +++-
 drivers/net/mdio/mdio-ipq8064.c               |  36 ++-
 4 files changed, 304 insertions(+), 43 deletions(-)

-- 
2.30.2

[PATCH 01/14] drivers: net: dsa: qca8k: handle error with set_page

[Ansuel Smith]

Better handle function qca8k_set_page. The original code requires a
deleay of 5us and set the current page only if the bus write has not
failed.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index cdaf9f85a2cb..a6d35b825c0e 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -133,9 +133,12 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
 	if (page == qca8k_current_page)
 		return;
 
-	if (bus->write(bus, 0x18, 0, page) < 0)
+	if (bus->write(bus, 0x18, 0, page)) {
 		dev_err_ratelimited(&bus->dev,
 				    "failed to set qca8k page\n");
+		return;
+	}
+
 	qca8k_current_page = page;
 }
 
-- 
2.30.2

Re: [PATCH 01/14] drivers: net: dsa: qca8k: handle error with set_page

[Florian Fainelli]

On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> Better handle function qca8k_set_page. The original code requires a
> deleay of 5us and set the current page only if the bus write has not
> failed.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index cdaf9f85a2cb..a6d35b825c0e 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -133,9 +133,12 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
>  	if (page == qca8k_current_page)
>  		return;
>  
> -	if (bus->write(bus, 0x18, 0, page) < 0)
> +	if (bus->write(bus, 0x18, 0, page)) {
>  		dev_err_ratelimited(&bus->dev,
>  				    "failed to set qca8k page\n");
> +		return;
> +	}

An improvement would be to propagate the return value to the two callers
which themselves do allow an error to be propagated no? If you cannot
set the page you are pretty much toast.
-- 
Florian

[PATCH 02/14] drivers: net: dsa: qca8k: tweak internal delay to oem spec

[Ansuel Smith]

The original code had the internal dalay set to 1 for tx and 2 for rx.
Apply the oem internal dalay to fix some switch communication error.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 6 ++++--
 drivers/net/dsa/qca8k.h | 9 ++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a6d35b825c0e..b8bfc7acf6f4 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -849,8 +849,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		 */
 		qca8k_write(priv, reg,
 			    QCA8K_PORT_PAD_RGMII_EN |
-			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
+			    QCA8K_PORT_PAD_RGMII_TX_DELAY(1) |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY(2) |
+			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
 		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
 			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
 		break;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 7ca4b93e0bb5..e0b679133880 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -32,12 +32,11 @@
 #define QCA8K_REG_PORT5_PAD_CTRL			0x008
 #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
 #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
-#define   QCA8K_PORT_PAD_RGMII_TX_DELAY(x)		\
-						((0x8 + (x & 0x3)) << 22)
-#define   QCA8K_PORT_PAD_RGMII_RX_DELAY(x)		\
-						((0x10 + (x & 0x3)) << 20)
-#define   QCA8K_MAX_DELAY				3
+#define   QCA8K_PORT_PAD_RGMII_TX_DELAY(x)		((x) << 22)
+#define   QCA8K_PORT_PAD_RGMII_RX_DELAY(x)		((x) << 20)
+#define	  QCA8K_PORT_PAD_RGMII_TX_DELAY_EN		BIT(25)
 #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN		BIT(24)
+#define   QCA8K_MAX_DELAY				3
 #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
 #define QCA8K_REG_PWS					0x010
 #define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
-- 
2.30.2

Re: [PATCH 02/14] drivers: net: dsa: qca8k: tweak internal delay to oem spec

[Florian Fainelli]

On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> The original code had the internal dalay set to 1 for tx and 2 for rx.
> Apply the oem internal dalay to fix some switch communication error.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 6 ++++--
>  drivers/net/dsa/qca8k.h | 9 ++++-----
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a6d35b825c0e..b8bfc7acf6f4 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -849,8 +849,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		 */
>  		qca8k_write(priv, reg,
>  			    QCA8K_PORT_PAD_RGMII_EN |
> -			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> -			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
> +			    QCA8K_PORT_PAD_RGMII_TX_DELAY(1) |
> +			    QCA8K_PORT_PAD_RGMII_RX_DELAY(2) |
> +			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
> +			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);

There are standard properties in order to configure a specific RX and TX
delay:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml#n125

can you use that mechanism and parse that property, or if nothing else,
allow an user to override delays via device tree using these standard
properties?
-- 
Florian

Re: [PATCH 02/14] drivers: net: dsa: qca8k: tweak internal delay to oem spec

[Ansuel Smith]

On Thu, Apr 22, 2021 at 06:53:45PM -0700, Florian Fainelli wrote:
> 
> 
> On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> > The original code had the internal dalay set to 1 for tx and 2 for rx.
> > Apply the oem internal dalay to fix some switch communication error.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 6 ++++--
> >  drivers/net/dsa/qca8k.h | 9 ++++-----
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index a6d35b825c0e..b8bfc7acf6f4 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -849,8 +849,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  		 */
> >  		qca8k_write(priv, reg,
> >  			    QCA8K_PORT_PAD_RGMII_EN |
> > -			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> > -			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
> > +			    QCA8K_PORT_PAD_RGMII_TX_DELAY(1) |
> > +			    QCA8K_PORT_PAD_RGMII_RX_DELAY(2) |
> > +			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
> > +			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> 
> There are standard properties in order to configure a specific RX and TX
> delay:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml#n125
> 
> can you use that mechanism and parse that property, or if nothing else,
> allow an user to override delays via device tree using these standard
> properties?

Since this is mac config, what would be the best way to parse these
data? Parse them in the qca8k_setup and put them in the
qca8k_priv?

> -- 
> Florian

Re: [PATCH 02/14] drivers: net: dsa: qca8k: tweak internal delay to oem spec

[Florian Fainelli]

On 4/22/2021 6:57 PM, Ansuel Smith wrote:
> On Thu, Apr 22, 2021 at 06:53:45PM -0700, Florian Fainelli wrote:
>>
>>
>> On 4/22/2021 6:47 PM, Ansuel Smith wrote:
>>> The original code had the internal dalay set to 1 for tx and 2 for rx.
>>> Apply the oem internal dalay to fix some switch communication error.
>>>
>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>> ---
>>>  drivers/net/dsa/qca8k.c | 6 ++++--
>>>  drivers/net/dsa/qca8k.h | 9 ++++-----
>>>  2 files changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
>>> index a6d35b825c0e..b8bfc7acf6f4 100644
>>> --- a/drivers/net/dsa/qca8k.c
>>> +++ b/drivers/net/dsa/qca8k.c
>>> @@ -849,8 +849,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>>  		 */
>>>  		qca8k_write(priv, reg,
>>>  			    QCA8K_PORT_PAD_RGMII_EN |
>>> -			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
>>> -			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
>>> +			    QCA8K_PORT_PAD_RGMII_TX_DELAY(1) |
>>> +			    QCA8K_PORT_PAD_RGMII_RX_DELAY(2) |
>>> +			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
>>> +			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
>>
>> There are standard properties in order to configure a specific RX and TX
>> delay:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml#n125
>>
>> can you use that mechanism and parse that property, or if nothing else,
>> allow an user to override delays via device tree using these standard
>> properties?
> 
> Since this is mac config, what would be the best way to parse these
> data? Parse them in the qca8k_setup and put them in the
> qca8k_priv?

Yes something like that would work.
-- 
Florian

Re: [PATCH 02/14] drivers: net: dsa: qca8k: tweak internal delay to oem spec

[Andrew Lunn]

On Fri, Apr 23, 2021 at 03:47:28AM +0200, Ansuel Smith wrote:
> The original code had the internal dalay set to 1 for tx and 2 for rx.

Do you have any idea what these values mean, in terms of pS?

What value is being passed to the PHY? Since the MAC is providing the
delays, you need to ensure the PHY is not adding a delay. Is there
code doing this?

     Andrew

[PATCH 03/14] drivers: net: mdio: mdio-ip8064: improve busy wait delay

[Ansuel Smith]

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(-)

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);
 }
-- 
2.30.2

Re: [PATCH 03/14] drivers: net: mdio: mdio-ip8064: improve busy wait delay

[Florian Fainelli]

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);
>  }
> 

-- 
Florian

Re: [PATCH 03/14] drivers: net: mdio: mdio-ip8064: improve busy wait delay

[Ansuel Smith]

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

Re: [PATCH 03/14] drivers: net: mdio: mdio-ip8064: improve busy wait delay

[Andrew Lunn]

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 04/14] drivers: net: dsa: qca8k: apply suggested packet priority

[Ansuel Smith]

The port 5 of the ar8337 have some problem in flood condition. The
original legacy driver had some specific buffer and priority settings
for the different port suggested by the QCA switch team. Add this
missing settings to improve switch stability under load condition.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 42 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h | 24 +++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b8bfc7acf6f4..7408cbee05c2 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -701,6 +701,7 @@ qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	int ret, i;
+	u32 mask;
 
 	/* Make sure that port 0 is the cpu port */
 	if (!dsa_is_cpu_port(ds, 0)) {
@@ -785,6 +786,47 @@ qca8k_setup(struct dsa_switch *ds)
 		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
 	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
 
+	/* The port 5 of the switch ar8337 have some problem in flood condition.
+	 * To fix this the original code has some specific priority values
+	 * suggested by the QCA switch team.
+	 */
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		switch (i) {
+		/* The 2 CPU port and port 5 requires some different
+		 * priority than any other ports.
+		 */
+		case 0:
+		case 5:
+		case 6:
+			mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
+				QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
+				QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x4) |
+				QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x4) |
+				QCA8K_PORT_HOL_CTRL0_EG_PRI4(0x6) |
+				QCA8K_PORT_HOL_CTRL0_EG_PRI5(0x8) |
+				QCA8K_PORT_HOL_CTRL0_EG_PORT(0x1e);
+			break;
+		default:
+			mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
+				QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
+				QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x6) |
+				QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x8) |
+				QCA8K_PORT_HOL_CTRL0_EG_PORT(0x19);
+		}
+		qca8k_write(priv, QCA8K_REG_PORT_HOL_CTRL0(i), mask);
+
+		mask = QCA8K_PORT_HOL_CTRL1_ING(0x6) |
+		       QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
+		       QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
+		       QCA8K_PORT_HOL_CTRL1_WRED_EN;
+		qca8k_rmw(priv, QCA8K_REG_PORT_HOL_CTRL1(i),
+			  QCA8K_PORT_HOL_CTRL1_ING_BUF |
+			  QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
+			  QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
+			  QCA8K_PORT_HOL_CTRL1_WRED_EN,
+			  mask);
+	}
+
 	/* Flush the FDB table */
 	qca8k_fdb_flush(priv);
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index e0b679133880..0ff7abbd40dc 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -163,6 +163,30 @@
 #define   QCA8K_PORT_LOOKUP_STATE			GENMASK(18, 16)
 #define   QCA8K_PORT_LOOKUP_LEARN			BIT(20)
 
+#define QCA8K_REG_PORT_HOL_CTRL0(_i)			(0x970 + (_i) * 0x8)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI0_BUF		GENMASK(3, 0)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI0(x)		((x) << 0)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI1_BUF		GENMASK(7, 4)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI1(x)		((x) << 4)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI2_BUF		GENMASK(11, 8)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI2(x)		((x) << 8)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI3_BUF		GENMASK(15, 12)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI3(x)		((x) << 12)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI4_BUF		GENMASK(19, 16)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI4(x)		((x) << 16)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI5_BUF		GENMASK(23, 20)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI5(x)		((x) << 20)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PORT_BUF		GENMASK(29, 24)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PORT(x)		((x) << 24)
+
+#define QCA8K_REG_PORT_HOL_CTRL1(_i)			(0x974 + (_i) * 0x8)
+#define   QCA8K_PORT_HOL_CTRL1_ING_BUF			GENMASK(3, 0)
+#define   QCA8K_PORT_HOL_CTRL1_ING(x)			((x) << 0)
+#define   QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN		BIT(6)
+#define   QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN		BIT(7)
+#define   QCA8K_PORT_HOL_CTRL1_WRED_EN			BIT(8)
+#define   QCA8K_PORT_HOL_CTRL1_EG_MIRROR_EN		BIT(16)
+
 /* Pkt edit registers */
 #define QCA8K_EGRESS_VLAN(x)				(0x0c70 + (4 * (x / 2)))
 
-- 
2.30.2

[PATCH 05/14] drivers: net: dsa: qca8k: add support for qca8327 switch

[Ansuel Smith]

qca8327 switch is a low tier version of the more recent qca8337.
It does share the same regs used by the qca8k driver and can be
supported with minimal change.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 21 ++++++++++++++++++---
 drivers/net/dsa/qca8k.h |  6 ++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 7408cbee05c2..ca12394c2ff7 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1440,6 +1440,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 static int
 qca8k_sw_probe(struct mdio_device *mdiodev)
 {
+	const struct qca8k_match_data *data;
 	struct qca8k_priv *priv;
 	u32 id;
 
@@ -1467,11 +1468,16 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 		gpiod_set_value_cansleep(priv->reset_gpio, 0);
 	}
 
+	/* get the switches ID from the compatible */
+	data = of_device_get_match_data(&mdiodev->dev);
+	if (!data)
+		return -ENODEV;
+
 	/* read the switches ID register */
 	id = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
 	id >>= QCA8K_MASK_CTRL_ID_S;
 	id &= QCA8K_MASK_CTRL_ID_M;
-	if (id != QCA8K_ID_QCA8337)
+	if (id != data->id)
 		return -ENODEV;
 
 	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
@@ -1537,9 +1543,18 @@ static int qca8k_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(qca8k_pm_ops,
 			 qca8k_suspend, qca8k_resume);
 
+static const struct qca8k_match_data qca832x = {
+	.id = QCA8K_ID_QCA8327,
+};
+
+static const struct qca8k_match_data qca833x = {
+	.id = QCA8K_ID_QCA8337,
+};
+
 static const struct of_device_id qca8k_of_match[] = {
-	{ .compatible = "qca,qca8334" },
-	{ .compatible = "qca,qca8337" },
+	{ .compatible = "qca,qca8327", .data = &qca832x },
+	{ .compatible = "qca,qca8334", .data = &qca833x },
+	{ .compatible = "qca,qca8337", .data = &qca833x },
 	{ /* sentinel */ },
 };
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 0ff7abbd40dc..d94129371a1c 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -15,6 +15,8 @@
 #define QCA8K_NUM_PORTS					7
 #define QCA8K_MAX_MTU					9000
 
+#define PHY_ID_QCA8327					0x004dd034
+#define QCA8K_ID_QCA8327				0x12
 #define PHY_ID_QCA8337					0x004dd036
 #define QCA8K_ID_QCA8337				0x13
 
@@ -234,6 +236,10 @@ struct ar8xxx_port_status {
 	int enabled;
 };
 
+struct qca8k_match_data {
+	u8 id;
+};
+
 struct qca8k_priv {
 	struct regmap *regmap;
 	struct mii_bus *bus;
-- 
2.30.2

Re: [PATCH 05/14] drivers: net: dsa: qca8k: add support for qca8327 switch

[Andrew Lunn]

> @@ -1467,11 +1468,16 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>  		gpiod_set_value_cansleep(priv->reset_gpio, 0);
>  	}
>  
> +	/* get the switches ID from the compatible */
> +	data = of_device_get_match_data(&mdiodev->dev);
> +	if (!data)
> +		return -ENODEV;
> +
>  	/* read the switches ID register */
>  	id = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
>  	id >>= QCA8K_MASK_CTRL_ID_S;
>  	id &= QCA8K_MASK_CTRL_ID_M;
> -	if (id != QCA8K_ID_QCA8337)
> +	if (id != data->id)
>  		return -ENODEV;

It is useful to print an error message here: Found X, expected
Y. Gives the DT writer an idea what they did wrong.

   Andrew

[PATCH 06/14] devicetree: net: dsa: qca8k: Document new compatible qca8327

[Ansuel Smith]

Add support for qca8327 in the compatible list.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/qca8k.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index ccbc6d89325d..1daf68e7ae19 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -3,6 +3,7 @@
 Required properties:
 
 - compatible: should be one of:
+    "qca,qca8327"
     "qca,qca8334"
     "qca,qca8337"
 
-- 
2.30.2

[PATCH 07/14] drivers: net: dsa: qca8k: limit priority tweak to qca8337 switch

[Ansuel Smith]

The packet priority tweak and the rx delay is specific to qca8337.
Limit this changes to qca8337 as now we also support 8327 switch.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 84 +++++++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 36 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index ca12394c2ff7..19bb3754d9ec 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -700,9 +700,13 @@ static int
 qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	const struct qca8k_match_data *data;
 	int ret, i;
 	u32 mask;
 
+	/* get the switches ID from the compatible */
+	data = of_device_get_match_data(priv->dev);
+
 	/* Make sure that port 0 is the cpu port */
 	if (!dsa_is_cpu_port(ds, 0)) {
 		pr_err("port 0 is not the CPU port\n");
@@ -790,41 +794,43 @@ qca8k_setup(struct dsa_switch *ds)
 	 * To fix this the original code has some specific priority values
 	 * suggested by the QCA switch team.
 	 */
-	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
-		switch (i) {
-		/* The 2 CPU port and port 5 requires some different
-		 * priority than any other ports.
-		 */
-		case 0:
-		case 5:
-		case 6:
-			mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
-				QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
-				QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x4) |
-				QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x4) |
-				QCA8K_PORT_HOL_CTRL0_EG_PRI4(0x6) |
-				QCA8K_PORT_HOL_CTRL0_EG_PRI5(0x8) |
-				QCA8K_PORT_HOL_CTRL0_EG_PORT(0x1e);
-			break;
-		default:
-			mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
-				QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
-				QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x6) |
-				QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x8) |
-				QCA8K_PORT_HOL_CTRL0_EG_PORT(0x19);
+	if (data->id == QCA8K_ID_QCA8337) {
+		for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+			switch (i) {
+			/* The 2 CPU port and port 5 requires some different
+			 * priority than any other ports.
+			 */
+			case 0:
+			case 5:
+			case 6:
+				mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x4) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x4) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI4(0x6) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI5(0x8) |
+					QCA8K_PORT_HOL_CTRL0_EG_PORT(0x1e);
+				break;
+			default:
+				mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x6) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x8) |
+					QCA8K_PORT_HOL_CTRL0_EG_PORT(0x19);
+			}
+			qca8k_write(priv, QCA8K_REG_PORT_HOL_CTRL0(i), mask);
+
+			mask = QCA8K_PORT_HOL_CTRL1_ING(0x6) |
+			QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
+			QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
+			QCA8K_PORT_HOL_CTRL1_WRED_EN;
+			qca8k_rmw(priv, QCA8K_REG_PORT_HOL_CTRL1(i),
+				  QCA8K_PORT_HOL_CTRL1_ING_BUF |
+				  QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
+				  QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
+				  QCA8K_PORT_HOL_CTRL1_WRED_EN,
+				  mask);
 		}
-		qca8k_write(priv, QCA8K_REG_PORT_HOL_CTRL0(i), mask);
-
-		mask = QCA8K_PORT_HOL_CTRL1_ING(0x6) |
-		       QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
-		       QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
-		       QCA8K_PORT_HOL_CTRL1_WRED_EN;
-		qca8k_rmw(priv, QCA8K_REG_PORT_HOL_CTRL1(i),
-			  QCA8K_PORT_HOL_CTRL1_ING_BUF |
-			  QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
-			  QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
-			  QCA8K_PORT_HOL_CTRL1_WRED_EN,
-			  mask);
 	}
 
 	/* Flush the FDB table */
@@ -840,9 +846,13 @@ static void
 qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			 const struct phylink_link_state *state)
 {
+	const struct qca8k_match_data *data;
 	struct qca8k_priv *priv = ds->priv;
 	u32 reg, val;
 
+	/* get the switches ID from the compatible */
+	data = of_device_get_match_data(priv->dev);
+
 	switch (port) {
 	case 0: /* 1st CPU port */
 		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
@@ -895,8 +905,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			    QCA8K_PORT_PAD_RGMII_RX_DELAY(2) |
 			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
 			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
-		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+		/* QCA8337 requires to set rgmii rx delay */
+		if (data->id == QCA8K_ID_QCA8337)
+			qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
+				    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
-- 
2.30.2

Re: [PATCH 07/14] drivers: net: dsa: qca8k: limit priority tweak to qca8337 switch

[Florian Fainelli]

On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> The packet priority tweak and the rx delay is specific to qca8337.
> Limit this changes to qca8337 as now we also support 8327 switch.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

If you re-order patches a bit, then we could avoid having this patch
completely, with the exception of the RX_DELAY_EN or maybe that can
folded into patch 5?
-- 
Florian

[PATCH 08/14] drivers: net: dsa: qca8k: add GLOBAL_FC settings needed for qca8327

[Ansuel Smith]

Switch qca8327 needs special settings for the GLOBAL_FC_THRES regs.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 10 ++++++++++
 drivers/net/dsa/qca8k.h |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 19bb3754d9ec..d469620e9344 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -833,6 +833,16 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 	}
 
+	/* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */
+	if (data->id == QCA8K_ID_QCA8327) {
+		mask = QCA8K_GLOBAL_FC_GOL_XON_THRES(288) |
+		       QCA8K_GLOBAL_FC_GOL_XOFF_THRES(496);
+		qca8k_rmw(priv, QCA8K_REG_GLOBAL_FC_THRESH,
+			  QCA8K_GLOBAL_FC_GOL_XON_THRES_S |
+			  QCA8K_GLOBAL_FC_GOL_XOFF_THRES_S,
+			  mask);
+	}
+
 	/* Flush the FDB table */
 	qca8k_fdb_flush(priv);
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index d94129371a1c..308d8410fdb6 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -165,6 +165,12 @@
 #define   QCA8K_PORT_LOOKUP_STATE			GENMASK(18, 16)
 #define   QCA8K_PORT_LOOKUP_LEARN			BIT(20)
 
+#define QCA8K_REG_GLOBAL_FC_THRESH			0x800
+#define   QCA8K_GLOBAL_FC_GOL_XON_THRES(x)		((x) << 16)
+#define   QCA8K_GLOBAL_FC_GOL_XON_THRES_S		GENMASK(24, 16)
+#define   QCA8K_GLOBAL_FC_GOL_XOFF_THRES(x)		((x) << 0)
+#define   QCA8K_GLOBAL_FC_GOL_XOFF_THRES_S		GENMASK(8, 0)
+
 #define QCA8K_REG_PORT_HOL_CTRL0(_i)			(0x970 + (_i) * 0x8)
 #define   QCA8K_PORT_HOL_CTRL0_EG_PRI0_BUF		GENMASK(3, 0)
 #define   QCA8K_PORT_HOL_CTRL0_EG_PRI0(x)		((x) << 0)
-- 
2.30.2

[PATCH 09/14] drivers: net: dsa: qca8k: add support for switch rev

[Ansuel Smith]

qca8k switch require some special debug value to be set based on the
switch revision. Rework the switch id read function to also read the
chip revision and make it accessible to the switch data.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 17 ++++++++++++-----
 drivers/net/dsa/qca8k.h |  7 +++++--
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index d469620e9344..20b507a35191 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1459,12 +1459,22 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,
 };
 
+static u8 qca8k_read_switch_id(struct qca8k_priv *priv)
+{
+	u32 val;
+
+	val = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
+
+	priv->switch_revision = (val & QCA8K_MASK_CTRL_REV_ID_MASK);
+
+	return QCA8K_MASK_CTRL_DEVICE_ID(val & QCA8K_MASK_CTRL_DEVICE_ID_MASK);
+}
+
 static int
 qca8k_sw_probe(struct mdio_device *mdiodev)
 {
 	const struct qca8k_match_data *data;
 	struct qca8k_priv *priv;
-	u32 id;
 
 	/* allocate the private data struct so that we can probe the switches
 	 * ID register
@@ -1496,10 +1506,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 		return -ENODEV;
 
 	/* read the switches ID register */
-	id = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
-	id >>= QCA8K_MASK_CTRL_ID_S;
-	id &= QCA8K_MASK_CTRL_ID_M;
-	if (id != data->id)
+	if (qca8k_read_switch_id(priv) != data->id)
 		return -ENODEV;
 
 	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 308d8410fdb6..dbd54d870a30 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -28,8 +28,10 @@
 
 /* Global control registers */
 #define QCA8K_REG_MASK_CTRL				0x000
-#define   QCA8K_MASK_CTRL_ID_M				0xff
-#define   QCA8K_MASK_CTRL_ID_S				8
+#define   QCA8K_MASK_CTRL_REV_ID_MASK			GENMASK(7, 0)
+#define   QCA8K_MASK_CTRL_REV_ID(x)			((x) >> 0)
+#define   QCA8K_MASK_CTRL_DEVICE_ID_MASK		GENMASK(15, 8)
+#define   QCA8K_MASK_CTRL_DEVICE_ID(x)			((x) >> 8)
 #define QCA8K_REG_PORT0_PAD_CTRL			0x004
 #define QCA8K_REG_PORT5_PAD_CTRL			0x008
 #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
@@ -247,6 +249,7 @@ struct qca8k_match_data {
 };
 
 struct qca8k_priv {
+	u8 switch_revision;
 	struct regmap *regmap;
 	struct mii_bus *bus;
 	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
-- 
2.30.2

[PATCH 10/14] drivers: net: dsa: qca8k: add support for specific QCA access function

[Ansuel Smith]

Some qca8k switch revision require some special dbg value to be set
based on the revision number. Add required function to write and read in
these specific registers.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 51 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  2 ++
 2 files changed, 53 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 20b507a35191..193c269d8ed3 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -69,6 +69,57 @@ static const struct qca8k_mib_desc ar8327_mib[] = {
 	MIB_DESC(1, 0xa4, "TxLateCol"),
 };
 
+/* QCA specific MII registers access function */
+void qca8k_phy_dbg_read(struct qca8k_priv *priv, int phy_addr, u16 dbg_addr, u16 *dbg_data)
+{
+	struct mii_bus *bus = priv->bus;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	bus->write(bus, phy_addr, MII_ATH_DBG_ADDR, dbg_addr);
+	*dbg_data = bus->read(bus, phy_addr, MII_ATH_DBG_DATA);
+	mutex_unlock(&bus->mdio_lock);
+}
+
+void qca8k_phy_dbg_write(struct qca8k_priv *priv, int phy_addr, u16 dbg_addr, u16 dbg_data)
+{
+	struct mii_bus *bus = priv->bus;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	bus->write(bus, phy_addr, MII_ATH_DBG_ADDR, dbg_addr);
+	bus->write(bus, phy_addr, MII_ATH_DBG_DATA, dbg_data);
+	mutex_unlock(&bus->mdio_lock);
+}
+
+static inline void qca8k_phy_mmd_prep(struct mii_bus *bus, int phy_addr, u16 addr, u16 reg)
+{
+	bus->write(bus, phy_addr, MII_ATH_MMD_ADDR, addr);
+	bus->write(bus, phy_addr, MII_ATH_MMD_DATA, reg);
+	bus->write(bus, phy_addr, MII_ATH_MMD_ADDR, addr | 0x4000);
+}
+
+void qca8k_phy_mmd_write(struct qca8k_priv *priv, int phy_addr, u16 addr, u16 reg, u16 data)
+{
+	struct mii_bus *bus = priv->bus;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	qca8k_phy_mmd_prep(bus, phy_addr, addr, reg);
+	bus->write(bus, phy_addr, MII_ATH_MMD_DATA, data);
+	mutex_unlock(&bus->mdio_lock);
+}
+
+u16 qca8k_phy_mmd_read(struct qca8k_priv *priv, int phy_addr, u16 addr, u16 reg)
+{
+	struct mii_bus *bus = priv->bus;
+	u16 data;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	qca8k_phy_mmd_prep(bus, phy_addr, addr, reg);
+	data = bus->read(bus, phy_addr, MII_ATH_MMD_DATA);
+	mutex_unlock(&bus->mdio_lock);
+
+	return data;
+}
+
 /* The 32bit switch registers are accessed indirectly. To achieve this we need
  * to set the page of the register. Track the last page that was set to reduce
  * mdio writes
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index dbd54d870a30..de00aa74868b 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -215,6 +215,8 @@
 /* QCA specific MII registers */
 #define MII_ATH_MMD_ADDR				0x0d
 #define MII_ATH_MMD_DATA				0x0e
+#define MII_ATH_DBG_ADDR				0x1d
+#define MII_ATH_DBG_DATA				0x1e
 
 enum {
 	QCA8K_PORT_SPEED_10M = 0,
-- 
2.30.2

Re: [PATCH 10/14] drivers: net: dsa: qca8k: add support for specific QCA access function

[Andrew Lunn]

> +static inline void qca8k_phy_mmd_prep(struct mii_bus *bus, int phy_addr, u16 addr, u16 reg)
> +{
> +	bus->write(bus, phy_addr, MII_ATH_MMD_ADDR, addr);
> +	bus->write(bus, phy_addr, MII_ATH_MMD_DATA, reg);
> +	bus->write(bus, phy_addr, MII_ATH_MMD_ADDR, addr | 0x4000);
> +}
> +
> +void qca8k_phy_mmd_write(struct qca8k_priv *priv, int phy_addr, u16 addr, u16 reg, u16 data)
> +{
> +	struct mii_bus *bus = priv->bus;
> +
> +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	qca8k_phy_mmd_prep(bus, phy_addr, addr, reg);
> +	bus->write(bus, phy_addr, MII_ATH_MMD_DATA, data);
> +	mutex_unlock(&bus->mdio_lock);
> +}
> +
> +u16 qca8k_phy_mmd_read(struct qca8k_priv *priv, int phy_addr, u16 addr, u16 reg)
> +{
> +	struct mii_bus *bus = priv->bus;
> +	u16 data;
> +
> +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	qca8k_phy_mmd_prep(bus, phy_addr, addr, reg);
> +	data = bus->read(bus, phy_addr, MII_ATH_MMD_DATA);
> +	mutex_unlock(&bus->mdio_lock);
> +
> +	return data;
> +}

Can you use the PHY core MMD access functions?

    Andrew

[PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[Ansuel Smith]

qca8k require special debug value based on the switch revision.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 193c269d8ed3..12d2c97d1417 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 {
 	const struct qca8k_match_data *data;
 	struct qca8k_priv *priv = ds->priv;
-	u32 reg, val;
+	u32 phy, reg, val;
 
 	/* get the switches ID from the compatible */
 	data = of_device_get_match_data(priv->dev);
@@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	case 3:
 	case 4:
 	case 5:
-		/* Internal PHY, nothing to do */
+		/* Internal PHY, apply revision fixup */
+		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
+		switch (priv->switch_revision) {
+		case 1:
+			/* For 100M waveform */
+			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);
+			/* Turn on Gigabit clock */
+			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);
+			break;
+
+		case 2:
+			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);
+			fallthrough;
+		case 4:
+			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);
+			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);
+			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);
+			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);
+			break;
+		}
 		return;
 	case 6: /* 2nd CPU port / external PHY */
 		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
-- 
2.30.2

Re: [PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[Florian Fainelli]

On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> qca8k require special debug value based on the switch revision.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 193c269d8ed3..12d2c97d1417 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  {
>  	const struct qca8k_match_data *data;
>  	struct qca8k_priv *priv = ds->priv;
> -	u32 reg, val;
> +	u32 phy, reg, val;
>  
>  	/* get the switches ID from the compatible */
>  	data = of_device_get_match_data(priv->dev);
> @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  	case 3:
>  	case 4:
>  	case 5:
> -		/* Internal PHY, nothing to do */
> +		/* Internal PHY, apply revision fixup */
> +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> +		switch (priv->switch_revision) {
> +		case 1:
> +			/* For 100M waveform */
> +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);
> +			/* Turn on Gigabit clock */
> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);
> +			break;
> +
> +		case 2:
> +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);
> +			fallthrough;
> +		case 4:
> +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);
> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);
> +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);
> +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);
> +			break;

This would be better done with a PHY driver that is specific to the
integrated PHY found in these switches, it would provide a nice clean
layer and would allow you to expose additional features like cable
tests, PHY statistics/counters, etc.
-- 
Florian

Re: [PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[Ansuel Smith]

On Thu, Apr 22, 2021 at 07:02:37PM -0700, Florian Fainelli wrote:
> 
> 
> On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> > qca8k require special debug value based on the switch revision.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 193c269d8ed3..12d2c97d1417 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  {
> >  	const struct qca8k_match_data *data;
> >  	struct qca8k_priv *priv = ds->priv;
> > -	u32 reg, val;
> > +	u32 phy, reg, val;
> >  
> >  	/* get the switches ID from the compatible */
> >  	data = of_device_get_match_data(priv->dev);
> > @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  	case 3:
> >  	case 4:
> >  	case 5:
> > -		/* Internal PHY, nothing to do */
> > +		/* Internal PHY, apply revision fixup */
> > +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> > +		switch (priv->switch_revision) {
> > +		case 1:
> > +			/* For 100M waveform */
> > +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);
> > +			/* Turn on Gigabit clock */
> > +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);
> > +			break;
> > +
> > +		case 2:
> > +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);
> > +			fallthrough;
> > +		case 4:
> > +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);
> > +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);
> > +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);
> > +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);
> > +			break;
> 
> This would be better done with a PHY driver that is specific to the
> integrated PHY found in these switches, it would provide a nice clean
> layer and would allow you to expose additional features like cable
> tests, PHY statistics/counters, etc.

I'm starting to do some work with this and a problem arised. Since these
value are based on the switch revision, how can I access these kind of
data from the phy driver? It's allowed to declare a phy driver in the
dsa directory? (The idea would be to create a qca8k dir with the dsa
driver and the dedicated internal phy driver.) This would facilitate the
use of normal qca8k_read/write (to access the switch revision from the
phy driver) using common function?

> -- 
> Florian

Re: [PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[Heiner Kallweit]

On 24.04.2021 23:18, Ansuel Smith wrote:
> On Thu, Apr 22, 2021 at 07:02:37PM -0700, Florian Fainelli wrote:
>>
>>
>> On 4/22/2021 6:47 PM, Ansuel Smith wrote:
>>> qca8k require special debug value based on the switch revision.
>>>
>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>> ---
>>>  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--
>>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
>>> index 193c269d8ed3..12d2c97d1417 100644
>>> --- a/drivers/net/dsa/qca8k.c
>>> +++ b/drivers/net/dsa/qca8k.c
>>> @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>>  {
>>>  	const struct qca8k_match_data *data;
>>>  	struct qca8k_priv *priv = ds->priv;
>>> -	u32 reg, val;
>>> +	u32 phy, reg, val;
>>>  
>>>  	/* get the switches ID from the compatible */
>>>  	data = of_device_get_match_data(priv->dev);
>>> @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>>  	case 3:
>>>  	case 4:
>>>  	case 5:
>>> -		/* Internal PHY, nothing to do */
>>> +		/* Internal PHY, apply revision fixup */
>>> +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
>>> +		switch (priv->switch_revision) {
>>> +		case 1:
>>> +			/* For 100M waveform */
>>> +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);
>>> +			/* Turn on Gigabit clock */
>>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);
>>> +			break;
>>> +
>>> +		case 2:
>>> +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);

Please replace the magic numbers with constants. Here even standard
registers are used: 0x7 = MDIO_MMD_AN, 0x3c = MDIO_AN_EEE_ADV
Effectively EEE advertisement is disabled.

>>> +			fallthrough;
>>> +		case 4:
>>> +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);
>>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);
>>> +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);
>>> +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);
>>> +			break;
>>
>> This would be better done with a PHY driver that is specific to the
>> integrated PHY found in these switches, it would provide a nice clean
>> layer and would allow you to expose additional features like cable
>> tests, PHY statistics/counters, etc.
> 
> I'm starting to do some work with this and a problem arised. Since these
> value are based on the switch revision, how can I access these kind of
> data from the phy driver? It's allowed to declare a phy driver in the
> dsa directory? (The idea would be to create a qca8k dir with the dsa
> driver and the dedicated internal phy driver.) This would facilitate the
> use of normal qca8k_read/write (to access the switch revision from the
> phy driver) using common function?
> 

PHY drivers reside under drivers/net/phy. Not sure whether your internal
PHY's have a proper PHY ID, you could assign pseudo PHY ID's differing
per switch revision. See mv88e6xxx_mdio_read() for a similar use case.

>> -- 
>> Florian

Heiner

Re: [PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[Florian Fainelli]

On 4/24/2021 2:18 PM, Ansuel Smith wrote:
> On Thu, Apr 22, 2021 at 07:02:37PM -0700, Florian Fainelli wrote:
>>
>>
>> On 4/22/2021 6:47 PM, Ansuel Smith wrote:
>>> qca8k require special debug value based on the switch revision.
>>>
>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>> ---
>>>  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--
>>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
>>> index 193c269d8ed3..12d2c97d1417 100644
>>> --- a/drivers/net/dsa/qca8k.c
>>> +++ b/drivers/net/dsa/qca8k.c
>>> @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>>  {
>>>  	const struct qca8k_match_data *data;
>>>  	struct qca8k_priv *priv = ds->priv;
>>> -	u32 reg, val;
>>> +	u32 phy, reg, val;
>>>  
>>>  	/* get the switches ID from the compatible */
>>>  	data = of_device_get_match_data(priv->dev);
>>> @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>>  	case 3:
>>>  	case 4:
>>>  	case 5:
>>> -		/* Internal PHY, nothing to do */
>>> +		/* Internal PHY, apply revision fixup */
>>> +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
>>> +		switch (priv->switch_revision) {
>>> +		case 1:
>>> +			/* For 100M waveform */
>>> +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);
>>> +			/* Turn on Gigabit clock */
>>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);
>>> +			break;
>>> +
>>> +		case 2:
>>> +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);
>>> +			fallthrough;
>>> +		case 4:
>>> +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);
>>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);
>>> +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);
>>> +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);
>>> +			break;
>>
>> This would be better done with a PHY driver that is specific to the
>> integrated PHY found in these switches, it would provide a nice clean
>> layer and would allow you to expose additional features like cable
>> tests, PHY statistics/counters, etc.
> 
> I'm starting to do some work with this and a problem arised. Since these
> value are based on the switch revision, how can I access these kind of
> data from the phy driver? It's allowed to declare a phy driver in the
> dsa directory? (The idea would be to create a qca8k dir with the dsa
> driver and the dedicated internal phy driver.) This would facilitate the
> use of normal qca8k_read/write (to access the switch revision from the
> phy driver) using common function?

The PHY driver should live under drivers/net/phy/ and if you need to
communicate the switch revision to the PHY driver you can use
phydev->dev_flags and implement a dsa_switch_ops::get_phy_flags()
callback and define a custom bitmask.

As far as the read/write operations if your switch implements a custom
mii_bus for the purpose of doing all of the underlying indirect register
accesses, then you should be fine. A lot of drivers do that however if
you want an example of both (communicating something to the PHY driver
and having a custom MII bus) you can look at drivers/net/dsa/bcm_sf2.c
for an example.
-- 
Florian

Re: [PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[Ansuel Smith]

On Sat, Apr 24, 2021 at 06:09:27PM -0700, Florian Fainelli wrote:
> 
> 
> On 4/24/2021 2:18 PM, Ansuel Smith wrote:
> > On Thu, Apr 22, 2021 at 07:02:37PM -0700, Florian Fainelli wrote:
> >>
> >>
> >> On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> >>> qca8k require special debug value based on the switch revision.
> >>>
> >>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> >>> ---
> >>>  drivers/net/dsa/qca8k.c | 23 +++++++++++++++++++++--
> >>>  1 file changed, 21 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> >>> index 193c269d8ed3..12d2c97d1417 100644
> >>> --- a/drivers/net/dsa/qca8k.c
> >>> +++ b/drivers/net/dsa/qca8k.c
> >>> @@ -909,7 +909,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >>>  {
> >>>  	const struct qca8k_match_data *data;
> >>>  	struct qca8k_priv *priv = ds->priv;
> >>> -	u32 reg, val;
> >>> +	u32 phy, reg, val;
> >>>  
> >>>  	/* get the switches ID from the compatible */
> >>>  	data = of_device_get_match_data(priv->dev);
> >>> @@ -928,7 +928,26 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >>>  	case 3:
> >>>  	case 4:
> >>>  	case 5:
> >>> -		/* Internal PHY, nothing to do */
> >>> +		/* Internal PHY, apply revision fixup */
> >>> +		phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> >>> +		switch (priv->switch_revision) {
> >>> +		case 1:
> >>> +			/* For 100M waveform */
> >>> +			qca8k_phy_dbg_write(priv, phy, 0, 0x02ea);
> >>> +			/* Turn on Gigabit clock */
> >>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x68a0);
> >>> +			break;
> >>> +
> >>> +		case 2:
> >>> +			qca8k_phy_mmd_write(priv, phy, 0x7, 0x3c, 0x0);
> >>> +			fallthrough;
> >>> +		case 4:
> >>> +			qca8k_phy_mmd_write(priv, phy, 0x3, 0x800d, 0x803f);
> >>> +			qca8k_phy_dbg_write(priv, phy, 0x3d, 0x6860);
> >>> +			qca8k_phy_dbg_write(priv, phy, 0x5, 0x2c46);
> >>> +			qca8k_phy_dbg_write(priv, phy, 0x3c, 0x6000);
> >>> +			break;
> >>
> >> This would be better done with a PHY driver that is specific to the
> >> integrated PHY found in these switches, it would provide a nice clean
> >> layer and would allow you to expose additional features like cable
> >> tests, PHY statistics/counters, etc.
> > 
> > I'm starting to do some work with this and a problem arised. Since these
> > value are based on the switch revision, how can I access these kind of
> > data from the phy driver? It's allowed to declare a phy driver in the
> > dsa directory? (The idea would be to create a qca8k dir with the dsa
> > driver and the dedicated internal phy driver.) This would facilitate the
> > use of normal qca8k_read/write (to access the switch revision from the
> > phy driver) using common function?
> 
> The PHY driver should live under drivers/net/phy/ and if you need to
> communicate the switch revision to the PHY driver you can use
> phydev->dev_flags and implement a dsa_switch_ops::get_phy_flags()
> callback and define a custom bitmask.
> 
> As far as the read/write operations if your switch implements a custom
> mii_bus for the purpose of doing all of the underlying indirect register
> accesses, then you should be fine. A lot of drivers do that however if
> you want an example of both (communicating something to the PHY driver
> and having a custom MII bus) you can look at drivers/net/dsa/bcm_sf2.c
> for an example.
> -- 
> Florian

Thanks a lot for the suggestions. Will send v2 to the net-next branch
hoping I did all the correct way. 

Re: [PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[DENG Qingfang]

Hi Ansuel,

On Sat, Apr 24, 2021 at 11:18:20PM +0200, Ansuel Smith wrote:
> 
> I'm starting to do some work with this and a problem arised. Since these
> value are based on the switch revision, how can I access these kind of
> data from the phy driver? It's allowed to declare a phy driver in the
> dsa directory? (The idea would be to create a qca8k dir with the dsa
> driver and the dedicated internal phy driver.) This would facilitate the
> use of normal qca8k_read/write (to access the switch revision from the
> phy driver) using common function?

In case of different switch revision, the PHY ID should also be different.
I think you can reuse the current at803x.c PHY driver, as they seem to
share similar registers.

> 
> > -- 
> > Florian

Re: [PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[Ansuel Smith]

On Sun, Apr 25, 2021 at 12:45:54PM +0800, DENG Qingfang wrote:
> Hi Ansuel,
> 
> On Sat, Apr 24, 2021 at 11:18:20PM +0200, Ansuel Smith wrote:
> > 
> > I'm starting to do some work with this and a problem arised. Since these
> > value are based on the switch revision, how can I access these kind of
> > data from the phy driver? It's allowed to declare a phy driver in the
> > dsa directory? (The idea would be to create a qca8k dir with the dsa
> > driver and the dedicated internal phy driver.) This would facilitate the
> > use of normal qca8k_read/write (to access the switch revision from the
> > phy driver) using common function?
> 
> In case of different switch revision, the PHY ID should also be different.
> I think you can reuse the current at803x.c PHY driver, as they seem to
> share similar registers.
>

Is this really necessary? Every PHY has the same ID linked to the switch
id but the revision can change across the same switch id. Isn't the phy
dev flag enought to differiante one id from another? 

> > 
> > > -- 
> > > Florian

Re: [PATCH 11/14] drivers: net: dsa: qca8k: apply switch revision fix

[Andrew Lunn]

On Sun, Apr 25, 2021 at 01:59:19PM +0200, Ansuel Smith wrote:
> On Sun, Apr 25, 2021 at 12:45:54PM +0800, DENG Qingfang wrote:
> > Hi Ansuel,
> > 
> > On Sat, Apr 24, 2021 at 11:18:20PM +0200, Ansuel Smith wrote:
> > > 
> > > I'm starting to do some work with this and a problem arised. Since these
> > > value are based on the switch revision, how can I access these kind of
> > > data from the phy driver? It's allowed to declare a phy driver in the
> > > dsa directory? (The idea would be to create a qca8k dir with the dsa
> > > driver and the dedicated internal phy driver.) This would facilitate the
> > > use of normal qca8k_read/write (to access the switch revision from the
> > > phy driver) using common function?
> > 
> > In case of different switch revision, the PHY ID should also be different.
> > I think you can reuse the current at803x.c PHY driver, as they seem to
> > share similar registers.
> >
> 
> Is this really necessary? Every PHY has the same ID linked to the switch
> id but the revision can change across the same switch id. Isn't the phy
> dev flag enought to differiante one id from another? 

Just as general background information: A PHY ID generally consists of
three parts.

1) OUI - Identifies the manufacture - 22 bits
2) device - Generally 6 bits
3) revision - Generally 4 bits

The 22 bits of OUI is standardized. But the last 10 bits the vendor
can use as they wish. But generally, this is how it is used.

Loading the PHY driver is generally based on matching the OUI and
device ID. The revision is ignored. But it is available to the driver
if needed.

It could be, the switch revision is also reflected in the PHY
revision.

	Andrew

[PATCH 12/14] drivers: net: dsa: qca8k: clear MASTER_EN after phy read/write

[Ansuel Smith]

Clear MDIO_MASTER_EN bit from MDIO_MASTER_CTRL after read/write
operation. The MDIO_MASTER_EN bit is not reset after read/write
operation and the next operation can be wrongly interpreted by the
switch as a mdio operation. This cause a production of wrong/garbage
data from the switch and underfined bheavior. (random port drop,
unplugged port flagged with link up, wrong port speed)

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 12d2c97d1417..88a0234f1a7b 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -613,6 +613,7 @@ static int
 qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 {
 	u32 phy, val;
+	int ret;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -628,8 +629,13 @@ qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 
 	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
 
-	return qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
-		QCA8K_MDIO_MASTER_BUSY);
+	ret = qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+			      QCA8K_MDIO_MASTER_BUSY);
+
+	qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
+			QCA8K_MDIO_MASTER_EN);
+
+	return ret;
 }
 
 static int
@@ -657,6 +663,9 @@ qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
 	val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
 		QCA8K_MDIO_MASTER_DATA_MASK);
 
+	qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
+			QCA8K_MDIO_MASTER_EN);
+
 	return val;
 }
 
-- 
2.30.2

[PATCH 13/14] drivers: net: dsa: qca8k: protect MASTER busy_wait with mdio mutex

[Ansuel Smith]

MDIO_MASTER operation have a dedicated busy wait that is not protected
by the mdio mutex. This can cause situation where the MASTER operation
is done and a normal operation is executed between the MASTER read/write
and the MASTER busy_wait. Rework the qca8k_mdio_read/write function to
address this issue by binding the lock for the whole MASTER operation
and not only the mdio read/write common operation.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 59 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 88a0234f1a7b..d2f5e0b1c721 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -609,9 +609,33 @@ qca8k_port_to_phy(int port)
 	return port - 1;
 }
 
+static int
+qca8k_mdio_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
+{
+	unsigned long timeout;
+	u16 r1, r2, page;
+
+	qca8k_split_addr(reg, &r1, &r2, &page);
+
+	timeout = jiffies + msecs_to_jiffies(20);
+
+	/* loop until the busy flag has cleared */
+	do {
+		u32 val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
+		int busy = val & mask;
+
+		if (!busy)
+			break;
+		cond_resched();
+	} while (!time_after_eq(jiffies, timeout));
+
+	return time_after_eq(jiffies, timeout);
+}
+
 static int
 qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 {
+	u16 r1, r2, page;
 	u32 phy, val;
 	int ret;
 
@@ -627,10 +651,17 @@ qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 	      QCA8K_MDIO_MASTER_REG_ADDR(regnum) |
 	      QCA8K_MDIO_MASTER_DATA(data);
 
-	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+	qca8k_split_addr(QCA8K_MDIO_MASTER_CTRL, &r1, &r2, &page);
+
+	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	qca8k_set_page(priv->bus, page);
+	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
 
-	ret = qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
-			      QCA8K_MDIO_MASTER_BUSY);
+	ret = qca8k_mdio_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+				   QCA8K_MDIO_MASTER_BUSY);
+
+	mutex_unlock(&priv->bus->mdio_lock);
 
 	qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
 			QCA8K_MDIO_MASTER_EN);
@@ -641,6 +672,7 @@ qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 static int
 qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
 {
+	u16 r1, r2, page;
 	u32 phy, val;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
@@ -654,14 +686,23 @@ qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
 	      QCA8K_MDIO_MASTER_READ | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
 	      QCA8K_MDIO_MASTER_REG_ADDR(regnum);
 
-	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+	qca8k_split_addr(QCA8K_MDIO_MASTER_CTRL, &r1, &r2, &page);
 
-	if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
-			    QCA8K_MDIO_MASTER_BUSY))
-		return -ETIMEDOUT;
+	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	qca8k_set_page(priv->bus, page);
+	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
+
+	if (qca8k_mdio_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+				 QCA8K_MDIO_MASTER_BUSY))
+		val = -ETIMEDOUT;
+	else
+		val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
+
+	mutex_unlock(&priv->bus->mdio_lock);
 
-	val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
-		QCA8K_MDIO_MASTER_DATA_MASK);
+	if (val >= 0)
+		val &= QCA8K_MDIO_MASTER_DATA_MASK;
 
 	qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
 			QCA8K_MDIO_MASTER_EN);
-- 
2.30.2

Re: [PATCH 13/14] drivers: net: dsa: qca8k: protect MASTER busy_wait with mdio mutex

[Andrew Lunn]

On Fri, Apr 23, 2021 at 03:47:39AM +0200, Ansuel Smith wrote:
> MDIO_MASTER operation have a dedicated busy wait that is not protected
> by the mdio mutex. This can cause situation where the MASTER operation
> is done and a normal operation is executed between the MASTER read/write
> and the MASTER busy_wait. Rework the qca8k_mdio_read/write function to
> address this issue by binding the lock for the whole MASTER operation
> and not only the mdio read/write common operation.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 59 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 88a0234f1a7b..d2f5e0b1c721 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -609,9 +609,33 @@ qca8k_port_to_phy(int port)
>  	return port - 1;
>  }
>  
> +static int
> +qca8k_mdio_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
> +{
> +	unsigned long timeout;
> +	u16 r1, r2, page;
> +
> +	qca8k_split_addr(reg, &r1, &r2, &page);
> +
> +	timeout = jiffies + msecs_to_jiffies(20);
> +
> +	/* loop until the busy flag has cleared */
> +	do {
> +		u32 val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
> +		int busy = val & mask;
> +
> +		if (!busy)
> +			break;
> +		cond_resched();
> +	} while (!time_after_eq(jiffies, timeout));
> +
> +	return time_after_eq(jiffies, timeout);

You should really be returning -ETIMEDOUT here on error.

    Andrew

[PATCH 14/14] drivers: net: dsa: qca8k: enlarge mdio delay and timeout

[Ansuel Smith]

- Enlarge set page delay to QDSK source
- Enlarge mdio MASTER timeout busy wait

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index d2f5e0b1c721..2ed1d5e283c2 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -191,6 +191,7 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
 	}
 
 	qca8k_current_page = page;
+	usleep_range(1000, 2000);
 }
 
 static u32
@@ -617,7 +618,7 @@ qca8k_mdio_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	timeout = jiffies + msecs_to_jiffies(20);
+	timeout = jiffies + msecs_to_jiffies(2000);
 
 	/* loop until the busy flag has cleared */
 	do {
-- 
2.30.2

Re: [PATCH 00/14] Multiple improvement to qca8k stability

[Florian Fainelli]

On 4/22/2021 6:47 PM, Ansuel Smith wrote:
> Currently qca8337 switch are widely used on ipq8064 based router.
> On these particular router it was notice a very unstable switch with
> port not link detected as link with unknown speed, port dropping
> randomly and general unreliability. Lots of testing and comparison
> between this dsa driver and the original qsdk driver showed lack of some
> additional delay and values. A main difference arised from the original
> driver and the dsa one. The original driver didn't use MASTER regs to
> read phy status and the dedicated mdio driver worked correctly. Now that
> the dsa driver actually use these regs, it was found that these special
> read/write operation required mutual exclusion to normal
> qca8k_read/write operation. The add of mutex for these operation fixed
> the random port dropping and now only the actual linked port randomly
> dropped. Adding additional delay for set_page operation and fixing a bug
> in the mdio dedicated driver fixed also this problem. The current driver
> requires also more time to apply vlan switch. All of these changes and
> tweak permit a now very stable and reliable dsa driver and 0 port
> dropping. This series is currently tested by at least 5 user with
> different routers and all reports positive results and no problems.

Since all of these changes are improvements and not really bug fixes,
please target them at the net-next tree:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst#n40

also, the subject for your patches should just be:

net: dsa: qca8k:
net: mdio: mdio-ipq8064:

to be consistent with previous submissions in these files.

> 
> Ansuel Smith (14):
>   drivers: net: dsa: qca8k: handle error with set_page
>   drivers: net: dsa: qca8k: tweak internal delay to oem spec
>   drivers: net: mdio: mdio-ip8064: improve busy wait delay
>   drivers: net: dsa: qca8k: apply suggested packet priority
>   drivers: net: dsa: qca8k: add support for qca8327 switch
>   devicetree: net: dsa: qca8k: Document new compatible qca8327
>   drivers: net: dsa: qca8k: limit priority tweak to qca8337 switch
>   drivers: net: dsa: qca8k: add GLOBAL_FC settings needed for qca8327
>   drivers: net: dsa: qca8k: add support for switch rev
>   drivers: net: dsa: qca8k: add support for specific QCA access function
>   drivers: net: dsa: qca8k: apply switch revision fix
>   drivers: net: dsa: qca8k: clear MASTER_EN after phy read/write
>   drivers: net: dsa: qca8k: protect MASTER busy_wait with mdio mutex
>   drivers: net: dsa: qca8k: enlarge mdio delay and timeout
> 
>  .../devicetree/bindings/net/dsa/qca8k.txt     |   1 +
>  drivers/net/dsa/qca8k.c                       | 256 ++++++++++++++++--
>  drivers/net/dsa/qca8k.h                       |  54 +++-
>  drivers/net/mdio/mdio-ipq8064.c               |  36 ++-
>  4 files changed, 304 insertions(+), 43 deletions(-)
> 

-- 
Florian