[RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Ansuel Smith]

Fix mixed whitespace and tab for define spacing.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/mdio/mdio-ipq8064.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c
index 1bd18857e1c5..fb1614242e13 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 */
-- 
2.30.2

[RFC PATCH net-next v4 02/28] net: mdio: ipq8064: add regmap config to disable REGCACHE

[Ansuel Smith]

mdio drivers should not use REGCHACHE. Also disable locking since it's
handled by the mdio users and regmap is always accessed atomically.

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

diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c
index fb1614242e13..9862745d1cee 100644
--- a/drivers/net/mdio/mdio-ipq8064.c
+++ b/drivers/net/mdio/mdio-ipq8064.c
@@ -10,9 +10,8 @@
 #include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/of_mdio.h>
-#include <linux/phy.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
-#include <linux/mfd/syscon.h>
 
 /* MII address register definitions */
 #define MII_ADDR_REG_ADDR			0x10
@@ -97,14 +96,34 @@ ipq8064_mdio_write(struct mii_bus *bus, int phy_addr, int reg_offset, u16 data)
 	return ipq8064_mdio_wait_busy(priv);
 }
 
+static const struct regmap_config ipq8064_mdio_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.can_multi_write = false,
+	/* the mdio lock is used by any user of this mdio driver */
+	.disable_locking = true,
+
+	.cache_type = REGCACHE_NONE,
+};
+
 static int
 ipq8064_mdio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct ipq8064_mdio *priv;
+	struct resource res;
 	struct mii_bus *bus;
+	void __iomem *base;
 	int ret;
 
+	if (of_address_to_resource(np, 0, &res))
+		return -ENOMEM;
+
+	base = ioremap(res.start, resource_size(&res));
+	if (!base)
+		return -ENOMEM;
+
 	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
 	if (!bus)
 		return -ENOMEM;
@@ -116,15 +135,10 @@ ipq8064_mdio_probe(struct platform_device *pdev)
 	bus->parent = &pdev->dev;
 
 	priv = bus->priv;
-	priv->base = device_node_to_regmap(np);
-	if (IS_ERR(priv->base)) {
-		if (priv->base == ERR_PTR(-EPROBE_DEFER))
-			return -EPROBE_DEFER;
-
-		dev_err(&pdev->dev, "error getting device regmap, error=%pe\n",
-			priv->base);
+	priv->base = devm_regmap_init_mmio(&pdev->dev, base,
+					   &ipq8064_mdio_regmap_config);
+	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
-	}
 
 	ret = of_mdiobus_register(bus, np);
 	if (ret)
-- 
2.30.2

Re: [RFC PATCH net-next v4 02/28] net: mdio: ipq8064: add regmap config to disable REGCACHE

[Florian Fainelli]

On 5/7/2021 5:28 PM, Ansuel Smith wrote:
> mdio drivers should not use REGCHACHE. Also disable locking since it's
> handled by the mdio users and regmap is always accessed atomically.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[RFC PATCH net-next v4 03/28] net: mdio: ipq8064: enlarge sleep after read/write operation

[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 behave 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 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c
index 9862745d1cee..a3f7f9de12b6 100644
--- a/drivers/net/mdio/mdio-ipq8064.c
+++ b/drivers/net/mdio/mdio-ipq8064.c
@@ -65,7 +65,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 +91,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 garbage data.
+	 */
+	if (reg_offset == 31)
+		usleep_range(30, 43);
+	else
+		usleep_range(10, 13);
 
 	return ipq8064_mdio_wait_busy(priv);
 }
-- 
2.30.2

Re: [RFC PATCH net-next v4 03/28] net: mdio: ipq8064: enlarge sleep after read/write operation

[Florian Fainelli]

On 5/7/2021 5:28 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 behave 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

I am still curious whether the problem is that you have lots of traffic
going through the same bus fabric (AXI?) and that eventually puts the
register accesses to a lower priority to get through. We would most
likely need someone from QCA to tell if this is even remotely a
possibility and this is unlikely to happen.
-- 
Florian

[RFC PATCH net-next v4 04/28] net: dsa: qca8k: change simple print to dev variant

[Ansuel Smith]

Change pr_err and pr_warn to dev variant.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index cdaf9f85a2cb..0b295da6c356 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -701,7 +701,7 @@ qca8k_setup(struct dsa_switch *ds)
 
 	/* 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");
+		dev_err(priv->dev, "port 0 is not the CPU port");
 		return -EINVAL;
 	}
 
@@ -711,7 +711,7 @@ qca8k_setup(struct dsa_switch *ds)
 	priv->regmap = devm_regmap_init(ds->dev, NULL, priv,
 					&qca8k_regmap_config);
 	if (IS_ERR(priv->regmap))
-		pr_warn("regmap initialization failed");
+		dev_warn(priv->dev, "regmap initialization failed");
 
 	ret = qca8k_setup_mdio_bus(priv);
 	if (ret)
-- 
2.30.2

Re: [RFC PATCH net-next v4 04/28] net: dsa: qca8k: change simple print to dev variant

[Florian Fainelli]

On 5/7/2021 5:28 PM, Ansuel Smith wrote:
> Change pr_err and pr_warn to dev variant.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[RFC PATCH net-next v4 05/28] net: dsa: qca8k: use iopoll macro for qca8k_busy_wait

[Ansuel Smith]

Use iopoll macro instead of while loop.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 0b295da6c356..0bfb7ae4c128 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -262,21 +262,20 @@ static struct regmap_config qca8k_regmap_config = {
 static int
 qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 {
-	unsigned long timeout;
-
-	timeout = jiffies + msecs_to_jiffies(20);
+	u32 val;
+	int ret;
 
-	/* loop until the busy flag has cleared */
-	do {
-		u32 val = qca8k_read(priv, reg);
-		int busy = val & mask;
+	ret = read_poll_timeout(qca8k_read, val, !(val & mask),
+				0, QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
+				priv, reg);
 
-		if (!busy)
-			break;
-		cond_resched();
-	} while (!time_after_eq(jiffies, timeout));
+	/* Check if qca8k_read has failed for a different reason
+	 * before returnting -ETIMEDOUT
+	 */
+	if (ret < 0 && val < 0)
+		return val;
 
-	return time_after_eq(jiffies, timeout);
+	return ret;
 }
 
 static void
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 7ca4b93e0bb5..86c585b7ec4a 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -18,6 +18,8 @@
 #define PHY_ID_QCA8337					0x004dd036
 #define QCA8K_ID_QCA8337				0x13
 
+#define QCA8K_BUSY_WAIT_TIMEOUT				20
+
 #define QCA8K_NUM_FDB_RECORDS				2048
 
 #define QCA8K_CPU_PORT					0
-- 
2.30.2

Re: [RFC PATCH net-next v4 05/28] net: dsa: qca8k: use iopoll macro for qca8k_busy_wait

[Andrew Lunn]

On Sat, May 08, 2021 at 02:28:55AM +0200, Ansuel Smith wrote:
> Use iopoll macro instead of while loop.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 23 +++++++++++------------
>  drivers/net/dsa/qca8k.h |  2 ++
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 0b295da6c356..0bfb7ae4c128 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -262,21 +262,20 @@ static struct regmap_config qca8k_regmap_config = {
>  static int
>  qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
>  {
> -	unsigned long timeout;
> -
> -	timeout = jiffies + msecs_to_jiffies(20);
> +	u32 val;
> +	int ret;
>  
> -	/* loop until the busy flag has cleared */
> -	do {
> -		u32 val = qca8k_read(priv, reg);
> -		int busy = val & mask;
> +	ret = read_poll_timeout(qca8k_read, val, !(val & mask),
> +				0, QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
> +				priv, reg);
>  
> -		if (!busy)
> -			break;
> -		cond_resched();
> -	} while (!time_after_eq(jiffies, timeout));
> +	/* Check if qca8k_read has failed for a different reason
> +	 * before returnting -ETIMEDOUT

returning

With that fixed

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

    Andrew

[RFC PATCH net-next v4 06/28] net: dsa: qca8k: improve qca8k read/write/rmw bus access

[Ansuel Smith]

Put bus in local variable to improve faster access to the mdio bus.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 0bfb7ae4c128..f58d4543ef1e 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -142,17 +142,18 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
 static u32
 qca8k_read(struct qca8k_priv *priv, u32 reg)
 {
+	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	qca8k_set_page(priv->bus, page);
-	val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
+	qca8k_set_page(bus, page);
+	val = qca8k_mii_read32(bus, 0x10 | r2, r1);
 
-	mutex_unlock(&priv->bus->mdio_lock);
+	mutex_unlock(&bus->mdio_lock);
 
 	return val;
 }
@@ -160,35 +161,37 @@ qca8k_read(struct qca8k_priv *priv, u32 reg)
 static void
 qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 {
+	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	qca8k_set_page(priv->bus, page);
-	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
+	qca8k_set_page(bus, page);
+	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
 
-	mutex_unlock(&priv->bus->mdio_lock);
+	mutex_unlock(&bus->mdio_lock);
 }
 
 static u32
 qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
 {
+	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 ret;
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	qca8k_set_page(priv->bus, page);
-	ret = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
+	qca8k_set_page(bus, page);
+	ret = qca8k_mii_read32(bus, 0x10 | r2, r1);
 	ret &= ~mask;
 	ret |= val;
-	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, ret);
+	qca8k_mii_write32(bus, 0x10 | r2, r1, ret);
 
-	mutex_unlock(&priv->bus->mdio_lock);
+	mutex_unlock(&bus->mdio_lock);
 
 	return ret;
 }
-- 
2.30.2

Re: [RFC PATCH net-next v4 06/28] net: dsa: qca8k: improve qca8k read/write/rmw bus access

[Andrew Lunn]

On Sat, May 08, 2021 at 02:28:56AM +0200, Ansuel Smith wrote:
> Put bus in local variable to improve faster access to the mdio bus.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

    Andrew

[RFC PATCH net-next v4 07/28] net: dsa: qca8k: handle qca8k_set_page errors

[Ansuel Smith]

With a remote possibility, the set_page function can fail. Since this is
a critical part of the write/read qca8k regs, propagate the error and
terminate any read/write operation.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index f58d4543ef1e..1c52d1b262f1 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -127,16 +127,23 @@ qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
 				    "failed to write qca8k 32bit register\n");
 }
 
-static void
+static int
 qca8k_set_page(struct mii_bus *bus, u16 page)
 {
+	int ret;
+
 	if (page == qca8k_current_page)
-		return;
+		return 0;
 
-	if (bus->write(bus, 0x18, 0, page) < 0)
+	ret = bus->write(bus, 0x18, 0, page);
+	if (ret < 0) {
 		dev_err_ratelimited(&bus->dev,
 				    "failed to set qca8k page\n");
+		return ret;
+	}
+
 	qca8k_current_page = page;
+	return 0;
 }
 
 static u32
@@ -150,11 +157,14 @@ qca8k_read(struct qca8k_priv *priv, u32 reg)
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	qca8k_set_page(bus, page);
+	val = qca8k_set_page(bus, page);
+	if (val < 0)
+		goto exit;
+
 	val = qca8k_mii_read32(bus, 0x10 | r2, r1);
 
+exit:
 	mutex_unlock(&bus->mdio_lock);
-
 	return val;
 }
 
@@ -163,14 +173,19 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 {
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
+	int ret;
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	qca8k_set_page(bus, page);
+	ret = qca8k_set_page(bus, page);
+	if (ret < 0)
+		goto exit;
+
 	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
 
+exit:
 	mutex_unlock(&bus->mdio_lock);
 }
 
@@ -185,12 +200,16 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	qca8k_set_page(bus, page);
+	ret = qca8k_set_page(bus, page);
+	if (ret < 0)
+		goto exit;
+
 	ret = qca8k_mii_read32(bus, 0x10 | r2, r1);
 	ret &= ~mask;
 	ret |= val;
 	qca8k_mii_write32(bus, 0x10 | r2, r1, ret);
 
+exit:
 	mutex_unlock(&bus->mdio_lock);
 
 	return ret;
-- 
2.30.2

Re: [RFC PATCH net-next v4 07/28] net: dsa: qca8k: handle qca8k_set_page errors

[Andrew Lunn]

On Sat, May 08, 2021 at 02:28:57AM +0200, Ansuel Smith wrote:
> With a remote possibility, the set_page function can fail. Since this is
> a critical part of the write/read qca8k regs, propagate the error and
> terminate any read/write operation.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

    Andrew

[RFC PATCH net-next v4 08/28] net: dsa: qca8k: handle error with qca8k_read operation

[Ansuel Smith]

qca8k_read can fail. Rework any user to handle error values and
correctly return.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 1c52d1b262f1..5087ee8ecebb 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -231,8 +231,13 @@ static int
 qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	int ret;
+
+	ret = qca8k_read(priv, reg);
+	if (ret < 0)
+		return ret;
 
-	*val = qca8k_read(priv, reg);
+	*val = ret;
 
 	return 0;
 }
@@ -300,15 +305,20 @@ qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 	return ret;
 }
 
-static void
+static int
 qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
 {
-	u32 reg[4];
+	u32 reg[4], val;
 	int i;
 
 	/* load the ARL table into an array */
-	for (i = 0; i < 4; i++)
-		reg[i] = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
+	for (i = 0; i < 4; i++) {
+		val = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
+		if (val < 0)
+			return val;
+
+		reg[i] = val;
+	}
 
 	/* vid - 83:72 */
 	fdb->vid = (reg[2] >> QCA8K_ATU_VID_S) & QCA8K_ATU_VID_M;
@@ -323,6 +333,8 @@ qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
 	fdb->mac[3] = (reg[0] >> QCA8K_ATU_ADDR3_S) & 0xff;
 	fdb->mac[4] = (reg[0] >> QCA8K_ATU_ADDR4_S) & 0xff;
 	fdb->mac[5] = reg[0] & 0xff;
+
+	return 0;
 }
 
 static void
@@ -374,6 +386,8 @@ qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
 	/* Check for table full violation when adding an entry */
 	if (cmd == QCA8K_FDB_LOAD) {
 		reg = qca8k_read(priv, QCA8K_REG_ATU_FUNC);
+		if (reg < 0)
+			return reg;
 		if (reg & QCA8K_ATU_FUNC_FULL)
 			return -1;
 	}
@@ -384,12 +398,15 @@ qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
 static int
 qca8k_fdb_next(struct qca8k_priv *priv, struct qca8k_fdb *fdb, int port)
 {
-	int ret;
+	int ret, ret_read;
 
 	qca8k_fdb_write(priv, fdb->vid, fdb->port_mask, fdb->mac, fdb->aging);
 	ret = qca8k_fdb_access(priv, QCA8K_FDB_NEXT, port);
-	if (ret >= 0)
-		qca8k_fdb_read(priv, fdb);
+	if (ret >= 0) {
+		ret_read = qca8k_fdb_read(priv, fdb);
+		if (ret_read < 0)
+			return ret_read;
+	}
 
 	return ret;
 }
@@ -449,6 +466,8 @@ qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
 	/* Check for table full violation when adding an entry */
 	if (cmd == QCA8K_VLAN_LOAD) {
 		reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC1);
+		if (reg < 0)
+			return reg;
 		if (reg & QCA8K_VTU_FUNC1_FULL)
 			return -ENOMEM;
 	}
@@ -475,6 +494,8 @@ qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
 		goto out;
 
 	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+	if (reg < 0)
+		return reg;
 	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
 	reg &= ~(QCA8K_VTU_FUNC0_EG_MODE_MASK << QCA8K_VTU_FUNC0_EG_MODE_S(port));
 	if (untagged)
@@ -506,6 +527,8 @@ qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
 		goto out;
 
 	reg = qca8k_read(priv, QCA8K_REG_VTU_FUNC0);
+	if (reg < 0)
+		return reg;
 	reg &= ~(3 << QCA8K_VTU_FUNC0_EG_MODE_S(port));
 	reg |= QCA8K_VTU_FUNC0_EG_MODE_NOT <<
 			QCA8K_VTU_FUNC0_EG_MODE_S(port);
@@ -621,8 +644,11 @@ qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
 			    QCA8K_MDIO_MASTER_BUSY))
 		return -ETIMEDOUT;
 
-	val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
-		QCA8K_MDIO_MASTER_DATA_MASK);
+	val = qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL);
+	if (val < 0)
+		return val;
+
+	val &= QCA8K_MDIO_MASTER_DATA_MASK;
 
 	return val;
 }
@@ -978,6 +1004,8 @@ qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
 	u32 reg;
 
 	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
+	if (reg < 0)
+		return reg;
 
 	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
 	state->an_complete = state->link;
@@ -1078,18 +1106,26 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	const struct qca8k_mib_desc *mib;
-	u32 reg, i;
+	u32 reg, i, val;
 	u64 hi;
 
 	for (i = 0; i < ARRAY_SIZE(ar8327_mib); i++) {
 		mib = &ar8327_mib[i];
 		reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
 
-		data[i] = qca8k_read(priv, reg);
+		val = qca8k_read(priv, reg);
+		if (val < 0)
+			continue;
+
 		if (mib->size == 2) {
 			hi = qca8k_read(priv, reg + 4);
-			data[i] |= hi << 32;
+			if (hi < 0)
+				continue;
 		}
+
+		data[i] = val;
+		if (mib->size == 2)
+			data[i] |= hi << 32;
 	}
 }
 
@@ -1107,18 +1143,25 @@ qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port);
+	int ret = 0;
 	u32 reg;
 
 	mutex_lock(&priv->reg_mutex);
 	reg = qca8k_read(priv, QCA8K_REG_EEE_CTRL);
+	if (reg < 0) {
+		ret = reg;
+		goto exit;
+	}
+
 	if (eee->eee_enabled)
 		reg |= lpi_en;
 	else
 		reg &= ~lpi_en;
 	qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
-	mutex_unlock(&priv->reg_mutex);
 
-	return 0;
+exit:
+	mutex_unlock(&priv->reg_mutex);
+	return ret;
 }
 
 static int
@@ -1443,6 +1486,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 
 	/* read the switches ID register */
 	id = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
+	if (id < 0)
+		return id;
+
 	id >>= QCA8K_MASK_CTRL_ID_S;
 	id &= QCA8K_MASK_CTRL_ID_M;
 	if (id != QCA8K_ID_QCA8337)
-- 
2.30.2

Re: [RFC PATCH net-next v4 08/28] net: dsa: qca8k: handle error with qca8k_read operation

[Andrew Lunn]

>  static int
>  qca8k_fdb_next(struct qca8k_priv *priv, struct qca8k_fdb *fdb, int port)
>  {
> -	int ret;
> +	int ret, ret_read;
>  
>  	qca8k_fdb_write(priv, fdb->vid, fdb->port_mask, fdb->mac, fdb->aging);
>  	ret = qca8k_fdb_access(priv, QCA8K_FDB_NEXT, port);
> -	if (ret >= 0)
> -		qca8k_fdb_read(priv, fdb);
> +	if (ret >= 0) {
> +		ret_read = qca8k_fdb_read(priv, fdb);
> +		if (ret_read < 0)
> +			return ret_read;
> +	}

This is O.K, but it is a bit of an odd structure. How about

  	qca8k_fdb_write(priv, fdb->vid, fdb->port_mask, fdb->mac, fdb->aging);
	ret = qca8k_fdb_access(priv, QCA8K_FDB_NEXT, port);
	if (ret < 0)
		return ret;

	return qca8k_fdb_read(priv, fdb);
}

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

    Andrew

[RFC PATCH net-next v4 09/28] net: dsa: qca8k: handle error with qca8k_write operation

[Ansuel Smith]

qca8k_write can fail. Rework any user to handle error values and
correctly return.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 5087ee8ecebb..0ec95ef16143 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -168,7 +168,7 @@ qca8k_read(struct qca8k_priv *priv, u32 reg)
 	return val;
 }
 
-static void
+static int
 qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 {
 	struct mii_bus *bus = priv->bus;
@@ -187,6 +187,7 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 
 exit:
 	mutex_unlock(&bus->mdio_lock);
+	return ret;
 }
 
 static u32
@@ -247,9 +248,7 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
 
-	qca8k_write(priv, reg, val);
-
-	return 0;
+	return qca8k_write(priv, reg, val);
 }
 
 static const struct regmap_range qca8k_readable_ranges[] = {
@@ -367,6 +366,7 @@ static int
 qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
 {
 	u32 reg;
+	int ret;
 
 	/* Set the command and FDB index */
 	reg = QCA8K_ATU_FUNC_BUSY;
@@ -377,7 +377,9 @@ qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
 	}
 
 	/* Write the function register triggering the table access */
-	qca8k_write(priv, QCA8K_REG_ATU_FUNC, reg);
+	ret = qca8k_write(priv, QCA8K_REG_ATU_FUNC, reg);
+	if (ret)
+		return ret;
 
 	/* wait for completion */
 	if (qca8k_busy_wait(priv, QCA8K_REG_ATU_FUNC, QCA8K_ATU_FUNC_BUSY))
@@ -450,6 +452,7 @@ static int
 qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
 {
 	u32 reg;
+	int ret;
 
 	/* Set the command and VLAN index */
 	reg = QCA8K_VTU_FUNC1_BUSY;
@@ -457,7 +460,9 @@ qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
 	reg |= vid << QCA8K_VTU_FUNC1_VID_S;
 
 	/* Write the function register triggering the table access */
-	qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
+	ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
+	if (ret)
+		return ret;
 
 	/* wait for completion */
 	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
@@ -505,7 +510,9 @@ qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
 		reg |= QCA8K_VTU_FUNC0_EG_MODE_TAG <<
 				QCA8K_VTU_FUNC0_EG_MODE_S(port);
 
-	qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+	ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+	if (ret)
+		return ret;
 	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
 
 out:
@@ -548,7 +555,9 @@ qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
 	if (del) {
 		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
 	} else {
-		qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+		ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+		if (ret)
+			return ret;
 		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
 	}
 
@@ -558,15 +567,21 @@ qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
 	return ret;
 }
 
-static void
+static int
 qca8k_mib_init(struct qca8k_priv *priv)
 {
+	int ret;
+
 	mutex_lock(&priv->reg_mutex);
 	qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
 	qca8k_busy_wait(priv, QCA8K_REG_MIB, QCA8K_MIB_BUSY);
 	qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
-	qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
+
+	ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
+
+exit:
 	mutex_unlock(&priv->reg_mutex);
+	return ret;
 }
 
 static void
@@ -603,6 +618,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;
@@ -616,7 +632,9 @@ 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);
+	ret = qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+	if (ret)
+		return ret;
 
 	return qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
 		QCA8K_MDIO_MASTER_BUSY);
@@ -626,6 +644,7 @@ static int
 qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
 {
 	u32 phy, val;
+	int ret;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -638,7 +657,9 @@ 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);
+	ret = qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+	if (ret)
+		return ret;
 
 	if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
 			    QCA8K_MDIO_MASTER_BUSY))
@@ -769,12 +790,18 @@ qca8k_setup(struct dsa_switch *ds)
 		      QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
 
 	/* Enable MIB counters */
-	qca8k_mib_init(priv);
+	ret = qca8k_mib_init(priv);
+	if (ret)
+		dev_warn(priv->dev, "mib init failed");
 
 	/* Enable QCA header mode on the cpu port */
-	qca8k_write(priv, QCA8K_REG_PORT_HDR_CTRL(QCA8K_CPU_PORT),
-		    QCA8K_PORT_HDR_CTRL_ALL << QCA8K_PORT_HDR_CTRL_TX_S |
-		    QCA8K_PORT_HDR_CTRL_ALL << QCA8K_PORT_HDR_CTRL_RX_S);
+	ret = qca8k_write(priv, QCA8K_REG_PORT_HDR_CTRL(QCA8K_CPU_PORT),
+			  QCA8K_PORT_HDR_CTRL_ALL << QCA8K_PORT_HDR_CTRL_TX_S |
+			  QCA8K_PORT_HDR_CTRL_ALL << QCA8K_PORT_HDR_CTRL_RX_S);
+	if (ret) {
+		dev_err(priv->dev, "failed enabling QCA header mode");
+		return ret;
+	}
 
 	/* Disable forwarding by default on all ports */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++)
@@ -786,11 +813,13 @@ qca8k_setup(struct dsa_switch *ds)
 		qca8k_port_set_status(priv, i, 0);
 
 	/* Forward all unknown frames to CPU port for Linux processing */
-	qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
-		    BIT(0) << QCA8K_GLOBAL_FW_CTRL1_IGMP_DP_S |
-		    BIT(0) << QCA8K_GLOBAL_FW_CTRL1_BC_DP_S |
-		    BIT(0) << QCA8K_GLOBAL_FW_CTRL1_MC_DP_S |
-		    BIT(0) << QCA8K_GLOBAL_FW_CTRL1_UC_DP_S);
+	ret = qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
+			  BIT(0) << QCA8K_GLOBAL_FW_CTRL1_IGMP_DP_S |
+			  BIT(0) << QCA8K_GLOBAL_FW_CTRL1_BC_DP_S |
+			  BIT(0) << QCA8K_GLOBAL_FW_CTRL1_MC_DP_S |
+			  BIT(0) << QCA8K_GLOBAL_FW_CTRL1_UC_DP_S);
+	if (ret)
+		return ret;
 
 	/* Setup connection between CPU port & user ports */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
@@ -818,16 +847,20 @@ qca8k_setup(struct dsa_switch *ds)
 			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
 				  0xfff << shift,
 				  QCA8K_PORT_VID_DEF << shift);
-			qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
-				    QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
-				    QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
+			ret = qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
+					  QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
+					  QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
+			if (ret)
+				return ret;
 		}
 	}
 
 	/* Setup our port MTUs to match power on defaults */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++)
 		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
-	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
+	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
+	if (ret)
+		dev_warn(priv->dev, "failed setting MTU settings");
 
 	/* Flush the FDB table */
 	qca8k_fdb_flush(priv);
@@ -1143,8 +1176,8 @@ qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u32 lpi_en = QCA8K_REG_EEE_CTRL_LPI_EN(port);
-	int ret = 0;
 	u32 reg;
+	int ret;
 
 	mutex_lock(&priv->reg_mutex);
 	reg = qca8k_read(priv, QCA8K_REG_EEE_CTRL);
@@ -1157,7 +1190,7 @@ qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
 		reg |= lpi_en;
 	else
 		reg &= ~lpi_en;
-	qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
+	ret = qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
 
 exit:
 	mutex_unlock(&priv->reg_mutex);
@@ -1287,9 +1320,7 @@ qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 			mtu = priv->port_mtu[i];
 
 	/* Include L2 header / FCS length */
-	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
-
-	return 0;
+	return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
 }
 
 static int
@@ -1384,7 +1415,7 @@ qca8k_port_vlan_add(struct dsa_switch *ds, int port,
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
 	struct qca8k_priv *priv = ds->priv;
-	int ret = 0;
+	int ret;
 
 	ret = qca8k_vlan_add(priv, port, vlan->vid, untagged);
 	if (ret) {
@@ -1397,9 +1428,11 @@ qca8k_port_vlan_add(struct dsa_switch *ds, int port,
 
 		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
 			  0xfff << shift, vlan->vid << shift);
-		qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
-			    QCA8K_PORT_VLAN_CVID(vlan->vid) |
-			    QCA8K_PORT_VLAN_SVID(vlan->vid));
+		ret = qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
+				  QCA8K_PORT_VLAN_CVID(vlan->vid) |
+				  QCA8K_PORT_VLAN_SVID(vlan->vid));
+		if (ret)
+			return ret;
 	}
 
 	return 0;
@@ -1410,7 +1443,7 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port,
 		    const struct switchdev_obj_port_vlan *vlan)
 {
 	struct qca8k_priv *priv = ds->priv;
-	int ret = 0;
+	int ret;
 
 	ret = qca8k_vlan_del(priv, port, vlan->vid);
 	if (ret)
-- 
2.30.2

Re: [RFC PATCH net-next v4 09/28] net: dsa: qca8k: handle error with qca8k_write operation

[Andrew Lunn]

On Sat, May 08, 2021 at 02:28:59AM +0200, Ansuel Smith wrote:
> qca8k_write can fail. Rework any user to handle error values and
> correctly return.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

    Andrew

[RFC PATCH net-next v4 10/28] net: dsa: qca8k: handle error with qca8k_rmw operation

[Ansuel Smith]

qca8k_rmw can fail. Rework any user to handle error values and
correctly return.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 0ec95ef16143..16ffff478de0 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -206,6 +206,9 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
 		goto exit;
 
 	ret = qca8k_mii_read32(bus, 0x10 | r2, r1);
+	if (ret < 0)
+		goto exit;
+
 	ret &= ~mask;
 	ret |= val;
 	qca8k_mii_write32(bus, 0x10 | r2, r1, ret);
@@ -216,16 +219,28 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
 	return ret;
 }
 
-static void
+static int
 qca8k_reg_set(struct qca8k_priv *priv, u32 reg, u32 val)
 {
-	qca8k_rmw(priv, reg, 0, val);
+	int ret;
+
+	ret = qca8k_rmw(priv, reg, 0, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
-static void
+static int
 qca8k_reg_clear(struct qca8k_priv *priv, u32 reg, u32 val)
 {
-	qca8k_rmw(priv, reg, val, 0);
+	int ret;
+
+	ret = qca8k_rmw(priv, reg, val, 0);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static int
@@ -573,9 +588,15 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	int ret;
 
 	mutex_lock(&priv->reg_mutex);
-	qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
+	ret = qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
+	if (ret)
+		goto exit;
+
 	qca8k_busy_wait(priv, QCA8K_REG_MIB, QCA8K_MIB_BUSY);
-	qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
+
+	ret = qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
+	if (ret)
+		goto exit;
 
 	ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
 
@@ -751,9 +772,8 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 		 * a dt-overlay and driver reload changed the configuration
 		 */
 
-		qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
-				QCA8K_MDIO_MASTER_EN);
-		return 0;
+		return qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
+				       QCA8K_MDIO_MASTER_EN);
 	}
 
 	priv->ops.phy_read = qca8k_phy_read;
@@ -786,8 +806,12 @@ qca8k_setup(struct dsa_switch *ds)
 		return ret;
 
 	/* Enable CPU Port */
-	qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
-		      QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
+	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
+			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
+	if (ret) {
+		dev_err(priv->dev, "failed enabling CPU port");
+		return ret;
+	}
 
 	/* Enable MIB counters */
 	ret = qca8k_mib_init(priv);
@@ -804,9 +828,12 @@ qca8k_setup(struct dsa_switch *ds)
 	}
 
 	/* Disable forwarding by default on all ports */
-	for (i = 0; i < QCA8K_NUM_PORTS; i++)
-		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-			  QCA8K_PORT_LOOKUP_MEMBER, 0);
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
+				QCA8K_PORT_LOOKUP_MEMBER, 0);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* Disable MAC by default on all ports */
 	for (i = 1; i < QCA8K_NUM_PORTS; i++)
@@ -825,28 +852,37 @@ qca8k_setup(struct dsa_switch *ds)
 	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
 		/* CPU port gets connected to all user ports of the switch */
 		if (dsa_is_cpu_port(ds, i)) {
-			qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(QCA8K_CPU_PORT),
-				  QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
+			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(QCA8K_CPU_PORT),
+					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
+			if (ret < 0)
+				return ret;
 		}
 
 		/* Individual user ports get connected to CPU port only */
 		if (dsa_is_user_port(ds, i)) {
 			int shift = 16 * (i % 2);
 
-			qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-				  QCA8K_PORT_LOOKUP_MEMBER,
-				  BIT(QCA8K_CPU_PORT));
+			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
+					QCA8K_PORT_LOOKUP_MEMBER,
+					BIT(QCA8K_CPU_PORT));
+			if (ret < 0)
+				return ret;
 
 			/* Enable ARP Auto-learning by default */
-			qca8k_reg_set(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-				      QCA8K_PORT_LOOKUP_LEARN);
+			ret = qca8k_reg_set(priv, QCA8K_PORT_LOOKUP_CTRL(i),
+					    QCA8K_PORT_LOOKUP_LEARN);
+			if (ret)
+				return ret;
 
 			/* For port based vlans to work we need to set the
 			 * default egress vid
 			 */
-			qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
-				  0xfff << shift,
-				  QCA8K_PORT_VID_DEF << shift);
+			ret = qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
+					0xfff << shift,
+					QCA8K_PORT_VID_DEF << shift);
+			if (ret < 0)
+				return ret;
+
 			ret = qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(i),
 					  QCA8K_PORT_VLAN_CVID(QCA8K_PORT_VID_DEF) |
 					  QCA8K_PORT_VLAN_SVID(QCA8K_PORT_VID_DEF));
@@ -1238,7 +1274,7 @@ qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	int port_mask = BIT(QCA8K_CPU_PORT);
-	int i;
+	int i, ret;
 
 	for (i = 1; i < QCA8K_NUM_PORTS; i++) {
 		if (dsa_to_port(ds, i)->bridge_dev != br)
@@ -1246,17 +1282,20 @@ qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br)
 		/* Add this port to the portvlan mask of the other ports
 		 * in the bridge
 		 */
-		qca8k_reg_set(priv,
-			      QCA8K_PORT_LOOKUP_CTRL(i),
-			      BIT(port));
+		ret = qca8k_reg_set(priv,
+				    QCA8K_PORT_LOOKUP_CTRL(i),
+				    BIT(port));
+		if (ret)
+			return ret;
 		if (i != port)
 			port_mask |= BIT(i);
 	}
+
 	/* Add all other ports to this ports portvlan mask */
-	qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-		  QCA8K_PORT_LOOKUP_MEMBER, port_mask);
+	ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+			QCA8K_PORT_LOOKUP_MEMBER, port_mask);
 
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 static void
@@ -1393,18 +1432,19 @@ qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 			  struct netlink_ext_ack *extack)
 {
 	struct qca8k_priv *priv = ds->priv;
+	int ret;
 
 	if (vlan_filtering) {
-		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-			  QCA8K_PORT_LOOKUP_VLAN_MODE,
-			  QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
+		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+				QCA8K_PORT_LOOKUP_VLAN_MODE,
+				QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
 	} else {
-		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-			  QCA8K_PORT_LOOKUP_VLAN_MODE,
-			  QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
+		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
+				QCA8K_PORT_LOOKUP_VLAN_MODE,
+				QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
 	}
 
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 static int
@@ -1426,16 +1466,17 @@ qca8k_port_vlan_add(struct dsa_switch *ds, int port,
 	if (pvid) {
 		int shift = 16 * (port % 2);
 
-		qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
-			  0xfff << shift, vlan->vid << shift);
+		ret = qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
+				0xfff << shift, vlan->vid << shift);
+		if (ret < 0)
+			return ret;
+
 		ret = qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
 				  QCA8K_PORT_VLAN_CVID(vlan->vid) |
 				  QCA8K_PORT_VLAN_SVID(vlan->vid));
-		if (ret)
-			return ret;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int
-- 
2.30.2

Re: [RFC PATCH net-next v4 10/28] net: dsa: qca8k: handle error with qca8k_rmw operation

[Andrew Lunn]

> -static void
> +static int
>  qca8k_reg_set(struct qca8k_priv *priv, u32 reg, u32 val)
>  {
> -	qca8k_rmw(priv, reg, 0, val);
> +	int ret;
> +
> +	ret = qca8k_rmw(priv, reg, 0, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

This is wrong. Look at qca8k_rmw():

static u32
qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
{
	u16 r1, r2, page;
	u32 ret;

	qca8k_split_addr(reg, &r1, &r2, &page);

	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);

	qca8k_set_page(priv->bus, page);
	ret = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
	ret &= ~mask;
	ret |= val;
	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, ret);

	mutex_unlock(&priv->bus->mdio_lock);

	return ret;
}

First off, it returns a u32. So you cannot actually represent negative
error numbers.

Also, since it is returning what is now contained in the register,
that value might actually look like an error code.

I had a quick look at all the places qca8k_rmw() is called. As far as
i could see, the return value is never used. I suggest you double
check that, and then change this function. Make it return a negative
error code, or 0 on success. You can then simplify the code using the
return value.

       Andrew

[RFC PATCH net-next v4 11/28] net: dsa: qca8k: handle error from qca8k_busy_wait

[Ansuel Smith]

Propagate errors from qca8k_busy_wait instead of hardcoding return
value.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 16ffff478de0..6c65c6013c5f 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -397,8 +397,9 @@ qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
 		return ret;
 
 	/* wait for completion */
-	if (qca8k_busy_wait(priv, QCA8K_REG_ATU_FUNC, QCA8K_ATU_FUNC_BUSY))
-		return -1;
+	ret = qca8k_busy_wait(priv, QCA8K_REG_ATU_FUNC, QCA8K_ATU_FUNC_BUSY);
+	if (ret)
+		return ret;
 
 	/* Check for table full violation when adding an entry */
 	if (cmd == QCA8K_FDB_LOAD) {
@@ -480,8 +481,9 @@ qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
 		return ret;
 
 	/* wait for completion */
-	if (qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY))
-		return -ETIMEDOUT;
+	ret = qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY);
+	if (ret)
+		return ret;
 
 	/* Check for table full violation when adding an entry */
 	if (cmd == QCA8K_VLAN_LOAD) {
@@ -592,7 +594,9 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	if (ret)
 		goto exit;
 
-	qca8k_busy_wait(priv, QCA8K_REG_MIB, QCA8K_MIB_BUSY);
+	ret = qca8k_busy_wait(priv, QCA8K_REG_MIB, QCA8K_MIB_BUSY);
+	if (ret)
+		goto exit;
 
 	ret = qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
 	if (ret)
@@ -682,9 +686,10 @@ qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
 	if (ret)
 		return ret;
 
-	if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
-			    QCA8K_MDIO_MASTER_BUSY))
-		return -ETIMEDOUT;
+	ret = qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+			      QCA8K_MDIO_MASTER_BUSY);
+	if (ret)
+		return ret;
 
 	val = qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL);
 	if (val < 0)
-- 
2.30.2

Re: [RFC PATCH net-next v4 11/28] net: dsa: qca8k: handle error from qca8k_busy_wait

[Andrew Lunn]

On Sat, May 08, 2021 at 02:29:01AM +0200, Ansuel Smith wrote:
> Propagate errors from qca8k_busy_wait instead of hardcoding return
> value.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

    Andrew

[RFC PATCH net-next v4 12/28] 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/qca8k.c | 23 ++++++++++++++++++++---
 drivers/net/dsa/qca8k.h |  6 ++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 6c65c6013c5f..9c2f09e84364 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1536,6 +1536,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;
 
@@ -1563,6 +1564,11 @@ 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);
 	if (id < 0)
@@ -1570,8 +1576,10 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 
 	id >>= QCA8K_MASK_CTRL_ID_S;
 	id &= QCA8K_MASK_CTRL_ID_M;
-	if (id != QCA8K_ID_QCA8337)
+	if (id != data->id) {
+		dev_err(&mdiodev->dev, "Switch id detected %x but expected %x", id, data->id);
 		return -ENODEV;
+	}
 
 	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
 	if (!priv->ds)
@@ -1636,9 +1644,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 86c585b7ec4a..87a8b10459c6 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
 
@@ -213,6 +215,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: [RFC PATCH net-next v4 12/28] net: dsa: qca8k: add support for qca8327 switch

[Florian Fainelli]

On 5/7/2021 5:29 PM, Ansuel Smith wrote:
> 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>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[RFC PATCH net-next v4 13/28] 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Rob Herring <robh@kernel.org>
---
 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

Re: [RFC PATCH net-next v4 13/28] devicetree: net: dsa: qca8k: Document new compatible qca8327

[Florian Fainelli]

On 5/7/2021 5:29 PM, Ansuel Smith wrote:
> Add support for qca8327 in the compatible list.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Acked-by: Rob Herring <robh@kernel.org>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[RFC PATCH net-next v4 14/28] net: dsa: qca8k: add priority tweak to qca8337 switch

[Ansuel Smith]

The port 5 of the qca8337 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.
The packet priority tweak is only needed for the qca8337 switch and
other qca8k switch are not affected.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 9c2f09e84364..69fd526344cc 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -791,6 +791,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)) {
@@ -896,6 +897,51 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 	}
 
+	/* The port 5 of the qca8337 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.
+	 * This problem is limited to qca8337 and other qca8k switch are not affected.
+	 */
+	if (priv->switch_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);
+		}
+	}
+
 	/* Setup our port MTUs to match power on defaults */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++)
 		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
@@ -1581,6 +1627,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 		return -ENODEV;
 	}
 
+	priv->switch_id = id;
 	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
 	if (!priv->ds)
 		return -ENOMEM;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 87a8b10459c6..42d90836dffa 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -168,6 +168,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)))
 
@@ -220,6 +244,7 @@ struct qca8k_match_data {
 };
 
 struct qca8k_priv {
+	u8 switch_id;
 	struct regmap *regmap;
 	struct mii_bus *bus;
 	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
-- 
2.30.2

[RFC PATCH net-next v4 15/28] net: dsa: qca8k: limit port5 delay to qca8337

[Ansuel Smith]

Limit port5 rx delay to qca8337. This is taken from the legacy QSDK code
that limits the rx delay on port5 to only this particular switch version,
on other switch only the tx and rx delay for port0 are needed.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 69fd526344cc..612fa77164ae 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1015,8 +1015,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			    QCA8K_PORT_PAD_RGMII_EN |
 			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
 			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
-		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+		/* QCA8337 requires to set rgmii rx delay */
+		if (priv->switch_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

[RFC PATCH net-next v4 16/28] 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 612fa77164ae..301bb94aba84 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -942,6 +942,16 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 	}
 
+	/* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */
+	if (priv->switch_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);
+	}
+
 	/* Setup our port MTUs to match power on defaults */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++)
 		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 42d90836dffa..eceeacfe2c5d 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -168,6 +168,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

[RFC PATCH net-next v4 17/28] net: dsa: qca8k: add support for switch rev

[Ansuel Smith]

qca8k internal phy driver 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.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 301bb94aba84..9905a2631420 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1591,12 +1591,40 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,
 };
 
+static int qca8k_read_switch_id(struct qca8k_priv *priv)
+{
+	const struct qca8k_match_data *data;
+	u32 val;
+	u8 id;
+
+	/* get the switches ID from the compatible */
+	data = of_device_get_match_data(priv->dev);
+	if (!data)
+		return -ENODEV;
+
+	val = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
+	if (val < 0)
+		return -ENODEV;
+
+	id = QCA8K_MASK_CTRL_DEVICE_ID(val & QCA8K_MASK_CTRL_DEVICE_ID_MASK);
+	if (id != data->id) {
+		dev_err(priv->dev, "Switch id detected %x but expected %x", id, data->id);
+		return -ENODEV;
+	}
+
+	priv->switch_id = id;
+
+	/* Save revision to communicate to the internal PHY driver */
+	priv->switch_revision = (val & QCA8K_MASK_CTRL_REV_ID_MASK);
+
+	return 0;
+}
+
 static int
 qca8k_sw_probe(struct mdio_device *mdiodev)
 {
-	const struct qca8k_match_data *data;
 	struct qca8k_priv *priv;
-	u32 id;
+	int ret;
 
 	/* allocate the private data struct so that we can probe the switches
 	 * ID register
@@ -1622,24 +1650,11 @@ 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);
-	if (id < 0)
-		return id;
-
-	id >>= QCA8K_MASK_CTRL_ID_S;
-	id &= QCA8K_MASK_CTRL_ID_M;
-	if (id != data->id) {
-		dev_err(&mdiodev->dev, "Switch id detected %x but expected %x", id, data->id);
-		return -ENODEV;
-	}
+	/* Check the detected switch id */
+	ret = qca8k_read_switch_id(priv);
+	if (ret)
+		return ret;
 
-	priv->switch_id = id;
 	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
 	if (!priv->ds)
 		return -ENOMEM;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index eceeacfe2c5d..338277978ec0 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -30,8 +30,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
@@ -251,6 +253,7 @@ struct qca8k_match_data {
 
 struct qca8k_priv {
 	u8 switch_id;
+	u8 switch_revision;
 	struct regmap *regmap;
 	struct mii_bus *bus;
 	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
-- 
2.30.2

Re: [RFC PATCH net-next v4 17/28] net: dsa: qca8k: add support for switch rev

[Florian Fainelli]

On 5/7/2021 5:29 PM, Ansuel Smith wrote:
> qca8k internal phy driver 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.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[RFC PATCH net-next v4 18/28] net: dsa: qca8k: add ethernet-ports fallback to setup_mdio_bus

[Ansuel Smith]

Dsa now also supports ethernet-ports. Add this new binding as a fallback
if the ports node can't be found.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 9905a2631420..285cce4fab9f 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -730,6 +730,9 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 	int err;
 
 	ports = of_get_child_by_name(priv->dev->of_node, "ports");
+	if (!ports)
+		ports = of_get_child_by_name(priv->dev->of_node, "ethernet-ports");
+
 	if (!ports)
 		return -EINVAL;
 
-- 
2.30.2

Re: [RFC PATCH net-next v4 18/28] net: dsa: qca8k: add ethernet-ports fallback to setup_mdio_bus

[Andrew Lunn]

On Sat, May 08, 2021 at 02:29:08AM +0200, Ansuel Smith wrote:
> Dsa now also supports ethernet-ports. Add this new binding as a fallback
> if the ports node can't be found.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

    Andrew

[RFC PATCH net-next v4 19/28] net: dsa: qca8k: make rgmii delay configurable

[Ansuel Smith]

The legacy qsdk code used a different delay instead of the max value.
Qsdk use 1 ps for rx and 2 ps for tx. Make these values configurable
using the standard rx/tx-internal-delay-ps ethernet binding and apply
qsdk values by default. The connected gmac doesn't add any delay so no
additional delay is added to tx/rx.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 285cce4fab9f..0c53b6fdf6ee 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -789,6 +789,50 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 	return 0;
 }
 
+static int
+qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)
+{
+	struct device_node *ports, *port;
+	u32 val;
+
+	ports = of_get_child_by_name(priv->dev->of_node, "ports");
+	if (!ports)
+		ports = of_get_child_by_name(priv->dev->of_node, "ethernet-ports");
+
+	if (!ports)
+		return -EINVAL;
+
+	/* Assume only one port with rgmii-id mode */
+	for_each_available_child_of_node(ports, port) {
+		if (!of_property_match_string(port, "phy-mode", "rgmii-id"))
+			continue;
+
+		if (of_property_read_u32(port, "rx-internal-delay-ps", &val))
+			val = 2;
+
+		if (val > QCA8K_MAX_DELAY) {
+			dev_err(priv->dev, "rgmii rx delay is limited to more than 3ps, setting to the max value");
+			priv->rgmii_rx_delay = 3;
+		} else {
+			priv->rgmii_rx_delay = val;
+		}
+
+		if (of_property_read_u32(port, "tx-internal-delay-ps", &val))
+			val = 1;
+
+		if (val > QCA8K_MAX_DELAY) {
+			dev_err(priv->dev, "rgmii tx delay is limited to more than 3ps, setting to the max value");
+			priv->rgmii_tx_delay = 3;
+		} else {
+			priv->rgmii_tx_delay = val;
+		}
+	}
+
+	of_node_put(ports);
+
+	return 0;
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -814,6 +858,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_of_rgmii_delay(priv);
+	if (ret)
+		return ret;
+
 	/* Enable CPU Port */
 	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
@@ -1026,8 +1074,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(priv->rgmii_tx_delay) |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY(priv->rgmii_rx_delay) |
+			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
 		/* QCA8337 requires to set rgmii rx delay */
 		if (priv->switch_id == QCA8K_ID_QCA8337)
 			qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 338277978ec0..a878486d9bcd 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -38,12 +38,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)
@@ -254,6 +253,8 @@ struct qca8k_match_data {
 struct qca8k_priv {
 	u8 switch_id;
 	u8 switch_revision;
+	u8 rgmii_tx_delay;
+	u8 rgmii_rx_delay;
 	struct regmap *regmap;
 	struct mii_bus *bus;
 	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
-- 
2.30.2

Re: [RFC PATCH net-next v4 19/28] net: dsa: qca8k: make rgmii delay configurable

[Andrew Lunn]

> +
> +	/* Assume only one port with rgmii-id mode */

Since this is only valid for the RGMII port, please look in that
specific node for these properties.

	 Andrew

Re: [RFC PATCH net-next v4 19/28] net: dsa: qca8k: make rgmii delay configurable

[Ansuel Smith]

On Sat, May 08, 2021 at 08:12:26PM +0200, Andrew Lunn wrote:
> > +
> > +	/* Assume only one port with rgmii-id mode */
> 
> Since this is only valid for the RGMII port, please look in that
> specific node for these properties.
> 
> 	 Andrew

Sorry, can you clarify this? You mean that I should check in the phandle
pointed by the phy-handle or that I should modify the logic to only
check for one (and the unique in this case) rgmii port?

Re: [RFC PATCH net-next v4 19/28] net: dsa: qca8k: make rgmii delay configurable

[Andrew Lunn]

On Sun, May 09, 2021 at 01:58:07AM +0200, Ansuel Smith wrote:
> On Sat, May 08, 2021 at 08:12:26PM +0200, Andrew Lunn wrote:
> > > +
> > > +	/* Assume only one port with rgmii-id mode */
> > 
> > Since this is only valid for the RGMII port, please look in that
> > specific node for these properties.
> > 
> > 	 Andrew
> 
> Sorry, can you clarify this? You mean that I should check in the phandle
> pointed by the phy-handle or that I should modify the logic to only
> check for one (and the unique in this case) rgmii port?

Despite there only being one register, it should be a property of the
port. If future chips have multiple RGMII ports, i expect there will
be multiple registers. To avoid confusion in the future, we should
make this a proper to the port it applies to. So assuming the RGMII
port is port 0:

                                #address-cells = <1>;
                                #size-cells = <0>;
                                port@0 {
                                        reg = <0>;
                                        label = "cpu";
                                        ethernet = <&gmac1>;
                                        phy-mode = "rgmii";
                                        fixed-link {
                                                speed = 1000;
                                                full-duplex;
                                        };
					rx-internal-delay-ps = <2000>;
					tx-internal-delay-ps = <2000>;
                                };

                                port@1 {
                                        reg = <1>;
                                        label = "lan1";
                                        phy-handle = <&phy_port1>;
                                };

                                port@2 {
                                        reg = <2>;
                                        label = "lan2";
                                        phy-handle = <&phy_port2>;
                                };

                                port@3 {
                                        reg = <3>;
                                        label = "lan3";
                                        phy-handle = <&phy_port3>;
                                };

                                port@4 {
                                        reg = <4>;
                                        label = "lan4";
                                        phy-handle = <&phy_port4>;
                                };

                                port@5 {
                                        reg = <5>;
                                        label = "wan";
                                        phy-handle = <&phy_port5>;
                                };
                        };
                };
        };

You also should validate that phy-mode is rgmii and only rgmii. You
get into odd situations if it is any of the other three rgmii modes.

    Andrew

Re: [RFC PATCH net-next v4 19/28] net: dsa: qca8k: make rgmii delay configurable

[Ansuel Smith]

On Sun, May 09, 2021 at 03:07:15AM +0200, Andrew Lunn wrote:
> On Sun, May 09, 2021 at 01:58:07AM +0200, Ansuel Smith wrote:
> > On Sat, May 08, 2021 at 08:12:26PM +0200, Andrew Lunn wrote:
> > > > +
> > > > +	/* Assume only one port with rgmii-id mode */
> > > 
> > > Since this is only valid for the RGMII port, please look in that
> > > specific node for these properties.
> > > 
> > > 	 Andrew
> > 
> > Sorry, can you clarify this? You mean that I should check in the phandle
> > pointed by the phy-handle or that I should modify the logic to only
> > check for one (and the unique in this case) rgmii port?
> 
> Despite there only being one register, it should be a property of the
> port. If future chips have multiple RGMII ports, i expect there will
> be multiple registers. To avoid confusion in the future, we should
> make this a proper to the port it applies to. So assuming the RGMII
> port is port 0:
> 
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>                                 port@0 {
>                                         reg = <0>;
>                                         label = "cpu";
>                                         ethernet = <&gmac1>;
>                                         phy-mode = "rgmii";
>                                         fixed-link {
>                                                 speed = 1000;
>                                                 full-duplex;
>                                         };
> 					rx-internal-delay-ps = <2000>;
> 					tx-internal-delay-ps = <2000>;
>                                 };
> 
>                                 port@1 {
>                                         reg = <1>;
>                                         label = "lan1";
>                                         phy-handle = <&phy_port1>;
>                                 };
> 
>                                 port@2 {
>                                         reg = <2>;
>                                         label = "lan2";
>                                         phy-handle = <&phy_port2>;
>                                 };
> 
>                                 port@3 {
>                                         reg = <3>;
>                                         label = "lan3";
>                                         phy-handle = <&phy_port3>;
>                                 };
> 
>                                 port@4 {
>                                         reg = <4>;
>                                         label = "lan4";
>                                         phy-handle = <&phy_port4>;
>                                 };
> 
>                                 port@5 {
>                                         reg = <5>;
>                                         label = "wan";
>                                         phy-handle = <&phy_port5>;
>                                 };
>                         };
>                 };
>         };
> 
> You also should validate that phy-mode is rgmii and only rgmii. You
> get into odd situations if it is any of the other three rgmii modes.
> 
>     Andrew

And that is the intention of the port. I didn't want the binding to be
set on the switch node but on the rgmii node. Correct me if I'm wrong
but isn't what this patch already do?

In of_rgmii_delay I get the ports node of the current switch, iterate
every port, find the one with the phy-mode set to "rgmii-id" and check
if it's present any value. And save the value in the priv struct.
The actual delay is applied in the phylink_mac_config only if the mode
is set to rgmii-id. (the current code set by default a delay of 3000
with rgmii-id, so this is just to make this value configurable)

Re: [RFC PATCH net-next v4 19/28] net: dsa: qca8k: make rgmii delay configurable

[Andrew Lunn]

> And that is the intention of the port. I didn't want the binding to be
> set on the switch node but on the rgmii node. Correct me if I'm wrong
> but isn't what this patch already do?
> 
> In of_rgmii_delay I get the ports node of the current switch, iterate
> every port, find the one with the phy-mode set to "rgmii-id"

You know this hardware only has one RGMII port, and you know which one
it is. So search the list of ports to find that one particular port,
and see if 'rgmii' is set as phy-mode and if so, what the delays are.
This is a port property, so you need to look at that specific port,
not any random port that might have rgmii set.

    Andrew

[RFC PATCH net-next v4 20/28] 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)
Also on driver remove the MASTER_CTRL can be left set and cause the
malfunction of any next driver using the mdio device.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 0c53b6fdf6ee..2e0d2f918efe 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -661,8 +661,14 @@ qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 	if (ret)
 		return ret;
 
-	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);
+
+	/* even if the busy_wait timeouts try to clear the MASTER_EN */
+	qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
+			QCA8K_MDIO_MASTER_EN);
+
+	return ret;
 }
 
 static int
@@ -697,6 +703,10 @@ qca8k_mdio_read(struct qca8k_priv *priv, int port, u32 regnum)
 
 	val &= QCA8K_MDIO_MASTER_DATA_MASK;
 
+	/* even if the busy_wait timeouts try to clear the MASTER_EN */
+	qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
+			QCA8K_MDIO_MASTER_EN);
+
 	return val;
 }
 
-- 
2.30.2

[RFC PATCH net-next v4 21/28] net: dsa: qca8k: 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 | 68 +++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 2e0d2f918efe..e272ccaaa7f6 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -639,9 +639,32 @@ qca8k_port_to_phy(int port)
 	return port - 1;
 }
 
+static int
+qca8k_mdio_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
+{
+	u16 r1, r2, page;
+	u32 val;
+	int ret;
+
+	qca8k_split_addr(reg, &r1, &r2, &page);
+
+	ret = read_poll_timeout(qca8k_mii_read32, val, !(val & mask), 0,
+				QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
+				priv->bus, 0x10 | r2, r1);
+
+	/* Check if qca8k_read has failed for a different reason
+	 * before returnting -ETIMEDOUT
+	 */
+	if (ret < 0 && val < 0)
+		return val;
+
+	return ret;
+}
+
 static int
 qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 {
+	u16 r1, r2, page;
 	u32 phy, val;
 	int ret;
 
@@ -657,12 +680,21 @@ qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 	      QCA8K_MDIO_MASTER_REG_ADDR(regnum) |
 	      QCA8K_MDIO_MASTER_DATA(data);
 
-	ret = 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);
+
+	ret = qca8k_set_page(priv->bus, page);
 	if (ret)
-		return ret;
+		goto exit;
+
+	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
+
+	ret = qca8k_mdio_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+				   QCA8K_MDIO_MASTER_BUSY);
 
-	ret = qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
-			      QCA8K_MDIO_MASTER_BUSY);
+exit:
+	mutex_unlock(&priv->bus->mdio_lock);
 
 	/* even if the busy_wait timeouts try to clear the MASTER_EN */
 	qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
@@ -674,6 +706,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;
 	int ret;
 
@@ -688,21 +721,30 @@ 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);
 
-	ret = qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
-	if (ret)
-		return ret;
+	qca8k_split_addr(QCA8K_MDIO_MASTER_CTRL, &r1, &r2, &page);
+
+	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	ret = qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
-			      QCA8K_MDIO_MASTER_BUSY);
+	ret = qca8k_set_page(priv->bus, page);
 	if (ret)
-		return ret;
+		goto exit;
 
-	val = qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL);
-	if (val < 0)
-		return val;
+	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
+
+	ret = qca8k_mdio_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+				   QCA8K_MDIO_MASTER_BUSY);
+	if (ret)
+		goto exit;
 
+	val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
 	val &= QCA8K_MDIO_MASTER_DATA_MASK;
 
+exit:
+	mutex_unlock(&priv->bus->mdio_lock);
+
+	if (val >= 0)
+		val &= QCA8K_MDIO_MASTER_DATA_MASK;
+
 	/* even if the busy_wait timeouts try to clear the MASTER_EN */
 	qca8k_reg_clear(priv, QCA8K_MDIO_MASTER_CTRL,
 			QCA8K_MDIO_MASTER_EN);
-- 
2.30.2

[RFC PATCH net-next v4 22/28] net: dsa: qca8k: enlarge mdio delay and timeout

[Ansuel Smith]

The witch require some extra delay after setting page or the next
read/write can use still use the old page. Add a delay after the
set_page function to address this as it's done in QSDK legacy driver.
Some timeouts were notice with VLAN and phy function, enlarge the
mdio busy wait timeout to fix these problems.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index e272ccaaa7f6..f96579c0bd46 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -143,6 +143,7 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
 	}
 
 	qca8k_current_page = page;
+	usleep_range(1000, 2000);
 	return 0;
 }
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index a878486d9bcd..d365f85ab34f 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -20,7 +20,7 @@
 #define PHY_ID_QCA8337					0x004dd036
 #define QCA8K_ID_QCA8337				0x13
 
-#define QCA8K_BUSY_WAIT_TIMEOUT				20
+#define QCA8K_BUSY_WAIT_TIMEOUT				2000
 
 #define QCA8K_NUM_FDB_RECORDS				2048
 
-- 
2.30.2

[RFC PATCH net-next v4 23/28] net: dsa: register of_mdiobus if a mdio node is declared

[Ansuel Smith]

Some switch have phy port that use the internal switch mdio bus and can
have different phy regs than the one declared in the ports node. Add
support for this specific case by registering the mdiobus with the mdio
node and permit the port to declare a phy-handle defined inside the
switch node.

This is an example from the qca8337 switch where the 5 phy port should
use the internal mdiobus and would benefits from this.

switch10: switch@10 {
        compatible = "qca,qca8337";
        #address-cells = <1>;
        #size-cells = <0>;
        reg = <0x10>;

        ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                        reg = <0>;
                        label = "cpu";
                        ethernet = <&gmac1>;
                        phy-mode = "rgmii-id";

                        fixed-link {
                                speed = <1000>;
                                full-duplex;
                        };
                };

                port@1 {
                        reg = <1>;
                        label = "lan1";

                        phy-handle = <&phy_port0>;
                        phy-mode = "internal";
                };

                port@2 {
                        reg = <2>;
                        label = "lan2";

                        phy-handle = <&phy_port1>;
                        phy-mode = "internal";
                };

                port@3 {
                        reg = <3>;
                        label = "lan3";

                        phy-handle = <&phy_port2>;
                        phy-mode = "internal";
                };

                port@4 {
                        reg = <4>;
                        label = "lan4";

                        phy-handle = <&phy_port3>;
                        phy-mode = "internal";
                };

                port@5 {
                        reg = <5>;
                        label = "wan";

                        phy-handle = <&phy_port4>;
                        phy-mode = "internal";
                };

                port@6 {
                        reg = <6>;
                        label = "cpu";
                        ethernet = <&gmac2>;
                        phy-mode = "sgmii";

                        fixed-link {
                                        speed = <1000>;
                                        full-duplex;
                        };
                };
        };

        mdio {
                #address-cells = <1>;
                #size-cells = <0>;

                phy_port0: phy@0 {
                        reg = <0>;
                };

                phy_port1: phy@1 {
                        reg = <1>;
                };

                phy_port2: phy@2 {
                        reg = <2>;
                };

                phy_port3: phy@3 {
                        reg = <3>;
                };

                phy_port4: phy@4 {
                        reg = <4>;
                };
        };
};

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

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 3c3e56a1f34d..79adabe3e2a7 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -14,6 +14,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
+#include <linux/of_mdio.h>
 #include <net/devlink.h>
 
 #include "dsa_priv.h"
@@ -721,6 +722,8 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	devlink_params_publish(ds->devlink);
 
 	if (!ds->slave_mii_bus && ds->ops->phy_read) {
+		struct device_node *mdio;
+
 		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
 		if (!ds->slave_mii_bus) {
 			err = -ENOMEM;
@@ -729,7 +732,15 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 
 		dsa_slave_mii_bus_init(ds);
 
-		err = mdiobus_register(ds->slave_mii_bus);
+		mdio = of_get_child_by_name(ds->dev->of_node, "mdio");
+
+		if (mdio) {
+			err = of_mdiobus_register(ds->slave_mii_bus, mdio);
+			of_node_put(mdio);
+		} else {
+			err = mdiobus_register(ds->slave_mii_bus);
+		}
+
 		if (err < 0)
 			goto teardown;
 	}
-- 
2.30.2

Re: [RFC PATCH net-next v4 23/28] net: dsa: register of_mdiobus if a mdio node is declared

[Florian Fainelli]

On 5/7/2021 5:29 PM, Ansuel Smith wrote:
> Some switch have phy port that use the internal switch mdio bus and can
> have different phy regs than the one declared in the ports node. Add
> support for this specific case by registering the mdiobus with the mdio
> node and permit the port to declare a phy-handle defined inside the
> switch node.
> 
> This is an example from the qca8337 switch where the 5 phy port should
> use the internal mdiobus and would benefits from this.
> 
> switch10: switch@10 {
>         compatible = "qca,qca8337";
>         #address-cells = <1>;
>         #size-cells = <0>;
>         reg = <0x10>;
> 
>         ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 port@0 {
>                         reg = <0>;
>                         label = "cpu";
>                         ethernet = <&gmac1>;
>                         phy-mode = "rgmii-id";
> 
>                         fixed-link {
>                                 speed = <1000>;
>                                 full-duplex;
>                         };
>                 };
> 
>                 port@1 {
>                         reg = <1>;
>                         label = "lan1";
> 
>                         phy-handle = <&phy_port0>;
>                         phy-mode = "internal";
>                 };
> 
>                 port@2 {
>                         reg = <2>;
>                         label = "lan2";
> 
>                         phy-handle = <&phy_port1>;
>                         phy-mode = "internal";
>                 };
> 
>                 port@3 {
>                         reg = <3>;
>                         label = "lan3";
> 
>                         phy-handle = <&phy_port2>;
>                         phy-mode = "internal";
>                 };
> 
>                 port@4 {
>                         reg = <4>;
>                         label = "lan4";
> 
>                         phy-handle = <&phy_port3>;
>                         phy-mode = "internal";
>                 };
> 
>                 port@5 {
>                         reg = <5>;
>                         label = "wan";
> 
>                         phy-handle = <&phy_port4>;
>                         phy-mode = "internal";
>                 };
> 
>                 port@6 {
>                         reg = <6>;
>                         label = "cpu";
>                         ethernet = <&gmac2>;
>                         phy-mode = "sgmii";
> 
>                         fixed-link {
>                                         speed = <1000>;
>                                         full-duplex;
>                         };
>                 };
>         };
> 
>         mdio {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 phy_port0: phy@0 {
>                         reg = <0>;
>                 };
> 
>                 phy_port1: phy@1 {
>                         reg = <1>;
>                 };
> 
>                 phy_port2: phy@2 {
>                         reg = <2>;
>                 };
> 
>                 phy_port3: phy@3 {
>                         reg = <3>;
>                 };
> 
>                 phy_port4: phy@4 {
>                         reg = <4>;
>                 };
>         };
> };
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  net/dsa/dsa2.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 3c3e56a1f34d..79adabe3e2a7 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -14,6 +14,7 @@
>  #include <linux/rtnetlink.h>
>  #include <linux/of.h>
>  #include <linux/of_net.h>
> +#include <linux/of_mdio.h>
>  #include <net/devlink.h>
>  
>  #include "dsa_priv.h"
> @@ -721,6 +722,8 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  	devlink_params_publish(ds->devlink);
>  
>  	if (!ds->slave_mii_bus && ds->ops->phy_read) {
> +		struct device_node *mdio;
> +
>  		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
>  		if (!ds->slave_mii_bus) {
>  			err = -ENOMEM;
> @@ -729,7 +732,15 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  
>  		dsa_slave_mii_bus_init(ds);
>  
> -		err = mdiobus_register(ds->slave_mii_bus);
> +		mdio = of_get_child_by_name(ds->dev->of_node, "mdio");
> +
> +		if (mdio) {

This probably needs to be if (of_device_is_available(mdio)), since one
could conceivably declare the switch internal MDIO bus but put a
disabled status property not to use it.
-- 
Florian

[RFC PATCH net-next v4 24/28] devicetree: net: dsa: Document use of mdio node inside switch node

[Ansuel Smith]

Switch node can contain mdio node to describe internal mdio and PHYs
connected to the switch ports.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/dsa/dsa.yaml      | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index 8a3494db4d8d..fbefaca884cc 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -15,6 +15,9 @@ description:
   This binding represents Ethernet Switches which have a dedicated CPU
   port. That port is usually connected to an Ethernet Controller of the
   SoC. Such setups are typical for embedded devices.
+  Switch can also have PHY port connected to an internal mdio bus by
+  declaring a mdio node inside the switch node and declaring the
+  phy-handle for each required port.
 
 select: false
 
@@ -87,6 +90,31 @@ patternProperties:
 
         additionalProperties: false
 
+patternProperties:
+  mdio:
+    description:
+      Describes the internal mdio of the Ethernet switch
+    type: object
+    properties:
+      '#address-cells':
+        const: 1
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      phy:
+        type: object
+        description: Ethernet switch internal PHY
+
+        properties:
+          reg:
+            description: PHY address
+
+        required:
+            - reg
+
+        additionalProperties: false
+
 oneOf:
   - required:
       - ports
-- 
2.30.2

Re: [RFC PATCH net-next v4 24/28] devicetree: net: dsa: Document use of mdio node inside switch node

[Rob Herring]

On Sat, 08 May 2021 02:29:14 +0200, Ansuel Smith wrote:
> Switch node can contain mdio node to describe internal mdio and PHYs
> connected to the switch ports.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/dsa.yaml      | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/dsa/dsa.yaml:93:1: [error] duplication of key "patternProperties" in mapping (key-duplicates)
./Documentation/devicetree/bindings/net/dsa/dsa.yaml:114:13: [warning] wrong indentation: expected 10 but found 12 (indentation)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/net/dsa/dsa.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 421, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 121, in construct_document
    for _dummy in generator:
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 714, in construct_yaml_map
    value = self.construct_mapping(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 435, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 253, in construct_mapping
    if self.check_mapping_key(node, key_node, mapping, key, value):
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 284, in check_mapping_key
    raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
  in "<unicode string>", line 4, column 1
found duplicate key "patternProperties" with value "{}" (original value: "{}")
  in "<unicode string>", line 93, column 1

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys

Duplicate keys will become an error in future releases, and are errors
by default when using the new API.

make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/net/dsa/dsa.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml: while constructing a mapping
  in "<unicode string>", line 4, column 1
found duplicate key "patternProperties" with value "{}" (original value: "{}")
  in "<unicode string>", line 93, column 1

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys

Duplicate keys will become an error in future releases, and are errors
by default when using the new API.

./Documentation/devicetree/bindings/net/dsa/hirschmann,hellcreek.yaml: while constructing a mapping
  in "<unicode string>", line 4, column 1
found duplicate key "patternProperties" with value "{}" (original value: "{}")
  in "<unicode string>", line 93, column 1

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys

Duplicate keys will become an error in future releases, and are errors
by default when using the new API.

./Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml: while constructing a mapping
  in "<unicode string>", line 4, column 1
found duplicate key "patternProperties" with value "{}" (original value: "{}")
  in "<unicode string>", line 93, column 1

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys

Duplicate keys will become an error in future releases, and are errors
by default when using the new API.

Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 67, in <module>
    ret = check_doc(f)
  File "/usr/local/bin/dt-doc-validate", line 25, in check_doc
    testtree = dtschema.load(filename, line_number=line_number)
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 625, in load
    return yaml.load(f.read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 421, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 121, in construct_document
    for _dummy in generator:
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 714, in construct_yaml_map
    value = self.construct_mapping(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 435, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 253, in construct_mapping
    if self.check_mapping_key(node, key_node, mapping, key, value):
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 284, in check_mapping_key
    raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
  in "<unicode string>", line 4, column 1
found duplicate key "patternProperties" with value "{}" (original value: "{}")
  in "<unicode string>", line 93, column 1

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys

Duplicate keys will become an error in future releases, and are errors
by default when using the new API.

make[1]: *** Deleting file 'Documentation/devicetree/bindings/processed-schema-examples.json'
Traceback (most recent call last):
  File "/usr/local/bin/dt-mk-schema", line 38, in <module>
    schemas = dtschema.process_schemas(args.schemas, core_schema=(not args.useronly))
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 587, in process_schemas
    sch = process_schema(os.path.abspath(filename))
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 561, in process_schema
    schema = load_schema(filename)
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 126, in load_schema
    return do_load(os.path.join(schema_basedir, schema))
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 112, in do_load
    return yaml.load(tmp)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 421, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 121, in construct_document
    for _dummy in generator:
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 714, in construct_yaml_map
    value = self.construct_mapping(node)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 435, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 253, in construct_mapping
    if self.check_mapping_key(node, key_node, mapping, key, value):
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 284, in check_mapping_key
    raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
  in "<unicode string>", line 4, column 1
found duplicate key "patternProperties" with value "{}" (original value: "{}")
  in "<unicode string>", line 93, column 1

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys

Duplicate keys will become an error in future releases, and are errors
by default when using the new API.

make[1]: *** [Documentation/devicetree/bindings/Makefile:62: Documentation/devicetree/bindings/processed-schema-examples.json] Error 1
make: *** [Makefile:1414: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1475695

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[RFC PATCH net-next v4 25/28] net: dsa: qca8k: add support for internal phy

[Ansuel Smith]

Add support to setup_mdio_bus for internal phy declaration. Introduce a
flag to use the legacy port phy mapping by default and use the direct
mapping if a mdio node is detected in the switch node.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index f96579c0bd46..3d195fdd7ed5 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -663,19 +663,15 @@ qca8k_mdio_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 }
 
 static int
-qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
+qca8k_mdio_write(struct qca8k_priv *priv, int phy, u32 regnum, u16 data)
 {
 	u16 r1, r2, page;
-	u32 phy, val;
+	u32 val;
 	int ret;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
 
-	/* callee is responsible for not passing bad ports,
-	 * but we still would like to make spills impossible.
-	 */
-	phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
 	val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
 	      QCA8K_MDIO_MASTER_WRITE | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
 	      QCA8K_MDIO_MASTER_REG_ADDR(regnum) |
@@ -705,19 +701,15 @@ 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)
+qca8k_mdio_read(struct qca8k_priv *priv, int phy, u32 regnum)
 {
 	u16 r1, r2, page;
-	u32 phy, val;
+	u32 val;
 	int ret;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
 
-	/* callee is responsible for not passing bad ports,
-	 * but we still would like to make spills impossible.
-	 */
-	phy = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
 	val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
 	      QCA8K_MDIO_MASTER_READ | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
 	      QCA8K_MDIO_MASTER_REG_ADDR(regnum);
@@ -758,6 +750,13 @@ qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
 {
 	struct qca8k_priv *priv = ds->priv;
 
+	/* Check if the legacy mapping should be used and the
+	 * port is not correctly mapped to the right PHY in the
+	 * devicetree
+	 */
+	if (priv->legacy_phy_port_mapping)
+		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
+
 	return qca8k_mdio_write(priv, port, regnum, data);
 }
 
@@ -767,6 +766,13 @@ qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
 	struct qca8k_priv *priv = ds->priv;
 	int ret;
 
+	/* Check if the legacy mapping should be used and the
+	 * port is not correctly mapped to the right PHY in the
+	 * devicetree
+	 */
+	if (priv->legacy_phy_port_mapping)
+		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
+
 	ret = qca8k_mdio_read(priv, port, regnum);
 
 	if (ret < 0)
@@ -779,7 +785,7 @@ static int
 qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 {
 	u32 internal_mdio_mask = 0, external_mdio_mask = 0, reg;
-	struct device_node *ports, *port;
+	struct device_node *ports, *port, *mdio;
 	int err;
 
 	ports = of_get_child_by_name(priv->dev->of_node, "ports");
@@ -800,7 +806,8 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 		if (!dsa_is_user_port(priv->ds, reg))
 			continue;
 
-		if (of_property_read_bool(port, "phy-handle"))
+		if (of_property_read_bool(port, "phy-handle") &&
+		    of_property_match_string(port, "phy-mode", "internal") < 0)
 			external_mdio_mask |= BIT(reg);
 		else
 			internal_mdio_mask |= BIT(reg);
@@ -837,6 +844,14 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 				       QCA8K_MDIO_MASTER_EN);
 	}
 
+	/* Check if the devicetree declare the port:phy mapping
+	 * If a mapping can't be found the legacy mapping is used,
+	 * using the qca8k_port_to_phy function
+	 */
+	mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
+	if (!mdio)
+		priv->legacy_phy_port_mapping = true;
+
 	priv->ops.phy_read = qca8k_phy_read;
 	priv->ops.phy_write = qca8k_phy_write;
 	return 0;
@@ -1198,7 +1213,8 @@ qca8k_phylink_validate(struct dsa_switch *ds, int port,
 	case 5:
 		/* Internal PHY */
 		if (state->interface != PHY_INTERFACE_MODE_NA &&
-		    state->interface != PHY_INTERFACE_MODE_GMII)
+		    state->interface != PHY_INTERFACE_MODE_GMII &&
+		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
 			goto unsupported;
 		break;
 	case 6: /* 2nd CPU port / external PHY */
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index d365f85ab34f..ed3b05ad6745 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -255,6 +255,7 @@ struct qca8k_priv {
 	u8 switch_revision;
 	u8 rgmii_tx_delay;
 	u8 rgmii_rx_delay;
+	bool legacy_phy_port_mapping;
 	struct regmap *regmap;
 	struct mii_bus *bus;
 	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
-- 
2.30.2

[RFC PATCH net-next v4 26/28] net: dsa: permit driver to provide custom phy_mii_mask for slave mdiobus

[Ansuel Smith]

Some switch doesn't have a 1:1 map phy to port. Permit driver to provide
a custom phy_mii_mask so the internal mdiobus can correctly use the
provided phy reg as it can differ from the port reg.
The qca8k driver is provided as a first user of this function.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 30 ++++++++++++++++++++++++++++++
 include/net/dsa.h       |  7 +++++++
 net/dsa/dsa2.c          |  7 ++++++-
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 3d195fdd7ed5..3c3e05735b2d 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1685,7 +1685,37 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 	return DSA_TAG_PROTO_QCA;
 }
 
+static u32
+qca8k_get_phys_mii_mask(struct dsa_switch *ds)
+{
+	struct device_node *mdio, *phy;
+	u32 reg, phy_mii_mask = 0;
+	int err;
+
+	mdio = of_get_child_by_name(ds->dev->of_node, "mdio");
+	if (mdio) {
+		for_each_available_child_of_node(mdio, phy) {
+			err = of_property_read_u32(phy, "reg", &reg);
+			if (err) {
+				of_node_put(phy);
+				of_node_put(mdio);
+				return 0;
+			}
+
+			phy_mii_mask |= BIT(reg);
+		}
+
+		of_node_put(mdio);
+		return phy_mii_mask;
+	}
+
+	/* Fallback to the lagacy mapping if mdio node is not found */
+	dev_warn(ds->dev, "Using the legacy phys_mii_mapping. Consider updating the dts.");
+	return dsa_user_ports(ds);
+}
+
 static const struct dsa_switch_ops qca8k_switch_ops = {
+	.get_phys_mii_mask	= qca8k_get_phys_mii_mask,
 	.get_tag_protocol	= qca8k_get_tag_protocol,
 	.setup			= qca8k_setup,
 	.get_strings		= qca8k_get_strings,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 83a933e563fe..4003ffc659a4 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -511,6 +511,13 @@ struct dsa_switch_ops {
 	void	(*teardown)(struct dsa_switch *ds);
 	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
 
+	/*
+	 * Provide a custom phys_mii_mask for the dsa slave mdiobus instead
+	 * of relying on the dsa_user_ports. Not every switch has a 1:1 map
+	 * port to PHY, hence the driver can provide their fixed mask.
+	 */
+	u32	(*get_phys_mii_mask)(struct dsa_switch *ds);
+
 	/*
 	 * Access to the switch's PHY registers.
 	 */
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 79adabe3e2a7..7eabd4d67849 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -682,8 +682,13 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	 * driver and before ops->setup() has run, since the switch drivers and
 	 * the slave MDIO bus driver rely on these values for probing PHY
 	 * devices or not
+	 * Driver can provide his on mask as some switch doesn't have a 1:1 map
+	 * phy to port.
 	 */
-	ds->phys_mii_mask |= dsa_user_ports(ds);
+	if (ds->ops->get_phys_mii_mask)
+		ds->phys_mii_mask = ds->ops->get_phys_mii_mask(ds);
+	else
+		ds->phys_mii_mask |= dsa_user_ports(ds);
 
 	/* Add the switch to devlink before calling setup, so that setup can
 	 * add dpipe tables
-- 
2.30.2

Re: [RFC PATCH net-next v4 26/28] net: dsa: permit driver to provide custom phy_mii_mask for slave mdiobus

[Florian Fainelli]

On 5/7/2021 5:29 PM, Ansuel Smith wrote:
> Some switch doesn't have a 1:1 map phy to port. Permit driver to provide
> a custom phy_mii_mask so the internal mdiobus can correctly use the
> provided phy reg as it can differ from the port reg.
> The qca8k driver is provided as a first user of this function.

Why not have qca8k be in charge of allocating its internal MDIO bus like
what mv88e6xxx or bcm_sf2 do? That would allow you to do all sorts of
customization there and you could skip having patches 23 and 24.
-- 
Florian

Re: [RFC PATCH net-next v4 26/28] net: dsa: permit driver to provide custom phy_mii_mask for slave mdiobus

[Ansuel Smith]

On Sat, May 08, 2021 at 08:52:03AM -0700, Florian Fainelli wrote:
> 
> 
> On 5/7/2021 5:29 PM, Ansuel Smith wrote:
> > Some switch doesn't have a 1:1 map phy to port. Permit driver to provide
> > a custom phy_mii_mask so the internal mdiobus can correctly use the
> > provided phy reg as it can differ from the port reg.
> > The qca8k driver is provided as a first user of this function.
> 
> Why not have qca8k be in charge of allocating its internal MDIO bus like
> what mv88e6xxx or bcm_sf2 do? That would allow you to do all sorts of
> customization there and you could skip having patches 23 and 24.

Oh ok, I will implement the internal MDIO bus directly in the qca8k
driver. Was thinking... Should I keep the extra mdio node and move the
documentation to qca8k or should I handle all of that internally?
To me it looks cleaner the direct definition in the devicetree than the 
port_to_phy function in the driver but as you can see extra checks are
required to handle the new implementation and still supports the old
one.

[RFC PATCH net-next v4 27/28] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Ansuel Smith]

Define get_phy_flags to pass switch_Revision needed to tweak the
internal PHY with debug values based on the revision.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 3c3e05735b2d..8dafa581b7fa 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1678,6 +1678,22 @@ qca8k_port_vlan_del(struct dsa_switch *ds, int port,
 	return ret;
 }
 
+static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	/* Communicate to the phy internal driver the switch revision.
+	 * Based on the switch revision different values needs to be
+	 * set to the dbg and mmd reg on the phy.
+	 * The first 2 bit are used to communicate the switch revision
+	 * to the phy driver.
+	 */
+	if (port > 0 && port < 6)
+		return priv->switch_revision;
+
+	return 0;
+}
+
 static enum dsa_tag_protocol
 qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 		       enum dsa_tag_protocol mp)
@@ -1741,6 +1757,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.phylink_mac_config	= qca8k_phylink_mac_config,
 	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,
 	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,
+	.get_phy_flags		= qca8k_get_phy_flags,
 };
 
 static int qca8k_read_switch_id(struct qca8k_priv *priv)
-- 
2.30.2

Re: [RFC PATCH net-next v4 27/28] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Florian Fainelli]

On 5/7/2021 5:29 PM, Ansuel Smith wrote:
> Define get_phy_flags to pass switch_Revision needed to tweak the
> internal PHY with debug values based on the revision.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[RFC PATCH net-next v4 28/28] net: phy: add qca8k driver for qca8k switch internal PHY

[Ansuel Smith]

Add initial support for qca8k internal PHYs. The internal PHYs requires
special mmd and debug values to be set based on the switch revision
passwd using the dev_flags. Supports output of idle, receive and eee_wake
errors stats.
Some debug values sets can't be translated as the documentation lacks any
reference about them.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/phy/Kconfig  |   7 ++
 drivers/net/phy/Makefile |   1 +
 drivers/net/phy/qca8k.c  | 172 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 180 insertions(+)
 create mode 100644 drivers/net/phy/qca8k.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 698bea312adc..cdf01613eb37 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -245,6 +245,13 @@ config QSEMI_PHY
 	help
 	  Currently supports the qs6612
 
+config QCA8K_PHY
+	tristate "Qualcomm Atheros AR833x Internal PHYs"
+	help
+	  This PHY is for the internal PHYs present on the QCA833x switch.
+
+	  Currently supports the AR8334, AR8337 model
+
 config REALTEK_PHY
 	tristate "Realtek PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf..5f3cfd5606bb 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -72,6 +72,7 @@ obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
 obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
+obj-$(CONFIG_QCA8K_PHY)		+= qca8k.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
 obj-$(CONFIG_RENESAS_PHY)	+= uPD60620.o
 obj-$(CONFIG_ROCKCHIP_PHY)	+= rockchip.o
diff --git a/drivers/net/phy/qca8k.c b/drivers/net/phy/qca8k.c
new file mode 100644
index 000000000000..53bbd506d184
--- /dev/null
+++ b/drivers/net/phy/qca8k.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0+
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+#include <linux/module.h>
+#include <linux/bitfield.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool_netlink.h>
+
+#define QCA8K_DEVFLAGS_REVISION_MASK		GENMASK(2, 0)
+
+#define QCA8K_PHY_ID_MASK			0xffffffff
+#define QCA8K_PHY_ID_QCA8327			0x004dd034
+#define QCA8K_PHY_ID_QCA8337			0x004dd036
+
+#define MDIO_AZ_DEBUG				0x800d
+
+#define MDIO_DBG_ANALOG_TEST			0x0
+#define MDIO_DBG_SYSTEM_CONTROL_MODE		0x5
+#define MDIO_DBG_CONTROL_FEATURE_CONF		0x3d
+
+/* QCA specific MII registers */
+#define MII_ATH_DBG_ADDR			0x1d
+#define MII_ATH_DBG_DATA			0x1e
+
+/* QCA specific MII registers access function */
+static void qca8k_phy_dbg_write(struct mii_bus *bus, int phy_addr, u16 dbg_addr, u16 dbg_data)
+{
+	bus->write(bus, phy_addr, MII_ATH_DBG_ADDR, dbg_addr);
+	bus->write(bus, phy_addr, MII_ATH_DBG_DATA, dbg_data);
+}
+
+enum stat_access_type {
+	PHY,
+	MMD
+};
+
+struct qca8k_hw_stat {
+	const char *string;
+	u8 reg;
+	u32 mask;
+	enum stat_access_type access_type;
+};
+
+static struct qca8k_hw_stat qca8k_hw_stats[] = {
+	{ "phy_idle_errors", 0xa, GENMASK(7, 0), PHY},
+	{ "phy_receive_errors", 0x15, GENMASK(15, 0), PHY},
+	{ "eee_wake_errors", 0x16, GENMASK(15, 0), MMD},
+};
+
+struct qca8k_phy_priv {
+	u8 switch_revision;
+	u64 stats[ARRAY_SIZE(qca8k_hw_stats)];
+};
+
+static int qca8k_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(qca8k_hw_stats);
+}
+
+static void qca8k_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(qca8k_hw_stats); i++) {
+		strscpy(data + i * ETH_GSTRING_LEN,
+			qca8k_hw_stats[i].string, ETH_GSTRING_LEN);
+	}
+}
+
+static u64 qca8k_get_stat(struct phy_device *phydev, int i)
+{
+	struct qca8k_hw_stat stat = qca8k_hw_stats[i];
+	struct qca8k_phy_priv *priv = phydev->priv;
+	int val;
+	u64 ret;
+
+	if (stat.access_type == MMD)
+		val = phy_read_mmd(phydev, MDIO_MMD_PCS, stat.reg);
+	else
+		val = phy_read(phydev, stat.reg);
+
+	if (val < 0) {
+		ret = U64_MAX;
+	} else {
+		val = val & stat.mask;
+		priv->stats[i] += val;
+		ret = priv->stats[i];
+	}
+
+	return ret;
+}
+
+static void qca8k_get_stats(struct phy_device *phydev,
+			    struct ethtool_stats *stats, u64 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(qca8k_hw_stats); i++)
+		data[i] = qca8k_get_stat(phydev, i);
+}
+
+static int qca8k_config_init(struct phy_device *phydev)
+{
+	struct qca8k_phy_priv *priv = phydev->priv;
+	struct mii_bus *bus = phydev->mdio.bus;
+	int phy_addr = phydev->mdio.addr;
+
+	priv->switch_revision = phydev->dev_flags & QCA8K_DEVFLAGS_REVISION_MASK;
+
+	switch (priv->switch_revision) {
+	case 1:
+		/* For 100M waveform */
+		qca8k_phy_dbg_write(bus, phy_addr, MDIO_DBG_ANALOG_TEST, 0x02ea);
+		/* Turn on Gigabit clock */
+		qca8k_phy_dbg_write(bus, phy_addr, MDIO_DBG_CONTROL_FEATURE_CONF, 0x68a0);
+		break;
+
+	case 2:
+		phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0x0);
+		fallthrough;
+	case 4:
+		phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_AZ_DEBUG, 0x803f);
+		qca8k_phy_dbg_write(bus, phy_addr, MDIO_DBG_CONTROL_FEATURE_CONF, 0x6860);
+		qca8k_phy_dbg_write(bus, phy_addr, MDIO_DBG_SYSTEM_CONTROL_MODE, 0x2c46);
+		qca8k_phy_dbg_write(bus, phy_addr, 0x3c, 0x6000);
+		break;
+	}
+
+	return 0;
+}
+
+static int qca8k_probe(struct phy_device *phydev)
+{
+	struct qca8k_phy_priv *priv;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
+static struct phy_driver qca8k_drivers[] = {
+	{
+		.phy_id = QCA8K_PHY_ID_QCA8337,
+		.phy_id_mask = QCA8K_PHY_ID_MASK,
+		.name = "QCA PHY 8337",
+		/* PHY_GBIT_FEATURES */
+		.probe = qca8k_probe,
+		.flags = PHY_IS_INTERNAL,
+		.config_init = qca8k_config_init,
+		.soft_reset = genphy_soft_reset,
+		.get_sset_count = qca8k_get_sset_count,
+		.get_strings = qca8k_get_strings,
+		.get_stats = qca8k_get_stats,
+	},
+};
+
+module_phy_driver(qca8k_drivers);
+
+static struct mdio_device_id __maybe_unused qca8k_tbl[] = {
+	{ QCA8K_PHY_ID_QCA8337, QCA8K_PHY_ID_MASK },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, qca8k_tbl);
+MODULE_DESCRIPTION("Qualcomm QCA8k PHY driver");
+MODULE_AUTHOR("Ansuel Smith");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [RFC PATCH net-next v4 28/28] net: phy: add qca8k driver for qca8k switch internal PHY

[DENG Qingfang]

On Sat, May 08, 2021 at 02:29:18AM +0200, Ansuel Smith wrote:
> Add initial support for qca8k internal PHYs. The internal PHYs requires
> special mmd and debug values to be set based on the switch revision
> passwd using the dev_flags. Supports output of idle, receive and eee_wake
> errors stats.
> Some debug values sets can't be translated as the documentation lacks any
> reference about them.

I think this can be merged into at803x.c, as they have almost the same
registers, and some features such as interrupt handler and cable test
can be reused.

> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Re: [RFC PATCH net-next v4 28/28] net: phy: add qca8k driver for qca8k switch internal PHY

[Ansuel Smith]

On Sat, May 08, 2021 at 12:35:35PM +0800, DENG Qingfang wrote:
> On Sat, May 08, 2021 at 02:29:18AM +0200, Ansuel Smith wrote:
> > Add initial support for qca8k internal PHYs. The internal PHYs requires
> > special mmd and debug values to be set based on the switch revision
> > passwd using the dev_flags. Supports output of idle, receive and eee_wake
> > errors stats.
> > Some debug values sets can't be translated as the documentation lacks any
> > reference about them.
> 
> I think this can be merged into at803x.c, as they have almost the same
> registers, and some features such as interrupt handler and cable test
> can be reused.
>

Wouldn't this be a little bit confusing? But actually yes... interrupt
handler and cable test have the same regs. My main concern is about the
phy_dev flags and the dbg regs that I think are different and would
create some confusion. If this It's not a proble, sure I can rework this
a put in the at803x.c phy driver.

> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---

Re: [RFC PATCH net-next v4 28/28] net: phy: add qca8k driver for qca8k switch internal PHY

[Florian Fainelli]

On 5/8/2021 4:30 AM, Ansuel Smith wrote:
> On Sat, May 08, 2021 at 12:35:35PM +0800, DENG Qingfang wrote:
>> On Sat, May 08, 2021 at 02:29:18AM +0200, Ansuel Smith wrote:
>>> Add initial support for qca8k internal PHYs. The internal PHYs requires
>>> special mmd and debug values to be set based on the switch revision
>>> passwd using the dev_flags. Supports output of idle, receive and eee_wake
>>> errors stats.
>>> Some debug values sets can't be translated as the documentation lacks any
>>> reference about them.
>>
>> I think this can be merged into at803x.c, as they have almost the same
>> registers, and some features such as interrupt handler and cable test
>> can be reused.
>>
> 
> Wouldn't this be a little bit confusing? But actually yes... interrupt
> handler and cable test have the same regs. My main concern is about the
> phy_dev flags and the dbg regs that I think are different and would
> create some confusion. If this It's not a proble, sure I can rework this
> a put in the at803x.c phy driver.

Consistency is key, and having all of your PHY eggs in the same PHY
driver basket is easier for maintenance and generalizing features.
drivers/net/phy/broadcom.c contains PHY entries for integrated PHY
switches, too (5395, 53125). As far as phydev::dev_flags, you can skip
interpreting those unless you match the QCA8K PHY driver entry/probe.
-- 
Florian

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Florian Fainelli]

On 5/7/2021 5:28 PM, Ansuel Smith wrote:
> Fix mixed whitespace and tab for define spacing.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Joe Perches]

On Sat, 2021-05-08 at 02:28 +0200, Ansuel Smith wrote:
> Fix mixed whitespace and tab for define spacing.

Incomplete commit message here.

> diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c
[]
> -#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_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)

Adding a define and using it isn't just whitespace.

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Andrew Lunn]

On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> Fix mixed whitespace and tab for define spacing.

Please add a patch [0/28] which describes the big picture of what
these changes are doing.

Also, this series is getting big. You might want to split it into two,
One containing the cleanup, and the second adding support for the new
switch.

	Andrew

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Ansuel Smith]

On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
> On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> > Fix mixed whitespace and tab for define spacing.
> 
> Please add a patch [0/28] which describes the big picture of what
> these changes are doing.
> 
> Also, this series is getting big. You might want to split it into two,
> One containing the cleanup, and the second adding support for the new
> switch.
> 
> 	Andrew

There is a 0/28. With all the changes. Could be that I messed the cc?
I agree think it's better to split this for the mdio part, the cleanup
and the changes needed for the internal phy.

Can I keep the review tag?

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Jonathan McDowell]

On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:
> On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
> > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> > > Fix mixed whitespace and tab for define spacing.
> > 
> > Please add a patch [0/28] which describes the big picture of what
> > these changes are doing.
> > 
> > Also, this series is getting big. You might want to split it into two,
> > One containing the cleanup, and the second adding support for the new
> > switch.
> > 
> > 	Andrew
> 
> There is a 0/28. With all the changes. Could be that I messed the cc?
> I agree think it's better to split this for the mdio part, the cleanup
> and the changes needed for the internal phy.

FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011
today. I currently use the GPIO MDIO driver because I saw issues with
the IPQ8064 driver in the past, and sticking with the GPIO driver I see
both QCA8337 devices and traffic flows as expected, so no obvious
regressions from your changes.

I also tried switching to the IPQ8064 MDIO driver for my first device
(which is on the MDIO0 bus), but it's still not happy:

qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13

J.

-- 
One of the nice things about standards is that there are so many of
them.

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Florian Fainelli]

On 5/15/2021 10:00 AM, Jonathan McDowell wrote:
> On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:
>> On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
>>> On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
>>>> Fix mixed whitespace and tab for define spacing.
>>>
>>> Please add a patch [0/28] which describes the big picture of what
>>> these changes are doing.
>>>
>>> Also, this series is getting big. You might want to split it into two,
>>> One containing the cleanup, and the second adding support for the new
>>> switch.
>>>
>>> 	Andrew
>>
>> There is a 0/28. With all the changes. Could be that I messed the cc?
>> I agree think it's better to split this for the mdio part, the cleanup
>> and the changes needed for the internal phy.
> 
> FWIW I didn't see the 0/28 mail either.I tried these out on my RB3011
> today. I currently use the GPIO MDIO driver because I saw issues with
> the IPQ8064 driver in the past, and sticking with the GPIO driver I see
> both QCA8337 devices and traffic flows as expected, so no obvious
> regressions from your changes.

The cover letter somehow appeared as the final patch in the submission
instead of having all patches in-reply-to it as one would expect.

Russell had some additional feedback that came in during or after the
patches being applied so it would be nice to address that.

> 
> I also tried switching to the IPQ8064 MDIO driver for my first device
> (which is on the MDIO0 bus), but it's still not happy:
> 
> qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13

If you do repeated reads of the revision register to you eventually get
13 as intended?
-- 
Florian

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Ansuel Smith]

On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:
> On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:
> > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
> > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> > > > Fix mixed whitespace and tab for define spacing.
> > > 
> > > Please add a patch [0/28] which describes the big picture of what
> > > these changes are doing.
> > > 
> > > Also, this series is getting big. You might want to split it into two,
> > > One containing the cleanup, and the second adding support for the new
> > > switch.
> > > 
> > > 	Andrew
> > 
> > There is a 0/28. With all the changes. Could be that I messed the cc?
> > I agree think it's better to split this for the mdio part, the cleanup
> > and the changes needed for the internal phy.
> 
> FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011
> today. I currently use the GPIO MDIO driver because I saw issues with
> the IPQ8064 driver in the past, and sticking with the GPIO driver I see
> both QCA8337 devices and traffic flows as expected, so no obvious
> regressions from your changes.
> 
> I also tried switching to the IPQ8064 MDIO driver for my first device
> (which is on the MDIO0 bus), but it's still not happy:
> 
> qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13
> 
> J.
> 
> -- 
> One of the nice things about standards is that there are so many of
> them.

Can you try the v6 version of this series? Anyway the fact that 0 is
produced instead of a wrong value let me think that there is some
problem with the mdio read. (on other device there was a problem of
wrong id read but nerver 0). Could be that the bootloader on your board
set the mdio MASTER disabled. I experienced this kind of problem when
switching from the dsa driver and the legacy swconfig driver. On remove
of the dsa driver, the swconfig didn't work as the bit was never cleared
by the dsa driver and resulted in your case. (id 0 read from the mdio)

Do you want to try a quick patch so we can check if this is the case?
(about the cover letter... sorry will check why i'm pushing this wrong)

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Jonathan McDowell]

On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:
> On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:
> > On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:
> > > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
> > > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> > > > > Fix mixed whitespace and tab for define spacing.
> > > > 
> > > > Please add a patch [0/28] which describes the big picture of what
> > > > these changes are doing.
> > > > 
> > > > Also, this series is getting big. You might want to split it into two,
> > > > One containing the cleanup, and the second adding support for the new
> > > > switch.
> > > > 
> > > > 	Andrew
> > > 
> > > There is a 0/28. With all the changes. Could be that I messed the cc?
> > > I agree think it's better to split this for the mdio part, the cleanup
> > > and the changes needed for the internal phy.
> > 
> > FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011
> > today. I currently use the GPIO MDIO driver because I saw issues with
> > the IPQ8064 driver in the past, and sticking with the GPIO driver I see
> > both QCA8337 devices and traffic flows as expected, so no obvious
> > regressions from your changes.
> > 
> > I also tried switching to the IPQ8064 MDIO driver for my first device
> > (which is on the MDIO0 bus), but it's still not happy:
> > 
> > qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13
> > 
> 
> Can you try the v6 version of this series?

Both the v6 qca8k series and the separate ipq806x mdio series, yes?

> Anyway the fact that 0 is produced instead of a wrong value let me
> think that there is some problem with the mdio read. (on other device
> there was a problem of wrong id read but nerver 0). Could be that the
> bootloader on your board set the mdio MASTER disabled. I experienced
> this kind of problem when switching from the dsa driver and the legacy
> swconfig driver. On remove of the dsa driver, the swconfig didn't work
> as the bit was never cleared by the dsa driver and resulted in your
> case. (id 0 read from the mdio)
> 
> Do you want to try a quick patch so we can check if this is the case?
> (about the cover letter... sorry will check why i'm pushing this
> wrong)

There's definitely something odd going on here. I went back to mainline
to see what the situation is there. With the GPIO MDIO driver both
switches work (expected, as this is what I run with). I changed switch0
over to use the IPQ MDIO driver and it wasn't detected (but switch1
still on the GPIO MDIO driver was fine).

I then tried putting both switches onto the IPQ MDIO driver and in that
instance switch0 came up fine, while switch1 wasn't detected.

If there's a simple patch that might give more debug I can try it out.

J.

-- 
   "Reality Or Nothing!" -- Cold   |  .''`.  Debian GNU/Linux Developer
              Lazarus              | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Ansuel Smith]

On Sat, May 15, 2021 at 07:08:57PM +0100, Jonathan McDowell wrote:
> On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:
> > On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:
> > > On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:
> > > > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
> > > > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> > > > > > Fix mixed whitespace and tab for define spacing.
> > > > > 
> > > > > Please add a patch [0/28] which describes the big picture of what
> > > > > these changes are doing.
> > > > > 
> > > > > Also, this series is getting big. You might want to split it into two,
> > > > > One containing the cleanup, and the second adding support for the new
> > > > > switch.
> > > > > 
> > > > > 	Andrew
> > > > 
> > > > There is a 0/28. With all the changes. Could be that I messed the cc?
> > > > I agree think it's better to split this for the mdio part, the cleanup
> > > > and the changes needed for the internal phy.
> > > 
> > > FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011
> > > today. I currently use the GPIO MDIO driver because I saw issues with
> > > the IPQ8064 driver in the past, and sticking with the GPIO driver I see
> > > both QCA8337 devices and traffic flows as expected, so no obvious
> > > regressions from your changes.
> > > 
> > > I also tried switching to the IPQ8064 MDIO driver for my first device
> > > (which is on the MDIO0 bus), but it's still not happy:
> > > 
> > > qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13
> > > 
> > 
> > Can you try the v6 version of this series?
> 
> Both the v6 qca8k series and the separate ipq806x mdio series, yes?
> 
> > Anyway the fact that 0 is produced instead of a wrong value let me
> > think that there is some problem with the mdio read. (on other device
> > there was a problem of wrong id read but nerver 0). Could be that the
> > bootloader on your board set the mdio MASTER disabled. I experienced
> > this kind of problem when switching from the dsa driver and the legacy
> > swconfig driver. On remove of the dsa driver, the swconfig didn't work
> > as the bit was never cleared by the dsa driver and resulted in your
> > case. (id 0 read from the mdio)
> > 
> > Do you want to try a quick patch so we can check if this is the case?
> > (about the cover letter... sorry will check why i'm pushing this
> > wrong)
> 
> There's definitely something odd going on here. I went back to mainline
> to see what the situation is there. With the GPIO MDIO driver both
> switches work (expected, as this is what I run with). I changed switch0
> over to use the IPQ MDIO driver and it wasn't detected (but switch1
> still on the GPIO MDIO driver was fine).
> 
> I then tried putting both switches onto the IPQ MDIO driver and in that
> instance switch0 came up fine, while switch1 wasn't detected.
> 

Oh wait, your board have 2 different switch? So they both use the master
bit when used... Mhhh I need to think about this if there is a clean way
to handle this. The idea would be that one of the 2 dsa switch should
use the already defined mdio bus.

The problem here is that to use the internal mdio bus, a bit must be
set or 0 is read on every value (as the bit actually disable the internal
mdio). This is good if one dsa driver is used but when 2 or more are
used I think this clash and only one of them work. The gpio mdio path is
not affected by this. Will check if I can find some way to address this.

> If there's a simple patch that might give more debug I can try it out.
> 
> J.
>
> -- 
>    "Reality Or Nothing!" -- Cold   |  .''`.  Debian GNU/Linux Developer
>               Lazarus              | : :' :  Happy to accept PGP signed
>                                    | `. `'   or encrypted mail - RSA
> |   `-    key on the keyservers.

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Jonathan McDowell]

On Sat, May 15, 2021 at 08:20:40PM +0200, Ansuel Smith wrote:
> On Sat, May 15, 2021 at 07:08:57PM +0100, Jonathan McDowell wrote:
> > On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:
> > > Do you want to try a quick patch so we can check if this is the case?
> > > (about the cover letter... sorry will check why i'm pushing this
> > > wrong)
> > 
> > There's definitely something odd going on here. I went back to mainline
> > to see what the situation is there. With the GPIO MDIO driver both
> > switches work (expected, as this is what I run with). I changed switch0
> > over to use the IPQ MDIO driver and it wasn't detected (but switch1
> > still on the GPIO MDIO driver was fine).
> > 
> > I then tried putting both switches onto the IPQ MDIO driver and in that
> > instance switch0 came up fine, while switch1 wasn't detected.
> > 
> 
> Oh wait, your board have 2 different switch? So they both use the master
> bit when used... Mhhh I need to think about this if there is a clean way
> to handle this. The idea would be that one of the 2 dsa switch should
> use the already defined mdio bus.
> 
> The problem here is that to use the internal mdio bus, a bit must be
> set or 0 is read on every value (as the bit actually disable the internal
> mdio). This is good if one dsa driver is used but when 2 or more are
> used I think this clash and only one of them work. The gpio mdio path is
> not affected by this. Will check if I can find some way to address this.

They're on 2 separate sets of GPIOs if that makes a difference - switch0
is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic
shared between these? Also even if that's the case it seems odd that
enabling the MDIO for just switch0 doesn't work?

J.

-- 
101 things you can't have too much of : 3 - Sleep.

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Ansuel Smith]

On Sat, May 15, 2021 at 08:40:47PM +0100, Jonathan McDowell wrote:
> On Sat, May 15, 2021 at 08:20:40PM +0200, Ansuel Smith wrote:
> > On Sat, May 15, 2021 at 07:08:57PM +0100, Jonathan McDowell wrote:
> > > On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:
> > > > Do you want to try a quick patch so we can check if this is the case?
> > > > (about the cover letter... sorry will check why i'm pushing this
> > > > wrong)
> > > 
> > > There's definitely something odd going on here. I went back to mainline
> > > to see what the situation is there. With the GPIO MDIO driver both
> > > switches work (expected, as this is what I run with). I changed switch0
> > > over to use the IPQ MDIO driver and it wasn't detected (but switch1
> > > still on the GPIO MDIO driver was fine).
> > > 
> > > I then tried putting both switches onto the IPQ MDIO driver and in that
> > > instance switch0 came up fine, while switch1 wasn't detected.
> > > 
> > 
> > Oh wait, your board have 2 different switch? So they both use the master
> > bit when used... Mhhh I need to think about this if there is a clean way
> > to handle this. The idea would be that one of the 2 dsa switch should
> > use the already defined mdio bus.
> > 
> > The problem here is that to use the internal mdio bus, a bit must be
> > set or 0 is read on every value (as the bit actually disable the internal
> > mdio). This is good if one dsa driver is used but when 2 or more are
> > used I think this clash and only one of them work. The gpio mdio path is
> > not affected by this. Will check if I can find some way to address this.
> 
> They're on 2 separate sets of GPIOs if that makes a difference - switch0
> is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic
> shared between these? Also even if that's the case it seems odd that
> enabling the MDIO for just switch0 doesn't work?
> 

The dedicated internal mdio on ipq8064 is unique and present on the
gmac0 address so yes it's shared between them. And this seems to be the
problem... As you notice the fact that different gpio are used for the
different switch fix the problem. So think that to use the dedicated
mdio bus with both switch we need to introduce some type of
syncronization or something like that.

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Andrew Lunn]

> > They're on 2 separate sets of GPIOs if that makes a difference - switch0
> > is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic
> > shared between these? Also even if that's the case it seems odd that
> > enabling the MDIO for just switch0 doesn't work?
> > 
> 
> The dedicated internal mdio on ipq8064 is unique and present on the
> gmac0 address so yes it's shared between them. And this seems to be the
> problem... As you notice the fact that different gpio are used for the
> different switch fix the problem. So think that to use the dedicated
> mdio bus with both switch we need to introduce some type of
> syncronization or something like that.

Please could you describe the hardware in a bit more details. Or point
me at a datasheet. It sounds like you have an MDIO mux? Linux has this
concept, so you might need to implement a mux driver.

	 Andrew

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Ansuel Smith]

On Sun, May 16, 2021 at 01:52:05AM +0200, Andrew Lunn wrote:
> > > They're on 2 separate sets of GPIOs if that makes a difference - switch0
> > > is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic
> > > shared between these? Also even if that's the case it seems odd that
> > > enabling the MDIO for just switch0 doesn't work?
> > > 
> > 
> > The dedicated internal mdio on ipq8064 is unique and present on the
> > gmac0 address so yes it's shared between them. And this seems to be the
> > problem... As you notice the fact that different gpio are used for the
> > different switch fix the problem. So think that to use the dedicated
> > mdio bus with both switch we need to introduce some type of
> > syncronization or something like that.
> 
> Please could you describe the hardware in a bit more details. Or point
> me at a datasheet. It sounds like you have an MDIO mux? Linux has this
> concept, so you might need to implement a mux driver.
> 
> 	 Andrew

Datasheet of ipq8064 are hard to find and pricey.
Will try hoping I don't write something very wrong.
Anyway on the SoC there are 4 gmac (most of the time 2 are used
and represent the 2 cpu port) and one mdio bus present on the gmac0
address. 
Normally on these SoC they add a qca8337 switch and it's common to use
2 gmac port as cpu port. The switch can be interfaced using UART or MDIO.
Normally the uart interface is used, but it's slower than using the mdio
dedicated interface. Only mdio or uart can be used as the switch use the
same pins.
So I think the problem here is that only one switch can use the mdio bus
exposed by gmac0 and any other must use a gpio slow path.
Anyway about the use of the MASTER path and all this mess, the 
qca8k: extend slave-bus implementations series contains lots of info
about this[1].

[1] http://patchwork.ozlabs.org/project/netdev/patch/20190319195419.12746-3-chunkeey@gmail.com/

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Jonathan McDowell]

On Sun, May 16, 2021 at 02:23:18AM +0200, Ansuel Smith wrote:
> On Sun, May 16, 2021 at 01:52:05AM +0200, Andrew Lunn wrote:
> > > > They're on 2 separate sets of GPIOs if that makes a difference - switch0
> > > > is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic
> > > > shared between these? Also even if that's the case it seems odd that
> > > > enabling the MDIO for just switch0 doesn't work?
> > > > 
> > > 
> > > The dedicated internal mdio on ipq8064 is unique and present on the
> > > gmac0 address so yes it's shared between them. And this seems to be the
> > > problem... As you notice the fact that different gpio are used for the
> > > different switch fix the problem. So think that to use the dedicated
> > > mdio bus with both switch we need to introduce some type of
> > > syncronization or something like that.
> > 
> > Please could you describe the hardware in a bit more details. Or point
> > me at a datasheet. It sounds like you have an MDIO mux? Linux has this
> > concept, so you might need to implement a mux driver.
> > 
> > 	 Andrew
> 
> Datasheet of ipq8064 are hard to find and pricey.
> Will try hoping I don't write something very wrong.
> Anyway on the SoC there are 4 gmac (most of the time 2 are used
> and represent the 2 cpu port) and one mdio bus present on the gmac0
> address. 

There's a suggestion of an additional mdio bus on the gmac1 address at:

https://github.com/adron-s/openwrt-rb3011/commit/dd63b3ef563fa77fd2fb7d6ca12ca9411cd18740

is that not accurate?

J.

-- 
   Funny how life imitates LSD.    |  .''`.  Debian GNU/Linux Developer
                                   | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Jonathan McDowell]

On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:
> On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:
> > On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:
> > > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
> > > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> > > > > Fix mixed whitespace and tab for define spacing.
> > > > 
> > > > Please add a patch [0/28] which describes the big picture of what
> > > > these changes are doing.
> > > > 
> > > > Also, this series is getting big. You might want to split it into two,
> > > > One containing the cleanup, and the second adding support for the new
> > > > switch.
> > > > 
> > > > 	Andrew
> > > 
> > > There is a 0/28. With all the changes. Could be that I messed the cc?
> > > I agree think it's better to split this for the mdio part, the cleanup
> > > and the changes needed for the internal phy.
> > 
> > FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011
> > today. I currently use the GPIO MDIO driver because I saw issues with
> > the IPQ8064 driver in the past, and sticking with the GPIO driver I see
> > both QCA8337 devices and traffic flows as expected, so no obvious
> > regressions from your changes.
> > 
> > I also tried switching to the IPQ8064 MDIO driver for my first device
> > (which is on the MDIO0 bus), but it's still not happy:
> > 
> > qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13
> > 
> Can you try the v6 version of this series?

FWIW I tried v6 without altering my DT at all (so still using the GPIO
MDIO driver, and not switching to use the alternate PHY support) and got
an oops due to a NULL pointer dereference, apparently in the mdio_bus
locking code. I'm back porting to 5.10.37 (because I track LTS on the
device) so I might be missing something, but the v4 version I tried
previously worked ok.

8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000558
pgd = (ptrval)
[00000558] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.10.37-00045-g7d01aa2545cb #2
Hardware name: Generic DT based system
Workqueue: events deferred_probe_work_func
PC is at mutex_lock+0x20/0x50
LR is at _cond_resched+0x28/0x4c
pc : [<c0a36b24>]    lr : [<c0a34530>]    psr: 60000013
sp : c1569c38  ip : 00000001  fp : c17bd1dc
r10: c17bd1c0  r9 : 00000000  r8 : 00000000
r7 : 00000002  r6 : 00000558  r5 : 00000000  r4 : 00000558
r3 : c1560000  r2 : c0e54389  r1 : 00000000  r0 : 00000000
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5787d  Table: 4220406a  DAC: 00000051
Process kworker/0:1 (pid: 20, stack limit = 0x(ptrval))
Stack: (0xc1569c38 to 0xc156a000)
9c20:                                                       c8020000 c07ee9b8
9c40: c17cc000 c17cc000 00000001 c07eea54 c17cc000 c07e25b8 c1569d14 c06d0f14
9c60: 00000000 30353630 ffff0a00 c17cc558 c17cc000 00000001 00000002 00000000
9c80: 00000000 c17bd1c0 c17bd1dc c07e2810 c0f04cc8 c17cc000 00000001 c0f04cc8
9ca0: 00000000 c07df518 ffffff08 ffff0a00 c0f0517c 00000000 00000000 ffffffff
9cc0: ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff
9ce0: ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff
9d00: ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff
9d20: ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff 58cdc110
9d40: c17cc578 c0f04cc8 c17cc000 c17cc000 00000001 c17cc578 00000000 c17bd1c0
9d60: c17bd1dc c07e20a0 00000000 c17bd1c0 c17bd1dc 58cdc110 00000001 c17cc000
9d80: 00000001 c17cc008 c17cc578 00000000 c17bd1c0 c07e29a4 c17bc040 00000000
9da0: c17bc040 c17d2800 c17bd0cc 00000001 c17bd1c0 c0a11814 00000000 00000dc0
9dc0: c0b81b20 c0f04cc8 c17bd1dc c0ceca94 c0d6e2b4 c17bd1c0 00000005 c17d2640
9de0: 00000040 c177c400 00000000 c0fe8b08 c17bc000 00000040 c177c400 00000000
9e00: 00000000 58cdc110 c0b59f78 c0fc4e44 c177c400 c102f58c 00000000 c0fe8b08
9e20: c0fc4e44 00000001 00000000 c07e2d04 c177c400 c102f584 c102f58c c0755e50
9e40: 60000013 58cdc110 c0d2b58c c177c400 c0fc4e44 c1569ecc c0fe8b08 00000001
9e60: c0d2b58c c0fe8b08 ffffe000 c0756400 c177c400 00000001 00000001 00000000
9e80: c0f04cc8 c1569ecc c0756664 00000001 c0d2b58c c0fe8b08 ffffe000 c0754028
9ea0: c0d2b58c c15aac6c c17808b8 58cdc110 c177c400 c177c400 c0f04cc8 c177c444
9ec0: c0fba314 c0755ca8 00000002 c177c400 00000001 58cdc110 c177c400 c177c400
9ee0: c0fc21f4 c0fba314 c0fe8b08 c0754dac c177c400 c0fba300 c0fba300 c075530c
9f00: c0fba340 c1403f00 df6917c0 df694900 00000000 c0fdf300 00000000 c033c034
9f20: c1560000 df6917c0 00000008 c1403f00 c1403f14 df6917c0 00000008 c0f03d00
9f40: df6917d8 df6917c0 ffffe000 c033c3cc c1560000 c0fdeb1e c0cdfe28 c033c388
9f60: c1403f00 c14e6040 c1549280 00000000 c1568000 c033c388 c1403f00 c14bdea4
9f80: c14e6064 c034282c 00000001 c1549280 c03426dc 00000000 00000000 00000000
9fa0: 00000000 00000000 00000000 c0300148 00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[<c0a36b24>] (mutex_lock) from [<c07ee9b8>] (qca8k_mdio_read+0x30/0xac)
[<c07ee9b8>] (qca8k_mdio_read) from [<c07eea54>] (qca8k_phy_read+0x20/0x30)
[<c07eea54>] (qca8k_phy_read) from [<c07e25b8>] (__mdiobus_read+0x3c/0x1ac)
[<c07e25b8>] (__mdiobus_read) from [<c07e2810>] (mdiobus_read+0x30/0x44)
[<c07e2810>] (mdiobus_read) from [<c07df518>] (get_phy_device+0x13c/0x25c)
[<c07df518>] (get_phy_device) from [<c07e20a0>] (mdiobus_scan+0xb0/0x190)
[<c07e20a0>] (mdiobus_scan) from [<c07e29a4>] (__mdiobus_register+0x17c/0x2fc)
[<c07e29a4>] (__mdiobus_register) from [<c0a11814>] (dsa_register_switch+0xbb8/0xc6c)
[<c0a11814>] (dsa_register_switch) from [<c07e2d04>] (mdio_probe+0x2c/0x50)
[<c07e2d04>] (mdio_probe) from [<c0755e50>] (really_probe+0x108/0x4f4)
[<c0755e50>] (really_probe) from [<c0756400>] (driver_probe_device+0x78/0x1e4)
[<c0756400>] (driver_probe_device) from [<c0754028>] (bus_for_each_drv+0x80/0xc4)
[<c0754028>] (bus_for_each_drv) from [<c0755ca8>] (__device_attach+0xe0/0x178)
[<c0755ca8>] (__device_attach) from [<c0754dac>] (bus_probe_device+0x84/0x8c)
[<c0754dac>] (bus_probe_device) from [<c075530c>] (deferred_probe_work_func+0x9c/0xdc)
[<c075530c>] (deferred_probe_work_func) from [<c033c034>] (process_one_work+0x22c/0x580)
[<c033c034>] (process_one_work) from [<c033c3cc>] (worker_thread+0x44/0x5c8)
[<c033c3cc>] (worker_thread) from [<c034282c>] (kthread+0x150/0x154)
[<c034282c>] (kthread) from [<c0300148>] (ret_from_fork+0x14/0x2c)
Exception stack(0xc1569fb0 to 0xc1569ff8)
9fa0:                                     00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
Code: e3c33d7f e3c3303f e593300c f594f000 (e1941f9f)
---[ end trace cc9433437c472cd4 ]---

J.

-- 
101 things you can't have too much of : 42 - Pepsi.
This .sig brought to you by the letter P and the number  1
Product of the Republic of HuggieTag

Re: [RFC PATCH net-next v4 01/28] net: mdio: ipq8064: clean whitespaces in define

[Jonathan McDowell]

On Sun, May 16, 2021 at 10:49:59AM +0100, Jonathan McDowell wrote:
> On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:
> > On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:
> > > On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:
> > > > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
> > > > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> > > > > > Fix mixed whitespace and tab for define spacing.
> > > > > 
> > > > > Please add a patch [0/28] which describes the big picture of what
> > > > > these changes are doing.
> > > > > 
> > > > > Also, this series is getting big. You might want to split it into two,
> > > > > One containing the cleanup, and the second adding support for the new
> > > > > switch.
> > > > > 
> > > > > 	Andrew
> > > > 
> > > > There is a 0/28. With all the changes. Could be that I messed the cc?
> > > > I agree think it's better to split this for the mdio part, the cleanup
> > > > and the changes needed for the internal phy.
> > > 
> > > FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011
> > > today. I currently use the GPIO MDIO driver because I saw issues with
> > > the IPQ8064 driver in the past, and sticking with the GPIO driver I see
> > > both QCA8337 devices and traffic flows as expected, so no obvious
> > > regressions from your changes.
> > > 
> > > I also tried switching to the IPQ8064 MDIO driver for my first device
> > > (which is on the MDIO0 bus), but it's still not happy:
> > > 
> > > qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13
> > > 
> > Can you try the v6 version of this series?
> 
> FWIW I tried v6 without altering my DT at all (so still using the GPIO
> MDIO driver, and not switching to use the alternate PHY support) and got
> an oops due to a NULL pointer dereference, apparently in the mdio_bus
> locking code. I'm back porting to 5.10.37 (because I track LTS on the
> device) so I might be missing something, but the v4 version I tried
> previously worked ok.

I dropped patches 20-25 of the series (i.e. the piece that adds the
internal phy/mdio support) and tried again and that works fine, so it
does look like I either managed to mismerge them somehow (and those
pieces weren't the ones with conflicts) or there's a problem (possibly
only when the DT hasn't been updated to use the internal bus?).

J.

-- 
   101 things you can't have too   |  .''`.  Debian GNU/Linux Developer
     much of : 45 - Toblerone.     | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.