[net-next PATCH v2 0/4] Reduce qca8k_priv space usage

[net-next PATCH v2 0/4] Reduce qca8k_priv space usage

[Ansuel Smith]

These 4 patch is a first attempt at reducting qca8k_priv space.
The code changed a lot during times and we have many old logic
that can be replaced with new implementation

The first patch drop the tracking of MTU. We mimic what was done
for mtk and we change MTU only when CPU port is changed.

The second patch finally drop a piece of story of this driver.
The ar8xxx_port_status struct was used by the first implementation
of this driver to put all sort of status data for the port...
With the evolution of DSA all that stuff got dropped till only
the enabled state was the only part of the that struct.
Since it's overkill to keep an array of int, we convert the variable
to a simple u8 where we store the status of each port. This is needed
to don't reanable ports on system resume.

The third patch is a preparation for patch 4. As Vladimir explained
in another patch, we waste a tons of space by keeping a duplicate of
the switch dsa ops in qca8k_priv. The only reason for this is to
dynamically set the correct mdiobus configuration (a legacy dsa one,
or a custom dedicated one)
To solve this problem, we just drop the phy_read/phy_write and we
declare a custom mdiobus in any case. 
This way we can use a static dsa switch ops struct and we can drop it
from qca8k_priv

Patch 4 finally drop the duplicated dsa_switch_ops.

This series is just a start of more cleanup.

The idea is to move this driver to the qca dir and split common code
from specific code. Also the mgmt eth code still requires some love
and can totally be optimized by recycling the same skb over time.

Also while working on the MTU it was notice some problem with
the stmmac driver and with the reloading phase that cause all
sort of problems with qca8k.

I'm sending this here just to try to keep small series instead of
proposing monster series hard to review.

v2:
- Rework MTU patch

Ansuel Smith (4):
  drivers: net: dsa: qca8k: drop MTU tracking from qca8k_priv
  drivers: net: dsa: qca8k: drop port_sts from qca8k_priv
  drivers: net: dsa: qca8k: rework and simplify mdiobus logic
  drivers: net: dsa: qca8k: drop dsa_switch_ops from qca8k_priv

 drivers/net/dsa/qca8k.c | 144 +++++++++++++++-------------------------
 drivers/net/dsa/qca8k.h |  12 ++--
 2 files changed, 56 insertions(+), 100 deletions(-)

-- 
2.34.1

[net-next PATCH v2 1/4] drivers: net: dsa: qca8k: drop MTU tracking from qca8k_priv

[Ansuel Smith]

DSA set the CPU port based on the largest MTU of all the slave ports.
Based on this we can drop the MTU array from qca8k_priv and set the
port_change_mtu logic on DSA changing MTU of the CPU port as the switch
have a global MTU settingfor each port.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index d3ed0a7f8077..820eeab19873 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -2367,16 +2367,17 @@ static int
 qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
 	struct qca8k_priv *priv = ds->priv;
-	int i, mtu = 0;
 
-	priv->port_mtu[port] = new_mtu;
-
-	for (i = 0; i < QCA8K_NUM_PORTS; i++)
-		if (priv->port_mtu[i] > mtu)
-			mtu = priv->port_mtu[i];
+	/* We have only have a general MTU setting.
+	 * DSA always set the CPU port's MTU to the largest MTU of the salve ports.
+	 * Setting MTU just for the CPU port is sufficient to correctly set a
+	 * value for every port.
+	 */
+	if (!dsa_is_cpu_port(ds, port))
+		return 0;
 
 	/* Include L2 header / FCS length */
-	return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
+	return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, new_mtu + ETH_HLEN + ETH_FCS_LEN);
 }
 
 static int
@@ -3033,16 +3034,6 @@ qca8k_setup(struct dsa_switch *ds)
 				  QCA8K_PORT_HOL_CTRL1_WRED_EN,
 				  mask);
 		}
-
-		/* Set initial MTU for every port.
-		 * We have only have a general MTU setting. So track
-		 * every port and set the max across all port.
-		 * Set per port MTU to 1500 as the MTU change function
-		 * will add the overhead and if its set to 1518 then it
-		 * will apply the overhead again and we will end up with
-		 * MTU of 1536 instead of 1518
-		 */
-		priv->port_mtu[i] = ETH_DATA_LEN;
 	}
 
 	/* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index f375627174c8..562d75997e55 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -398,7 +398,6 @@ struct qca8k_priv {
 	struct device *dev;
 	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
-	unsigned int port_mtu[QCA8K_NUM_PORTS];
 	struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
 	struct qca8k_mgmt_eth_data mgmt_eth_data;
 	struct qca8k_mib_eth_data mib_eth_data;
-- 
2.34.1

Re: [net-next PATCH v2 1/4] drivers: net: dsa: qca8k: drop MTU tracking from qca8k_priv

[Paolo Abeni]

On Tue, 2022-04-12 at 19:30 +0200, Ansuel Smith wrote:
> DSA set the CPU port based on the largest MTU of all the slave ports.
> Based on this we can drop the MTU array from qca8k_priv and set the
> port_change_mtu logic on DSA changing MTU of the CPU port as the switch
> have a global MTU settingfor each port.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

@Vladimir: it looks like this addresses your concern on the previous
iteration, could you please have a look?

Thanks!

Paolo

Re: [net-next PATCH v2 1/4] drivers: net: dsa: qca8k: drop MTU tracking from qca8k_priv

[Vladimir Oltean]

On Tue, Apr 12, 2022 at 07:30:16PM +0200, Ansuel Smith wrote:
> DSA set the CPU port based on the largest MTU of all the slave ports.
> Based on this we can drop the MTU array from qca8k_priv and set the
> port_change_mtu logic on DSA changing MTU of the CPU port as the switch
> have a global MTU settingfor each port.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/qca8k.c | 25 ++++++++-----------------
>  drivers/net/dsa/qca8k.h |  1 -
>  2 files changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index d3ed0a7f8077..820eeab19873 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -2367,16 +2367,17 @@ static int
>  qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  {
>  	struct qca8k_priv *priv = ds->priv;
> -	int i, mtu = 0;
>  
> -	priv->port_mtu[port] = new_mtu;
> -
> -	for (i = 0; i < QCA8K_NUM_PORTS; i++)
> -		if (priv->port_mtu[i] > mtu)
> -			mtu = priv->port_mtu[i];
> +	/* We have only have a general MTU setting.
> +	 * DSA always set the CPU port's MTU to the largest MTU of the salve ports.

s/salve/slave/

Also watch for the 80 characters limit.

> +	 * Setting MTU just for the CPU port is sufficient to correctly set a
> +	 * value for every port.
> +	 */
> +	if (!dsa_is_cpu_port(ds, port))
> +		return 0;
>  
>  	/* Include L2 header / FCS length */
> -	return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> +	return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, new_mtu + ETH_HLEN + ETH_FCS_LEN);
>  }
>  
>  static int
> @@ -3033,16 +3034,6 @@ qca8k_setup(struct dsa_switch *ds)
>  				  QCA8K_PORT_HOL_CTRL1_WRED_EN,
>  				  mask);
>  		}
> -
> -		/* Set initial MTU for every port.
> -		 * We have only have a general MTU setting. So track
> -		 * every port and set the max across all port.
> -		 * Set per port MTU to 1500 as the MTU change function
> -		 * will add the overhead and if its set to 1518 then it
> -		 * will apply the overhead again and we will end up with
> -		 * MTU of 1536 instead of 1518
> -		 */
> -		priv->port_mtu[i] = ETH_DATA_LEN;
>  	}
>  
>  	/* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index f375627174c8..562d75997e55 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -398,7 +398,6 @@ struct qca8k_priv {
>  	struct device *dev;
>  	struct dsa_switch_ops ops;
>  	struct gpio_desc *reset_gpio;
> -	unsigned int port_mtu[QCA8K_NUM_PORTS];
>  	struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
>  	struct qca8k_mgmt_eth_data mgmt_eth_data;
>  	struct qca8k_mib_eth_data mib_eth_data;
> -- 
> 2.34.1
> 

Re: [net-next PATCH v2 1/4] drivers: net: dsa: qca8k: drop MTU tracking from qca8k_priv

[Florian Fainelli]

On 4/12/2022 10:30 AM, Ansuel Smith wrote:
> DSA set the CPU port based on the largest MTU of all the slave ports.
> Based on this we can drop the MTU array from qca8k_priv and set the
> port_change_mtu logic on DSA changing MTU of the CPU port as the switch
> have a global MTU settingfor each port.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

[net-next PATCH v2 2/4] drivers: net: dsa: qca8k: drop port_sts from qca8k_priv

[Ansuel Smith]

Port_sts is a thing of the past for this driver. It was something
present on the initial implementation of this driver and parts of the
original struct were dropped over time. Using an array of int to store if
a port is enabled or not to handle PM operation seems overkill. Switch
and use a simple u8 to store the port status where each bit correspond
to a port. (bit is set port is enabled, bit is not set, port is disabled)
Also add some comments to better describe why we need to track port
status.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 820eeab19873..5f447b586cfa 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -2346,7 +2346,7 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
 	qca8k_port_set_status(priv, port, 1);
-	priv->port_sts[port].enabled = 1;
+	priv->port_enabled_map |= BIT(port);
 
 	if (dsa_is_user_port(ds, port))
 		phy_support_asym_pause(phy);
@@ -2360,7 +2360,7 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
 	qca8k_port_set_status(priv, port, 0);
-	priv->port_sts[port].enabled = 0;
+	priv->port_enabled_map &= ~BIT(port);
 }
 
 static int
@@ -3234,13 +3234,16 @@ static void qca8k_sw_shutdown(struct mdio_device *mdiodev)
 static void
 qca8k_set_pm(struct qca8k_priv *priv, int enable)
 {
-	int i;
+	int port;
 
-	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
-		if (!priv->port_sts[i].enabled)
+	for (port = 0; port < QCA8K_NUM_PORTS; port++) {
+		/* Do not enable on resume if the port was
+		 * disabled before.
+		 */
+		if (!(priv->port_enabled_map & BIT(port)))
 			continue;
 
-		qca8k_port_set_status(priv, i, enable);
+		qca8k_port_set_status(priv, port, enable);
 	}
 }
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 562d75997e55..12d8d090298b 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -324,10 +324,6 @@ enum qca8k_mid_cmd {
 	QCA8K_MIB_CAST = 3,
 };
 
-struct ar8xxx_port_status {
-	int enabled;
-};
-
 struct qca8k_match_data {
 	u8 id;
 	bool reduced_package;
@@ -388,11 +384,14 @@ struct qca8k_priv {
 	u8 mirror_rx;
 	u8 mirror_tx;
 	u8 lag_hash_mode;
+	/* Each bit correspond to a port. This switch can support a max of 7 port.
+	 * Bit 1: port enabled. Bit 0: port disabled.
+	 */
+	u8 port_enabled_map;
 	bool legacy_phy_port_mapping;
 	struct qca8k_ports_config ports_config;
 	struct regmap *regmap;
 	struct mii_bus *bus;
-	struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
 	struct dsa_switch *ds;
 	struct mutex reg_mutex;
 	struct device *dev;
-- 
2.34.1

Re: [net-next PATCH v2 2/4] drivers: net: dsa: qca8k: drop port_sts from qca8k_priv

[Vladimir Oltean]

On Tue, Apr 12, 2022 at 07:30:17PM +0200, Ansuel Smith wrote:
> Port_sts is a thing of the past for this driver. It was something
> present on the initial implementation of this driver and parts of the
> original struct were dropped over time. Using an array of int to store if
> a port is enabled or not to handle PM operation seems overkill. Switch
> and use a simple u8 to store the port status where each bit correspond
> to a port. (bit is set port is enabled, bit is not set, port is disabled)
> Also add some comments to better describe why we need to track port
> status.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [net-next PATCH v2 2/4] drivers: net: dsa: qca8k: drop port_sts from qca8k_priv

[Florian Fainelli]

On 4/12/2022 10:30 AM, Ansuel Smith wrote:
> Port_sts is a thing of the past for this driver. It was something
> present on the initial implementation of this driver and parts of the
> original struct were dropped over time. Using an array of int to store if
> a port is enabled or not to handle PM operation seems overkill. Switch
> and use a simple u8 to store the port status where each bit correspond
> to a port. (bit is set port is enabled, bit is not set, port is disabled)
> Also add some comments to better describe why we need to track port
> status.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

[net-next PATCH v2 3/4] drivers: net: dsa: qca8k: rework and simplify mdiobus logic

[Ansuel Smith]

In an attempt to reduce qca8k_priv space, rework and simplify mdiobus
logic.
We now declare a mdiobus instead of relying on DSA phy_read/write even
if a mdio node is not present. This is all to make the qca8k ops static
and not switch specific. With a legacy implementation where port doesn't
have a phy map declared in the dts with a mdio node, we declare a
'qca8k-legacy' mdiobus. The conversion logic is used as legacy read and
write ops are used instead of the internal one.
While at it also improve the bus id to support multiple switch.
Also drop the legacy_phy_port_mapping as we now declare mdiobus with ops
that already address the workaround.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 5f447b586cfa..9c4c5af79f9a 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1287,87 +1287,71 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
 	if (ret >= 0)
 		return ret;
 
-	return qca8k_mdio_read(priv, phy, regnum);
+	ret = qca8k_mdio_read(priv, phy, regnum);
+
+	if (ret < 0)
+		return 0xffff;
+
+	return ret;
 }
 
 static int
-qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
+qca8k_legacy_mdio_write(struct mii_bus *slave_bus, int port, int regnum, u16 data)
 {
-	struct qca8k_priv *priv = ds->priv;
-	int ret;
-
-	/* Check if the legacy mapping should be used and the
-	 * port is not correctly mapped to the right PHY in the
-	 * devicetree
-	 */
-	if (priv->legacy_phy_port_mapping)
-		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
-
-	/* Use mdio Ethernet when available, fallback to legacy one on error */
-	ret = qca8k_phy_eth_command(priv, false, port, regnum, 0);
-	if (!ret)
-		return ret;
+	port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
 
-	return qca8k_mdio_write(priv, port, regnum, data);
+	return qca8k_internal_mdio_write(slave_bus, port, regnum, data);
 }
 
 static int
-qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
+qca8k_legacy_mdio_read(struct mii_bus *slave_bus, int port, int regnum)
 {
-	struct qca8k_priv *priv = ds->priv;
-	int ret;
-
-	/* Check if the legacy mapping should be used and the
-	 * port is not correctly mapped to the right PHY in the
-	 * devicetree
-	 */
-	if (priv->legacy_phy_port_mapping)
-		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
-
-	/* Use mdio Ethernet when available, fallback to legacy one on error */
-	ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
-	if (ret >= 0)
-		return ret;
-
-	ret = qca8k_mdio_read(priv, port, regnum);
-
-	if (ret < 0)
-		return 0xffff;
+	port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
 
-	return ret;
+	return qca8k_internal_mdio_read(slave_bus, port, regnum);
 }
 
 static int
-qca8k_mdio_register(struct qca8k_priv *priv, struct device_node *mdio)
+qca8k_mdio_register(struct qca8k_priv *priv)
 {
 	struct dsa_switch *ds = priv->ds;
+	struct device_node *mdio;
 	struct mii_bus *bus;
 
 	bus = devm_mdiobus_alloc(ds->dev);
-
 	if (!bus)
 		return -ENOMEM;
 
 	bus->priv = (void *)priv;
-	bus->name = "qca8k slave mii";
-	bus->read = qca8k_internal_mdio_read;
-	bus->write = qca8k_internal_mdio_write;
-	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d",
-		 ds->index);
-
+	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d",
+		 ds->dst->index, ds->index);
 	bus->parent = ds->dev;
 	bus->phy_mask = ~ds->phys_mii_mask;
-
 	ds->slave_mii_bus = bus;
 
-	return devm_of_mdiobus_register(priv->dev, bus, mdio);
+	/* Check if the devicetree declare the port:phy mapping */
+	mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
+	if (of_device_is_available(mdio)) {
+		bus->name = "qca8k slave mii";
+		bus->read = qca8k_internal_mdio_read;
+		bus->write = qca8k_internal_mdio_write;
+		return devm_of_mdiobus_register(priv->dev, bus, mdio);
+	}
+
+	/* If a mapping can't be found the legacy mapping is used,
+	 * using the qca8k_port_to_phy function
+	 */
+	bus->name = "qca8k-legacy slave mii";
+	bus->read = qca8k_legacy_mdio_read;
+	bus->write = qca8k_legacy_mdio_write;
+	return devm_mdiobus_register(priv->dev, bus);
 }
 
 static int
 qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 {
 	u32 internal_mdio_mask = 0, external_mdio_mask = 0, reg;
-	struct device_node *ports, *port, *mdio;
+	struct device_node *ports, *port;
 	phy_interface_t mode;
 	int err;
 
@@ -1429,24 +1413,7 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 					 QCA8K_MDIO_MASTER_EN);
 	}
 
-	/* Check if the devicetree declare the port:phy mapping */
-	mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
-	if (of_device_is_available(mdio)) {
-		err = qca8k_mdio_register(priv, mdio);
-		if (err)
-			of_node_put(mdio);
-
-		return err;
-	}
-
-	/* If a mapping can't be found the legacy mapping is used,
-	 * using the qca8k_port_to_phy function
-	 */
-	priv->legacy_phy_port_mapping = true;
-	priv->ops.phy_read = qca8k_phy_read;
-	priv->ops.phy_write = qca8k_phy_write;
-
-	return 0;
+	return qca8k_mdio_register(priv);
 }
 
 static int
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 12d8d090298b..8bbe36f135b5 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -388,7 +388,6 @@ struct qca8k_priv {
 	 * Bit 1: port enabled. Bit 0: port disabled.
 	 */
 	u8 port_enabled_map;
-	bool legacy_phy_port_mapping;
 	struct qca8k_ports_config ports_config;
 	struct regmap *regmap;
 	struct mii_bus *bus;
-- 
2.34.1

Re: [net-next PATCH v2 3/4] drivers: net: dsa: qca8k: rework and simplify mdiobus logic

[Vladimir Oltean]

On Tue, Apr 12, 2022 at 07:30:18PM +0200, Ansuel Smith wrote:
> In an attempt to reduce qca8k_priv space, rework and simplify mdiobus
> logic.
> We now declare a mdiobus instead of relying on DSA phy_read/write even
> if a mdio node is not present. This is all to make the qca8k ops static
> and not switch specific. With a legacy implementation where port doesn't
> have a phy map declared in the dts with a mdio node, we declare a
> 'qca8k-legacy' mdiobus. The conversion logic is used as legacy read and
> write ops are used instead of the internal one.
> While at it also improve the bus id to support multiple switch.
> Also drop the legacy_phy_port_mapping as we now declare mdiobus with ops
> that already address the workaround.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 101 ++++++++++++++--------------------------
>  drivers/net/dsa/qca8k.h |   1 -
>  2 files changed, 34 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 5f447b586cfa..9c4c5af79f9a 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -1287,87 +1287,71 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
>  	if (ret >= 0)
>  		return ret;
>  
> -	return qca8k_mdio_read(priv, phy, regnum);
> +	ret = qca8k_mdio_read(priv, phy, regnum);
> +
> +	if (ret < 0)
> +		return 0xffff;
> +
> +	return ret;

Unrelated change?

>  }
>  
>  static int
> -qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
> +qca8k_legacy_mdio_write(struct mii_bus *slave_bus, int port, int regnum, u16 data)
>  {
> -	struct qca8k_priv *priv = ds->priv;
> -	int ret;
> -
> -	/* Check if the legacy mapping should be used and the
> -	 * port is not correctly mapped to the right PHY in the
> -	 * devicetree
> -	 */
> -	if (priv->legacy_phy_port_mapping)
> -		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> -
> -	/* Use mdio Ethernet when available, fallback to legacy one on error */
> -	ret = qca8k_phy_eth_command(priv, false, port, regnum, 0);
> -	if (!ret)
> -		return ret;
> +	port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
>  
> -	return qca8k_mdio_write(priv, port, regnum, data);
> +	return qca8k_internal_mdio_write(slave_bus, port, regnum, data);
>  }
>  
>  static int
> -qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
> +qca8k_legacy_mdio_read(struct mii_bus *slave_bus, int port, int regnum)
>  {
> -	struct qca8k_priv *priv = ds->priv;
> -	int ret;
> -
> -	/* Check if the legacy mapping should be used and the
> -	 * port is not correctly mapped to the right PHY in the
> -	 * devicetree
> -	 */
> -	if (priv->legacy_phy_port_mapping)
> -		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> -
> -	/* Use mdio Ethernet when available, fallback to legacy one on error */
> -	ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
> -	if (ret >= 0)
> -		return ret;
> -
> -	ret = qca8k_mdio_read(priv, port, regnum);
> -
> -	if (ret < 0)
> -		return 0xffff;
> +	port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
>  
> -	return ret;
> +	return qca8k_internal_mdio_read(slave_bus, port, regnum);
>  }
>  
>  static int
> -qca8k_mdio_register(struct qca8k_priv *priv, struct device_node *mdio)
> +qca8k_mdio_register(struct qca8k_priv *priv)
>  {
>  	struct dsa_switch *ds = priv->ds;
> +	struct device_node *mdio;
>  	struct mii_bus *bus;
>  
>  	bus = devm_mdiobus_alloc(ds->dev);
> -
>  	if (!bus)
>  		return -ENOMEM;
>  
>  	bus->priv = (void *)priv;
> -	bus->name = "qca8k slave mii";
> -	bus->read = qca8k_internal_mdio_read;
> -	bus->write = qca8k_internal_mdio_write;
> -	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d",
> -		 ds->index);
> -
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d",
> +		 ds->dst->index, ds->index);

So the bus->id here is the same, regardless if this is the OF-based MDIO
bus or the legacy slave MII bus. dsa_slave_mii_bus_init() used to set
the bus->id to "dsa-%d.%d", ds->dst->index, ds->index), so you're doing
this to kind of preserve the structure (although not completely).

In any case, this is an unrelated change, because not only are you
modifying the bus->id of the legacy MII bus, but also the bus->id of the
OF-based one. I would rather have it be a separate patch just for that.

Does the MDIO bus id constitute unbreakable ABI? I hope not, otherwise
as you say, we couldn't support multiple switches.

>  	bus->parent = ds->dev;
>  	bus->phy_mask = ~ds->phys_mii_mask;
> -
>  	ds->slave_mii_bus = bus;
>  
> -	return devm_of_mdiobus_register(priv->dev, bus, mdio);
> +	/* Check if the devicetree declare the port:phy mapping */
> +	mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
> +	if (of_device_is_available(mdio)) {
> +		bus->name = "qca8k slave mii";
> +		bus->read = qca8k_internal_mdio_read;
> +		bus->write = qca8k_internal_mdio_write;
> +		return devm_of_mdiobus_register(priv->dev, bus, mdio);
> +	}
> +
> +	/* If a mapping can't be found the legacy mapping is used,
> +	 * using the qca8k_port_to_phy function
> +	 */
> +	bus->name = "qca8k-legacy slave mii";
> +	bus->read = qca8k_legacy_mdio_read;
> +	bus->write = qca8k_legacy_mdio_write;
> +	return devm_mdiobus_register(priv->dev, bus);
>  }
>  
>  static int
>  qca8k_setup_mdio_bus(struct qca8k_priv *priv)
>  {
>  	u32 internal_mdio_mask = 0, external_mdio_mask = 0, reg;
> -	struct device_node *ports, *port, *mdio;
> +	struct device_node *ports, *port;
>  	phy_interface_t mode;
>  	int err;
>  
> @@ -1429,24 +1413,7 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
>  					 QCA8K_MDIO_MASTER_EN);
>  	}
>  
> -	/* Check if the devicetree declare the port:phy mapping */
> -	mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
> -	if (of_device_is_available(mdio)) {
> -		err = qca8k_mdio_register(priv, mdio);
> -		if (err)
> -			of_node_put(mdio);
> -
> -		return err;
> -	}
> -
> -	/* If a mapping can't be found the legacy mapping is used,
> -	 * using the qca8k_port_to_phy function
> -	 */
> -	priv->legacy_phy_port_mapping = true;
> -	priv->ops.phy_read = qca8k_phy_read;
> -	priv->ops.phy_write = qca8k_phy_write;
> -
> -	return 0;
> +	return qca8k_mdio_register(priv);
>  }
>  
>  static int
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 12d8d090298b..8bbe36f135b5 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -388,7 +388,6 @@ struct qca8k_priv {
>  	 * Bit 1: port enabled. Bit 0: port disabled.
>  	 */
>  	u8 port_enabled_map;
> -	bool legacy_phy_port_mapping;
>  	struct qca8k_ports_config ports_config;
>  	struct regmap *regmap;
>  	struct mii_bus *bus;
> -- 
> 2.34.1
> 

Re: [net-next PATCH v2 3/4] drivers: net: dsa: qca8k: rework and simplify mdiobus logic

[Andrew Lunn]

> Does the MDIO bus id constitute unbreakable ABI? I hope not, otherwise
> as you say, we couldn't support multiple switches.

In theory yes, but in practice i doubt it is. I would suggest you make
the change. If we get a report of a regression, then we can think
about it some more.

      Andrew

[net-next PATCH v2 4/4] drivers: net: dsa: qca8k: drop dsa_switch_ops from qca8k_priv

[Ansuel Smith]

Now that dsa_switch_ops is not switch specific anymore, we can drop it
from qca8k_priv and use the static ops directly for the dsa_switch
pointer.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 9c4c5af79f9a..48a71d85b4ff 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -3160,8 +3160,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
 	priv->ds->priv = priv;
-	priv->ops = qca8k_switch_ops;
-	priv->ds->ops = &priv->ops;
+	priv->ds->ops = &qca8k_switch_ops;
 	mutex_init(&priv->reg_mutex);
 	dev_set_drvdata(&mdiodev->dev, priv);
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 8bbe36f135b5..04408e11402a 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -394,7 +394,6 @@ struct qca8k_priv {
 	struct dsa_switch *ds;
 	struct mutex reg_mutex;
 	struct device *dev;
-	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
 	struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
 	struct qca8k_mgmt_eth_data mgmt_eth_data;
-- 
2.34.1

Re: [net-next PATCH v2 4/4] drivers: net: dsa: qca8k: drop dsa_switch_ops from qca8k_priv

[Vladimir Oltean]

On Tue, Apr 12, 2022 at 07:30:19PM +0200, Ansuel Smith wrote:
> Now that dsa_switch_ops is not switch specific anymore, we can drop it
> from qca8k_priv and use the static ops directly for the dsa_switch
> pointer.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [net-next PATCH v2 4/4] drivers: net: dsa: qca8k: drop dsa_switch_ops from qca8k_priv

[Florian Fainelli]

On 4/12/2022 10:30 AM, Ansuel Smith wrote:
> Now that dsa_switch_ops is not switch specific anymore, we can drop it
> from qca8k_priv and use the static ops directly for the dsa_switch
> pointer.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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