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

[RFC PATCH net-next v3 01/20] 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>
---
 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 v3 02/20] 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

[RFC PATCH net-next v3 03/20] 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

[RFC PATCH net-next v3 04/20] 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 | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index cdaf9f85a2cb..411b42d38819 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -127,16 +127,20 @@ 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)
 {
 	if (page == qca8k_current_page)
-		return;
+		return 0;
 
-	if (bus->write(bus, 0x18, 0, page) < 0)
+	if (bus->write(bus, 0x18, 0, page) < 0) {
 		dev_err_ratelimited(&bus->dev,
 				    "failed to set qca8k page\n");
+		return -EBUSY;
+	}
+
 	qca8k_current_page = page;
+	return 0;
 }
 
 static u32
@@ -149,9 +153,13 @@ qca8k_read(struct qca8k_priv *priv, u32 reg)
 
 	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	qca8k_set_page(priv->bus, page);
+	val = qca8k_set_page(priv->bus, page);
+	if (val < 0)
+		goto exit;
+
 	val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
 
+exit:
 	mutex_unlock(&priv->bus->mdio_lock);
 
 	return val;
@@ -161,14 +169,19 @@ static void
 qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 {
 	u16 r1, r2, page;
+	int 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_set_page(priv->bus, page);
+	if (ret < 0)
+		goto exit;
+
 	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
 
+exit:
 	mutex_unlock(&priv->bus->mdio_lock);
 }
 
@@ -182,12 +195,16 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
 
 	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	qca8k_set_page(priv->bus, page);
+	ret = qca8k_set_page(priv->bus, page);
+	if (ret < 0)
+		goto exit;
+
 	ret = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
 	ret &= ~mask;
 	ret |= val;
 	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, ret);
 
+exit:
 	mutex_unlock(&priv->bus->mdio_lock);
 
 	return ret;
-- 
2.30.2

Re: [RFC PATCH net-next v3 04/20] net: dsa: qca8k: handle qca8k_set_page errors

[Andrew Lunn]

> +static int
>  qca8k_set_page(struct mii_bus *bus, u16 page)
>  {
>  	if (page == qca8k_current_page)
> -		return;
> +		return 0;
>  
> -	if (bus->write(bus, 0x18, 0, page) < 0)
> +	if (bus->write(bus, 0x18, 0, page) < 0) {
>  		dev_err_ratelimited(&bus->dev,
>  				    "failed to set qca8k page\n");
> +		return -EBUSY;

EBUSY is a bit odd. bus->write() should return an error code. Please
return that.

> @@ -161,14 +169,19 @@ static void
>  qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
>  {
>  	u16 r1, r2, page;
> +	int 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_set_page(priv->bus, page);
> +	if (ret < 0)
> +		goto exit;
> +
>  	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
>  
> +exit:
>  	mutex_unlock(&priv->bus->mdio_lock);

Maybe make this function also return the error? 

>  }

Re: [RFC PATCH net-next v3 04/20] net: dsa: qca8k: handle qca8k_set_page errors

[Andrew Lunn]

> > @@ -161,14 +169,19 @@ static void
> >  qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
> >  {
> >  	u16 r1, r2, page;
> > +	int 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_set_page(priv->bus, page);
> > +	if (ret < 0)
> > +		goto exit;
> > +
> >  	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
> >  
> > +exit:
> >  	mutex_unlock(&priv->bus->mdio_lock);
> 
> Maybe make this function also return the error? 

Ah, sorry, a later patch does exactly that.

    Andrew

[RFC PATCH net-next v3 05/20] 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 | 90 +++++++++++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 21 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 411b42d38819..cde68ed6856b 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -146,12 +146,13 @@ 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);
 
 	val = qca8k_set_page(priv->bus, page);
 	if (val < 0)
@@ -160,8 +161,7 @@ qca8k_read(struct qca8k_priv *priv, u32 reg)
 	val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
 
 exit:
-	mutex_unlock(&priv->bus->mdio_lock);
-
+	mutex_unlock(&bus->mdio_lock);
 	return val;
 }
 
@@ -226,8 +226,13 @@ static int
 qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	int ret;
 
-	*val = qca8k_read(priv, reg);
+	ret = qca8k_read(priv, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
 
 	return 0;
 }
@@ -280,15 +285,17 @@ static int
 qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 {
 	unsigned long timeout;
+	u32 val;
 
 	timeout = jiffies + msecs_to_jiffies(20);
 
 	/* loop until the busy flag has cleared */
 	do {
-		u32 val = qca8k_read(priv, reg);
-		int busy = val & mask;
+		val = qca8k_read(priv, reg);
+		if (val < 0)
+			continue;
 
-		if (!busy)
+		if (!(val & mask))
 			break;
 		cond_resched();
 	} while (!time_after_eq(jiffies, timeout));
@@ -296,15 +303,20 @@ qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 	return time_after_eq(jiffies, timeout);
 }
 
-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;
@@ -319,6 +331,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
@@ -370,6 +384,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;
 	}
@@ -380,12 +396,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;
 }
@@ -445,6 +464,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;
 	}
@@ -471,6 +492,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)
@@ -502,6 +525,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);
@@ -617,8 +642,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;
 }
@@ -974,6 +1002,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;
@@ -1074,18 +1104,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;
 	}
 }
 
@@ -1103,18 +1141,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
@@ -1439,6 +1484,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 v3 05/20] net: dsa: qca8k: handle error with qca8k_read operation

[Andrew Lunn]

On Wed, May 05, 2021 at 12:28:59AM +0200, Ansuel Smith wrote:
> 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 | 90 +++++++++++++++++++++++++++++++----------
>  1 file changed, 69 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 411b42d38819..cde68ed6856b 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -146,12 +146,13 @@ 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);
>  
>  	val = qca8k_set_page(priv->bus, page);
>  	if (val < 0)
> @@ -160,8 +161,7 @@ qca8k_read(struct qca8k_priv *priv, u32 reg)
>  	val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
>  
>  exit:
> -	mutex_unlock(&priv->bus->mdio_lock);
> -
> +	mutex_unlock(&bus->mdio_lock);
>  	return val;

This change does not have anything to do with the commit message.

>  }
>  
> @@ -226,8 +226,13 @@ static int
>  qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
> +	int ret;
>  
> -	*val = qca8k_read(priv, reg);
> +	ret = qca8k_read(priv, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
>  
>  	return 0;
>  }
> @@ -280,15 +285,17 @@ static int
>  qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
>  {
>  	unsigned long timeout;
> +	u32 val;
>  
>  	timeout = jiffies + msecs_to_jiffies(20);
>  
>  	/* loop until the busy flag has cleared */
>  	do {
> -		u32 val = qca8k_read(priv, reg);
> -		int busy = val & mask;
> +		val = qca8k_read(priv, reg);
> +		if (val < 0)
> +			continue;
>  
> -		if (!busy)
> +		if (!(val & mask))
>  			break;
>  		cond_resched();

Maybe there is a patch doing this already, but it would be good to
make use of include/linux/iopoll.h

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

This is oddly structured. Why not:

qca8k_fdb_next(struct qca8k_priv *priv, struct qca8k_fdb *fdb, int port)
{
	int ret;

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

	Andrew

Re: [RFC PATCH net-next v3 05/20] net: dsa: qca8k: handle error with qca8k_read operation

[Ansuel Smith]

On Wed, May 05, 2021 at 02:36:15AM +0200, Andrew Lunn wrote:
> On Wed, May 05, 2021 at 12:28:59AM +0200, Ansuel Smith wrote:
> > 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 | 90 +++++++++++++++++++++++++++++++----------
> >  1 file changed, 69 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 411b42d38819..cde68ed6856b 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -146,12 +146,13 @@ 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);
> >  
> >  	val = qca8k_set_page(priv->bus, page);
> >  	if (val < 0)
> > @@ -160,8 +161,7 @@ qca8k_read(struct qca8k_priv *priv, u32 reg)
> >  	val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
> >  
> >  exit:
> > -	mutex_unlock(&priv->bus->mdio_lock);
> > -
> > +	mutex_unlock(&bus->mdio_lock);
> >  	return val;
> 
> This change does not have anything to do with the commit message.
> 
> >  }

Will split in another patch.

> >  
> > @@ -226,8 +226,13 @@ static int
> >  qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> >  {
> >  	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
> > +	int ret;
> >  
> > -	*val = qca8k_read(priv, reg);
> > +	ret = qca8k_read(priv, reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = ret;
> >  
> >  	return 0;
> >  }
> > @@ -280,15 +285,17 @@ static int
> >  qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
> >  {
> >  	unsigned long timeout;
> > +	u32 val;
> >  
> >  	timeout = jiffies + msecs_to_jiffies(20);
> >  
> >  	/* loop until the busy flag has cleared */
> >  	do {
> > -		u32 val = qca8k_read(priv, reg);
> > -		int busy = val & mask;
> > +		val = qca8k_read(priv, reg);
> > +		if (val < 0)
> > +			continue;
> >  
> > -		if (!busy)
> > +		if (!(val & mask))
> >  			break;
> >  		cond_resched();
> 
> Maybe there is a patch doing this already, but it would be good to
> make use of include/linux/iopoll.h
> 

Will check if I can find something to replace this.

> >  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;
> >  }
> 
> This is oddly structured. Why not:
> 
> qca8k_fdb_next(struct qca8k_priv *priv, struct qca8k_fdb *fdb, int port)
> {
> 	int ret;
> 
> 	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);
> }
> 

It's late here and I could be wrong...
Doesn't your suggested code change the original function return value?
In the original function we returned qca8k_fdb_access, isn't wrong to
return qca8k_fdb_read on success? Or the function was wrong from the
start?

> 	Andrew

[RFC PATCH net-next v3 06/20] 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 | 112 +++++++++++++++++++++++++++-------------
 1 file changed, 75 insertions(+), 37 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index cde68ed6856b..899bf93118eb 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -165,15 +165,16 @@ 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;
 	u16 r1, r2, page;
 	int 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);
 
 	ret = qca8k_set_page(priv->bus, page);
 	if (ret < 0)
@@ -183,6 +184,7 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 
 exit:
 	mutex_unlock(&priv->bus->mdio_lock);
+	return ret;
 }
 
 static u32
@@ -242,9 +244,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[] = {
@@ -365,6 +365,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;
@@ -375,7 +376,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))
@@ -448,6 +451,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;
@@ -455,7 +459,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))
@@ -503,7 +509,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:
@@ -546,7 +554,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);
 	}
 
@@ -556,15 +566,25 @@ 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);
+
+	ret = qca8k_busy_wait(priv, QCA8K_REG_MIB, QCA8K_MIB_BUSY);
+	if (ret)
+		goto exit;
+
 	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
@@ -601,6 +621,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;
@@ -614,7 +635,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);
@@ -624,6 +647,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;
@@ -636,7 +660,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))
@@ -767,12 +793,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)
+		pr_warn("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) {
+		pr_err("failed enabling QCA header mode");
+		return ret;
+	}
 
 	/* Disable forwarding by default on all ports */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++)
@@ -784,11 +816,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++) {
@@ -816,16 +850,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)
+		pr_warn("failed setting MTU settings");
 
 	/* Flush the FDB table */
 	qca8k_fdb_flush(priv);
@@ -1141,8 +1179,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);
@@ -1155,7 +1193,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);
@@ -1285,9 +1323,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
@@ -1382,7 +1418,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) {
@@ -1395,9 +1431,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;
@@ -1408,7 +1446,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 v3 06/20] net: dsa: qca8k: handle error with qca8k_write operation

[Andrew Lunn]

> -static void
> +static int
>  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(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
>  
>  	ret = qca8k_set_page(priv->bus, page);
>  	if (ret < 0)
> @@ -183,6 +184,7 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
>  
>  exit:
>  	mutex_unlock(&priv->bus->mdio_lock);
> +	return ret;
>  }

Same comment as read. Maybe put this and the other similar change into one patch.

> @@ -636,7 +660,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))
> @@ -767,12 +793,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)
> +		pr_warn("mib init failed");

Please use dev_warn().

>  
>  	/* 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) {
> +		pr_err("failed enabling QCA header mode");

dev_err()

In general, always use dev_err(), dev_warn(), dev_info() etc, so we
know which device returned an error.

     Andrew

Re: [RFC PATCH net-next v3 06/20] net: dsa: qca8k: handle error with qca8k_write operation

[Ansuel Smith]

On Wed, May 05, 2021 at 02:41:24AM +0200, Andrew Lunn wrote:
> > -static void
> > +static int
> >  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(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
> > +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> >  
> >  	ret = qca8k_set_page(priv->bus, page);
> >  	if (ret < 0)
> > @@ -183,6 +184,7 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
> >  
> >  exit:
> >  	mutex_unlock(&priv->bus->mdio_lock);
> > +	return ret;
> >  }
> 
> Same comment as read. Maybe put this and the other similar change into one patch.
> 
> > @@ -636,7 +660,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))
> > @@ -767,12 +793,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)
> > +		pr_warn("mib init failed");
> 
> Please use dev_warn().
> 
> >  
> >  	/* 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) {
> > +		pr_err("failed enabling QCA header mode");
> 
> dev_err()
> 
> In general, always use dev_err(), dev_warn(), dev_info() etc, so we
> know which device returned an error.
>

I notice that in the driver we use pr function so I assumed this was the
correct way to report errors with a dsa driver. Will change that.

>      Andrew

Re: [RFC PATCH net-next v3 06/20] net: dsa: qca8k: handle error with qca8k_write operation

[Vladimir Oltean]

On Wed, May 05, 2021 at 02:47:25AM +0200, Ansuel Smith wrote:
> On Wed, May 05, 2021 at 02:41:24AM +0200, Andrew Lunn wrote:
> > dev_err()
> > 
> > In general, always use dev_err(), dev_warn(), dev_info() etc, so we
> > know which device returned an error.
> >
> 
> I notice that in the driver we use pr function so I assumed this was the
> correct way to report errors with a dsa driver.

Nope.

[RFC PATCH net-next v3 07/20] 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 | 130 +++++++++++++++++++++++++---------------
 1 file changed, 83 insertions(+), 47 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 899bf93118eb..33875ad58d59 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -190,12 +190,13 @@ qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
 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;
+	int 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);
 
 	ret = qca8k_set_page(priv->bus, page);
 	if (ret < 0)
@@ -207,21 +208,32 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
 	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, ret);
 
 exit:
-	mutex_unlock(&priv->bus->mdio_lock);
-
+	mutex_unlock(&bus->mdio_lock);
 	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
@@ -572,13 +584,17 @@ 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;
 
 	ret = qca8k_busy_wait(priv, QCA8K_REG_MIB, QCA8K_MIB_BUSY);
 	if (ret)
 		goto exit;
 
-	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);
 
@@ -754,9 +770,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;
@@ -789,8 +804,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) {
+		pr_err("failed enabling CPU port");
+		return ret;
+	}
 
 	/* Enable MIB counters */
 	ret = qca8k_mib_init(priv);
@@ -807,9 +826,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++)
@@ -828,28 +850,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));
@@ -1241,7 +1272,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)
@@ -1249,17 +1280,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
@@ -1396,18 +1430,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
@@ -1429,16 +1464,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 v3 07/20] 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;

Maybe return qca8k_rmw(priv, reg, 0, val); ??

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

Maybe return qca8k_rmw(priv, reg, val, 0);

> @@ -1249,17 +1280,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;

Can this is simplified to 

	return  = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
			    QCA8K_PORT_LOOKUP_MEMBER, port_mask);

> @@ -1396,18 +1430,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;

What does qca8k_rmw() actually return?

     Andrew

Re: [RFC PATCH net-next v3 07/20] net: dsa: qca8k: handle error with qca8k_rmw operation

[Ansuel Smith]

On Wed, May 05, 2021 at 02:46:23AM +0200, Andrew Lunn wrote:
> > -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;
> 
> Maybe return qca8k_rmw(priv, reg, 0, val); ??
> 
> > -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;
> >  }
> 
> Maybe return qca8k_rmw(priv, reg, val, 0);
> 
> > @@ -1249,17 +1280,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;
> 
> Can this is simplified to 
> 
> 	return  = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> 			    QCA8K_PORT_LOOKUP_MEMBER, port_mask);
> 
> > @@ -1396,18 +1430,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;
> 
> What does qca8k_rmw() actually return?
>
>      Andrew

qca8k_rmw in the original code returns the value read from the reg and
modified. So this is why all the strange checks. The idea is to not
change the original return values of the users so I check if every return
value is negative and return 0 in all the other cases.

[RFC PATCH net-next v3 08/20] net: dsa: qca8k: add support for qca8327 switch

[Ansuel Smith]

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

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 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 33875ad58d59..17c6fd4afa7d 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1529,6 +1529,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;
 
@@ -1556,6 +1557,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)
@@ -1563,8 +1569,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)
@@ -1629,9 +1637,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 7ca4b93e0bb5..86e8d479c9f9 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
 
@@ -211,6 +213,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 v3 08/20] net: dsa: qca8k: add support for qca8327 switch

[Andrew Lunn]

On Wed, May 05, 2021 at 12:29:02AM +0200, 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>

    Andrew

Re: [RFC PATCH net-next v3 08/20] net: dsa: qca8k: add support for qca8327 switch

[Vladimir Oltean]

On Wed, May 05, 2021 at 12:29:02AM +0200, 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: Vladimir Oltean <olteanv@gmail.com>

[RFC PATCH net-next v3 09/20] devicetree: net: dsa: qca8k: Document new compatible qca8327

[Ansuel Smith]

Add support for qca8327 in the compatible list.

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

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

Re: [RFC PATCH net-next v3 09/20] devicetree: net: dsa: qca8k: Document new compatible qca8327

[Andrew Lunn]

On Wed, May 05, 2021 at 12:29:03AM +0200, 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>

    Andrew

Re: [RFC PATCH net-next v3 09/20] devicetree: net: dsa: qca8k: Document new compatible qca8327

[Rob Herring]

On Wed, 05 May 2021 00:29:03 +0200, Ansuel Smith wrote:
> Add support for qca8327 in the compatible list.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[RFC PATCH net-next v3 10/20] net: dsa: qca8k: add priority tweak to qca8337 switch

[Ansuel Smith]

The port 5 of the ar8337 have some problem in flood condition. The
original legacy driver had some specific buffer and priority settings
for the different port suggested by the QCA switch team. Add this
missing settings to improve switch stability under load condition.
The packet priority tweak and the rx delay is specific to qca8337.
Limit this changes to qca8337 as now we also support 8327 switch.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 17c6fd4afa7d..9e034c445085 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -783,7 +783,12 @@ static int
 qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	const struct qca8k_match_data *data;
 	int ret, i;
+	u32 mask;
+
+	/* get the switches ID from the compatible */
+	data = of_device_get_match_data(priv->dev);
 
 	/* Make sure that port 0 is the cpu port */
 	if (!dsa_is_cpu_port(ds, 0)) {
@@ -889,6 +894,45 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 	}
 
+	if (data->id == QCA8K_ID_QCA8337) {
+		for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+			switch (i) {
+			/* The 2 CPU port and port 5 requires some different
+			 * priority than any other ports.
+			 */
+			case 0:
+			case 5:
+			case 6:
+				mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x4) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x4) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI4(0x6) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI5(0x8) |
+					QCA8K_PORT_HOL_CTRL0_EG_PORT(0x1e);
+				break;
+			default:
+				mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x6) |
+					QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x8) |
+					QCA8K_PORT_HOL_CTRL0_EG_PORT(0x19);
+			}
+			qca8k_write(priv, QCA8K_REG_PORT_HOL_CTRL0(i), mask);
+
+			mask = QCA8K_PORT_HOL_CTRL1_ING(0x6) |
+			QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
+			QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
+			QCA8K_PORT_HOL_CTRL1_WRED_EN;
+			qca8k_rmw(priv, QCA8K_REG_PORT_HOL_CTRL1(i),
+				  QCA8K_PORT_HOL_CTRL1_ING_BUF |
+				  QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
+				  QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
+				  QCA8K_PORT_HOL_CTRL1_WRED_EN,
+				  mask);
+		}
+	}
+
 	/* 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;
@@ -909,9 +953,13 @@ static void
 qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			 const struct phylink_link_state *state)
 {
+	const struct qca8k_match_data *data;
 	struct qca8k_priv *priv = ds->priv;
 	u32 reg, val;
 
+	/* get the switches ID from the compatible */
+	data = of_device_get_match_data(priv->dev);
+
 	switch (port) {
 	case 0: /* 1st CPU port */
 		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
@@ -962,8 +1010,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 (data->id == QCA8K_ID_QCA8337)
+			qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
+				    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 86e8d479c9f9..34c5522e7202 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -166,6 +166,30 @@
 #define   QCA8K_PORT_LOOKUP_STATE			GENMASK(18, 16)
 #define   QCA8K_PORT_LOOKUP_LEARN			BIT(20)
 
+#define QCA8K_REG_PORT_HOL_CTRL0(_i)			(0x970 + (_i) * 0x8)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI0_BUF		GENMASK(3, 0)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI0(x)		((x) << 0)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI1_BUF		GENMASK(7, 4)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI1(x)		((x) << 4)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI2_BUF		GENMASK(11, 8)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI2(x)		((x) << 8)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI3_BUF		GENMASK(15, 12)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI3(x)		((x) << 12)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI4_BUF		GENMASK(19, 16)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI4(x)		((x) << 16)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI5_BUF		GENMASK(23, 20)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PRI5(x)		((x) << 20)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PORT_BUF		GENMASK(29, 24)
+#define   QCA8K_PORT_HOL_CTRL0_EG_PORT(x)		((x) << 24)
+
+#define QCA8K_REG_PORT_HOL_CTRL1(_i)			(0x974 + (_i) * 0x8)
+#define   QCA8K_PORT_HOL_CTRL1_ING_BUF			GENMASK(3, 0)
+#define   QCA8K_PORT_HOL_CTRL1_ING(x)			((x) << 0)
+#define   QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN		BIT(6)
+#define   QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN		BIT(7)
+#define   QCA8K_PORT_HOL_CTRL1_WRED_EN			BIT(8)
+#define   QCA8K_PORT_HOL_CTRL1_EG_MIRROR_EN		BIT(16)
+
 /* Pkt edit registers */
 #define QCA8K_EGRESS_VLAN(x)				(0x0c70 + (4 * (x / 2)))
 
-- 
2.30.2

Re: [RFC PATCH net-next v3 10/20] net: dsa: qca8k: add priority tweak to qca8337 switch

[Andrew Lunn]

On Wed, May 05, 2021 at 12:29:04AM +0200, Ansuel Smith wrote:
> The port 5 of the ar8337 have some problem in flood condition. The
> original legacy driver had some specific buffer and priority settings
> for the different port suggested by the QCA switch team. Add this
> missing settings to improve switch stability under load condition.
> The packet priority tweak and the rx delay is specific to qca8337.
> Limit this changes to qca8337 as now we also support 8327 switch.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 54 +++++++++++++++++++++++++++++++++++++++--
>  drivers/net/dsa/qca8k.h | 24 ++++++++++++++++++
>  2 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 17c6fd4afa7d..9e034c445085 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -783,7 +783,12 @@ static int
>  qca8k_setup(struct dsa_switch *ds)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> +	const struct qca8k_match_data *data;
>  	int ret, i;
> +	u32 mask;
> +
> +	/* get the switches ID from the compatible */
> +	data = of_device_get_match_data(priv->dev);

You have already done this once in probe. Either store data in qca8k_priv,
or add the id to qca8K_priv.

   Andrew   

Re: [RFC PATCH net-next v3 10/20] net: dsa: qca8k: add priority tweak to qca8337 switch

[Vladimir Oltean]

On Wed, May 05, 2021 at 12:29:04AM +0200, Ansuel Smith wrote:
> The port 5 of the ar8337 have some problem in flood condition. The
> original legacy driver had some specific buffer and priority settings
> for the different port suggested by the QCA switch team. Add this
> missing settings to improve switch stability under load condition.
> The packet priority tweak and the rx delay is specific to qca8337.
> Limit this changes to qca8337 as now we also support 8327 switch.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 54 +++++++++++++++++++++++++++++++++++++++--
>  drivers/net/dsa/qca8k.h | 24 ++++++++++++++++++
>  2 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 17c6fd4afa7d..9e034c445085 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -783,7 +783,12 @@ static int
>  qca8k_setup(struct dsa_switch *ds)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> +	const struct qca8k_match_data *data;
>  	int ret, i;
> +	u32 mask;
> +
> +	/* get the switches ID from the compatible */
> +	data = of_device_get_match_data(priv->dev);
>  
>  	/* Make sure that port 0 is the cpu port */
>  	if (!dsa_is_cpu_port(ds, 0)) {
> @@ -889,6 +894,45 @@ qca8k_setup(struct dsa_switch *ds)
>  		}
>  	}
>  
> +	if (data->id == QCA8K_ID_QCA8337) {
> +		for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +			switch (i) {
> +			/* The 2 CPU port and port 5 requires some different
> +			 * priority than any other ports.
> +			 */
> +			case 0:
> +			case 5:
> +			case 6:
> +				mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x4) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x4) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PRI4(0x6) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PRI5(0x8) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PORT(0x1e);
> +				break;
> +			default:
> +				mask = QCA8K_PORT_HOL_CTRL0_EG_PRI0(0x3) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PRI1(0x4) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PRI2(0x6) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x8) |
> +					QCA8K_PORT_HOL_CTRL0_EG_PORT(0x19);
> +			}
> +			qca8k_write(priv, QCA8K_REG_PORT_HOL_CTRL0(i), mask);
> +
> +			mask = QCA8K_PORT_HOL_CTRL1_ING(0x6) |
> +			QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
> +			QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
> +			QCA8K_PORT_HOL_CTRL1_WRED_EN;
> +			qca8k_rmw(priv, QCA8K_REG_PORT_HOL_CTRL1(i),
> +				  QCA8K_PORT_HOL_CTRL1_ING_BUF |
> +				  QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
> +				  QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
> +				  QCA8K_PORT_HOL_CTRL1_WRED_EN,
> +				  mask);
> +		}

What is this actually doing and why is it needed?

> +	}
> +
>  	/* 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;
> @@ -909,9 +953,13 @@ static void
>  qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  			 const struct phylink_link_state *state)
>  {
> +	const struct qca8k_match_data *data;
>  	struct qca8k_priv *priv = ds->priv;
>  	u32 reg, val;
>  
> +	/* get the switches ID from the compatible */
> +	data = of_device_get_match_data(priv->dev);
> +
>  	switch (port) {
>  	case 0: /* 1st CPU port */
>  		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> @@ -962,8 +1010,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 (data->id == QCA8K_ID_QCA8337)
> +			qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
> +				    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);

Why are we looking at an RGMII delay change in a patch about "priority
tweaks"? This patch is very confusing.

>  		break;
>  	case PHY_INTERFACE_MODE_SGMII:
>  	case PHY_INTERFACE_MODE_1000BASEX:
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 86e8d479c9f9..34c5522e7202 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -166,6 +166,30 @@
>  #define   QCA8K_PORT_LOOKUP_STATE			GENMASK(18, 16)
>  #define   QCA8K_PORT_LOOKUP_LEARN			BIT(20)
>  
> +#define QCA8K_REG_PORT_HOL_CTRL0(_i)			(0x970 + (_i) * 0x8)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI0_BUF		GENMASK(3, 0)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI0(x)		((x) << 0)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI1_BUF		GENMASK(7, 4)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI1(x)		((x) << 4)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI2_BUF		GENMASK(11, 8)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI2(x)		((x) << 8)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI3_BUF		GENMASK(15, 12)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI3(x)		((x) << 12)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI4_BUF		GENMASK(19, 16)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI4(x)		((x) << 16)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI5_BUF		GENMASK(23, 20)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PRI5(x)		((x) << 20)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PORT_BUF		GENMASK(29, 24)
> +#define   QCA8K_PORT_HOL_CTRL0_EG_PORT(x)		((x) << 24)
> +
> +#define QCA8K_REG_PORT_HOL_CTRL1(_i)			(0x974 + (_i) * 0x8)
> +#define   QCA8K_PORT_HOL_CTRL1_ING_BUF			GENMASK(3, 0)
> +#define   QCA8K_PORT_HOL_CTRL1_ING(x)			((x) << 0)
> +#define   QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN		BIT(6)
> +#define   QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN		BIT(7)
> +#define   QCA8K_PORT_HOL_CTRL1_WRED_EN			BIT(8)
> +#define   QCA8K_PORT_HOL_CTRL1_EG_MIRROR_EN		BIT(16)
> +
>  /* Pkt edit registers */
>  #define QCA8K_EGRESS_VLAN(x)				(0x0c70 + (4 * (x / 2)))
>  
> -- 
> 2.30.2
> 

[RFC PATCH net-next v3 11/20] 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 |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 9e034c445085..ce3606d8e6a4 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -933,6 +933,16 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 	}
 
+	/* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */
+	if (data->id == QCA8K_ID_QCA8327) {
+		mask = QCA8K_GLOBAL_FC_GOL_XON_THRES(288) |
+		       QCA8K_GLOBAL_FC_GOL_XOFF_THRES(496);
+		qca8k_rmw(priv, QCA8K_REG_GLOBAL_FC_THRESH,
+			  QCA8K_GLOBAL_FC_GOL_XON_THRES_S |
+			  QCA8K_GLOBAL_FC_GOL_XOFF_THRES_S,
+			  mask);
+	}
+
 	/* 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 34c5522e7202..5fb68dbfa85a 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -166,6 +166,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)
@@ -242,6 +248,7 @@ struct qca8k_match_data {
 };
 
 struct qca8k_priv {
+	u8 switch_revision;
 	struct regmap *regmap;
 	struct mii_bus *bus;
 	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
-- 
2.30.2

Re: [RFC PATCH net-next v3 11/20] net: dsa: qca8k: add GLOBAL_FC settings needed for qca8327

[Andrew Lunn]

>  struct qca8k_priv {
> +	u8 switch_revision;
>  	struct regmap *regmap;
>  	struct mii_bus *bus;
>  	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];

This belongs in some other patch.

     Andrew

[RFC PATCH net-next v3 12/20] 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 | 50 ++++++++++++++++++++++++++---------------
 drivers/net/dsa/qca8k.h |  6 +++--
 2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index ce3606d8e6a4..22334d416f53 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1586,12 +1586,38 @@ 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;
+	}
+
+	/* 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
@@ -1617,22 +1643,10 @@ 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->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
 	if (!priv->ds)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 5fb68dbfa85a..0b503f78bf92 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -28,8 +28,10 @@
 
 /* Global control registers */
 #define QCA8K_REG_MASK_CTRL				0x000
-#define   QCA8K_MASK_CTRL_ID_M				0xff
-#define   QCA8K_MASK_CTRL_ID_S				8
+#define   QCA8K_MASK_CTRL_REV_ID_MASK			GENMASK(7, 0)
+#define   QCA8K_MASK_CTRL_REV_ID(x)			((x) >> 0)
+#define   QCA8K_MASK_CTRL_DEVICE_ID_MASK		GENMASK(15, 8)
+#define   QCA8K_MASK_CTRL_DEVICE_ID(x)			((x) >> 8)
 #define QCA8K_REG_PORT0_PAD_CTRL			0x004
 #define QCA8K_REG_PORT5_PAD_CTRL			0x008
 #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
-- 
2.30.2

[RFC PATCH net-next v3 13/20] 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 | 51 +++++++++++++++++++++++++++++++++++++++--
 drivers/net/dsa/qca8k.h | 11 +++++----
 2 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 22334d416f53..cb9b44769e92 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -779,6 +779,47 @@ 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)
+		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, "rx-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_rx_delay = val;
+		}
+	}
+
+	of_node_put(ports);
+
+	return 0;
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -808,6 +849,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);
@@ -1018,8 +1063,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 (data->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 0b503f78bf92..80830bb42736 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -36,12 +36,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)
@@ -251,6 +250,8 @@ struct qca8k_match_data {
 
 struct qca8k_priv {
 	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 v3 13/20] net: dsa: qca8k: make rgmii delay configurable

[Andrew Lunn]

On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote:
> 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 | 51 +++++++++++++++++++++++++++++++++++++++--
>  drivers/net/dsa/qca8k.h | 11 +++++----
>  2 files changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 22334d416f53..cb9b44769e92 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -779,6 +779,47 @@ 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)
> +		return -EINVAL;
> +
> +	/* Assume only one port with rgmii-id mode */
> +	for_each_available_child_of_node(ports, port) {

Are delays global? Or per port? They really should be per port.  If it
is global, one value that applies to all ports, i would not use
it. Have the PHY apply the delay, not the MAC.

    Andrew

Re: [RFC PATCH net-next v3 13/20] net: dsa: qca8k: make rgmii delay configurable

[Ansuel Smith]

On Wed, May 05, 2021 at 03:00:45AM +0200, Andrew Lunn wrote:
> On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote:
> > 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 | 51 +++++++++++++++++++++++++++++++++++++++--
> >  drivers/net/dsa/qca8k.h | 11 +++++----
> >  2 files changed, 55 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 22334d416f53..cb9b44769e92 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -779,6 +779,47 @@ 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)
> > +		return -EINVAL;
> > +
> > +	/* Assume only one port with rgmii-id mode */
> > +	for_each_available_child_of_node(ports, port) {
> 
> Are delays global? Or per port? They really should be per port.  If it
> is global, one value that applies to all ports, i would not use
> it. Have the PHY apply the delay, not the MAC.
> 
>     Andrew

It's expected to set the delay in the port node. But yes the value is
global and assigned to every port (in reality this is only assigned to
the unique rgmii cpu port). The switch has only one rgmii port
and one sgmii, that's why I skipped any logic about handling more than
one case with internal delay. If you want I can try to add some logic to
handle this value per port. But again on the switch there is only one
reg to set the delay and the qca8k_phylink_mac_config function use this
only for the rgmii cpu port.

Re: [RFC PATCH net-next v3 13/20] net: dsa: qca8k: make rgmii delay configurable

[Vladimir Oltean]

On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote:
> 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 | 51 +++++++++++++++++++++++++++++++++++++++--
>  drivers/net/dsa/qca8k.h | 11 +++++----
>  2 files changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 22334d416f53..cb9b44769e92 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -779,6 +779,47 @@ 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");

Consider falling back to searching for the "ethernet-ports" name too,
DSA should now support both.

> +	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;

?!
3 picoseconds is not a lot of clock skew for a 125/25/2.5 MHz clock. 3 nanoseconds maybe?

> +		} else {
> +			priv->rgmii_rx_delay = val;
> +		}
> +
> +		if (of_property_read_u32(port, "rx-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_rx_delay = val;
> +		}
> +	}
> +
> +	of_node_put(ports);
> +
> +	return 0;
> +}
> +
>  static int
>  qca8k_setup(struct dsa_switch *ds)
>  {
> @@ -808,6 +849,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);
> @@ -1018,8 +1063,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 (data->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 0b503f78bf92..80830bb42736 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -36,12 +36,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)
> @@ -251,6 +250,8 @@ struct qca8k_match_data {
>  
>  struct qca8k_priv {
>  	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 v3 13/20] net: dsa: qca8k: make rgmii delay configurable

[Ansuel Smith]

On Thu, May 06, 2021 at 02:10:33PM +0300, Vladimir Oltean wrote:
> On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote:
> > 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 | 51 +++++++++++++++++++++++++++++++++++++++--
> >  drivers/net/dsa/qca8k.h | 11 +++++----
> >  2 files changed, 55 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 22334d416f53..cb9b44769e92 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -779,6 +779,47 @@ 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");
> 
> Consider falling back to searching for the "ethernet-ports" name too,
> DSA should now support both.
>

The function qca8k_setup_mdio_bus also checks for ports node. Should I
also there the fallback correct?

> > +	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;
> 
> ?!
> 3 picoseconds is not a lot of clock skew for a 125/25/2.5 MHz clock. 3 nanoseconds maybe?
> 
> > +		} else {
> > +			priv->rgmii_rx_delay = val;
> > +		}
> > +
> > +		if (of_property_read_u32(port, "rx-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_rx_delay = val;
> > +		}
> > +	}
> > +
> > +	of_node_put(ports);
> > +
> > +	return 0;
> > +}
> > +
> >  static int
> >  qca8k_setup(struct dsa_switch *ds)
> >  {
> > @@ -808,6 +849,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);
> > @@ -1018,8 +1063,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 (data->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 0b503f78bf92..80830bb42736 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -36,12 +36,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)
> > @@ -251,6 +250,8 @@ struct qca8k_match_data {
> >  
> >  struct qca8k_priv {
> >  	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 v3 13/20] net: dsa: qca8k: make rgmii delay configurable

[Vladimir Oltean]

On Thu, May 06, 2021 at 11:53:36PM +0200, Ansuel Smith wrote:
> On Thu, May 06, 2021 at 02:10:33PM +0300, Vladimir Oltean wrote:
> > On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote:
> > > 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 | 51 +++++++++++++++++++++++++++++++++++++++--
> > >  drivers/net/dsa/qca8k.h | 11 +++++----
> > >  2 files changed, 55 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > index 22334d416f53..cb9b44769e92 100644
> > > --- a/drivers/net/dsa/qca8k.c
> > > +++ b/drivers/net/dsa/qca8k.c
> > > @@ -779,6 +779,47 @@ 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");
> > 
> > Consider falling back to searching for the "ethernet-ports" name too,
> > DSA should now support both.
> >
> 
> The function qca8k_setup_mdio_bus also checks for ports node. Should I
> also there the fallback correct?

Yes, ideally the entire driver should work fine with "ethernet-ports"
and not just pieces of it.

[RFC PATCH net-next v3 14/20] 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 cb9b44769e92..b21835d719b5 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -655,8 +655,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
@@ -690,6 +696,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 v3 15/20] 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 | 69 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b21835d719b5..27234dd4c74a 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -633,9 +633,33 @@ qca8k_port_to_phy(int port)
 	return port - 1;
 }
 
+static int
+qca8k_mdio_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
+{
+	unsigned long timeout;
+	u16 r1, r2, page;
+
+	qca8k_split_addr(reg, &r1, &r2, &page);
+
+	timeout = jiffies + msecs_to_jiffies(20);
+
+	/* loop until the busy flag has cleared */
+	do {
+		u32 val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
+		int busy = val & mask;
+
+		if (!busy)
+			break;
+		cond_resched();
+	} while (!time_after_eq(jiffies, timeout));
+
+	return time_after_eq(jiffies, timeout);
+}
+
 static int
 qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
 {
+	u16 r1, r2, page;
 	u32 phy, val;
 	int ret;
 
@@ -651,12 +675,22 @@ 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;
 
-	ret = qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
-			      QCA8K_MDIO_MASTER_BUSY);
+	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
+
+	if (qca8k_mdio_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+				 QCA8K_MDIO_MASTER_BUSY))
+		ret = -ETIMEDOUT;
+
+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,
@@ -668,6 +702,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;
 
@@ -682,20 +717,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);
+	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;
 
-	if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
-			    QCA8K_MDIO_MASTER_BUSY))
-		return -ETIMEDOUT;
+	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
 
-	val = qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL);
-	if (val < 0)
-		return val;
+	if (qca8k_mdio_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+				 QCA8K_MDIO_MASTER_BUSY))
+		val = -ETIMEDOUT;
+	else
+		val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
 
 	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

Re: [RFC PATCH net-next v3 15/20] net: dsa: qca8k: dsa: qca8k: protect MASTER busy_wait with mdio mutex

[Andrew Lunn]

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

include/linux/iopoll.h


> +
> +	return time_after_eq(jiffies, timeout);
> +}
> +
>  static int
>  qca8k_mdio_write(struct qca8k_priv *priv, int port, u32 regnum, u16 data)
>  {
> +	u16 r1, r2, page;
>  	u32 phy, val;
>  	int ret;
>  
> @@ -651,12 +675,22 @@ 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;
>  
> -	ret = qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
> -			      QCA8K_MDIO_MASTER_BUSY);
> +	qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
> +
> +	if (qca8k_mdio_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
> +				 QCA8K_MDIO_MASTER_BUSY))
> +		ret = -ETIMEDOUT;

qca8k_mdio_busy_wait() should be returning -EIMEDOUT.

       Andrew

[RFC PATCH net-next v3 16/20] net: dsa: qca8k: enlarge mdio delay and timeout

[Ansuel Smith]

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

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 27234dd4c74a..b4cd891ad35d 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -140,6 +140,7 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
 	}
 
 	qca8k_current_page = page;
+	usleep_range(1000, 2000);
 	return 0;
 }
 
@@ -641,7 +642,7 @@ qca8k_mdio_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	timeout = jiffies + msecs_to_jiffies(20);
+	timeout = jiffies + msecs_to_jiffies(2000);
 
 	/* loop until the busy flag has cleared */
 	do {
-- 
2.30.2

Re: [RFC PATCH net-next v3 16/20] net: dsa: qca8k: enlarge mdio delay and timeout

[Vladimir Oltean]

On Wed, May 05, 2021 at 12:29:10AM +0200, Ansuel Smith wrote:
> - Enlarge set page delay to QDSK source
> - Enlarge mdio MASTER timeout busy wait
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Some "why"s would go a long way here. What did you see? What behaves
differently? Is this only a preventative change?

[RFC PATCH net-next v3 17/20] net: phy: phylink: permit to pass dev_flags to phylink_connect_phy

[Ansuel Smith]

Add support for phylink_connect_phy to pass dev_flags to the PHY driver.
Change any user of phylink_connect_phy to pass 0 as dev_flags by
default.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/ethernet/cadence/macb_main.c          |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  2 +-
 drivers/net/phy/phylink.c                         | 12 +++++++-----
 include/linux/phylink.h                           |  2 +-
 net/dsa/slave.c                                   |  2 +-
 5 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 0f6a6cb7e98d..459243c08b0c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -834,7 +834,7 @@ static int macb_phylink_connect(struct macb *bp)
 		}
 
 		/* attach the mac to the phy */
-		ret = phylink_connect_phy(bp->phylink, phydev);
+		ret = phylink_connect_phy(bp->phylink, phydev, 0);
 	}
 
 	if (ret) {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 4749bd0af160..ece84bb64b37 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1099,7 +1099,7 @@ static int stmmac_init_phy(struct net_device *dev)
 			return -ENODEV;
 		}
 
-		ret = phylink_connect_phy(priv->phylink, phydev);
+		ret = phylink_connect_phy(priv->phylink, phydev, 0);
 	}
 
 	phylink_ethtool_get_wol(priv->phylink, &wol);
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index dc2800beacc3..95f6a10e90ef 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1018,7 +1018,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 }
 
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
-			      phy_interface_t interface)
+			      phy_interface_t interface, u32 flags)
 {
 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
@@ -1028,13 +1028,14 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->phydev)
 		return -EBUSY;
 
-	return phy_attach_direct(pl->netdev, phy, 0, interface);
+	return phy_attach_direct(pl->netdev, phy, flags, interface);
 }
 
 /**
  * phylink_connect_phy() - connect a PHY to the phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
  * @phy: a pointer to a &struct phy_device.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
  *
  * Connect @phy to the phylink instance specified by @pl by calling
  * phy_attach_direct(). Configure the @phy according to the MAC driver's
@@ -1046,7 +1047,8 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
  *
  * Returns 0 on success or a negative errno.
  */
-int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
+int phylink_connect_phy(struct phylink *pl, struct phy_device *phy,
+			u32 flags)
 {
 	int ret;
 
@@ -1056,7 +1058,7 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 		pl->link_config.interface = pl->link_interface;
 	}
 
-	ret = phylink_attach_phy(pl, phy, pl->link_interface);
+	ret = phylink_attach_phy(pl, phy, pl->link_interface, flags);
 	if (ret < 0)
 		return ret;
 
@@ -2207,7 +2209,7 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 		return ret;
 
 	interface = pl->link_config.interface;
-	ret = phylink_attach_phy(pl, phy, interface);
+	ret = phylink_attach_phy(pl, phy, interface, 0);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d81a714cfbbd..cd563ba67ca0 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -437,7 +437,7 @@ struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
 void phylink_set_pcs(struct phylink *, struct phylink_pcs *pcs);
 void phylink_destroy(struct phylink *);
 
-int phylink_connect_phy(struct phylink *, struct phy_device *);
+int phylink_connect_phy(struct phylink *, struct phy_device *, u32 flags);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
 void phylink_disconnect_phy(struct phylink *);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 992fcab4b552..8ecfcb553ac1 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1718,7 +1718,7 @@ static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
 		return -ENODEV;
 	}
 
-	return phylink_connect_phy(dp->pl, slave_dev->phydev);
+	return phylink_connect_phy(dp->pl, slave_dev->phydev, 0);
 }
 
 static int dsa_slave_phy_setup(struct net_device *slave_dev)
-- 
2.30.2

[RFC PATCH net-next v3 17/20] net: phy: phylink: permit to pass dev_flags to phylink_connect_phy

[Ansuel Smith]

Add support for phylink_connect_phy to pass dev_flags to the PHY driver.
Change any user of phylink_connect_phy to pass 0 as dev_flags by
default.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/ethernet/cadence/macb_main.c          |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  2 +-
 drivers/net/phy/phylink.c                         | 12 +++++++-----
 include/linux/phylink.h                           |  2 +-
 net/dsa/slave.c                                   |  2 +-
 5 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 0f6a6cb7e98d..459243c08b0c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -834,7 +834,7 @@ static int macb_phylink_connect(struct macb *bp)
 		}
 
 		/* attach the mac to the phy */
-		ret = phylink_connect_phy(bp->phylink, phydev);
+		ret = phylink_connect_phy(bp->phylink, phydev, 0);
 	}
 
 	if (ret) {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 4749bd0af160..ece84bb64b37 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1099,7 +1099,7 @@ static int stmmac_init_phy(struct net_device *dev)
 			return -ENODEV;
 		}
 
-		ret = phylink_connect_phy(priv->phylink, phydev);
+		ret = phylink_connect_phy(priv->phylink, phydev, 0);
 	}
 
 	phylink_ethtool_get_wol(priv->phylink, &wol);
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index dc2800beacc3..95f6a10e90ef 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1018,7 +1018,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 }
 
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
-			      phy_interface_t interface)
+			      phy_interface_t interface, u32 flags)
 {
 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
@@ -1028,13 +1028,14 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->phydev)
 		return -EBUSY;
 
-	return phy_attach_direct(pl->netdev, phy, 0, interface);
+	return phy_attach_direct(pl->netdev, phy, flags, interface);
 }
 
 /**
  * phylink_connect_phy() - connect a PHY to the phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
  * @phy: a pointer to a &struct phy_device.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
  *
  * Connect @phy to the phylink instance specified by @pl by calling
  * phy_attach_direct(). Configure the @phy according to the MAC driver's
@@ -1046,7 +1047,8 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
  *
  * Returns 0 on success or a negative errno.
  */
-int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
+int phylink_connect_phy(struct phylink *pl, struct phy_device *phy,
+			u32 flags)
 {
 	int ret;
 
@@ -1056,7 +1058,7 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 		pl->link_config.interface = pl->link_interface;
 	}
 
-	ret = phylink_attach_phy(pl, phy, pl->link_interface);
+	ret = phylink_attach_phy(pl, phy, pl->link_interface, flags);
 	if (ret < 0)
 		return ret;
 
@@ -2207,7 +2209,7 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 		return ret;
 
 	interface = pl->link_config.interface;
-	ret = phylink_attach_phy(pl, phy, interface);
+	ret = phylink_attach_phy(pl, phy, interface, 0);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d81a714cfbbd..cd563ba67ca0 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -437,7 +437,7 @@ struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
 void phylink_set_pcs(struct phylink *, struct phylink_pcs *pcs);
 void phylink_destroy(struct phylink *);
 
-int phylink_connect_phy(struct phylink *, struct phy_device *);
+int phylink_connect_phy(struct phylink *, struct phy_device *, u32 flags);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
 void phylink_disconnect_phy(struct phylink *);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 992fcab4b552..8ecfcb553ac1 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1718,7 +1718,7 @@ static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
 		return -ENODEV;
 	}
 
-	return phylink_connect_phy(dp->pl, slave_dev->phydev);
+	return phylink_connect_phy(dp->pl, slave_dev->phydev, 0);
 }
 
 static int dsa_slave_phy_setup(struct net_device *slave_dev)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH net-next v3 17/20] net: phy: phylink: permit to pass dev_flags to phylink_connect_phy

[Florian Fainelli]

On 5/4/21 3:29 PM, Ansuel Smith wrote:
> Add support for phylink_connect_phy to pass dev_flags to the PHY driver.
> Change any user of phylink_connect_phy to pass 0 as dev_flags by
> default.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

I do not think that this patch and the next one are necessary at all,
because phylink_of_phy_connect() already supports passing a dev_flags.

That means that you should be representing the switch's internal MDIO
bus in the Device Tree and then describe how each port of the switch
connects to the internal PHY on that same bus. Once you do that the
logic in net/dsa/slave.c will call phylink_of_phy_connect() and all you
will have to do is implement dsa_switch_ops::get_phy_flags. Can you try
that?
-- 
Florian

Re: [RFC PATCH net-next v3 17/20] net: phy: phylink: permit to pass dev_flags to phylink_connect_phy

[Florian Fainelli]

On 5/4/21 3:29 PM, Ansuel Smith wrote:
> Add support for phylink_connect_phy to pass dev_flags to the PHY driver.
> Change any user of phylink_connect_phy to pass 0 as dev_flags by
> default.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

I do not think that this patch and the next one are necessary at all,
because phylink_of_phy_connect() already supports passing a dev_flags.

That means that you should be representing the switch's internal MDIO
bus in the Device Tree and then describe how each port of the switch
connects to the internal PHY on that same bus. Once you do that the
logic in net/dsa/slave.c will call phylink_of_phy_connect() and all you
will have to do is implement dsa_switch_ops::get_phy_flags. Can you try
that?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH net-next v3 17/20] net: phy: phylink: permit to pass dev_flags to phylink_connect_phy

[Ansuel Smith]

On Tue, May 04, 2021 at 03:33:36PM -0700, Florian Fainelli wrote:
> On 5/4/21 3:29 PM, Ansuel Smith wrote:
> > Add support for phylink_connect_phy to pass dev_flags to the PHY driver.
> > Change any user of phylink_connect_phy to pass 0 as dev_flags by
> > default.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> 
> I do not think that this patch and the next one are necessary at all,
> because phylink_of_phy_connect() already supports passing a dev_flags.
> 
> That means that you should be representing the switch's internal MDIO
> bus in the Device Tree and then describe how each port of the switch
> connects to the internal PHY on that same bus. Once you do that the
> logic in net/dsa/slave.c will call phylink_of_phy_connect() and all you
> will have to do is implement dsa_switch_ops::get_phy_flags. Can you try
> that?

I did some testing. Just to make sure I'm correctly implementing this I'm
using the phy-handle binding and the phy-mode set to internal. It does
work with a quick test but I think with this implementation we would be
back to this problem [0].
(I'm declaring the phy_port to the top mdio driver like it was done
before [0])

I was thinking if a good solution would be to register a internal mdio driver
in the qca8k code so that it can use the MASTER reg.
(it's late here so I could be very confused about this)

I think that using this solution we would be able to better describe the phy
by declaring them INSIDE the switch node instead of declaring them
outside in the top mdio node. The internal mdio driver would register
with this new mdio node inside the switch node and use the custom mdio
read/write that use the MASTER reg.

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

> -- 
> Florian

Re: [RFC PATCH net-next v3 17/20] net: phy: phylink: permit to pass dev_flags to phylink_connect_phy

[Ansuel Smith]

On Tue, May 04, 2021 at 03:33:36PM -0700, Florian Fainelli wrote:
> On 5/4/21 3:29 PM, Ansuel Smith wrote:
> > Add support for phylink_connect_phy to pass dev_flags to the PHY driver.
> > Change any user of phylink_connect_phy to pass 0 as dev_flags by
> > default.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> 
> I do not think that this patch and the next one are necessary at all,
> because phylink_of_phy_connect() already supports passing a dev_flags.
> 
> That means that you should be representing the switch's internal MDIO
> bus in the Device Tree and then describe how each port of the switch
> connects to the internal PHY on that same bus. Once you do that the
> logic in net/dsa/slave.c will call phylink_of_phy_connect() and all you
> will have to do is implement dsa_switch_ops::get_phy_flags. Can you try
> that?

I did some testing. Just to make sure I'm correctly implementing this I'm
using the phy-handle binding and the phy-mode set to internal. It does
work with a quick test but I think with this implementation we would be
back to this problem [0].
(I'm declaring the phy_port to the top mdio driver like it was done
before [0])

I was thinking if a good solution would be to register a internal mdio driver
in the qca8k code so that it can use the MASTER reg.
(it's late here so I could be very confused about this)

I think that using this solution we would be able to better describe the phy
by declaring them INSIDE the switch node instead of declaring them
outside in the top mdio node. The internal mdio driver would register
with this new mdio node inside the switch node and use the custom mdio
read/write that use the MASTER reg.

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

> -- 
> Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH net-next v3 18/20] net: dsa: slave: pass dev_flags also to internal PHY

[Ansuel Smith]

Add support to dsa_slave_phy_connect to properly pass dev_flags if
defined by the dsa driver.

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

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 8ecfcb553ac1..339280330357 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1707,7 +1707,7 @@ static void dsa_slave_phylink_fixed_state(struct phylink_config *config,
 }
 
 /* slave device setup *******************************************************/
-static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
+static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr, u32 flags)
 {
 	struct dsa_port *dp = dsa_slave_to_port(slave_dev);
 	struct dsa_switch *ds = dp->ds;
@@ -1718,7 +1718,7 @@ static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
 		return -ENODEV;
 	}
 
-	return phylink_connect_phy(dp->pl, slave_dev->phydev, 0);
+	return phylink_connect_phy(dp->pl, slave_dev->phydev, flags);
 }
 
 static int dsa_slave_phy_setup(struct net_device *slave_dev)
@@ -1762,7 +1762,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 		/* We could not connect to a designated PHY or SFP, so try to
 		 * use the switch internal MDIO bus instead
 		 */
-		ret = dsa_slave_phy_connect(slave_dev, dp->index);
+		ret = dsa_slave_phy_connect(slave_dev, dp->index, phy_flags);
 		if (ret) {
 			netdev_err(slave_dev,
 				   "failed to connect to port %d: %d\n",
-- 
2.30.2

[RFC PATCH net-next v3 19/20] 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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b4cd891ad35d..237e09bb1425 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1654,6 +1654,24 @@ 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;
+
+	pr_info("revision from phy %d", priv->switch_revision);
+
+	/* 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)
@@ -1687,6 +1705,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 v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Vladimir Oltean]

On Wed, May 05, 2021 at 12:29:13AM +0200, 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>
> ---
>  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index b4cd891ad35d..237e09bb1425 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -1654,6 +1654,24 @@ 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;
> +
> +	pr_info("revision from phy %d", priv->switch_revision);

Log spam.

> +	/* 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)
> @@ -1687,6 +1705,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
> 

Florian, I think at one point you said that a correct user of
phydev->dev_flags should first check the PHY revision and not apply
dev_flags in blind, since they are namespaced to each PHY driver?
It sounds a bit circular to pass the PHY revision to the PHY through
phydev->dev_flags, either that or I'm missing some piece.

Re: [RFC PATCH net-next v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Ansuel Smith]

On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:
> On Wed, May 05, 2021 at 12:29:13AM +0200, 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>
> > ---
> >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index b4cd891ad35d..237e09bb1425 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -1654,6 +1654,24 @@ 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;
> > +
> > +	pr_info("revision from phy %d", priv->switch_revision);
> 
> Log spam.
> 
> > +	/* 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)
> > @@ -1687,6 +1705,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
> > 
> 
> Florian, I think at one point you said that a correct user of
> phydev->dev_flags should first check the PHY revision and not apply
> dev_flags in blind, since they are namespaced to each PHY driver?
> It sounds a bit circular to pass the PHY revision to the PHY through
> phydev->dev_flags, either that or I'm missing some piece.

Just to make sure. This is the SWITCH revision not the PHY revision. It
was pointed out in old version that I should get this value from the PHY
regs but they are different values. This is why the dsa driver needs to
use the dev_flags to pass the SWITCH revision to the phy driver. Am I
implementing this in the wrong way and I should declare something to
pass this value in a more standard way? (anyway i'm pushing v4 so i
don't know if we should continue that there)

Re: [RFC PATCH net-next v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Russell King - ARM Linux admin]

On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote:
> On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:
> > On Wed, May 05, 2021 at 12:29:13AM +0200, 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>
> > > ---
> > >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > index b4cd891ad35d..237e09bb1425 100644
> > > --- a/drivers/net/dsa/qca8k.c
> > > +++ b/drivers/net/dsa/qca8k.c
> > > @@ -1654,6 +1654,24 @@ 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;
> > > +
> > > +	pr_info("revision from phy %d", priv->switch_revision);
> > 
> > Log spam.
> > 
> > > +	/* 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)
> > > @@ -1687,6 +1705,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
> > > 
> > 
> > Florian, I think at one point you said that a correct user of
> > phydev->dev_flags should first check the PHY revision and not apply
> > dev_flags in blind, since they are namespaced to each PHY driver?
> > It sounds a bit circular to pass the PHY revision to the PHY through
> > phydev->dev_flags, either that or I'm missing some piece.
> 
> Just to make sure. This is the SWITCH revision not the PHY revision. It
> was pointed out in old version that I should get this value from the PHY
> regs but they are different values. This is why the dsa driver needs to
> use the dev_flags to pass the SWITCH revision to the phy driver. Am I
> implementing this in the wrong way and I should declare something to
> pass this value in a more standard way? (anyway i'm pushing v4 so i
> don't know if we should continue that there)

Vladimir is confused - it is not PHY revision at all, but the PHY
identifiers.

What was actually suggested was checking the PHY identifiers before
passing PHY-driver specific flags, so that we didn't end up setting
driver private flags that are intending for one driver, but end up
actually binding a different driver, and mis-interpreting the flags.

This is one of the problems of the current scheme: it's just a
meaningless opaque u32 variable with no defined structure to it that
the various PHY drivers themselves use in whatever way they see fit.
That is only fine to use _if_ you know for certain which driver is
going to bind ahead of time.

As I mentioned in direct reply to your patch, there was discussions
about this back in February, but they seem to have stalled.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Ansuel Smith]

On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote:
> On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote:
> > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:
> > > On Wed, May 05, 2021 at 12:29:13AM +0200, 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>
> > > > ---
> > > >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > > index b4cd891ad35d..237e09bb1425 100644
> > > > --- a/drivers/net/dsa/qca8k.c
> > > > +++ b/drivers/net/dsa/qca8k.c
> > > > @@ -1654,6 +1654,24 @@ 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;
> > > > +
> > > > +	pr_info("revision from phy %d", priv->switch_revision);
> > > 
> > > Log spam.
> > > 
> > > > +	/* 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)
> > > > @@ -1687,6 +1705,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
> > > > 
> > > 
> > > Florian, I think at one point you said that a correct user of
> > > phydev->dev_flags should first check the PHY revision and not apply
> > > dev_flags in blind, since they are namespaced to each PHY driver?
> > > It sounds a bit circular to pass the PHY revision to the PHY through
> > > phydev->dev_flags, either that or I'm missing some piece.
> > 
> > Just to make sure. This is the SWITCH revision not the PHY revision. It
> > was pointed out in old version that I should get this value from the PHY
> > regs but they are different values. This is why the dsa driver needs to
> > use the dev_flags to pass the SWITCH revision to the phy driver. Am I
> > implementing this in the wrong way and I should declare something to
> > pass this value in a more standard way? (anyway i'm pushing v4 so i
> > don't know if we should continue that there)
> 
> Vladimir is confused - it is not PHY revision at all, but the PHY
> identifiers.
> 
> What was actually suggested was checking the PHY identifiers before
> passing PHY-driver specific flags, so that we didn't end up setting
> driver private flags that are intending for one driver, but end up
> actually binding a different driver, and mis-interpreting the flags.
> 
> This is one of the problems of the current scheme: it's just a
> meaningless opaque u32 variable with no defined structure to it that
> the various PHY drivers themselves use in whatever way they see fit.
> That is only fine to use _if_ you know for certain which driver is
> going to bind ahead of time.
>

The problem here was find a way to pass data from the dsa driver to the
phy driver. In this specific case the phy driver is an internal phy
present in the switch so it won't appear on anything else. Aside from
this I agree that it seems wrong that random values are used without
some type of rules or definition but I think that try to address this
problem is too much for this already large series. In theory this should
be safe to use as this driver would only be used by qca8k dsa driver.

> As I mentioned in direct reply to your patch, there was discussions
> about this back in February, but they seem to have stalled.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Andrew Lunn]

> The problem here was find a way to pass data from the dsa driver to the
> phy driver. In this specific case the phy driver is an internal phy
> present in the switch so it won't appear on anything else.

For internal PHYs, you are safe. But please keep in mind any RGMII
ports which the switch might have. Somebody could attach an external
PHY on such a port. So you should not return any flags for such ports.

    Andrew

Re: [RFC PATCH net-next v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Vladimir Oltean]

On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote:
> On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote:
> > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:
> > > On Wed, May 05, 2021 at 12:29:13AM +0200, 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>
> > > > ---
> > > >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > > index b4cd891ad35d..237e09bb1425 100644
> > > > --- a/drivers/net/dsa/qca8k.c
> > > > +++ b/drivers/net/dsa/qca8k.c
> > > > @@ -1654,6 +1654,24 @@ 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;
> > > > +
> > > > +	pr_info("revision from phy %d", priv->switch_revision);
> > > 
> > > Log spam.
> > > 
> > > > +	/* 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)
> > > > @@ -1687,6 +1705,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
> > > > 
> > > 
> > > Florian, I think at one point you said that a correct user of
> > > phydev->dev_flags should first check the PHY revision and not apply
> > > dev_flags in blind, since they are namespaced to each PHY driver?
> > > It sounds a bit circular to pass the PHY revision to the PHY through
> > > phydev->dev_flags, either that or I'm missing some piece.
> > 
> > Just to make sure. This is the SWITCH revision not the PHY revision. It
> > was pointed out in old version that I should get this value from the PHY
> > regs but they are different values. This is why the dsa driver needs to
> > use the dev_flags to pass the SWITCH revision to the phy driver. Am I
> > implementing this in the wrong way and I should declare something to
> > pass this value in a more standard way? (anyway i'm pushing v4 so i
> > don't know if we should continue that there)
> 
> Vladimir is confused - it is not PHY revision at all, but the PHY
> identifiers.
> 
> What was actually suggested was checking the PHY identifiers before
> passing PHY-driver specific flags, so that we didn't end up setting
> driver private flags that are intending for one driver, but end up
> actually binding a different driver, and mis-interpreting the flags.
> 
> This is one of the problems of the current scheme: it's just a
> meaningless opaque u32 variable with no defined structure to it that
> the various PHY drivers themselves use in whatever way they see fit.
> That is only fine to use _if_ you know for certain which driver is
> going to bind ahead of time.
> 
> As I mentioned in direct reply to your patch, there was discussions
> about this back in February, but they seem to have stalled.

Yes, I was indeed confused. My problem was mixing up the PHY OUI/device ID
and revision concepts in one big fuzzy notion. I remembered Heiner's
suggestion to do something similar to mv88e6xxx_mdio_read from here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210423014741.11858-12-ansuelsmth@gmail.com/
(where the problem is that some internal PHYs are lacking a device
identifier) and thought that the problem here is the same.

Nonetheless, now it is clear to me that with care (don't set dev_flags
except for internal PHYs which are statically known), it is possible for
the PHY driver to have a larger identifier (PHY ID concatenated with
switch revision passed through dev_flags) based on which it can
configure the hardware.

Sorry.

Re: [RFC PATCH net-next v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Russell King - ARM Linux admin]

On Sat, May 08, 2021 at 09:26:20PM +0300, Vladimir Oltean wrote:
> On Sat, May 08, 2021 at 12:33:53AM +0100, Russell King - ARM Linux admin wrote:
> > On Sat, May 08, 2021 at 01:26:02AM +0200, Ansuel Smith wrote:
> > > On Thu, May 06, 2021 at 02:24:58PM +0300, Vladimir Oltean wrote:
> > > > On Wed, May 05, 2021 at 12:29:13AM +0200, 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>
> > > > > ---
> > > > >  drivers/net/dsa/qca8k.c | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > > > index b4cd891ad35d..237e09bb1425 100644
> > > > > --- a/drivers/net/dsa/qca8k.c
> > > > > +++ b/drivers/net/dsa/qca8k.c
> > > > > @@ -1654,6 +1654,24 @@ 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;
> > > > > +
> > > > > +	pr_info("revision from phy %d", priv->switch_revision);
> > > > 
> > > > Log spam.
> > > > 
> > > > > +	/* 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)
> > > > > @@ -1687,6 +1705,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
> > > > > 
> > > > 
> > > > Florian, I think at one point you said that a correct user of
> > > > phydev->dev_flags should first check the PHY revision and not apply
> > > > dev_flags in blind, since they are namespaced to each PHY driver?
> > > > It sounds a bit circular to pass the PHY revision to the PHY through
> > > > phydev->dev_flags, either that or I'm missing some piece.
> > > 
> > > Just to make sure. This is the SWITCH revision not the PHY revision. It
> > > was pointed out in old version that I should get this value from the PHY
> > > regs but they are different values. This is why the dsa driver needs to
> > > use the dev_flags to pass the SWITCH revision to the phy driver. Am I
> > > implementing this in the wrong way and I should declare something to
> > > pass this value in a more standard way? (anyway i'm pushing v4 so i
> > > don't know if we should continue that there)
> > 
> > Vladimir is confused - it is not PHY revision at all, but the PHY
> > identifiers.
> > 
> > What was actually suggested was checking the PHY identifiers before
> > passing PHY-driver specific flags, so that we didn't end up setting
> > driver private flags that are intending for one driver, but end up
> > actually binding a different driver, and mis-interpreting the flags.
> > 
> > This is one of the problems of the current scheme: it's just a
> > meaningless opaque u32 variable with no defined structure to it that
> > the various PHY drivers themselves use in whatever way they see fit.
> > That is only fine to use _if_ you know for certain which driver is
> > going to bind ahead of time.
> > 
> > As I mentioned in direct reply to your patch, there was discussions
> > about this back in February, but they seem to have stalled.
> 
> Yes, I was indeed confused. My problem was mixing up the PHY OUI/device ID
> and revision concepts in one big fuzzy notion. I remembered Heiner's
> suggestion to do something similar to mv88e6xxx_mdio_read from here:
> https://patchwork.kernel.org/project/netdevbpf/patch/20210423014741.11858-12-ansuelsmth@gmail.com/
> (where the problem is that some internal PHYs are lacking a device
> identifier) and thought that the problem here is the same.
> 
> Nonetheless, now it is clear to me that with care (don't set dev_flags
> except for internal PHYs which are statically known), it is possible for
> the PHY driver to have a larger identifier (PHY ID concatenated with
> switch revision passed through dev_flags) based on which it can
> configure the hardware.

We do have the problem with Marvell DSA vs Marvell PHY setup in that
the Marvell DSA driver assumes that all integrated PHYs that do not
have an ID are all the same. They are most definitely not, and this
shows itself up when we register the hwmon stuff inappropriately, or
access the wrong registers to report hwmon values.

We really need to solve this problem properly rather than bodging
around it with driver specific usage of dev_flags.

We already have the ability for drivers to have custom match functions
(match_phy_device) that do not depend on the probed ID - maybe we
should have an additional u32 member in struct phy_device for the
switch_id that the PHY is a part of that PHY drivers can check in their
match_phy_device method if necessary, or otherwise use that to parse
the switch revision from. Or something like that?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Andrew Lunn]

> We do have the problem with Marvell DSA vs Marvell PHY setup in that
> the Marvell DSA driver assumes that all integrated PHYs that do not
> have an ID are all the same. They are most definitely not, and this
> shows itself up when we register the hwmon stuff inappropriately, or
> access the wrong registers to report hwmon values.

Hi Russell

This was to some degree fixed recently. Rather than always use the
6390 ID for everything, the family ID is now used. This should avoid
the issue with the SERDES being incorrectly considered a PHY, since
that particular family ID is not listed in the PHY driver.

      Andrew

Re: [RFC PATCH net-next v3 19/20] net: dsa: qca8k: pass switch_revision info to phy dev_flags

[Russell King - ARM Linux admin]

On Wed, May 05, 2021 at 12:29:13AM +0200, Ansuel Smith wrote:
> +static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +
> +	pr_info("revision from phy %d", priv->switch_revision);

Should this be a "pr_info" ?

> +
> +	/* 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;

We had some discussion back in February about the PHY flags argument
("Rework of phydev->dev_flags") as there is a need to generically
identify whether a PHY is on a SFP module or not. This discussion
hasn't progressed to any changes (yet) but some of the points remain
valid: if we do go down the route of needing to have generic PHY flags
in dev_flags, then we need vendor specific stuff to avoid those bits.
So, rather than introduce a new case of passing some undefined data
through the flags argument, can we come up with some sort of proposal
for this.

It may also be a good idea if we document it. Maybe something like
"low 16 bits are free for driver use, upper 16 bits are reserved
for generic use"?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[RFC PATCH net-next v3 20/20] 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  | 174 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 182 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..514250bb9e71
--- /dev/null
+++ b/drivers/net/phy/qca8k.c
@@ -0,0 +1,174 @@
+// 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)
+{
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	bus->write(bus, phy_addr, MII_ATH_DBG_ADDR, dbg_addr);
+	bus->write(bus, phy_addr, MII_ATH_DBG_DATA, dbg_data);
+	mutex_unlock(&bus->mdio_lock);
+}
+
+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 v3 20/20] net: phy: add qca8k driver for qca8k switch internal PHY

[Andrew Lunn]

> +/* 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)
> +{
> +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	bus->write(bus, phy_addr, MII_ATH_DBG_ADDR, dbg_addr);
> +	bus->write(bus, phy_addr, MII_ATH_DBG_DATA, dbg_data);
> +	mutex_unlock(&bus->mdio_lock);
> +}

What are you locking against here?

     Andrew

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

[Ansuel Smith]

On Wed, May 05, 2021 at 03:11:36AM +0200, Andrew Lunn wrote:
> > +/* 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)
> > +{
> > +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > +	bus->write(bus, phy_addr, MII_ATH_DBG_ADDR, dbg_addr);
> > +	bus->write(bus, phy_addr, MII_ATH_DBG_DATA, dbg_data);
> > +	mutex_unlock(&bus->mdio_lock);
> > +}
> 
> What are you locking against here?
> 
>      Andrew

Added the locking if in the future it will be used outside the
config_init function but since it's used only there, yes, I can drop the
useless lock.

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

[Andrew Lunn]

On Wed, May 05, 2021 at 03:17:20AM +0200, Ansuel Smith wrote:
> On Wed, May 05, 2021 at 03:11:36AM +0200, Andrew Lunn wrote:
> > > +/* 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)
> > > +{
> > > +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > > +	bus->write(bus, phy_addr, MII_ATH_DBG_ADDR, dbg_addr);
> > > +	bus->write(bus, phy_addr, MII_ATH_DBG_DATA, dbg_data);
> > > +	mutex_unlock(&bus->mdio_lock);
> > > +}
> > 
> > What are you locking against here?
> > 
> >      Andrew
> 
> Added the locking if in the future it will be used outside the
> config_init function but since it's used only there, yes, I can drop the
> useless lock.

The PHY core will take the phydev->lock whenever it calls the PHY
driver functions. The only exception to this is suspend/resume. So
long as you only access the devices own addresses on the MDIO bus, you
don't need any additional locks.

     Andrew

[RFC PATCH next-next v3 00/20] Multiple improvement to qca8k stability

[Ansuel Smith]

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

Changes v3:
- Revert mdio writel changes (use regmap with REGCACHE disabled)
- Split propagate error patch to 4 different patch
Changes v2:
- Implemented phy driver for internal PHYs
  I'm testing cable test functions as I found some documentation that
  actually declare regs about it. Problem is that it doesn't actually
  work. It seems that the value set are ignored by the phy.
- Made the rgmii delay configurable
- Reordered patch
- Split mdio patches to more specific ones
- Reworked mdio driver to use readl/writel instead of regmap
- Reworked the entire driver to make it aware of any read/write error.
- Added phy generic patch to pass flags with phylink_connect_phy
  function

A decision about the extra mdio delay is still to be taken but I
preferred to push a v2 since there is a new driver and more changes than
v0.

Ansuel Smith (20):
  net: mdio: ipq8064: clean whitespaces in define
  net: mdio: ipq8064: add regmap config to disable REGCACHE
  net: mdio: ipq8064: enlarge sleep after read/write operation
  net: dsa: qca8k: handle qca8k_set_page errors
  net: dsa: qca8k: handle error with qca8k_read operation
  net: dsa: qca8k: handle error with qca8k_write operation
  net: dsa: qca8k: handle error with qca8k_rmw operation
  net: dsa: qca8k: add support for qca8327 switch
  devicetree: net: dsa: qca8k: Document new compatible qca8327
  net: dsa: qca8k: add priority tweak to qca8337 switch
  net: dsa: qca8k: add GLOBAL_FC settings needed for qca8327
  net: dsa: qca8k: add support for switch rev
  net: dsa: qca8k: make rgmii delay configurable
  net: dsa: qca8k: clear MASTER_EN after phy read/write
  net: dsa: qca8k: dsa: qca8k: protect MASTER busy_wait with mdio mutex
  net: dsa: qca8k: enlarge mdio delay and timeout
  net: phy: phylink: permit to pass dev_flags to phylink_connect_phy
  net: dsa: slave: pass dev_flags also to internal PHY
  net: dsa: qca8k: pass switch_revision info to phy dev_flags
  net: phy: add qca8k driver for qca8k switch internal PHY

 .../devicetree/bindings/net/dsa/qca8k.txt     |   1 +
 drivers/net/dsa/qca8k.c                       | 602 ++++++++++++++----
 drivers/net/dsa/qca8k.h                       |  54 +-
 drivers/net/ethernet/cadence/macb_main.c      |   2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |   2 +-
 drivers/net/mdio/mdio-ipq8064.c               |  58 +-
 drivers/net/phy/Kconfig                       |   7 +
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/phylink.c                     |  12 +-
 drivers/net/phy/qca8k.c                       | 174 +++++
 include/linux/phylink.h                       |   2 +-
 net/dsa/slave.c                               |   6 +-
 12 files changed, 756 insertions(+), 165 deletions(-)
 create mode 100644 drivers/net/phy/qca8k.c

-- 
2.30.2

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

[Andrew Lunn]

On Wed, May 05, 2021 at 12:28:55AM +0200, 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>

    Andrew