[net-next RFC PATCH 0/4] net: dsa: qca8k: code split for qca8k

[net-next RFC PATCH 0/4] net: dsa: qca8k: code split for qca8k

[Christian Marangi]

This is posted as an RFC as it does contain changes that depends on a
regmap patch. The patch is here [1] hoping it will get approved.

If it will be NACKed, I will have to rework this and revert one of the
patch that makes use of the new regmap bulk implementation.

Anyway, this is needed ad ipq4019 SoC have an internal switch that is
based on qca8k with very minor changes. The general function is equal.

Because of this we split the driver to common and specific code.

As the common function needs to be moved to a different file to be
reused, we had to convert every remaining user of qca8k_read/write/rmw
to regmap variant.
We had also to generilized the special handling for the ethtool_stats
function that makes use of the autocast mib. (ipq4019 will have a
different tagger and use mmio so it could be quicker to use mmio instead
of automib feature)
And we had to convert the regmap read/write to bulk implementation to
drop the special function that makes use of it. This will be compatible
with ipq4019 and at the same time permits normal switch to use the eth
mgmt way to send the entire ATU table read/write in one go.

(the bulk implementation could not be done when it was introduced as
regmap didn't support at times bulk read/write without a bus)

[1] https://lore.kernel.org/lkml/20220715201032.19507-1-ansuelsmth@gmail.com/

Christian Marangi (4):
  net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant
  net: dsa: qca8k: convert to regmap read/write API
  net: dsa: qca8k: rework mib autocast handling
  net: dsa: qca8k: split qca8k in common and 8xxx specific code

 drivers/net/dsa/qca/Makefile                  |    1 +
 drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} | 1638 +++--------------
 drivers/net/dsa/qca/qca8k-common.c            | 1174 ++++++++++++
 drivers/net/dsa/qca/qca8k.h                   |   61 +
 4 files changed, 1463 insertions(+), 1411 deletions(-)
 rename drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} (60%)
 create mode 100644 drivers/net/dsa/qca/qca8k-common.c

-- 
2.36.1

[net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Christian Marangi]

In preparation for code split, drop the remaining qca8k_read/write/rmw
and use regmap helper directly.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/qca8k.c | 206 +++++++++++++++++-------------------
 1 file changed, 95 insertions(+), 111 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k.c
index 1cbb05b0323f..2d34e15c2e6f 100644
--- a/drivers/net/dsa/qca/qca8k.c
+++ b/drivers/net/dsa/qca/qca8k.c
@@ -184,24 +184,6 @@ qca8k_set_page(struct qca8k_priv *priv, u16 page)
 	return 0;
 }
 
-static int
-qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val)
-{
-	return regmap_read(priv->regmap, reg, val);
-}
-
-static int
-qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
-{
-	return regmap_write(priv->regmap, reg, val);
-}
-
-static int
-qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
-{
-	return regmap_update_bits(priv->regmap, reg, mask, write_val);
-}
-
 static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data;
@@ -647,7 +629,7 @@ qca8k_fdb_access(struct qca8k_priv *priv, enum qca8k_fdb_cmd cmd, int port)
 	}
 
 	/* Write the function register triggering the table access */
-	ret = qca8k_write(priv, QCA8K_REG_ATU_FUNC, reg);
+	ret = regmap_write(priv->regmap, QCA8K_REG_ATU_FUNC, reg);
 	if (ret)
 		return ret;
 
@@ -658,7 +640,7 @@ 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) {
-		ret = qca8k_read(priv, QCA8K_REG_ATU_FUNC, &reg);
+		ret = regmap_read(priv->regmap, QCA8K_REG_ATU_FUNC, &reg);
 		if (ret < 0)
 			return ret;
 		if (reg & QCA8K_ATU_FUNC_FULL)
@@ -803,7 +785,7 @@ qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid)
 	reg |= FIELD_PREP(QCA8K_VTU_FUNC1_VID_MASK, vid);
 
 	/* Write the function register triggering the table access */
-	ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC1, reg);
+	ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC1, reg);
 	if (ret)
 		return ret;
 
@@ -814,7 +796,7 @@ 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) {
-		ret = qca8k_read(priv, QCA8K_REG_VTU_FUNC1, &reg);
+		ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC1, &reg);
 		if (ret < 0)
 			return ret;
 		if (reg & QCA8K_VTU_FUNC1_FULL)
@@ -842,7 +824,7 @@ qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
 	if (ret < 0)
 		goto out;
 
-	ret = qca8k_read(priv, QCA8K_REG_VTU_FUNC0, &reg);
+	ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC0, &reg);
 	if (ret < 0)
 		goto out;
 	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
@@ -852,7 +834,7 @@ qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
 	else
 		reg |= QCA8K_VTU_FUNC0_EG_MODE_PORT_TAG(port);
 
-	ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+	ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC0, reg);
 	if (ret)
 		goto out;
 	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
@@ -875,7 +857,7 @@ qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
 	if (ret < 0)
 		goto out;
 
-	ret = qca8k_read(priv, QCA8K_REG_VTU_FUNC0, &reg);
+	ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC0, &reg);
 	if (ret < 0)
 		goto out;
 	reg &= ~QCA8K_VTU_FUNC0_EG_MODE_PORT_MASK(port);
@@ -895,7 +877,7 @@ qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
 	if (del) {
 		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
 	} else {
-		ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+		ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC0, reg);
 		if (ret)
 			goto out;
 		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
@@ -928,7 +910,7 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	if (ret)
 		goto exit;
 
-	ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
+	ret = regmap_write(priv->regmap, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
 
 exit:
 	mutex_unlock(&priv->reg_mutex);
@@ -1434,10 +1416,10 @@ qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
 		mask |= QCA8K_MAC_PWR_RGMII1_1_8V;
 
 	if (mask) {
-		ret = qca8k_rmw(priv, QCA8K_REG_MAC_PWR_SEL,
-				QCA8K_MAC_PWR_RGMII0_1_8V |
-				QCA8K_MAC_PWR_RGMII1_1_8V,
-				mask);
+		ret = regmap_update_bits(priv->regmap, QCA8K_REG_MAC_PWR_SEL,
+					 QCA8K_MAC_PWR_RGMII0_1_8V |
+					 QCA8K_MAC_PWR_RGMII1_1_8V,
+					 mask);
 	}
 
 	return ret;
@@ -1478,8 +1460,8 @@ qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
 		if (data->reduced_package)
 			val |= QCA8327_PWS_PACKAGE148_EN;
 
-		ret = qca8k_rmw(priv, QCA8K_REG_PWS, QCA8327_PWS_PACKAGE148_EN,
-				val);
+		ret = regmap_update_bits(priv->regmap, QCA8K_REG_PWS,
+					 QCA8327_PWS_PACKAGE148_EN, val);
 		if (ret)
 			return ret;
 	}
@@ -1496,7 +1478,7 @@ qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
 		val |= QCA8K_PWS_LED_OPEN_EN_CSR;
 	}
 
-	return qca8k_rmw(priv, QCA8K_REG_PWS,
+	return regmap_update_bits(priv->regmap, QCA8K_REG_PWS,
 			QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL,
 			val);
 }
@@ -1629,12 +1611,12 @@ qca8k_mac_config_setup_internal_delay(struct qca8k_priv *priv, int cpu_port_inde
 	}
 
 	/* Set RGMII delay based on the selected values */
-	ret = qca8k_rmw(priv, reg,
-			QCA8K_PORT_PAD_RGMII_TX_DELAY_MASK |
-			QCA8K_PORT_PAD_RGMII_RX_DELAY_MASK |
-			QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
-			QCA8K_PORT_PAD_RGMII_RX_DELAY_EN,
-			val);
+	ret = regmap_update_bits(priv->regmap, reg,
+				 QCA8K_PORT_PAD_RGMII_TX_DELAY_MASK |
+				 QCA8K_PORT_PAD_RGMII_RX_DELAY_MASK |
+				 QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
+				 QCA8K_PORT_PAD_RGMII_RX_DELAY_EN,
+				 val);
 	if (ret)
 		dev_err(priv->dev, "Failed to set internal delay for CPU port%d",
 			cpu_port_index == QCA8K_CPU_PORT0 ? 0 : 6);
@@ -1723,7 +1705,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	case PHY_INTERFACE_MODE_RGMII_ID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 	case PHY_INTERFACE_MODE_RGMII_RXID:
-		qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
+		regmap_write(priv->regmap, reg, QCA8K_PORT_PAD_RGMII_EN);
 
 		/* Configure rgmii delay */
 		qca8k_mac_config_setup_internal_delay(priv, cpu_port_index, reg);
@@ -1733,13 +1715,13 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		 * rather than individual port registers.
 		 */
 		if (priv->switch_id == QCA8K_ID_QCA8337)
-			qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
-				    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+			regmap_write(priv->regmap, QCA8K_REG_PORT5_PAD_CTRL,
+				     QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
 		/* Enable SGMII on the port */
-		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+		regmap_write(priv->regmap, reg, QCA8K_PORT_PAD_SGMII_EN);
 		break;
 	default:
 		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
@@ -1832,7 +1814,7 @@ qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
 
 	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
 
-	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
+	regmap_write(priv->regmap, QCA8K_REG_PORT_STATUS(port), reg);
 }
 
 static struct qca8k_pcs *pcs_to_qca8k_pcs(struct phylink_pcs *pcs)
@@ -1848,7 +1830,7 @@ static void qca8k_pcs_get_state(struct phylink_pcs *pcs,
 	u32 reg;
 	int ret;
 
-	ret = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port), &reg);
+	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
 	if (ret < 0) {
 		state->link = false;
 		return;
@@ -1908,17 +1890,17 @@ static int qca8k_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 	}
 
 	/* Enable/disable SerDes auto-negotiation as necessary */
-	ret = qca8k_read(priv, QCA8K_REG_PWS, &val);
+	ret = regmap_read(priv->regmap, QCA8K_REG_PWS, &val);
 	if (ret)
 		return ret;
 	if (phylink_autoneg_inband(mode))
 		val &= ~QCA8K_PWS_SERDES_AEN_DIS;
 	else
 		val |= QCA8K_PWS_SERDES_AEN_DIS;
-	qca8k_write(priv, QCA8K_REG_PWS, val);
+	regmap_write(priv->regmap, QCA8K_REG_PWS, val);
 
 	/* Configure the SGMII parameters */
-	ret = qca8k_read(priv, QCA8K_REG_SGMII_CTRL, &val);
+	ret = regmap_read(priv->regmap, QCA8K_REG_SGMII_CTRL, &val);
 	if (ret)
 		return ret;
 
@@ -1940,7 +1922,7 @@ static int qca8k_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		val |= QCA8K_SGMII_MODE_CTRL_BASEX;
 	}
 
-	qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
+	regmap_write(priv->regmap, QCA8K_REG_SGMII_CTRL, val);
 
 	/* From original code is reported port instability as SGMII also
 	 * require delay set. Apply advised values here or take them from DT.
@@ -1964,10 +1946,10 @@ static int qca8k_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		val |= QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE;
 
 	if (val)
-		ret = qca8k_rmw(priv, reg,
-				QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE |
-				QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
-				val);
+		ret = regmap_update_bits(priv->regmap, reg,
+					 QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE |
+					 QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
+					 val);
 
 	return 0;
 }
@@ -2122,12 +2104,12 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 		mib = &ar8327_mib[i];
 		reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
 
-		ret = qca8k_read(priv, reg, &val);
+		ret = regmap_read(priv->regmap, reg, &val);
 		if (ret < 0)
 			continue;
 
 		if (mib->size == 2) {
-			ret = qca8k_read(priv, reg + 4, &hi);
+			ret = regmap_read(priv->regmap, reg + 4, &hi);
 			if (ret < 0)
 				continue;
 		}
@@ -2161,7 +2143,7 @@ qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
 	int ret;
 
 	mutex_lock(&priv->reg_mutex);
-	ret = qca8k_read(priv, QCA8K_REG_EEE_CTRL, &reg);
+	ret = regmap_read(priv->regmap, QCA8K_REG_EEE_CTRL, &reg);
 	if (ret < 0)
 		goto exit;
 
@@ -2169,7 +2151,7 @@ qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee)
 		reg |= lpi_en;
 	else
 		reg &= ~lpi_en;
-	ret = qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
+	ret = regmap_write(priv->regmap, QCA8K_REG_EEE_CTRL, reg);
 
 exit:
 	mutex_unlock(&priv->reg_mutex);
@@ -2208,8 +2190,8 @@ qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 		break;
 	}
 
-	qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-		  QCA8K_PORT_LOOKUP_STATE_MASK, stp_state);
+	regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+			   QCA8K_PORT_LOOKUP_STATE_MASK, stp_state);
 }
 
 static int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
@@ -2242,8 +2224,8 @@ static int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
 	}
 
 	/* Add all other ports to this ports portvlan mask */
-	ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-			QCA8K_PORT_LOOKUP_MEMBER, port_mask);
+	ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+				 QCA8K_PORT_LOOKUP_MEMBER, port_mask);
 
 	return ret;
 }
@@ -2272,8 +2254,8 @@ static void qca8k_port_bridge_leave(struct dsa_switch *ds, int port,
 	/* Set the cpu port to be the only one in the portvlan mask of
 	 * this port
 	 */
-	qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-		  QCA8K_PORT_LOOKUP_MEMBER, BIT(cpu_port));
+	regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+			   QCA8K_PORT_LOOKUP_MEMBER, BIT(cpu_port));
 }
 
 static void
@@ -2357,7 +2339,7 @@ qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 		qca8k_port_set_status(priv, 6, 0);
 
 	/* Include L2 header / FCS length */
-	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, new_mtu + ETH_HLEN + ETH_FCS_LEN);
+	ret = regmap_write(priv->regmap, QCA8K_MAX_FRAME_SIZE, new_mtu + ETH_HLEN + ETH_FCS_LEN);
 
 	if (priv->port_enabled_map & BIT(0))
 		qca8k_port_set_status(priv, 0, 1);
@@ -2560,13 +2542,13 @@ qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 	int ret;
 
 	if (vlan_filtering) {
-		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-				QCA8K_PORT_LOOKUP_VLAN_MODE_MASK,
-				QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
+		ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+					 QCA8K_PORT_LOOKUP_VLAN_MODE_MASK,
+					 QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
 	} else {
-		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-				QCA8K_PORT_LOOKUP_VLAN_MODE_MASK,
-				QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
+		ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+					 QCA8K_PORT_LOOKUP_VLAN_MODE_MASK,
+					 QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
 	}
 
 	return ret;
@@ -2589,15 +2571,15 @@ qca8k_port_vlan_add(struct dsa_switch *ds, int port,
 	}
 
 	if (pvid) {
-		ret = qca8k_rmw(priv, QCA8K_EGRESS_VLAN(port),
-				QCA8K_EGREES_VLAN_PORT_MASK(port),
-				QCA8K_EGREES_VLAN_PORT(port, vlan->vid));
+		ret = regmap_update_bits(priv->regmap, QCA8K_EGRESS_VLAN(port),
+					 QCA8K_EGREES_VLAN_PORT_MASK(port),
+					 QCA8K_EGREES_VLAN_PORT(port, vlan->vid));
 		if (ret)
 			return ret;
 
-		ret = qca8k_write(priv, QCA8K_REG_PORT_VLAN_CTRL0(port),
-				  QCA8K_PORT_VLAN_CVID(vlan->vid) |
-				  QCA8K_PORT_VLAN_SVID(vlan->vid));
+		ret = regmap_write(priv->regmap, QCA8K_REG_PORT_VLAN_CTRL0(port),
+				   QCA8K_PORT_VLAN_CVID(vlan->vid) |
+				   QCA8K_PORT_VLAN_SVID(vlan->vid));
 	}
 
 	return ret;
@@ -2905,16 +2887,18 @@ qca8k_setup(struct dsa_switch *ds)
 	/* Initial setup of all ports */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
 		/* Disable forwarding by default on all ports */
-		ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-				QCA8K_PORT_LOOKUP_MEMBER, 0);
+		ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
+					 QCA8K_PORT_LOOKUP_MEMBER, 0);
 		if (ret)
 			return ret;
 
 		/* Enable QCA header mode on all cpu ports */
 		if (dsa_is_cpu_port(ds, i)) {
-			ret = qca8k_write(priv, QCA8K_REG_PORT_HDR_CTRL(i),
-					  FIELD_PREP(QCA8K_PORT_HDR_CTRL_TX_MASK, QCA8K_PORT_HDR_CTRL_ALL) |
-					  FIELD_PREP(QCA8K_PORT_HDR_CTRL_RX_MASK, QCA8K_PORT_HDR_CTRL_ALL));
+			ret = regmap_write(priv->regmap, QCA8K_REG_PORT_HDR_CTRL(i),
+					   FIELD_PREP(QCA8K_PORT_HDR_CTRL_TX_MASK,
+						      QCA8K_PORT_HDR_CTRL_ALL) |
+					   FIELD_PREP(QCA8K_PORT_HDR_CTRL_RX_MASK,
+						      QCA8K_PORT_HDR_CTRL_ALL));
 			if (ret) {
 				dev_err(priv->dev, "failed enabling QCA header mode");
 				return ret;
@@ -2930,11 +2914,11 @@ qca8k_setup(struct dsa_switch *ds)
 	 * Notice that in multi-cpu config only one port should be set
 	 * for igmp, unknown, multicast and broadcast packet
 	 */
-	ret = qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
-			  FIELD_PREP(QCA8K_GLOBAL_FW_CTRL1_IGMP_DP_MASK, BIT(cpu_port)) |
-			  FIELD_PREP(QCA8K_GLOBAL_FW_CTRL1_BC_DP_MASK, BIT(cpu_port)) |
-			  FIELD_PREP(QCA8K_GLOBAL_FW_CTRL1_MC_DP_MASK, BIT(cpu_port)) |
-			  FIELD_PREP(QCA8K_GLOBAL_FW_CTRL1_UC_DP_MASK, BIT(cpu_port)));
+	ret = regmap_write(priv->regmap, QCA8K_REG_GLOBAL_FW_CTRL1,
+			   FIELD_PREP(QCA8K_GLOBAL_FW_CTRL1_IGMP_DP_MASK, BIT(cpu_port)) |
+			   FIELD_PREP(QCA8K_GLOBAL_FW_CTRL1_BC_DP_MASK, BIT(cpu_port)) |
+			   FIELD_PREP(QCA8K_GLOBAL_FW_CTRL1_MC_DP_MASK, BIT(cpu_port)) |
+			   FIELD_PREP(QCA8K_GLOBAL_FW_CTRL1_UC_DP_MASK, BIT(cpu_port)));
 	if (ret)
 		return ret;
 
@@ -2944,17 +2928,17 @@ 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)) {
-			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
+			ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
+						 QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
 			if (ret)
 				return ret;
 		}
 
 		/* Individual user ports get connected to CPU port only */
 		if (dsa_is_user_port(ds, i)) {
-			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-					QCA8K_PORT_LOOKUP_MEMBER,
-					BIT(cpu_port));
+			ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(i),
+						 QCA8K_PORT_LOOKUP_MEMBER,
+						 BIT(cpu_port));
 			if (ret)
 				return ret;
 
@@ -2967,15 +2951,15 @@ qca8k_setup(struct dsa_switch *ds)
 			/* For port based vlans to work we need to set the
 			 * default egress vid
 			 */
-			ret = qca8k_rmw(priv, QCA8K_EGRESS_VLAN(i),
-					QCA8K_EGREES_VLAN_PORT_MASK(i),
-					QCA8K_EGREES_VLAN_PORT(i, QCA8K_PORT_VID_DEF));
+			ret = regmap_update_bits(priv->regmap, QCA8K_EGRESS_VLAN(i),
+						 QCA8K_EGREES_VLAN_PORT_MASK(i),
+						 QCA8K_EGREES_VLAN_PORT(i, QCA8K_PORT_VID_DEF));
 			if (ret)
 				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));
+			ret = regmap_write(priv->regmap, 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;
 		}
@@ -3009,18 +2993,18 @@ qca8k_setup(struct dsa_switch *ds)
 					QCA8K_PORT_HOL_CTRL0_EG_PRI3(0x8) |
 					QCA8K_PORT_HOL_CTRL0_EG_PORT(0x19);
 			}
-			qca8k_write(priv, QCA8K_REG_PORT_HOL_CTRL0(i), mask);
+			regmap_write(priv->regmap, 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_MASK |
-				  QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
-				  QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
-				  QCA8K_PORT_HOL_CTRL1_WRED_EN,
-				  mask);
+			regmap_update_bits(priv->regmap, QCA8K_REG_PORT_HOL_CTRL1(i),
+					   QCA8K_PORT_HOL_CTRL1_ING_BUF_MASK |
+					   QCA8K_PORT_HOL_CTRL1_EG_PRI_BUF_EN |
+					   QCA8K_PORT_HOL_CTRL1_EG_PORT_BUF_EN |
+					   QCA8K_PORT_HOL_CTRL1_WRED_EN,
+					   mask);
 		}
 	}
 
@@ -3028,14 +3012,14 @@ qca8k_setup(struct dsa_switch *ds)
 	if (priv->switch_id == QCA8K_ID_QCA8327) {
 		mask = QCA8K_GLOBAL_FC_GOL_XON_THRES(288) |
 		       QCA8K_GLOBAL_FC_GOL_XOFF_THRES(496);
-		qca8k_rmw(priv, QCA8K_REG_GLOBAL_FC_THRESH,
-			  QCA8K_GLOBAL_FC_GOL_XON_THRES_MASK |
-			  QCA8K_GLOBAL_FC_GOL_XOFF_THRES_MASK,
-			  mask);
+		regmap_update_bits(priv->regmap, QCA8K_REG_GLOBAL_FC_THRESH,
+				   QCA8K_GLOBAL_FC_GOL_XON_THRES_MASK |
+				   QCA8K_GLOBAL_FC_GOL_XOFF_THRES_MASK,
+				   mask);
 	}
 
 	/* Setup our port MTUs to match power on defaults */
-	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
+	ret = regmap_write(priv->regmap, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
 	if (ret)
 		dev_warn(priv->dev, "failed setting MTU settings");
 
@@ -3103,7 +3087,7 @@ static int qca8k_read_switch_id(struct qca8k_priv *priv)
 	if (!data)
 		return -ENODEV;
 
-	ret = qca8k_read(priv, QCA8K_REG_MASK_CTRL, &val);
+	ret = regmap_read(priv->regmap, QCA8K_REG_MASK_CTRL, &val);
 	if (ret < 0)
 		return -ENODEV;
 
-- 
2.36.1

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Vladimir Oltean]

On Sat, Jul 16, 2022 at 07:49:55PM +0200, Christian Marangi wrote:
> In preparation for code split, drop the remaining qca8k_read/write/rmw
> and use regmap helper directly.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca/qca8k.c | 206 +++++++++++++++++-------------------
>  1 file changed, 95 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k.c
> index 1cbb05b0323f..2d34e15c2e6f 100644
> --- a/drivers/net/dsa/qca/qca8k.c
> +++ b/drivers/net/dsa/qca/qca8k.c
> @@ -184,24 +184,6 @@ qca8k_set_page(struct qca8k_priv *priv, u16 page)
>  	return 0;
>  }
>  
> -static int
> -qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val)
> -{
> -	return regmap_read(priv->regmap, reg, val);
> -}
> -
> -static int
> -qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
> -{
> -	return regmap_write(priv->regmap, reg, val);
> -}
> -
> -static int
> -qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> -{
> -	return regmap_update_bits(priv->regmap, reg, mask, write_val);
> -}
> -

Could you please explain slowly to me why this change is needed? I don't get it.
Can't qca8k_read(), qca8k_write() and qca8k_rmw() be part of qca8k-common.c?

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Christian Marangi]

On Mon, Jul 18, 2022 at 09:04:52PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 16, 2022 at 07:49:55PM +0200, Christian Marangi wrote:
> > In preparation for code split, drop the remaining qca8k_read/write/rmw
> > and use regmap helper directly.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca/qca8k.c | 206 +++++++++++++++++-------------------
> >  1 file changed, 95 insertions(+), 111 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k.c
> > index 1cbb05b0323f..2d34e15c2e6f 100644
> > --- a/drivers/net/dsa/qca/qca8k.c
> > +++ b/drivers/net/dsa/qca/qca8k.c
> > @@ -184,24 +184,6 @@ qca8k_set_page(struct qca8k_priv *priv, u16 page)
> >  	return 0;
> >  }
> >  
> > -static int
> > -qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val)
> > -{
> > -	return regmap_read(priv->regmap, reg, val);
> > -}
> > -
> > -static int
> > -qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
> > -{
> > -	return regmap_write(priv->regmap, reg, val);
> > -}
> > -
> > -static int
> > -qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> > -{
> > -	return regmap_update_bits(priv->regmap, reg, mask, write_val);
> > -}
> > -
> 
> Could you please explain slowly to me why this change is needed? I don't get it.
> Can't qca8k_read(), qca8k_write() and qca8k_rmw() be part of qca8k-common.c?

Sure.
When the regmap conversion was done at times, to limit patch delta it
was suggested to keep these function. This was to not get crazy with
eventual backports and fixes.

The logic here is:
As we are moving these function AND the function will use regmap api
anyway, we can finally drop them and user the regmap api directly
instead of these additional function.

When the regmap conversion was done, I pointed out that in the future
the driver had to be split in specific and common code and it was said
that only at that times there was a good reason to make all these
changes and drop these special functions.

Now these function are used by both setup function for qca8k and by
common function that will be moved to a different file.


If we really want I can skip the dropping of these function and move
them to qca8k common code.

An alternative is to keep them for qca8k specific code and migrate the
common function to regmap api.

So it's really a choice of drop these additional function or keep using
them for the sake of not modifying too much source.

Hope it's clear now the reason of this change.

-- 
	Ansuel

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Vladimir Oltean]

On Mon, Jul 18, 2022 at 07:55:26PM +0200, Christian Marangi wrote:
> Sure.
> When the regmap conversion was done at times, to limit patch delta it
> was suggested to keep these function. This was to not get crazy with
> eventual backports and fixes.
> 
> The logic here is:
> As we are moving these function AND the function will use regmap api
> anyway, we can finally drop them and user the regmap api directly
> instead of these additional function.
> 
> When the regmap conversion was done, I pointed out that in the future
> the driver had to be split in specific and common code and it was said
> that only at that times there was a good reason to make all these
> changes and drop these special functions.
> 
> Now these function are used by both setup function for qca8k and by
> common function that will be moved to a different file.
> 
> 
> If we really want I can skip the dropping of these function and move
> them to qca8k common code.

I don't really have a preference, I just want to understand why you want
to call regmap_read(priv->regmap) directly every time as opposed to
qca8k_read(priv) which is shorter to type and allows more stuff to fit
on one line.

I think if you run "make drivers/net/dsa/qca/qca8k.lst" and you look at
the generated code listing before and after, you'll find it is identical
(note, I haven't actually done that).

> An alternative is to keep them for qca8k specific code and migrate the
> common function to regmap api.

No, that's silly and I can't even find a reason to do that.
It's not like you're trying to create a policy to not call qca8k-common.c
functions from qca8k-8xxx.c, right? That should work just fine (in this
case, qca8k_read etc).

In fact, while typing this I realized that in your code structure,
you'll have one struct dsa_switch_ops in qca8k-8xxx.c and another one in
qca8k-ipq4019.c. But the vast majority of dsa_switch_ops are common,
with the exception of .setup() which is switch-specific, correct?

Wouldn't you consider, as an alternative, to move the dsa_switch_ops
structure to the common C file as well, and have a switch-specific
(*setup) operation in the match_data structure? Or even much better,
make the switch-specific ops as fine-grained as possible, rather than
reimplementing the entire qca8k_setup() (note, I don't know how similar
they are, but there should be as little duplication of logic as possible,
the common code should dictate what there is to do, and the switch
specific code just how to do it).

> So it's really a choice of drop these additional function or keep using
> them for the sake of not modifying too much source.
> 
> Hope it's clear now the reason of this change.

If I were to summarize your reason, it would be "because I prefer it
that way and because now is a good time", right? That's fine with me,
but I honestly didn't understand that while reading the commit message.

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Christian Marangi]

On Mon, Jul 18, 2022 at 09:40:17PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 18, 2022 at 07:55:26PM +0200, Christian Marangi wrote:
> > Sure.
> > When the regmap conversion was done at times, to limit patch delta it
> > was suggested to keep these function. This was to not get crazy with
> > eventual backports and fixes.
> > 
> > The logic here is:
> > As we are moving these function AND the function will use regmap api
> > anyway, we can finally drop them and user the regmap api directly
> > instead of these additional function.
> > 
> > When the regmap conversion was done, I pointed out that in the future
> > the driver had to be split in specific and common code and it was said
> > that only at that times there was a good reason to make all these
> > changes and drop these special functions.
> > 
> > Now these function are used by both setup function for qca8k and by
> > common function that will be moved to a different file.
> > 
> > 
> > If we really want I can skip the dropping of these function and move
> > them to qca8k common code.
> 
> I don't really have a preference, I just want to understand why you want
> to call regmap_read(priv->regmap) directly every time as opposed to
> qca8k_read(priv) which is shorter to type and allows more stuff to fit
> on one line.

The main reason is that it's one less function. qca8k_read calls
directly the regmap ops so it seems a good time to drop it.

> 
> I think if you run "make drivers/net/dsa/qca/qca8k.lst" and you look at
> the generated code listing before and after, you'll find it is identical
> (note, I haven't actually done that).
> 
> > An alternative is to keep them for qca8k specific code and migrate the
> > common function to regmap api.
> 
> No, that's silly and I can't even find a reason to do that.
> It's not like you're trying to create a policy to not call qca8k-common.c
> functions from qca8k-8xxx.c, right? That should work just fine (in this
> case, qca8k_read etc).

The idea of qca8k-common is to keep them as generilized as possible.
Considering ipq4019 will have a different way to write/read regs we can't
lock common function to specific implementation.

> 
> In fact, while typing this I realized that in your code structure,
> you'll have one struct dsa_switch_ops in qca8k-8xxx.c and another one in
> qca8k-ipq4019.c. But the vast majority of dsa_switch_ops are common,
> with the exception of .setup() which is switch-specific, correct?

Phylink ops will also be different as ipq4019 will have qsgmii and will
require some calibration logic.

> 
> Wouldn't you consider, as an alternative, to move the dsa_switch_ops
> structure to the common C file as well, and have a switch-specific
> (*setup) operation in the match_data structure? Or even much better,
> make the switch-specific ops as fine-grained as possible, rather than
> reimplementing the entire qca8k_setup() (note, I don't know how similar
> they are, but there should be as little duplication of logic as possible,
> the common code should dictate what there is to do, and the switch
> specific code just how to do it).
> 

qca8k_setup will require major investigation and I think it would be
better to do do a qca8k_setup generalization when ipq4019 will be
proposed.

On the other hand I like the idea of putting the qca8k ops in common.c
and make the driver adds the relevant specific options.
Think I will also move that to common.c. That would permit to keep
function static aka even less delta and less bloat in the header file.

(is it a problem if it won't be const?)

> > So it's really a choice of drop these additional function or keep using
> > them for the sake of not modifying too much source.
> > 
> > Hope it's clear now the reason of this change.
> 
> If I were to summarize your reason, it would be "because I prefer it
> that way and because now is a good time", right? That's fine with me,
> but I honestly didn't understand that while reading the commit message.

I have to be honest... Yes you are right... This is really my opinion
and I don't have a particular strong reason on why dropping them.

It's really that I don't like keeping function that are just leftover of
an old implementation. But my target here is not argue and find a
solution so it's OK for me if I should keep these compat function and
migrate them to common.c.

-- 
	Ansuel

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Vladimir Oltean]

On Mon, Jul 18, 2022 at 08:40:14PM +0200, Christian Marangi wrote:
> > I don't really have a preference, I just want to understand why you want
> > to call regmap_read(priv->regmap) directly every time as opposed to
> > qca8k_read(priv) which is shorter to type and allows more stuff to fit
> > on one line.
> 
> The main reason is that it's one less function. qca8k_read calls
> directly the regmap ops so it seems a good time to drop it.

This is before applying your patch 1/4, with an armv7 compiler:
make drivers/net/dsa/qca/qca8k.lst

I'm looking at the qca8k_read() call from qca8k_pcs_get_state():

000009d8 <qca8k_pcs_get_state>:
{
     9d8:	e92d4030 	push	{r4, r5, lr}
     9dc:	e3005000 	movw	r5, #0
			9dc: R_ARM_MOVW_ABS_NC	__stack_chk_guard
	ret = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port), &reg);
     9e0:	e590300c 	ldr	r3, [r0, #12]
{
     9e4:	e3405000 	movt	r5, #0
			9e4: R_ARM_MOVT_ABS	__stack_chk_guard
     9e8:	e24dd00c 	sub	sp, sp, #12
     9ec:	e1a04001 	mov	r4, r1
	return regmap_read(priv->regmap, reg, val);
     9f0:	e5900008 	ldr	r0, [r0, #8]
     9f4:	e1a0200d 	mov	r2, sp
{
     9f8:	e595c000 	ldr	ip, [r5]
	ret = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port), &reg);
     9fc:	e283101f 	add	r1, r3, #31
	return regmap_read(priv->regmap, reg, val);
     a00:	e1a01101 	lsl	r1, r1, #2
     a04:	e5900010 	ldr	r0, [r0, #16]
{
     a08:	e58dc004 	str	ip, [sp, #4]
	return regmap_read(priv->regmap, reg, val);
     a0c:	ebfffffe 	bl	0 <regmap_read>
			a0c: R_ARM_CALL	regmap_read
(portions irrelevant to regmap cut out)

And this is how it looks like after applying your patch 1/4:

000009d8 <qca8k_pcs_get_state>:
{
     9d8:	e92d4030 	push	{r4, r5, lr}
     9dc:	e3005000 	movw	r5, #0
			9dc: R_ARM_MOVW_ABS_NC	__stack_chk_guard
	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
     9e0:	e590300c 	ldr	r3, [r0, #12]
{
     9e4:	e3405000 	movt	r5, #0
			9e4: R_ARM_MOVT_ABS	__stack_chk_guard
     9e8:	e24dd00c 	sub	sp, sp, #12
     9ec:	e1a04001 	mov	r4, r1
	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
     9f0:	e5900008 	ldr	r0, [r0, #8]
     9f4:	e1a0200d 	mov	r2, sp
{
     9f8:	e595c000 	ldr	ip, [r5]
	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
     9fc:	e283101f 	add	r1, r3, #31
     a00:	e1a01101 	lsl	r1, r1, #2
     a04:	e5900010 	ldr	r0, [r0, #16]
{
     a08:	e58dc004 	str	ip, [sp, #4]
	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
     a0c:	ebfffffe 	bl	0 <regmap_read>
			a0c: R_ARM_CALL	regmap_read

You don't even need to recognize the instructions or calling conventions
to figure out that the generated assembly code is identical.

> > 
> > I think if you run "make drivers/net/dsa/qca/qca8k.lst" and you look at
> > the generated code listing before and after, you'll find it is identical
> > (note, I haven't actually done that).
> > 
> > > An alternative is to keep them for qca8k specific code and migrate the
> > > common function to regmap api.
> > 
> > No, that's silly and I can't even find a reason to do that.
> > It's not like you're trying to create a policy to not call qca8k-common.c
> > functions from qca8k-8xxx.c, right? That should work just fine (in this
> > case, qca8k_read etc).
> 
> The idea of qca8k-common is to keep them as generilized as possible.
> Considering ipq4019 will have a different way to write/read regs we can't
> lock common function to specific implementation.

Wait a minute, what's the difference between having this in common.c:

	qca8k_read(priv)

vs this:

	regmap_read(priv->regmap)

when qca8k_read is implemented *exactly* as a call to regmap_read(priv->regmap)?
There's nothing *specific* to a switch in the implementation of qca8k_read().
But rather, all differences lie in the regmap_config structure and in
the way the regmap was created. But the common code operates with a
pointer to a generic regmap structure, regardless of how that was created.

So no, sorry, there is no technical argument for which you cannot have
calls to qca8k_read() in common.c. I can work with "that's the way I prefer",
but let's not try to invent technical arguments when there aren't any.

> > In fact, while typing this I realized that in your code structure,
> > you'll have one struct dsa_switch_ops in qca8k-8xxx.c and another one in
> > qca8k-ipq4019.c. But the vast majority of dsa_switch_ops are common,
> > with the exception of .setup() which is switch-specific, correct?
> 
> Phylink ops will also be different as ipq4019 will have qsgmii and will
> require some calibration logic.

Ok, phylink too, the point is that they aren't radically different switches
for the majority of operations.

> qca8k_setup will require major investigation and I think it would be
> better to do do a qca8k_setup generalization when ipq4019 will be
> proposed.

Ok, "major investigation" sounds about right, that's what I was looking
to hear. The alternative would have been to plop a separate ipq4019_setup(),
leave qca8k_setup() alone, and call it a day. FWIW, that's essentially
where the microchip ksz set of drivers were, before Arun Ramadoss
started doing some major cleanup through them. After some point, this
strategy simply stops scaling.

> On the other hand I like the idea of putting the qca8k ops in common.c
> and make the driver adds the relevant specific options.
> Think I will also move that to common.c. That would permit to keep
> function static aka even less delta and less bloat in the header file.
> 
> (is it a problem if it won't be const?)

yeah, it's a problem if it won't be const, why wouldn't it?

> > If I were to summarize your reason, it would be "because I prefer it
> > that way and because now is a good time", right? That's fine with me,
> > but I honestly didn't understand that while reading the commit message.
> 
> I have to be honest... Yes you are right... This is really my opinion
> and I don't have a particular strong reason on why dropping them.
> 
> It's really that I don't like keeping function that are just leftover of
> an old implementation. But my target here is not argue and find a
> solution so it's OK for me if I should keep these compat function and
> migrate them to common.c.

I know that the revolutionary spirit can be strong, but it's good to keep
in mind that "older/newer" is not always synonymous with "worse/better" ;)

Again, I don't have a strong objection against the change and I'm not
going to argue about it either. My comment was simply because I didn't
physically UNDERSTAND you. My expectations were also a bit confused,
because I initially thought it's a necessary change (that's why I
replied to it last), and I just didn't understand what's so necessary
about it.

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Christian Marangi]

On Mon, Jul 18, 2022 at 10:35:21PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 18, 2022 at 08:40:14PM +0200, Christian Marangi wrote:
> > > I don't really have a preference, I just want to understand why you want
> > > to call regmap_read(priv->regmap) directly every time as opposed to
> > > qca8k_read(priv) which is shorter to type and allows more stuff to fit
> > > on one line.
> > 
> > The main reason is that it's one less function. qca8k_read calls
> > directly the regmap ops so it seems a good time to drop it.
> 
> This is before applying your patch 1/4, with an armv7 compiler:
> make drivers/net/dsa/qca/qca8k.lst
> 
> I'm looking at the qca8k_read() call from qca8k_pcs_get_state():
> 
> 000009d8 <qca8k_pcs_get_state>:
> {
>      9d8:	e92d4030 	push	{r4, r5, lr}
>      9dc:	e3005000 	movw	r5, #0
> 			9dc: R_ARM_MOVW_ABS_NC	__stack_chk_guard
> 	ret = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port), &reg);
>      9e0:	e590300c 	ldr	r3, [r0, #12]
> {
>      9e4:	e3405000 	movt	r5, #0
> 			9e4: R_ARM_MOVT_ABS	__stack_chk_guard
>      9e8:	e24dd00c 	sub	sp, sp, #12
>      9ec:	e1a04001 	mov	r4, r1
> 	return regmap_read(priv->regmap, reg, val);
>      9f0:	e5900008 	ldr	r0, [r0, #8]
>      9f4:	e1a0200d 	mov	r2, sp
> {
>      9f8:	e595c000 	ldr	ip, [r5]
> 	ret = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port), &reg);
>      9fc:	e283101f 	add	r1, r3, #31
> 	return regmap_read(priv->regmap, reg, val);
>      a00:	e1a01101 	lsl	r1, r1, #2
>      a04:	e5900010 	ldr	r0, [r0, #16]
> {
>      a08:	e58dc004 	str	ip, [sp, #4]
> 	return regmap_read(priv->regmap, reg, val);
>      a0c:	ebfffffe 	bl	0 <regmap_read>
> 			a0c: R_ARM_CALL	regmap_read
> (portions irrelevant to regmap cut out)
> 
> And this is how it looks like after applying your patch 1/4:
> 
> 000009d8 <qca8k_pcs_get_state>:
> {
>      9d8:	e92d4030 	push	{r4, r5, lr}
>      9dc:	e3005000 	movw	r5, #0
> 			9dc: R_ARM_MOVW_ABS_NC	__stack_chk_guard
> 	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
>      9e0:	e590300c 	ldr	r3, [r0, #12]
> {
>      9e4:	e3405000 	movt	r5, #0
> 			9e4: R_ARM_MOVT_ABS	__stack_chk_guard
>      9e8:	e24dd00c 	sub	sp, sp, #12
>      9ec:	e1a04001 	mov	r4, r1
> 	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
>      9f0:	e5900008 	ldr	r0, [r0, #8]
>      9f4:	e1a0200d 	mov	r2, sp
> {
>      9f8:	e595c000 	ldr	ip, [r5]
> 	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
>      9fc:	e283101f 	add	r1, r3, #31
>      a00:	e1a01101 	lsl	r1, r1, #2
>      a04:	e5900010 	ldr	r0, [r0, #16]
> {
>      a08:	e58dc004 	str	ip, [sp, #4]
> 	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
>      a0c:	ebfffffe 	bl	0 <regmap_read>
> 			a0c: R_ARM_CALL	regmap_read
> 
> You don't even need to recognize the instructions or calling conventions
> to figure out that the generated assembly code is identical.
> 
> > > 
> > > I think if you run "make drivers/net/dsa/qca/qca8k.lst" and you look at
> > > the generated code listing before and after, you'll find it is identical
> > > (note, I haven't actually done that).
> > > 
> > > > An alternative is to keep them for qca8k specific code and migrate the
> > > > common function to regmap api.
> > > 
> > > No, that's silly and I can't even find a reason to do that.
> > > It's not like you're trying to create a policy to not call qca8k-common.c
> > > functions from qca8k-8xxx.c, right? That should work just fine (in this
> > > case, qca8k_read etc).
> > 
> > The idea of qca8k-common is to keep them as generilized as possible.
> > Considering ipq4019 will have a different way to write/read regs we can't
> > lock common function to specific implementation.
> 
> Wait a minute, what's the difference between having this in common.c:
> 
> 	qca8k_read(priv)
> 
> vs this:
> 
> 	regmap_read(priv->regmap)
> 
> when qca8k_read is implemented *exactly* as a call to regmap_read(priv->regmap)?
> There's nothing *specific* to a switch in the implementation of qca8k_read().
> But rather, all differences lie in the regmap_config structure and in
> the way the regmap was created. But the common code operates with a
> pointer to a generic regmap structure, regardless of how that was created.
> 
> So no, sorry, there is no technical argument for which you cannot have
> calls to qca8k_read() in common.c. I can work with "that's the way I prefer",
> but let's not try to invent technical arguments when there aren't any.
> 
> > > In fact, while typing this I realized that in your code structure,
> > > you'll have one struct dsa_switch_ops in qca8k-8xxx.c and another one in
> > > qca8k-ipq4019.c. But the vast majority of dsa_switch_ops are common,
> > > with the exception of .setup() which is switch-specific, correct?
> > 
> > Phylink ops will also be different as ipq4019 will have qsgmii and will
> > require some calibration logic.
> 
> Ok, phylink too, the point is that they aren't radically different switches
> for the majority of operations.
> 
> > qca8k_setup will require major investigation and I think it would be
> > better to do do a qca8k_setup generalization when ipq4019 will be
> > proposed.
> 
> Ok, "major investigation" sounds about right, that's what I was looking
> to hear. The alternative would have been to plop a separate ipq4019_setup(),
> leave qca8k_setup() alone, and call it a day. FWIW, that's essentially
> where the microchip ksz set of drivers were, before Arun Ramadoss
> started doing some major cleanup through them. After some point, this
> strategy simply stops scaling.
> 
> > On the other hand I like the idea of putting the qca8k ops in common.c
> > and make the driver adds the relevant specific options.
> > Think I will also move that to common.c. That would permit to keep
> > function static aka even less delta and less bloat in the header file.
> > 
> > (is it a problem if it won't be const?)
> 
> yeah, it's a problem if it won't be const, why wouldn't it?
>

Tell me if I got this wrong.

The suggestion was to move the struct dsa_switch_ops to qca8k.h and add
in the specific code probe the needed ops to add to the generic
struct...

But now that I think about it I was just confused and it can totally be
const so sorry for the stupid question

Anyway to make it clear, these are the option that will be the same for
qca8k and ipq4019

.get_strings		= qca8k_get_strings,
	.get_ethtool_stats	= qca8k_get_ethtool_stats,
	.get_sset_count		= qca8k_get_sset_count,
	.set_ageing_time	= qca8k_set_ageing_time,
	.get_mac_eee		= qca8k_get_mac_eee,
	.set_mac_eee		= qca8k_set_mac_eee,
	.port_enable		= qca8k_port_enable,
	.port_disable		= qca8k_port_disable,
	.port_change_mtu	= qca8k_port_change_mtu,
	.port_max_mtu		= qca8k_port_max_mtu,
	.port_stp_state_set	= qca8k_port_stp_state_set,
	.port_bridge_join	= qca8k_port_bridge_join,
	.port_bridge_leave	= qca8k_port_bridge_leave,
	.port_fast_age		= qca8k_port_fast_age,
	.port_fdb_add		= qca8k_port_fdb_add,
	.port_fdb_del		= qca8k_port_fdb_del,
	.port_fdb_dump		= qca8k_port_fdb_dump,
	.port_mdb_add		= qca8k_port_mdb_add,
	.port_mdb_del		= qca8k_port_mdb_del,
	.port_mirror_add	= qca8k_port_mirror_add,
	.port_mirror_del	= qca8k_port_mirror_del,
	.port_vlan_filtering	= qca8k_port_vlan_filtering,
	.port_vlan_add		= qca8k_port_vlan_add,
	.port_vlan_del		= qca8k_port_vlan_del,
	.port_lag_join		= qca8k_port_lag_join,
	.port_lag_leave		= qca8k_port_lag_leave,

Everything else has to be set by the driver. So yes the amount of shared
function is enough to have a reason to declare a general dsa_switch_ops.

> > > If I were to summarize your reason, it would be "because I prefer it
> > > that way and because now is a good time", right? That's fine with me,
> > > but I honestly didn't understand that while reading the commit message.
> > 
> > I have to be honest... Yes you are right... This is really my opinion
> > and I don't have a particular strong reason on why dropping them.
> > 
> > It's really that I don't like keeping function that are just leftover of
> > an old implementation. But my target here is not argue and find a
> > solution so it's OK for me if I should keep these compat function and
> > migrate them to common.c.
> 
> I know that the revolutionary spirit can be strong, but it's good to keep
> in mind that "older/newer" is not always synonymous with "worse/better" ;)
> 
> Again, I don't have a strong objection against the change and I'm not
> going to argue about it either. My comment was simply because I didn't
> physically UNDERSTAND you. My expectations were also a bit confused,
> because I initially thought it's a necessary change (that's why I
> replied to it last), and I just didn't understand what's so necessary
> about it.

Ok at the end I probably chose the wrong words since this is really a
cleanup and nothing necessary for the function of the code split.

Will send a v2 with RFC removed and a more friendly series hoping it
won't grow too much with the code split.

Again thanks for the all the good review :D

-- 
	Ansuel

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Vladimir Oltean]

On Mon, Jul 18, 2022 at 09:30:58PM +0200, Christian Marangi wrote:
> Tell me if I got this wrong.
> 
> The suggestion was to move the struct dsa_switch_ops to qca8k.h and add
> in the specific code probe the needed ops to add to the generic
> struct...

The declaration yes; the definition to qca8k-common.c. See for example
where felix_switch_ops is, relative to felix_vsc9959.c, seville_vsc9953.c
(users), felix.h (declaration), and felix.c (definition). Or how
mv88e6xxx_switch_ops does things and still supports a gazillion of switches.

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Christian Marangi]

On Mon, Jul 18, 2022 at 11:30:42PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 18, 2022 at 09:30:58PM +0200, Christian Marangi wrote:
> > Tell me if I got this wrong.
> > 
> > The suggestion was to move the struct dsa_switch_ops to qca8k.h and add
> > in the specific code probe the needed ops to add to the generic
> > struct...
> 
> The declaration yes; the definition to qca8k-common.c. See for example
> where felix_switch_ops is, relative to felix_vsc9959.c, seville_vsc9953.c
> (users), felix.h (declaration), and felix.c (definition). Or how
> mv88e6xxx_switch_ops does things and still supports a gazillion of switches.

Mhh I checked the example and they doesn't seems to be useful from my
problem. But I think it's better to discuss this to the patch directly
so you can better understand whay I intended with having dsa_switch_ops
set to const.

-- 
	Ansuel

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Vladimir Oltean]

On Mon, Jul 18, 2022 at 11:54:44PM +0200, Christian Marangi wrote:
> On Mon, Jul 18, 2022 at 11:30:42PM +0300, Vladimir Oltean wrote:
> > On Mon, Jul 18, 2022 at 09:30:58PM +0200, Christian Marangi wrote:
> > > Tell me if I got this wrong.
> > > 
> > > The suggestion was to move the struct dsa_switch_ops to qca8k.h and add
> > > in the specific code probe the needed ops to add to the generic
> > > struct...
> > 
> > The declaration yes; the definition to qca8k-common.c. See for example
> > where felix_switch_ops is, relative to felix_vsc9959.c, seville_vsc9953.c
> > (users), felix.h (declaration), and felix.c (definition). Or how
> > mv88e6xxx_switch_ops does things and still supports a gazillion of switches.
> 
> Mhh I checked the example and they doesn't seems to be useful from my
> problem. But I think it's better to discuss this to the patch directly
> so you can better understand whay I intended with having dsa_switch_ops
> set to const.

So you don't modify the common dsa_switch_ops from the switch-specific
probe path, but rather, from the common dsa_switch_ops method, you call
a second function pointer.

static void felix_phylink_validate(struct dsa_switch *ds, int port,
				   unsigned long *supported,
				   struct phylink_link_state *state)
{
	struct ocelot *ocelot = ds->priv;
	struct felix *felix = ocelot_to_felix(ocelot);

	if (felix->info->phylink_validate)
		felix->info->phylink_validate(ocelot, port, supported, state);
}

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Christian Marangi]

On Tue, Jul 19, 2022 at 02:43:58AM +0300, Vladimir Oltean wrote:
> On Mon, Jul 18, 2022 at 11:54:44PM +0200, Christian Marangi wrote:
> > On Mon, Jul 18, 2022 at 11:30:42PM +0300, Vladimir Oltean wrote:
> > > On Mon, Jul 18, 2022 at 09:30:58PM +0200, Christian Marangi wrote:
> > > > Tell me if I got this wrong.
> > > > 
> > > > The suggestion was to move the struct dsa_switch_ops to qca8k.h and add
> > > > in the specific code probe the needed ops to add to the generic
> > > > struct...
> > > 
> > > The declaration yes; the definition to qca8k-common.c. See for example
> > > where felix_switch_ops is, relative to felix_vsc9959.c, seville_vsc9953.c
> > > (users), felix.h (declaration), and felix.c (definition). Or how
> > > mv88e6xxx_switch_ops does things and still supports a gazillion of switches.
> > 
> > Mhh I checked the example and they doesn't seems to be useful from my
> > problem. But I think it's better to discuss this to the patch directly
> > so you can better understand whay I intended with having dsa_switch_ops
> > set to const.
> 
> So you don't modify the common dsa_switch_ops from the switch-specific
> probe path, but rather, from the common dsa_switch_ops method, you call
> a second function pointer.
> 
> static void felix_phylink_validate(struct dsa_switch *ds, int port,
> 				   unsigned long *supported,
> 				   struct phylink_link_state *state)
> {
> 	struct ocelot *ocelot = ds->priv;
> 	struct felix *felix = ocelot_to_felix(ocelot);
> 
> 	if (felix->info->phylink_validate)
> 		felix->info->phylink_validate(ocelot, port, supported, state);
> }

Ohhh ok now it makes sense.

If the ops is not supported should I return -ENOSUPP?
Example some ops won't be supported like the get_phy_flags or
connect_tag_protocol for example.

Anyway the series is ready, I was just pushing it... At the end it's 23
patch big... (I know you will hate me but at least it's reviewable)

My solution currently was this...

	ops = devm_kzalloc(&mdiodev->dev, sizeof(*ops), GFP_KERNEL);
	if (!ops)
		return -ENOMEM;

	/* Copy common ops */
	memcpy(ops, &qca8k_switch_ops, sizeof(*ops));

	/* Setup specific ops */
	ops->get_tag_protocol = qca8k_get_tag_protocol;
	ops->setup = qca8k_setup;
	ops->phylink_get_caps = qca8k_phylink_get_caps;
	ops->phylink_mac_select_pcs = qca8k_phylink_mac_select_pcs;
	ops->phylink_mac_config = qca8k_phylink_mac_config;
	ops->phylink_mac_link_down = qca8k_phylink_mac_link_down;
	ops->phylink_mac_link_up = qca8k_phylink_mac_link_up;
	ops->get_phy_flags = qca8k_get_phy_flags;
	ops->master_state_change = qca8k_master_change;
	ops->connect_tag_protocol = qca8k_connect_tag_protocol;

	/* Assign the final ops */
	priv->ds->ops = ops;

Will wait your response on how to hanle ops that are not supported.
(I assume dsa checks if an ops is declared and not if it does return
ENOSUPP, so this is my concern your example)

-- 
	Ansuel

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Vladimir Oltean]

On Tue, Jul 19, 2022 at 01:32:26AM +0200, Christian Marangi wrote:
> If the ops is not supported should I return -ENOSUPP?
> Example some ops won't be supported like the get_phy_flags or
> connect_tag_protocol for example.

That's a slight disadvantage of this approach, that DSA sometimes checks
for the presence of a certain function pointer as an indication of
whether a feature is supported or not. However that doesn't work in all
cases, and then, it is actually necessary to call and see if it returns
-EOPNOTSUPP or not. For example, commit 1054457006d4 ("net: phy:
phylink: fix DSA mac_select_pcs() introduction") had to do just that
in phylink because of DSA.

However, you need to check how each specific DSA operation is handled.
For example, the no-op implementation of get_phy_flags is to return 0
(meaning "no special flags, thank you"). The no-op implementation for
connect_tag_protocol is to return success (0) for the tagging protocol
you support, and -EOPNOTSUPP for everything else. Here -EOPNOTSUPP isn't
a special code, it is an actual hard error that denies a certain tag
protocol from attaching.

The advantage is that your driver-private ops don't have to map 1:1 with
the dsa_switch_ops, so there is more potential for code reuse than if
you had to reimplement an entire (*setup) function for example. You can
have ops for small things like regmap creation, things like that.

> Anyway the series is ready, I was just pushing it... At the end it's 23
> patch big... (I know you will hate me but at least it's reviewable)

Please optimize the patches for a reviewer with average intelligence and
the attention span of a fish. 23 patches sounds like the series would
fail on the attention span count.

> My solution currently was this...
> 
> 	ops = devm_kzalloc(&mdiodev->dev, sizeof(*ops), GFP_KERNEL);
> 	if (!ops)
> 		return -ENOMEM;
> 
> 	/* Copy common ops */
> 	memcpy(ops, &qca8k_switch_ops, sizeof(*ops));
> 
> 	/* Setup specific ops */
> 	ops->get_tag_protocol = qca8k_get_tag_protocol;

Answered above.

> 	ops->setup = qca8k_setup;

Separate sub-operation, although this is a sub-optimal short-term
solution that kind of undermines the approach with a single
dsa_switch_ops in the long run.

> 	ops->phylink_get_caps = qca8k_phylink_get_caps;

Not sure what's going to be common and what's going to be different, but
you can take other drivers as an example, some parts will be common and
some hidden behind priv->info->mac_port_get_caps().

static void mt753x_phylink_get_caps(struct dsa_switch *ds, int port,
				    struct phylink_config *config)
{
	struct mt7530_priv *priv = ds->priv;

	/* This switch only supports full-duplex at 1Gbps */
	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
				   MAC_10 | MAC_100 | MAC_1000FD;

	/* This driver does not make use of the speed, duplex, pause or the
	 * advertisement in its mac_config, so it is safe to mark this driver
	 * as non-legacy.
	 */
	config->legacy_pre_march2020 = false;

	priv->info->mac_port_get_caps(ds, port, config);
}

> 	ops->phylink_mac_select_pcs = qca8k_phylink_mac_select_pcs;
> 	ops->phylink_mac_config = qca8k_phylink_mac_config;
> 	ops->phylink_mac_link_down = qca8k_phylink_mac_link_down;
> 	ops->phylink_mac_link_up = qca8k_phylink_mac_link_up;

Hard to comment for these phylink ops how to organize the switch
differences in the best way, since I don't actually know what those
differences are. Again, other drivers may be useful.

> 	ops->get_phy_flags = qca8k_get_phy_flags;
> 	ops->master_state_change = qca8k_master_change;
> 	ops->connect_tag_protocol = qca8k_connect_tag_protocol;
> 
> 	/* Assign the final ops */
> 	priv->ds->ops = ops;
> 
> Will wait your response on how to hanle ops that are not supported.
> (I assume dsa checks if an ops is declared and not if it does return
> ENOSUPP, so this is my concern your example)

Maybe it's best to think this conversion through and not rush a patch set.
I don't want you to blindly follow my advice to have a single dsa_switch_ops,
then half-ass it. This kind of thing needs to be done with care and
forethought.

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Christian Marangi]

On Tue, Jul 19, 2022 at 03:18:11AM +0300, Vladimir Oltean wrote:
> On Tue, Jul 19, 2022 at 01:32:26AM +0200, Christian Marangi wrote:
> > If the ops is not supported should I return -ENOSUPP?
> > Example some ops won't be supported like the get_phy_flags or
> > connect_tag_protocol for example.
> 
> That's a slight disadvantage of this approach, that DSA sometimes checks
> for the presence of a certain function pointer as an indication of
> whether a feature is supported or not. However that doesn't work in all
> cases, and then, it is actually necessary to call and see if it returns
> -EOPNOTSUPP or not. For example, commit 1054457006d4 ("net: phy:
> phylink: fix DSA mac_select_pcs() introduction") had to do just that
> in phylink because of DSA.
> 
> However, you need to check how each specific DSA operation is handled.
> For example, the no-op implementation of get_phy_flags is to return 0
> (meaning "no special flags, thank you"). The no-op implementation for
> connect_tag_protocol is to return success (0) for the tagging protocol
> you support, and -EOPNOTSUPP for everything else. Here -EOPNOTSUPP isn't
> a special code, it is an actual hard error that denies a certain tag
> protocol from attaching.
> 
> The advantage is that your driver-private ops don't have to map 1:1 with
> the dsa_switch_ops, so there is more potential for code reuse than if
> you had to reimplement an entire (*setup) function for example. You can
> have ops for small things like regmap creation, things like that.
> 
> > Anyway the series is ready, I was just pushing it... At the end it's 23
> > patch big... (I know you will hate me but at least it's reviewable)
> 
> Please optimize the patches for a reviewer with average intelligence and
> the attention span of a fish. 23 patches sounds like the series would
> fail on the attention span count.
> 
> > My solution currently was this...
> > 
> > 	ops = devm_kzalloc(&mdiodev->dev, sizeof(*ops), GFP_KERNEL);
> > 	if (!ops)
> > 		return -ENOMEM;
> > 
> > 	/* Copy common ops */
> > 	memcpy(ops, &qca8k_switch_ops, sizeof(*ops));
> > 
> > 	/* Setup specific ops */
> > 	ops->get_tag_protocol = qca8k_get_tag_protocol;
> 
> Answered above.
> 
> > 	ops->setup = qca8k_setup;
> 
> Separate sub-operation, although this is a sub-optimal short-term
> solution that kind of undermines the approach with a single
> dsa_switch_ops in the long run.
> 
> > 	ops->phylink_get_caps = qca8k_phylink_get_caps;
> 
> Not sure what's going to be common and what's going to be different, but
> you can take other drivers as an example, some parts will be common and
> some hidden behind priv->info->mac_port_get_caps().
> 
> static void mt753x_phylink_get_caps(struct dsa_switch *ds, int port,
> 				    struct phylink_config *config)
> {
> 	struct mt7530_priv *priv = ds->priv;
> 
> 	/* This switch only supports full-duplex at 1Gbps */
> 	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> 				   MAC_10 | MAC_100 | MAC_1000FD;
> 
> 	/* This driver does not make use of the speed, duplex, pause or the
> 	 * advertisement in its mac_config, so it is safe to mark this driver
> 	 * as non-legacy.
> 	 */
> 	config->legacy_pre_march2020 = false;
> 
> 	priv->info->mac_port_get_caps(ds, port, config);
> }
> 
> > 	ops->phylink_mac_select_pcs = qca8k_phylink_mac_select_pcs;
> > 	ops->phylink_mac_config = qca8k_phylink_mac_config;
> > 	ops->phylink_mac_link_down = qca8k_phylink_mac_link_down;
> > 	ops->phylink_mac_link_up = qca8k_phylink_mac_link_up;
> 
> Hard to comment for these phylink ops how to organize the switch
> differences in the best way, since I don't actually know what those
> differences are. Again, other drivers may be useful.
> 
> > 	ops->get_phy_flags = qca8k_get_phy_flags;
> > 	ops->master_state_change = qca8k_master_change;
> > 	ops->connect_tag_protocol = qca8k_connect_tag_protocol;
> > 
> > 	/* Assign the final ops */
> > 	priv->ds->ops = ops;
> > 
> > Will wait your response on how to hanle ops that are not supported.
> > (I assume dsa checks if an ops is declared and not if it does return
> > ENOSUPP, so this is my concern your example)
> 
> Maybe it's best to think this conversion through and not rush a patch set.
> I don't want you to blindly follow my advice to have a single dsa_switch_ops,
> then half-ass it. This kind of thing needs to be done with care and
> forethought.

Wonder if a good idea would be leave things as is for now and work of a
single dsa_switch_ops on another series.

With "leave things as is" I mean that function will get migrated to
qca8k-common.c and exposed with the header file.

And the dsa_switch_ops is defined in qca8k specific code.

The warn about the 23 patch was scary so considering this series is
already a bit big and I can squash only a few patch, putting extra logic
to correctly handle each would make this even bigger.

Think the right thing to do is handling the changes for single
dsa_switch_ops to a separate series and at the same time also get some
info on ipq4019 and what can be generalized.

What do you think?

-- 
	Ansuel

Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Vladimir Oltean]

On Tue, Jul 19, 2022 at 02:17:20AM +0200, Christian Marangi wrote:
> Wonder if a good idea would be leave things as is for now and work of a
> single dsa_switch_ops on another series.
> 
> With "leave things as is" I mean that function will get migrated to
> qca8k-common.c and exposed with the header file.
> 
> And the dsa_switch_ops is defined in qca8k specific code.
> 
> The warn about the 23 patch was scary so considering this series is
> already a bit big and I can squash only a few patch, putting extra logic
> to correctly handle each would make this even bigger.
> 
> Think the right thing to do is handling the changes for single
> dsa_switch_ops to a separate series and at the same time also get some
> info on ipq4019 and what can be generalized.
> 
> What do you think?

I don't have a clear mental image right now of how things would look like,
but I suppose you can try and I can review the result. I imagine the
only code added now that you'll need to delete when you later migrate from
switch-specific dsa_switch_ops to common dsa_switch_ops are the function
prototypes from qca8k.h, since the implementations of the dsa_switch_ops
will become static functions at some point in the future.

[net-next RFC PATCH 2/4] net: dsa: qca8k: convert to regmap read/write API

[Christian Marangi]

Convert qca8k to regmap read/write bulk API. The mgmt eth can write up
to 16 bytes of data at times. Currently we use a custom function to do
it but regmap now supports declaration of read/write bulk even without a
bus.

Drop the custom function and rework the regmap function to this new
implementation.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/qca8k.c | 133 ++++++++++++++++++++----------------
 drivers/net/dsa/qca/qca8k.h |   2 +
 2 files changed, 76 insertions(+), 59 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k.c
index 2d34e15c2e6f..fd738d718cd6 100644
--- a/drivers/net/dsa/qca/qca8k.c
+++ b/drivers/net/dsa/qca/qca8k.c
@@ -394,53 +394,12 @@ qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 wri
 }
 
 static int
-qca8k_bulk_read(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
+qca8k_read_mii(struct qca8k_priv *priv, u32 reg, u32 *val)
 {
-	int i, count = len / sizeof(u32), ret;
-
-	if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val, len))
-		return 0;
-
-	for (i = 0; i < count; i++) {
-		ret = regmap_read(priv->regmap, reg + (i * 4), val + i);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
-static int
-qca8k_bulk_write(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
-{
-	int i, count = len / sizeof(u32), ret;
-	u32 tmp;
-
-	if (priv->mgmt_master && !qca8k_write_eth(priv, reg, val, len))
-		return 0;
-
-	for (i = 0; i < count; i++) {
-		tmp = val[i];
-
-		ret = regmap_write(priv->regmap, reg + (i * 4), tmp);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
-static int
-qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	int ret;
 
-	if (!qca8k_read_eth(priv, reg, val, sizeof(*val)))
-		return 0;
-
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -457,16 +416,12 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 }
 
 static int
-qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
+qca8k_write_mii(struct qca8k_priv *priv, u32 reg, u32 val)
 {
-	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	int ret;
 
-	if (!qca8k_write_eth(priv, reg, &val, sizeof(val)))
-		return 0;
-
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -483,17 +438,14 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 }
 
 static int
-qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_val)
+qca8k_regmap_update_bits_mii(struct qca8k_priv *priv, u32 reg,
+			     u32 mask, u32 write_val)
 {
-	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
 	int ret;
 
-	if (!qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
-		return 0;
-
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -516,6 +468,66 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
 	return ret;
 }
 
+static int
+qca8k_bulk_read(void *ctx, const void *reg_buf, size_t reg_len,
+		void *val_buf, size_t val_len)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	int i, count = val_len / sizeof(u32), ret;
+
+	if (priv->mgmt_master && !qca8k_read_eth(priv, *(u32 *)reg_buf, val_buf, val_len))
+		return 0;
+
+	for (i = 0; i < count; i++) {
+		ret = qca8k_read_mii(priv, *(u32 *)reg_buf + (i * sizeof(u32)), val_buf + i);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_bulk_gather_write(void *ctx, const void *reg_buf, size_t reg_len,
+			const void *val_buf, size_t val_len)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	int i, count = val_len / sizeof(u32), ret;
+	u32 tmp;
+
+	if (priv->mgmt_master &&
+	    !qca8k_write_eth(priv, *(u32 *)reg_buf, (u32 *)val_buf, val_len))
+		return 0;
+
+	for (i = 0; i < count; i++) {
+		tmp = *((u32 *)val_buf + i);
+
+		ret = qca8k_write_mii(priv, *(u32 *)reg_buf + (i * sizeof(u32)), tmp);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_bulk_write(void *ctx, const void *data, size_t bytes)
+{
+	return qca8k_bulk_gather_write(ctx, data, sizeof(u32), data + sizeof(u32),
+				       bytes - sizeof(u32));
+}
+
+static int
+qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_val)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+
+	if (!qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
+		return 0;
+
+	return qca8k_regmap_update_bits_mii(priv, reg, mask, write_val);
+}
+
 static const struct regmap_range qca8k_readable_ranges[] = {
 	regmap_reg_range(0x0000, 0x00e4), /* Global control */
 	regmap_reg_range(0x0100, 0x0168), /* EEE control */
@@ -541,16 +553,18 @@ static const struct regmap_access_table qca8k_readable_table = {
 };
 
 static struct regmap_config qca8k_regmap_config = {
-	.reg_bits = 16,
+	.reg_bits = 32,
 	.val_bits = 32,
 	.reg_stride = 4,
 	.max_register = 0x16ac, /* end MIB - Port6 range */
-	.reg_read = qca8k_regmap_read,
-	.reg_write = qca8k_regmap_write,
+	.read = qca8k_bulk_read,
+	.write = qca8k_bulk_write,
 	.reg_update_bits = qca8k_regmap_update_bits,
 	.rd_table = &qca8k_readable_table,
 	.disable_locking = true, /* Locking is handled by qca8k read/write */
 	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
+	.max_raw_read = 16, /* mgmt eth can read/write up to 4 bytes at times */
+	.max_raw_write = 16,
 };
 
 static int
@@ -565,11 +579,12 @@ qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
 static int
 qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
 {
-	u32 reg[3];
+	u32 reg[QCA8K_ATU_TABLE_SIZE];
 	int ret;
 
 	/* load the ARL table into an array */
-	ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg));
+	ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
+			       QCA8K_ATU_TABLE_SIZE);
 	if (ret)
 		return ret;
 
@@ -594,7 +609,7 @@ static void
 qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
 		u8 aging)
 {
-	u32 reg[3] = { 0 };
+	u32 reg[QCA8K_ATU_TABLE_SIZE] = { 0 };
 
 	/* vid - 83:72 */
 	reg[2] = FIELD_PREP(QCA8K_ATU_VID_MASK, vid);
@@ -611,7 +626,7 @@ qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
 	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR5_MASK, mac[5]);
 
 	/* load the array into the ARL table */
-	qca8k_bulk_write(priv, QCA8K_REG_ATU_DATA0, reg, sizeof(reg));
+	regmap_bulk_write(priv->regmap, QCA8K_REG_ATU_DATA0, reg, QCA8K_ATU_TABLE_SIZE);
 }
 
 static int
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index ec58d0e80a70..22ece14e06dc 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -148,6 +148,8 @@
 #define QCA8K_REG_IPV4_PRI_ADDR_MASK			0x474
 
 /* Lookup registers */
+#define QCA8K_ATU_TABLE_SIZE				3 /* 12 bytes wide table / sizeof(u32) */
+
 #define QCA8K_REG_ATU_DATA0				0x600
 #define   QCA8K_ATU_ADDR2_MASK				GENMASK(31, 24)
 #define   QCA8K_ATU_ADDR3_MASK				GENMASK(23, 16)
-- 
2.36.1

[net-next RFC PATCH 3/4] net: dsa: qca8k: rework mib autocast handling

[Christian Marangi]

In preparation for code split, move the autocast mib function used to
receive mib data from eth packet in priv struct and use that in
get_ethtool_stats instead of referencing the function directly. This is
needed as the get_ethtool_stats function will be moved to a common file.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/qca8k.c | 7 +++++--
 drivers/net/dsa/qca/qca8k.h | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k.c
index fd738d718cd6..e527d15d5065 100644
--- a/drivers/net/dsa/qca/qca8k.c
+++ b/drivers/net/dsa/qca/qca8k.c
@@ -2109,8 +2109,8 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 	u32 hi = 0;
 	int ret;
 
-	if (priv->mgmt_master &&
-	    qca8k_get_ethtool_stats_eth(ds, port, data) > 0)
+	if (priv->mgmt_master && priv->autocast_mib &&
+	    priv->autocast_mib(ds, port, data) > 0)
 		return;
 
 	match_data = of_device_get_match_data(priv->dev);
@@ -3177,6 +3177,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	mutex_init(&priv->mib_eth_data.mutex);
 	init_completion(&priv->mib_eth_data.rw_done);
 
+	/* Assign autocast_mib function as it's supported by this switch */
+	priv->autocast_mib = &qca8k_get_ethtool_stats_eth;
+
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
 	priv->ds->priv = priv;
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 22ece14e06dc..a306638a7100 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -403,6 +403,7 @@ struct qca8k_priv {
 	struct qca8k_mdio_cache mdio_cache;
 	struct qca8k_pcs pcs_port_0;
 	struct qca8k_pcs pcs_port_6;
+	int (*autocast_mib)(struct dsa_switch *ds, int port, u64 *data);
 };
 
 struct qca8k_mib_desc {
-- 
2.36.1

Re: [net-next RFC PATCH 3/4] net: dsa: qca8k: rework mib autocast handling

[Vladimir Oltean]

On Sat, Jul 16, 2022 at 07:49:57PM +0200, Christian Marangi wrote:
> In preparation for code split, move the autocast mib function used to
> receive mib data from eth packet in priv struct and use that in
> get_ethtool_stats instead of referencing the function directly. This is
> needed as the get_ethtool_stats function will be moved to a common file.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---

Can this change be deferred until there actually appears a second
implementation of (*autocast_mib)?

> diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> index 22ece14e06dc..a306638a7100 100644
> --- a/drivers/net/dsa/qca/qca8k.h
> +++ b/drivers/net/dsa/qca/qca8k.h
> @@ -403,6 +403,7 @@ struct qca8k_priv {
>  	struct qca8k_mdio_cache mdio_cache;
>  	struct qca8k_pcs pcs_port_0;
>  	struct qca8k_pcs pcs_port_6;
> +	int (*autocast_mib)(struct dsa_switch *ds, int port, u64 *data);

Typically we hold function pointers in separate read-only structures rather
than in the stateful private structure of the driver, see struct sja1105_info,
struct felix_info, struct mv88e6xxx_info and mv88e6xxx_ops, struct b53_io_ops,
etc etc.

>  };
>  
>  struct qca8k_mib_desc {
> -- 
> 2.36.1
> 

Re: [net-next RFC PATCH 3/4] net: dsa: qca8k: rework mib autocast handling

[Christian Marangi]

On Mon, Jul 18, 2022 at 08:27:12PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 16, 2022 at 07:49:57PM +0200, Christian Marangi wrote:
> > In preparation for code split, move the autocast mib function used to
> > receive mib data from eth packet in priv struct and use that in
> > get_ethtool_stats instead of referencing the function directly. This is
> > needed as the get_ethtool_stats function will be moved to a common file.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> 
> Can this change be deferred until there actually appears a second
> implementation of (*autocast_mib)?
>

Mhhh it would be problematic since I would like to move the ethtools
stats function to common code and keep the autocast_mib handler in the
qca8k specific code.

An alternative would be to keep the entire ethtool stats function in
qca8k specific code but it needs to be moved anyway.

This change is required as probably ipq4019 mmio will be faster to
access mib data than using the autocast way.

Tell me how to proceed. Think to skip this we have to leave ethtool
stats function in qca8k specific code and move it later?

> > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> > index 22ece14e06dc..a306638a7100 100644
> > --- a/drivers/net/dsa/qca/qca8k.h
> > +++ b/drivers/net/dsa/qca/qca8k.h
> > @@ -403,6 +403,7 @@ struct qca8k_priv {
> >  	struct qca8k_mdio_cache mdio_cache;
> >  	struct qca8k_pcs pcs_port_0;
> >  	struct qca8k_pcs pcs_port_6;
> > +	int (*autocast_mib)(struct dsa_switch *ds, int port, u64 *data);
> 
> Typically we hold function pointers in separate read-only structures rather
> than in the stateful private structure of the driver, see struct sja1105_info,
> struct felix_info, struct mv88e6xxx_info and mv88e6xxx_ops, struct b53_io_ops,
> etc etc.
> 

Oh ok it's just match data. We should already have something like that
in qca8k but I wasn't aware of the _info suffix. If we decide to keep
this, can i allign the match struct we use in qca8k to the new pattern
and add the function pointer there?

> >  };
> >  
> >  struct qca8k_mib_desc {
> > -- 
> > 2.36.1
> > 
> 

-- 
	Ansuel

Re: [net-next RFC PATCH 3/4] net: dsa: qca8k: rework mib autocast handling

[Vladimir Oltean]

On Mon, Jul 18, 2022 at 07:20:07PM +0200, Christian Marangi wrote:
> On Mon, Jul 18, 2022 at 08:27:12PM +0300, Vladimir Oltean wrote:
> > On Sat, Jul 16, 2022 at 07:49:57PM +0200, Christian Marangi wrote:
> > > In preparation for code split, move the autocast mib function used to
> > > receive mib data from eth packet in priv struct and use that in
> > > get_ethtool_stats instead of referencing the function directly. This is
> > > needed as the get_ethtool_stats function will be moved to a common file.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > 
> > Can this change be deferred until there actually appears a second
> > implementation of (*autocast_mib)?
> >
> 
> Mhhh it would be problematic since I would like to move the ethtools
> stats function to common code and keep the autocast_mib handler in the
> qca8k specific code.
> 
> An alternative would be to keep the entire ethtool stats function in
> qca8k specific code but it needs to be moved anyway.
> 
> This change is required as probably ipq4019 mmio will be faster to
> access mib data than using the autocast way.
> 
> Tell me how to proceed. Think to skip this we have to leave ethtool
> stats function in qca8k specific code and move it later?

Sorry, I think I initially misread the patch. So ipq4019 is not going to
have an implementation of (*autocast_mib) at all? In that case I don't
have an objection to make it a function pointer now, but you need to
state exactly that in the commit message: make MIB autocast optional in
qca8k_get_ethtool_stats(), because we'll need to support an MMIO-based
switch in the future where we won't need to implement this function.

> > > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> > > index 22ece14e06dc..a306638a7100 100644
> > > --- a/drivers/net/dsa/qca/qca8k.h
> > > +++ b/drivers/net/dsa/qca/qca8k.h
> > > @@ -403,6 +403,7 @@ struct qca8k_priv {
> > >  	struct qca8k_mdio_cache mdio_cache;
> > >  	struct qca8k_pcs pcs_port_0;
> > >  	struct qca8k_pcs pcs_port_6;
> > > +	int (*autocast_mib)(struct dsa_switch *ds, int port, u64 *data);
> > 
> > Typically we hold function pointers in separate read-only structures rather
> > than in the stateful private structure of the driver, see struct sja1105_info,
> > struct felix_info, struct mv88e6xxx_info and mv88e6xxx_ops, struct b53_io_ops,
> > etc etc.
> > 
> 
> Oh ok it's just match data. We should already have something like that
> in qca8k but I wasn't aware of the _info suffix. If we decide to keep
> this, can i allign the match struct we use in qca8k to the new pattern
> and add the function pointer there?

I think struct qca8k_match_data could serve that purpose, and have a
sub-structure called qca8k_ops for function pointers, yes.

[net-next RFC PATCH 4/4] net: dsa: qca8k: split qca8k in common and 8xxx specific code

[Christian Marangi]

The qca8k family reg structure is also used in the internal ipq40xx
switch. Split qca8k common code from specific code for future
implementation of ipq40xx internal switch based on qca8k.

While at it also fix minor wrong format for comments and reallign
function as we had to drop static declaration.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/Makefile                  |    1 +
 drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} | 1210 +----------------
 drivers/net/dsa/qca/qca8k-common.c            | 1174 ++++++++++++++++
 drivers/net/dsa/qca/qca8k.h                   |   58 +
 4 files changed, 1245 insertions(+), 1198 deletions(-)
 rename drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} (64%)
 create mode 100644 drivers/net/dsa/qca/qca8k-common.c

diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
index 40bb7c27285b..701f1d199e93 100644
--- a/drivers/net/dsa/qca/Makefile
+++ b/drivers/net/dsa/qca/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_NET_DSA_AR9331)	+= ar9331.o
 obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
+qca8k-y 			+= qca8k-common.o qca8k-8xxx.o
diff --git a/drivers/net/dsa/qca/qca8k.c b/drivers/net/dsa/qca/qca8k-8xxx.c
similarity index 64%
rename from drivers/net/dsa/qca/qca8k.c
rename to drivers/net/dsa/qca/qca8k-8xxx.c
index e527d15d5065..85d138fced66 100644
--- a/drivers/net/dsa/qca/qca8k.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -24,57 +24,6 @@
 
 #include "qca8k.h"
 
-#define MIB_DESC(_s, _o, _n)	\
-	{			\
-		.size = (_s),	\
-		.offset = (_o),	\
-		.name = (_n),	\
-	}
-
-static const struct qca8k_mib_desc ar8327_mib[] = {
-	MIB_DESC(1, 0x00, "RxBroad"),
-	MIB_DESC(1, 0x04, "RxPause"),
-	MIB_DESC(1, 0x08, "RxMulti"),
-	MIB_DESC(1, 0x0c, "RxFcsErr"),
-	MIB_DESC(1, 0x10, "RxAlignErr"),
-	MIB_DESC(1, 0x14, "RxRunt"),
-	MIB_DESC(1, 0x18, "RxFragment"),
-	MIB_DESC(1, 0x1c, "Rx64Byte"),
-	MIB_DESC(1, 0x20, "Rx128Byte"),
-	MIB_DESC(1, 0x24, "Rx256Byte"),
-	MIB_DESC(1, 0x28, "Rx512Byte"),
-	MIB_DESC(1, 0x2c, "Rx1024Byte"),
-	MIB_DESC(1, 0x30, "Rx1518Byte"),
-	MIB_DESC(1, 0x34, "RxMaxByte"),
-	MIB_DESC(1, 0x38, "RxTooLong"),
-	MIB_DESC(2, 0x3c, "RxGoodByte"),
-	MIB_DESC(2, 0x44, "RxBadByte"),
-	MIB_DESC(1, 0x4c, "RxOverFlow"),
-	MIB_DESC(1, 0x50, "Filtered"),
-	MIB_DESC(1, 0x54, "TxBroad"),
-	MIB_DESC(1, 0x58, "TxPause"),
-	MIB_DESC(1, 0x5c, "TxMulti"),
-	MIB_DESC(1, 0x60, "TxUnderRun"),
-	MIB_DESC(1, 0x64, "Tx64Byte"),
-	MIB_DESC(1, 0x68, "Tx128Byte"),
-	MIB_DESC(1, 0x6c, "Tx256Byte"),
-	MIB_DESC(1, 0x70, "Tx512Byte"),
-	MIB_DESC(1, 0x74, "Tx1024Byte"),
-	MIB_DESC(1, 0x78, "Tx1518Byte"),
-	MIB_DESC(1, 0x7c, "TxMaxByte"),
-	MIB_DESC(1, 0x80, "TxOverSize"),
-	MIB_DESC(2, 0x84, "TxByte"),
-	MIB_DESC(1, 0x8c, "TxCollision"),
-	MIB_DESC(1, 0x90, "TxAbortCol"),
-	MIB_DESC(1, 0x94, "TxMultiCol"),
-	MIB_DESC(1, 0x98, "TxSingleCol"),
-	MIB_DESC(1, 0x9c, "TxExcDefer"),
-	MIB_DESC(1, 0xa0, "TxDefer"),
-	MIB_DESC(1, 0xa4, "TxLateCol"),
-	MIB_DESC(1, 0xa8, "RXUnicast"),
-	MIB_DESC(1, 0xac, "TXUnicast"),
-};
-
 static void
 qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
 {
@@ -528,30 +477,6 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
 	return qca8k_regmap_update_bits_mii(priv, reg, mask, write_val);
 }
 
-static const struct regmap_range qca8k_readable_ranges[] = {
-	regmap_reg_range(0x0000, 0x00e4), /* Global control */
-	regmap_reg_range(0x0100, 0x0168), /* EEE control */
-	regmap_reg_range(0x0200, 0x0270), /* Parser control */
-	regmap_reg_range(0x0400, 0x0454), /* ACL */
-	regmap_reg_range(0x0600, 0x0718), /* Lookup */
-	regmap_reg_range(0x0800, 0x0b70), /* QM */
-	regmap_reg_range(0x0c00, 0x0c80), /* PKT */
-	regmap_reg_range(0x0e00, 0x0e98), /* L3 */
-	regmap_reg_range(0x1000, 0x10ac), /* MIB - Port0 */
-	regmap_reg_range(0x1100, 0x11ac), /* MIB - Port1 */
-	regmap_reg_range(0x1200, 0x12ac), /* MIB - Port2 */
-	regmap_reg_range(0x1300, 0x13ac), /* MIB - Port3 */
-	regmap_reg_range(0x1400, 0x14ac), /* MIB - Port4 */
-	regmap_reg_range(0x1500, 0x15ac), /* MIB - Port5 */
-	regmap_reg_range(0x1600, 0x16ac), /* MIB - Port6 */
-
-};
-
-static const struct regmap_access_table qca8k_readable_table = {
-	.yes_ranges = qca8k_readable_ranges,
-	.n_yes_ranges = ARRAY_SIZE(qca8k_readable_ranges),
-};
-
 static struct regmap_config qca8k_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
@@ -567,386 +492,6 @@ static struct regmap_config qca8k_regmap_config = {
 	.max_raw_write = 16,
 };
 
-static int
-qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
-{
-	u32 val;
-
-	return regmap_read_poll_timeout(priv->regmap, reg, val, !(val & mask), 0,
-				       QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC);
-}
-
-static int
-qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
-{
-	u32 reg[QCA8K_ATU_TABLE_SIZE];
-	int ret;
-
-	/* load the ARL table into an array */
-	ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
-			       QCA8K_ATU_TABLE_SIZE);
-	if (ret)
-		return ret;
-
-	/* vid - 83:72 */
-	fdb->vid = FIELD_GET(QCA8K_ATU_VID_MASK, reg[2]);
-	/* aging - 67:64 */
-	fdb->aging = FIELD_GET(QCA8K_ATU_STATUS_MASK, reg[2]);
-	/* portmask - 54:48 */
-	fdb->port_mask = FIELD_GET(QCA8K_ATU_PORT_MASK, reg[1]);
-	/* mac - 47:0 */
-	fdb->mac[0] = FIELD_GET(QCA8K_ATU_ADDR0_MASK, reg[1]);
-	fdb->mac[1] = FIELD_GET(QCA8K_ATU_ADDR1_MASK, reg[1]);
-	fdb->mac[2] = FIELD_GET(QCA8K_ATU_ADDR2_MASK, reg[0]);
-	fdb->mac[3] = FIELD_GET(QCA8K_ATU_ADDR3_MASK, reg[0]);
-	fdb->mac[4] = FIELD_GET(QCA8K_ATU_ADDR4_MASK, reg[0]);
-	fdb->mac[5] = FIELD_GET(QCA8K_ATU_ADDR5_MASK, reg[0]);
-
-	return 0;
-}
-
-static void
-qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
-		u8 aging)
-{
-	u32 reg[QCA8K_ATU_TABLE_SIZE] = { 0 };
-
-	/* vid - 83:72 */
-	reg[2] = FIELD_PREP(QCA8K_ATU_VID_MASK, vid);
-	/* aging - 67:64 */
-	reg[2] |= FIELD_PREP(QCA8K_ATU_STATUS_MASK, aging);
-	/* portmask - 54:48 */
-	reg[1] = FIELD_PREP(QCA8K_ATU_PORT_MASK, port_mask);
-	/* mac - 47:0 */
-	reg[1] |= FIELD_PREP(QCA8K_ATU_ADDR0_MASK, mac[0]);
-	reg[1] |= FIELD_PREP(QCA8K_ATU_ADDR1_MASK, mac[1]);
-	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR2_MASK, mac[2]);
-	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR3_MASK, mac[3]);
-	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR4_MASK, mac[4]);
-	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR5_MASK, mac[5]);
-
-	/* load the array into the ARL table */
-	regmap_bulk_write(priv->regmap, QCA8K_REG_ATU_DATA0, reg, QCA8K_ATU_TABLE_SIZE);
-}
-
-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;
-	reg |= cmd;
-	if (port >= 0) {
-		reg |= QCA8K_ATU_FUNC_PORT_EN;
-		reg |= FIELD_PREP(QCA8K_ATU_FUNC_PORT_MASK, port);
-	}
-
-	/* Write the function register triggering the table access */
-	ret = regmap_write(priv->regmap, QCA8K_REG_ATU_FUNC, reg);
-	if (ret)
-		return ret;
-
-	/* wait for completion */
-	ret = qca8k_busy_wait(priv, QCA8K_REG_ATU_FUNC, QCA8K_ATU_FUNC_BUSY);
-	if (ret)
-		return ret;
-
-	/* Check for table full violation when adding an entry */
-	if (cmd == QCA8K_FDB_LOAD) {
-		ret = regmap_read(priv->regmap, QCA8K_REG_ATU_FUNC, &reg);
-		if (ret < 0)
-			return ret;
-		if (reg & QCA8K_ATU_FUNC_FULL)
-			return -1;
-	}
-
-	return 0;
-}
-
-static int
-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);
-}
-
-static int
-qca8k_fdb_add(struct qca8k_priv *priv, const u8 *mac, u16 port_mask,
-	      u16 vid, u8 aging)
-{
-	int ret;
-
-	mutex_lock(&priv->reg_mutex);
-	qca8k_fdb_write(priv, vid, port_mask, mac, aging);
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
-	mutex_unlock(&priv->reg_mutex);
-
-	return ret;
-}
-
-static int
-qca8k_fdb_del(struct qca8k_priv *priv, const u8 *mac, u16 port_mask, u16 vid)
-{
-	int ret;
-
-	mutex_lock(&priv->reg_mutex);
-	qca8k_fdb_write(priv, vid, port_mask, mac, 0);
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
-	mutex_unlock(&priv->reg_mutex);
-
-	return ret;
-}
-
-static void
-qca8k_fdb_flush(struct qca8k_priv *priv)
-{
-	mutex_lock(&priv->reg_mutex);
-	qca8k_fdb_access(priv, QCA8K_FDB_FLUSH, -1);
-	mutex_unlock(&priv->reg_mutex);
-}
-
-static int
-qca8k_fdb_search_and_insert(struct qca8k_priv *priv, u8 port_mask,
-			    const u8 *mac, u16 vid)
-{
-	struct qca8k_fdb fdb = { 0 };
-	int ret;
-
-	mutex_lock(&priv->reg_mutex);
-
-	qca8k_fdb_write(priv, vid, 0, mac, 0);
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_SEARCH, -1);
-	if (ret < 0)
-		goto exit;
-
-	ret = qca8k_fdb_read(priv, &fdb);
-	if (ret < 0)
-		goto exit;
-
-	/* Rule exist. Delete first */
-	if (!fdb.aging) {
-		ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
-		if (ret)
-			goto exit;
-	}
-
-	/* Add port to fdb portmask */
-	fdb.port_mask |= port_mask;
-
-	qca8k_fdb_write(priv, vid, fdb.port_mask, mac, fdb.aging);
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
-
-exit:
-	mutex_unlock(&priv->reg_mutex);
-	return ret;
-}
-
-static int
-qca8k_fdb_search_and_del(struct qca8k_priv *priv, u8 port_mask,
-			 const u8 *mac, u16 vid)
-{
-	struct qca8k_fdb fdb = { 0 };
-	int ret;
-
-	mutex_lock(&priv->reg_mutex);
-
-	qca8k_fdb_write(priv, vid, 0, mac, 0);
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_SEARCH, -1);
-	if (ret < 0)
-		goto exit;
-
-	/* Rule doesn't exist. Why delete? */
-	if (!fdb.aging) {
-		ret = -EINVAL;
-		goto exit;
-	}
-
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
-	if (ret)
-		goto exit;
-
-	/* Only port in the rule is this port. Don't re insert */
-	if (fdb.port_mask == port_mask)
-		goto exit;
-
-	/* Remove port from port mask */
-	fdb.port_mask &= ~port_mask;
-
-	qca8k_fdb_write(priv, vid, fdb.port_mask, mac, fdb.aging);
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
-
-exit:
-	mutex_unlock(&priv->reg_mutex);
-	return ret;
-}
-
-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;
-	reg |= cmd;
-	reg |= FIELD_PREP(QCA8K_VTU_FUNC1_VID_MASK, vid);
-
-	/* Write the function register triggering the table access */
-	ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC1, reg);
-	if (ret)
-		return ret;
-
-	/* wait for completion */
-	ret = qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY);
-	if (ret)
-		return ret;
-
-	/* Check for table full violation when adding an entry */
-	if (cmd == QCA8K_VLAN_LOAD) {
-		ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC1, &reg);
-		if (ret < 0)
-			return ret;
-		if (reg & QCA8K_VTU_FUNC1_FULL)
-			return -ENOMEM;
-	}
-
-	return 0;
-}
-
-static int
-qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
-{
-	u32 reg;
-	int ret;
-
-	/*
-	   We do the right thing with VLAN 0 and treat it as untagged while
-	   preserving the tag on egress.
-	 */
-	if (vid == 0)
-		return 0;
-
-	mutex_lock(&priv->reg_mutex);
-	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
-	if (ret < 0)
-		goto out;
-
-	ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC0, &reg);
-	if (ret < 0)
-		goto out;
-	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
-	reg &= ~QCA8K_VTU_FUNC0_EG_MODE_PORT_MASK(port);
-	if (untagged)
-		reg |= QCA8K_VTU_FUNC0_EG_MODE_PORT_UNTAG(port);
-	else
-		reg |= QCA8K_VTU_FUNC0_EG_MODE_PORT_TAG(port);
-
-	ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC0, reg);
-	if (ret)
-		goto out;
-	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
-
-out:
-	mutex_unlock(&priv->reg_mutex);
-
-	return ret;
-}
-
-static int
-qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
-{
-	u32 reg, mask;
-	int ret, i;
-	bool del;
-
-	mutex_lock(&priv->reg_mutex);
-	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
-	if (ret < 0)
-		goto out;
-
-	ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC0, &reg);
-	if (ret < 0)
-		goto out;
-	reg &= ~QCA8K_VTU_FUNC0_EG_MODE_PORT_MASK(port);
-	reg |= QCA8K_VTU_FUNC0_EG_MODE_PORT_NOT(port);
-
-	/* Check if we're the last member to be removed */
-	del = true;
-	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
-		mask = QCA8K_VTU_FUNC0_EG_MODE_PORT_NOT(i);
-
-		if ((reg & mask) != mask) {
-			del = false;
-			break;
-		}
-	}
-
-	if (del) {
-		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
-	} else {
-		ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC0, reg);
-		if (ret)
-			goto out;
-		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
-	}
-
-out:
-	mutex_unlock(&priv->reg_mutex);
-
-	return ret;
-}
-
-static int
-qca8k_mib_init(struct qca8k_priv *priv)
-{
-	int ret;
-
-	mutex_lock(&priv->reg_mutex);
-	ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
-				 QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
-				 FIELD_PREP(QCA8K_MIB_FUNC, 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;
-
-	ret = regmap_set_bits(priv->regmap, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
-	if (ret)
-		goto exit;
-
-	ret = regmap_write(priv->regmap, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
-
-exit:
-	mutex_unlock(&priv->reg_mutex);
-	return ret;
-}
-
-static void
-qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
-{
-	u32 mask = QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
-
-	/* Port 0 and 6 have no internal PHY */
-	if (port > 0 && port < 6)
-		mask |= QCA8K_PORT_STATUS_LINK_AUTO;
-
-	if (enable)
-		regmap_set_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
-	else
-		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
-}
-
 static int
 qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 			struct sk_buff *read_skb, u32 *val)
@@ -1990,23 +1535,6 @@ static void qca8k_setup_pcs(struct qca8k_priv *priv, struct qca8k_pcs *qpcs,
 	qpcs->port = port;
 }
 
-static void
-qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data)
-{
-	const struct qca8k_match_data *match_data;
-	struct qca8k_priv *priv = ds->priv;
-	int i;
-
-	if (stringset != ETH_SS_STATS)
-		return;
-
-	match_data = of_device_get_match_data(priv->dev);
-
-	for (i = 0; i < match_data->mib_count; i++)
-		strncpy(data + i * ETH_GSTRING_LEN, ar8327_mib[i].name,
-			ETH_GSTRING_LEN);
-}
-
 static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *skb)
 {
 	const struct qca8k_match_data *match_data;
@@ -2098,537 +1626,21 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 	return ret;
 }
 
-static void
-qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
-			uint64_t *data)
+static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)
 {
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	const struct qca8k_match_data *match_data;
-	const struct qca8k_mib_desc *mib;
-	u32 reg, i, val;
-	u32 hi = 0;
-	int ret;
+	struct qca8k_priv *priv = ds->priv;
 
-	if (priv->mgmt_master && priv->autocast_mib &&
-	    priv->autocast_mib(ds, port, data) > 0)
-		return;
+	/* 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;
 
-	match_data = of_device_get_match_data(priv->dev);
-
-	for (i = 0; i < match_data->mib_count; i++) {
-		mib = &ar8327_mib[i];
-		reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
-
-		ret = regmap_read(priv->regmap, reg, &val);
-		if (ret < 0)
-			continue;
-
-		if (mib->size == 2) {
-			ret = regmap_read(priv->regmap, reg + 4, &hi);
-			if (ret < 0)
-				continue;
-		}
-
-		data[i] = val;
-		if (mib->size == 2)
-			data[i] |= (u64)hi << 32;
-	}
-}
-
-static int
-qca8k_get_sset_count(struct dsa_switch *ds, int port, int sset)
-{
-	const struct qca8k_match_data *match_data;
-	struct qca8k_priv *priv = ds->priv;
-
-	if (sset != ETH_SS_STATS)
-		return 0;
-
-	match_data = of_device_get_match_data(priv->dev);
-
-	return match_data->mib_count;
-}
-
-static int
-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);
-	u32 reg;
-	int ret;
-
-	mutex_lock(&priv->reg_mutex);
-	ret = regmap_read(priv->regmap, QCA8K_REG_EEE_CTRL, &reg);
-	if (ret < 0)
-		goto exit;
-
-	if (eee->eee_enabled)
-		reg |= lpi_en;
-	else
-		reg &= ~lpi_en;
-	ret = regmap_write(priv->regmap, QCA8K_REG_EEE_CTRL, reg);
-
-exit:
-	mutex_unlock(&priv->reg_mutex);
-	return ret;
-}
-
-static int
-qca8k_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
-{
-	/* Nothing to do on the port's MAC */
-	return 0;
-}
-
-static void
-qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	u32 stp_state;
-
-	switch (state) {
-	case BR_STATE_DISABLED:
-		stp_state = QCA8K_PORT_LOOKUP_STATE_DISABLED;
-		break;
-	case BR_STATE_BLOCKING:
-		stp_state = QCA8K_PORT_LOOKUP_STATE_BLOCKING;
-		break;
-	case BR_STATE_LISTENING:
-		stp_state = QCA8K_PORT_LOOKUP_STATE_LISTENING;
-		break;
-	case BR_STATE_LEARNING:
-		stp_state = QCA8K_PORT_LOOKUP_STATE_LEARNING;
-		break;
-	case BR_STATE_FORWARDING:
-	default:
-		stp_state = QCA8K_PORT_LOOKUP_STATE_FORWARD;
-		break;
-	}
-
-	regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
-			   QCA8K_PORT_LOOKUP_STATE_MASK, stp_state);
-}
-
-static int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
-				  struct dsa_bridge bridge,
-				  bool *tx_fwd_offload,
-				  struct netlink_ext_ack *extack)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	int port_mask, cpu_port;
-	int i, ret;
-
-	cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
-	port_mask = BIT(cpu_port);
-
-	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
-		if (dsa_is_cpu_port(ds, i))
-			continue;
-		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
-			continue;
-		/* Add this port to the portvlan mask of the other ports
-		 * in the bridge
-		 */
-		ret = regmap_set_bits(priv->regmap,
-				      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 */
-	ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
-				 QCA8K_PORT_LOOKUP_MEMBER, port_mask);
-
-	return ret;
-}
-
-static void qca8k_port_bridge_leave(struct dsa_switch *ds, int port,
-				    struct dsa_bridge bridge)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	int cpu_port, i;
-
-	cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
-
-	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
-		if (dsa_is_cpu_port(ds, i))
-			continue;
-		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
-			continue;
-		/* Remove this port to the portvlan mask of the other ports
-		 * in the bridge
-		 */
-		regmap_clear_bits(priv->regmap,
-				  QCA8K_PORT_LOOKUP_CTRL(i),
-				  BIT(port));
-	}
-
-	/* Set the cpu port to be the only one in the portvlan mask of
-	 * this port
-	 */
-	regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
-			   QCA8K_PORT_LOOKUP_MEMBER, BIT(cpu_port));
-}
-
-static void
-qca8k_port_fast_age(struct dsa_switch *ds, int port)
-{
-	struct qca8k_priv *priv = ds->priv;
-
-	mutex_lock(&priv->reg_mutex);
-	qca8k_fdb_access(priv, QCA8K_FDB_FLUSH_PORT, port);
-	mutex_unlock(&priv->reg_mutex);
-}
-
-static int
-qca8k_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
-{
-	struct qca8k_priv *priv = ds->priv;
-	unsigned int secs = msecs / 1000;
-	u32 val;
-
-	/* AGE_TIME reg is set in 7s step */
-	val = secs / 7;
-
-	/* Handle case with 0 as val to NOT disable
-	 * learning
-	 */
-	if (!val)
-		val = 1;
-
-	return regmap_update_bits(priv->regmap, QCA8K_REG_ATU_CTRL, QCA8K_ATU_AGE_TIME_MASK,
-				  QCA8K_ATU_AGE_TIME(val));
-}
-
-static int
-qca8k_port_enable(struct dsa_switch *ds, int port,
-		  struct phy_device *phy)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-
-	qca8k_port_set_status(priv, port, 1);
-	priv->port_enabled_map |= BIT(port);
-
-	if (dsa_is_user_port(ds, port))
-		phy_support_asym_pause(phy);
-
-	return 0;
-}
-
-static void
-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_enabled_map &= ~BIT(port);
-}
-
-static int
-qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
-{
-	struct qca8k_priv *priv = ds->priv;
-	int ret;
-
-	/* We have only have a general MTU setting.
-	 * DSA always set the CPU port's MTU to the largest MTU of the slave
-	 * 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;
-
-	/* To change the MAX_FRAME_SIZE the cpu ports must be off or
-	 * the switch panics.
-	 * Turn off both cpu ports before applying the new value to prevent
-	 * this.
-	 */
-	if (priv->port_enabled_map & BIT(0))
-		qca8k_port_set_status(priv, 0, 0);
-
-	if (priv->port_enabled_map & BIT(6))
-		qca8k_port_set_status(priv, 6, 0);
-
-	/* Include L2 header / FCS length */
-	ret = regmap_write(priv->regmap, QCA8K_MAX_FRAME_SIZE, new_mtu + ETH_HLEN + ETH_FCS_LEN);
-
-	if (priv->port_enabled_map & BIT(0))
-		qca8k_port_set_status(priv, 0, 1);
-
-	if (priv->port_enabled_map & BIT(6))
-		qca8k_port_set_status(priv, 6, 1);
-
-	return ret;
-}
-
-static int
-qca8k_port_max_mtu(struct dsa_switch *ds, int port)
-{
-	return QCA8K_MAX_MTU;
-}
-
-static int
-qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
-		      u16 port_mask, u16 vid)
-{
-	/* Set the vid to the port vlan id if no vid is set */
-	if (!vid)
-		vid = QCA8K_PORT_VID_DEF;
-
-	return qca8k_fdb_add(priv, addr, port_mask, vid,
-			     QCA8K_ATU_STATUS_STATIC);
-}
-
-static int
-qca8k_port_fdb_add(struct dsa_switch *ds, int port,
-		   const unsigned char *addr, u16 vid,
-		   struct dsa_db db)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	u16 port_mask = BIT(port);
-
-	return qca8k_port_fdb_insert(priv, addr, port_mask, vid);
-}
-
-static int
-qca8k_port_fdb_del(struct dsa_switch *ds, int port,
-		   const unsigned char *addr, u16 vid,
-		   struct dsa_db db)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	u16 port_mask = BIT(port);
-
-	if (!vid)
-		vid = QCA8K_PORT_VID_DEF;
-
-	return qca8k_fdb_del(priv, addr, port_mask, vid);
-}
-
-static int
-qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
-		    dsa_fdb_dump_cb_t *cb, void *data)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	struct qca8k_fdb _fdb = { 0 };
-	int cnt = QCA8K_NUM_FDB_RECORDS;
-	bool is_static;
-	int ret = 0;
-
-	mutex_lock(&priv->reg_mutex);
-	while (cnt-- && !qca8k_fdb_next(priv, &_fdb, port)) {
-		if (!_fdb.aging)
-			break;
-		is_static = (_fdb.aging == QCA8K_ATU_STATUS_STATIC);
-		ret = cb(_fdb.mac, _fdb.vid, is_static, data);
-		if (ret)
-			break;
-	}
-	mutex_unlock(&priv->reg_mutex);
-
-	return 0;
-}
-
-static int
-qca8k_port_mdb_add(struct dsa_switch *ds, int port,
-		   const struct switchdev_obj_port_mdb *mdb,
-		   struct dsa_db db)
-{
-	struct qca8k_priv *priv = ds->priv;
-	const u8 *addr = mdb->addr;
-	u16 vid = mdb->vid;
-
-	return qca8k_fdb_search_and_insert(priv, BIT(port), addr, vid);
-}
-
-static int
-qca8k_port_mdb_del(struct dsa_switch *ds, int port,
-		   const struct switchdev_obj_port_mdb *mdb,
-		   struct dsa_db db)
-{
-	struct qca8k_priv *priv = ds->priv;
-	const u8 *addr = mdb->addr;
-	u16 vid = mdb->vid;
-
-	return qca8k_fdb_search_and_del(priv, BIT(port), addr, vid);
-}
-
-static int
-qca8k_port_mirror_add(struct dsa_switch *ds, int port,
-		      struct dsa_mall_mirror_tc_entry *mirror,
-		      bool ingress, struct netlink_ext_ack *extack)
-{
-	struct qca8k_priv *priv = ds->priv;
-	int monitor_port, ret;
-	u32 reg, val;
-
-	/* Check for existent entry */
-	if ((ingress ? priv->mirror_rx : priv->mirror_tx) & BIT(port))
-		return -EEXIST;
-
-	ret = regmap_read(priv->regmap, QCA8K_REG_GLOBAL_FW_CTRL0, &val);
-	if (ret)
-		return ret;
-
-	/* QCA83xx can have only one port set to mirror mode.
-	 * Check that the correct port is requested and return error otherwise.
-	 * When no mirror port is set, the values is set to 0xF
-	 */
-	monitor_port = FIELD_GET(QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM, val);
-	if (monitor_port != 0xF && monitor_port != mirror->to_local_port)
-		return -EEXIST;
-
-	/* Set the monitor port */
-	val = FIELD_PREP(QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM,
-			 mirror->to_local_port);
-	ret = regmap_update_bits(priv->regmap, QCA8K_REG_GLOBAL_FW_CTRL0,
-				 QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM, val);
-	if (ret)
-		return ret;
-
-	if (ingress) {
-		reg = QCA8K_PORT_LOOKUP_CTRL(port);
-		val = QCA8K_PORT_LOOKUP_ING_MIRROR_EN;
-	} else {
-		reg = QCA8K_REG_PORT_HOL_CTRL1(port);
-		val = QCA8K_PORT_HOL_CTRL1_EG_MIRROR_EN;
-	}
-
-	ret = regmap_update_bits(priv->regmap, reg, val, val);
-	if (ret)
-		return ret;
-
-	/* Track mirror port for tx and rx to decide when the
-	 * mirror port has to be disabled.
-	 */
-	if (ingress)
-		priv->mirror_rx |= BIT(port);
-	else
-		priv->mirror_tx |= BIT(port);
-
-	return 0;
-}
-
-static void
-qca8k_port_mirror_del(struct dsa_switch *ds, int port,
-		      struct dsa_mall_mirror_tc_entry *mirror)
-{
-	struct qca8k_priv *priv = ds->priv;
-	u32 reg, val;
-	int ret;
-
-	if (mirror->ingress) {
-		reg = QCA8K_PORT_LOOKUP_CTRL(port);
-		val = QCA8K_PORT_LOOKUP_ING_MIRROR_EN;
-	} else {
-		reg = QCA8K_REG_PORT_HOL_CTRL1(port);
-		val = QCA8K_PORT_HOL_CTRL1_EG_MIRROR_EN;
-	}
-
-	ret = regmap_clear_bits(priv->regmap, reg, val);
-	if (ret)
-		goto err;
-
-	if (mirror->ingress)
-		priv->mirror_rx &= ~BIT(port);
-	else
-		priv->mirror_tx &= ~BIT(port);
-
-	/* No port set to send packet to mirror port. Disable mirror port */
-	if (!priv->mirror_rx && !priv->mirror_tx) {
-		val = FIELD_PREP(QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM, 0xF);
-		ret = regmap_update_bits(priv->regmap, QCA8K_REG_GLOBAL_FW_CTRL0,
-					 QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM, val);
-		if (ret)
-			goto err;
-	}
-err:
-	dev_err(priv->dev, "Failed to del mirror port from %d", port);
-}
-
-static int
-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) {
-		ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
-					 QCA8K_PORT_LOOKUP_VLAN_MODE_MASK,
-					 QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
-	} else {
-		ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
-					 QCA8K_PORT_LOOKUP_VLAN_MODE_MASK,
-					 QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
-	}
-
-	return ret;
-}
-
-static int
-qca8k_port_vlan_add(struct dsa_switch *ds, int port,
-		    const struct switchdev_obj_port_vlan *vlan,
-		    struct netlink_ext_ack *extack)
-{
-	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
-	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
-	struct qca8k_priv *priv = ds->priv;
-	int ret;
-
-	ret = qca8k_vlan_add(priv, port, vlan->vid, untagged);
-	if (ret) {
-		dev_err(priv->dev, "Failed to add VLAN to port %d (%d)", port, ret);
-		return ret;
-	}
-
-	if (pvid) {
-		ret = regmap_update_bits(priv->regmap, QCA8K_EGRESS_VLAN(port),
-					 QCA8K_EGREES_VLAN_PORT_MASK(port),
-					 QCA8K_EGREES_VLAN_PORT(port, vlan->vid));
-		if (ret)
-			return ret;
-
-		ret = regmap_write(priv->regmap, QCA8K_REG_PORT_VLAN_CTRL0(port),
-				   QCA8K_PORT_VLAN_CVID(vlan->vid) |
-				   QCA8K_PORT_VLAN_SVID(vlan->vid));
-	}
-
-	return ret;
-}
-
-static int
-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;
-
-	ret = qca8k_vlan_del(priv, port, vlan->vid);
-	if (ret)
-		dev_err(priv->dev, "Failed to delete VLAN from port %d (%d)", port, ret);
-
-	return ret;
-}
-
-static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)
-{
-	struct qca8k_priv *priv = ds->priv;
-
-	/* Communicate to the phy internal driver the switch revision.
-	 * Based on the switch revision different values needs to be
-	 * set to the dbg and mmd reg on the phy.
-	 * The first 2 bit are used to communicate the switch revision
-	 * to the phy driver.
-	 */
-	if (port > 0 && port < 6)
-		return priv->switch_revision;
-
-	return 0;
-}
+	return 0;
+}
 
 static enum dsa_tag_protocol
 qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
@@ -2637,174 +1649,6 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 	return DSA_TAG_PROTO_QCA;
 }
 
-static bool
-qca8k_lag_can_offload(struct dsa_switch *ds, struct dsa_lag lag,
-		      struct netdev_lag_upper_info *info)
-{
-	struct dsa_port *dp;
-	int members = 0;
-
-	if (!lag.id)
-		return false;
-
-	dsa_lag_foreach_port(dp, ds->dst, &lag)
-		/* Includes the port joining the LAG */
-		members++;
-
-	if (members > QCA8K_NUM_PORTS_FOR_LAG)
-		return false;
-
-	if (info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
-		return false;
-
-	if (info->hash_type != NETDEV_LAG_HASH_L2 &&
-	    info->hash_type != NETDEV_LAG_HASH_L23)
-		return false;
-
-	return true;
-}
-
-static int
-qca8k_lag_setup_hash(struct dsa_switch *ds, struct dsa_lag lag,
-		     struct netdev_lag_upper_info *info)
-{
-	struct net_device *lag_dev = lag.dev;
-	struct qca8k_priv *priv = ds->priv;
-	bool unique_lag = true;
-	unsigned int i;
-	u32 hash = 0;
-
-	switch (info->hash_type) {
-	case NETDEV_LAG_HASH_L23:
-		hash |= QCA8K_TRUNK_HASH_SIP_EN;
-		hash |= QCA8K_TRUNK_HASH_DIP_EN;
-		fallthrough;
-	case NETDEV_LAG_HASH_L2:
-		hash |= QCA8K_TRUNK_HASH_SA_EN;
-		hash |= QCA8K_TRUNK_HASH_DA_EN;
-		break;
-	default: /* We should NEVER reach this */
-		return -EOPNOTSUPP;
-	}
-
-	/* Check if we are the unique configured LAG */
-	dsa_lags_foreach_id(i, ds->dst)
-		if (i != lag.id && dsa_lag_by_id(ds->dst, i)) {
-			unique_lag = false;
-			break;
-		}
-
-	/* Hash Mode is global. Make sure the same Hash Mode
-	 * is set to all the 4 possible lag.
-	 * If we are the unique LAG we can set whatever hash
-	 * mode we want.
-	 * To change hash mode it's needed to remove all LAG
-	 * and change the mode with the latest.
-	 */
-	if (unique_lag) {
-		priv->lag_hash_mode = hash;
-	} else if (priv->lag_hash_mode != hash) {
-		netdev_err(lag_dev, "Error: Mismatched Hash Mode across different lag is not supported\n");
-		return -EOPNOTSUPP;
-	}
-
-	return regmap_update_bits(priv->regmap, QCA8K_TRUNK_HASH_EN_CTRL,
-				  QCA8K_TRUNK_HASH_MASK, hash);
-}
-
-static int
-qca8k_lag_refresh_portmap(struct dsa_switch *ds, int port,
-			  struct dsa_lag lag, bool delete)
-{
-	struct qca8k_priv *priv = ds->priv;
-	int ret, id, i;
-	u32 val;
-
-	/* DSA LAG IDs are one-based, hardware is zero-based */
-	id = lag.id - 1;
-
-	/* Read current port member */
-	ret = regmap_read(priv->regmap, QCA8K_REG_GOL_TRUNK_CTRL0, &val);
-	if (ret)
-		return ret;
-
-	/* Shift val to the correct trunk */
-	val >>= QCA8K_REG_GOL_TRUNK_SHIFT(id);
-	val &= QCA8K_REG_GOL_TRUNK_MEMBER_MASK;
-	if (delete)
-		val &= ~BIT(port);
-	else
-		val |= BIT(port);
-
-	/* Update port member. With empty portmap disable trunk */
-	ret = regmap_update_bits(priv->regmap, QCA8K_REG_GOL_TRUNK_CTRL0,
-				 QCA8K_REG_GOL_TRUNK_MEMBER(id) |
-				 QCA8K_REG_GOL_TRUNK_EN(id),
-				 !val << QCA8K_REG_GOL_TRUNK_SHIFT(id) |
-				 val << QCA8K_REG_GOL_TRUNK_SHIFT(id));
-
-	/* Search empty member if adding or port on deleting */
-	for (i = 0; i < QCA8K_NUM_PORTS_FOR_LAG; i++) {
-		ret = regmap_read(priv->regmap, QCA8K_REG_GOL_TRUNK_CTRL(id), &val);
-		if (ret)
-			return ret;
-
-		val >>= QCA8K_REG_GOL_TRUNK_ID_MEM_ID_SHIFT(id, i);
-		val &= QCA8K_REG_GOL_TRUNK_ID_MEM_ID_MASK;
-
-		if (delete) {
-			/* If port flagged to be disabled assume this member is
-			 * empty
-			 */
-			if (val != QCA8K_REG_GOL_TRUNK_ID_MEM_ID_EN_MASK)
-				continue;
-
-			val &= QCA8K_REG_GOL_TRUNK_ID_MEM_ID_PORT_MASK;
-			if (val != port)
-				continue;
-		} else {
-			/* If port flagged to be enabled assume this member is
-			 * already set
-			 */
-			if (val == QCA8K_REG_GOL_TRUNK_ID_MEM_ID_EN_MASK)
-				continue;
-		}
-
-		/* We have found the member to add/remove */
-		break;
-	}
-
-	/* Set port in the correct port mask or disable port if in delete mode */
-	return regmap_update_bits(priv->regmap, QCA8K_REG_GOL_TRUNK_CTRL(id),
-				  QCA8K_REG_GOL_TRUNK_ID_MEM_ID_EN(id, i) |
-				  QCA8K_REG_GOL_TRUNK_ID_MEM_ID_PORT(id, i),
-				  !delete << QCA8K_REG_GOL_TRUNK_ID_MEM_ID_SHIFT(id, i) |
-				  port << QCA8K_REG_GOL_TRUNK_ID_MEM_ID_SHIFT(id, i));
-}
-
-static int
-qca8k_port_lag_join(struct dsa_switch *ds, int port, struct dsa_lag lag,
-		    struct netdev_lag_upper_info *info)
-{
-	int ret;
-
-	if (!qca8k_lag_can_offload(ds, lag, info))
-		return -EOPNOTSUPP;
-
-	ret = qca8k_lag_setup_hash(ds, lag, info);
-	if (ret)
-		return ret;
-
-	return qca8k_lag_refresh_portmap(ds, port, lag, false);
-}
-
-static int
-qca8k_port_lag_leave(struct dsa_switch *ds, int port,
-		     struct dsa_lag lag)
-{
-	return qca8k_lag_refresh_portmap(ds, port, lag, true);
-}
-
 static void
 qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
 		    bool operational)
@@ -3090,36 +1934,6 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.connect_tag_protocol	= qca8k_connect_tag_protocol,
 };
 
-static int qca8k_read_switch_id(struct qca8k_priv *priv)
-{
-	const struct qca8k_match_data *data;
-	u32 val;
-	u8 id;
-	int ret;
-
-	/* get the switches ID from the compatible */
-	data = of_device_get_match_data(priv->dev);
-	if (!data)
-		return -ENODEV;
-
-	ret = regmap_read(priv->regmap, QCA8K_REG_MASK_CTRL, &val);
-	if (ret < 0)
-		return -ENODEV;
-
-	id = QCA8K_MASK_CTRL_DEVICE_ID(val);
-	if (id != data->id) {
-		dev_err(priv->dev, "Switch id detected %x but expected %x", id, data->id);
-		return -ENODEV;
-	}
-
-	priv->switch_id = id;
-
-	/* Save revision to communicate to the internal PHY driver */
-	priv->switch_revision = QCA8K_MASK_CTRL_REV_ID(val);
-
-	return 0;
-}
-
 static int
 qca8k_sw_probe(struct mdio_device *mdiodev)
 {
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
new file mode 100644
index 000000000000..b8a18c5e7664
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -0,0 +1,1174 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2009 Felix Fietkau <nbd@nbd.name>
+ * Copyright (C) 2011-2012 Gabor Juhos <juhosg@openwrt.org>
+ * Copyright (c) 2015, 2019, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2016 John Crispin <john@phrozen.org>
+ */
+
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/netdevice.h>
+#include <linux/bitfield.h>
+#include <net/dsa.h>
+#include <linux/of_platform.h>
+#include <linux/if_bridge.h>
+
+#include "qca8k.h"
+
+#define MIB_DESC(_s, _o, _n)	\
+	{			\
+		.size = (_s),	\
+		.offset = (_o),	\
+		.name = (_n),	\
+	}
+
+const struct qca8k_mib_desc ar8327_mib[] = {
+	MIB_DESC(1, 0x00, "RxBroad"),
+	MIB_DESC(1, 0x04, "RxPause"),
+	MIB_DESC(1, 0x08, "RxMulti"),
+	MIB_DESC(1, 0x0c, "RxFcsErr"),
+	MIB_DESC(1, 0x10, "RxAlignErr"),
+	MIB_DESC(1, 0x14, "RxRunt"),
+	MIB_DESC(1, 0x18, "RxFragment"),
+	MIB_DESC(1, 0x1c, "Rx64Byte"),
+	MIB_DESC(1, 0x20, "Rx128Byte"),
+	MIB_DESC(1, 0x24, "Rx256Byte"),
+	MIB_DESC(1, 0x28, "Rx512Byte"),
+	MIB_DESC(1, 0x2c, "Rx1024Byte"),
+	MIB_DESC(1, 0x30, "Rx1518Byte"),
+	MIB_DESC(1, 0x34, "RxMaxByte"),
+	MIB_DESC(1, 0x38, "RxTooLong"),
+	MIB_DESC(2, 0x3c, "RxGoodByte"),
+	MIB_DESC(2, 0x44, "RxBadByte"),
+	MIB_DESC(1, 0x4c, "RxOverFlow"),
+	MIB_DESC(1, 0x50, "Filtered"),
+	MIB_DESC(1, 0x54, "TxBroad"),
+	MIB_DESC(1, 0x58, "TxPause"),
+	MIB_DESC(1, 0x5c, "TxMulti"),
+	MIB_DESC(1, 0x60, "TxUnderRun"),
+	MIB_DESC(1, 0x64, "Tx64Byte"),
+	MIB_DESC(1, 0x68, "Tx128Byte"),
+	MIB_DESC(1, 0x6c, "Tx256Byte"),
+	MIB_DESC(1, 0x70, "Tx512Byte"),
+	MIB_DESC(1, 0x74, "Tx1024Byte"),
+	MIB_DESC(1, 0x78, "Tx1518Byte"),
+	MIB_DESC(1, 0x7c, "TxMaxByte"),
+	MIB_DESC(1, 0x80, "TxOverSize"),
+	MIB_DESC(2, 0x84, "TxByte"),
+	MIB_DESC(1, 0x8c, "TxCollision"),
+	MIB_DESC(1, 0x90, "TxAbortCol"),
+	MIB_DESC(1, 0x94, "TxMultiCol"),
+	MIB_DESC(1, 0x98, "TxSingleCol"),
+	MIB_DESC(1, 0x9c, "TxExcDefer"),
+	MIB_DESC(1, 0xa0, "TxDefer"),
+	MIB_DESC(1, 0xa4, "TxLateCol"),
+	MIB_DESC(1, 0xa8, "RXUnicast"),
+	MIB_DESC(1, 0xac, "TXUnicast"),
+};
+
+static const struct regmap_range qca8k_readable_ranges[] = {
+	regmap_reg_range(0x0000, 0x00e4), /* Global control */
+	regmap_reg_range(0x0100, 0x0168), /* EEE control */
+	regmap_reg_range(0x0200, 0x0270), /* Parser control */
+	regmap_reg_range(0x0400, 0x0454), /* ACL */
+	regmap_reg_range(0x0600, 0x0718), /* Lookup */
+	regmap_reg_range(0x0800, 0x0b70), /* QM */
+	regmap_reg_range(0x0c00, 0x0c80), /* PKT */
+	regmap_reg_range(0x0e00, 0x0e98), /* L3 */
+	regmap_reg_range(0x1000, 0x10ac), /* MIB - Port0 */
+	regmap_reg_range(0x1100, 0x11ac), /* MIB - Port1 */
+	regmap_reg_range(0x1200, 0x12ac), /* MIB - Port2 */
+	regmap_reg_range(0x1300, 0x13ac), /* MIB - Port3 */
+	regmap_reg_range(0x1400, 0x14ac), /* MIB - Port4 */
+	regmap_reg_range(0x1500, 0x15ac), /* MIB - Port5 */
+	regmap_reg_range(0x1600, 0x16ac), /* MIB - Port6 */
+
+};
+
+const struct regmap_access_table qca8k_readable_table = {
+	.yes_ranges = qca8k_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(qca8k_readable_ranges),
+};
+
+static int
+qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
+{
+	u32 val;
+
+	return regmap_read_poll_timeout(priv->regmap, reg, val, !(val & mask), 0,
+				       QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC);
+}
+
+static int
+qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
+{
+	u32 reg[QCA8K_ATU_TABLE_SIZE];
+	int ret;
+
+	/* load the ARL table into an array */
+	ret = regmap_bulk_read(priv->regmap, QCA8K_REG_ATU_DATA0, reg,
+			       QCA8K_ATU_TABLE_SIZE);
+	if (ret)
+		return ret;
+
+	/* vid - 83:72 */
+	fdb->vid = FIELD_GET(QCA8K_ATU_VID_MASK, reg[2]);
+	/* aging - 67:64 */
+	fdb->aging = FIELD_GET(QCA8K_ATU_STATUS_MASK, reg[2]);
+	/* portmask - 54:48 */
+	fdb->port_mask = FIELD_GET(QCA8K_ATU_PORT_MASK, reg[1]);
+	/* mac - 47:0 */
+	fdb->mac[0] = FIELD_GET(QCA8K_ATU_ADDR0_MASK, reg[1]);
+	fdb->mac[1] = FIELD_GET(QCA8K_ATU_ADDR1_MASK, reg[1]);
+	fdb->mac[2] = FIELD_GET(QCA8K_ATU_ADDR2_MASK, reg[0]);
+	fdb->mac[3] = FIELD_GET(QCA8K_ATU_ADDR3_MASK, reg[0]);
+	fdb->mac[4] = FIELD_GET(QCA8K_ATU_ADDR4_MASK, reg[0]);
+	fdb->mac[5] = FIELD_GET(QCA8K_ATU_ADDR5_MASK, reg[0]);
+
+	return 0;
+}
+
+static void
+qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
+		u8 aging)
+{
+	u32 reg[QCA8K_ATU_TABLE_SIZE] = { 0 };
+
+	/* vid - 83:72 */
+	reg[2] = FIELD_PREP(QCA8K_ATU_VID_MASK, vid);
+	/* aging - 67:64 */
+	reg[2] |= FIELD_PREP(QCA8K_ATU_STATUS_MASK, aging);
+	/* portmask - 54:48 */
+	reg[1] = FIELD_PREP(QCA8K_ATU_PORT_MASK, port_mask);
+	/* mac - 47:0 */
+	reg[1] |= FIELD_PREP(QCA8K_ATU_ADDR0_MASK, mac[0]);
+	reg[1] |= FIELD_PREP(QCA8K_ATU_ADDR1_MASK, mac[1]);
+	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR2_MASK, mac[2]);
+	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR3_MASK, mac[3]);
+	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR4_MASK, mac[4]);
+	reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR5_MASK, mac[5]);
+
+	/* load the array into the ARL table */
+	regmap_bulk_write(priv->regmap, QCA8K_REG_ATU_DATA0, reg, QCA8K_ATU_TABLE_SIZE);
+}
+
+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;
+	reg |= cmd;
+	if (port >= 0) {
+		reg |= QCA8K_ATU_FUNC_PORT_EN;
+		reg |= FIELD_PREP(QCA8K_ATU_FUNC_PORT_MASK, port);
+	}
+
+	/* Write the function register triggering the table access */
+	ret = regmap_write(priv->regmap, QCA8K_REG_ATU_FUNC, reg);
+	if (ret)
+		return ret;
+
+	/* wait for completion */
+	ret = qca8k_busy_wait(priv, QCA8K_REG_ATU_FUNC, QCA8K_ATU_FUNC_BUSY);
+	if (ret)
+		return ret;
+
+	/* Check for table full violation when adding an entry */
+	if (cmd == QCA8K_FDB_LOAD) {
+		ret = regmap_read(priv->regmap, QCA8K_REG_ATU_FUNC, &reg);
+		if (ret < 0)
+			return ret;
+		if (reg & QCA8K_ATU_FUNC_FULL)
+			return -1;
+	}
+
+	return 0;
+}
+
+static int
+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);
+}
+
+static int
+qca8k_fdb_add(struct qca8k_priv *priv, const u8 *mac, u16 port_mask,
+	      u16 vid, u8 aging)
+{
+	int ret;
+
+	mutex_lock(&priv->reg_mutex);
+	qca8k_fdb_write(priv, vid, port_mask, mac, aging);
+	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+static int
+qca8k_fdb_del(struct qca8k_priv *priv, const u8 *mac, u16 port_mask, u16 vid)
+{
+	int ret;
+
+	mutex_lock(&priv->reg_mutex);
+	qca8k_fdb_write(priv, vid, port_mask, mac, 0);
+	ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+void qca8k_fdb_flush(struct qca8k_priv *priv)
+{
+	mutex_lock(&priv->reg_mutex);
+	qca8k_fdb_access(priv, QCA8K_FDB_FLUSH, -1);
+	mutex_unlock(&priv->reg_mutex);
+}
+
+static int
+qca8k_fdb_search_and_insert(struct qca8k_priv *priv, u8 port_mask,
+			    const u8 *mac, u16 vid)
+{
+	struct qca8k_fdb fdb = { 0 };
+	int ret;
+
+	mutex_lock(&priv->reg_mutex);
+
+	qca8k_fdb_write(priv, vid, 0, mac, 0);
+	ret = qca8k_fdb_access(priv, QCA8K_FDB_SEARCH, -1);
+	if (ret < 0)
+		goto exit;
+
+	ret = qca8k_fdb_read(priv, &fdb);
+	if (ret < 0)
+		goto exit;
+
+	/* Rule exist. Delete first */
+	if (!fdb.aging) {
+		ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
+		if (ret)
+			goto exit;
+	}
+
+	/* Add port to fdb portmask */
+	fdb.port_mask |= port_mask;
+
+	qca8k_fdb_write(priv, vid, fdb.port_mask, mac, fdb.aging);
+	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
+
+exit:
+	mutex_unlock(&priv->reg_mutex);
+	return ret;
+}
+
+static int
+qca8k_fdb_search_and_del(struct qca8k_priv *priv, u8 port_mask,
+			 const u8 *mac, u16 vid)
+{
+	struct qca8k_fdb fdb = { 0 };
+	int ret;
+
+	mutex_lock(&priv->reg_mutex);
+
+	qca8k_fdb_write(priv, vid, 0, mac, 0);
+	ret = qca8k_fdb_access(priv, QCA8K_FDB_SEARCH, -1);
+	if (ret < 0)
+		goto exit;
+
+	/* Rule doesn't exist. Why delete? */
+	if (!fdb.aging) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
+	if (ret)
+		goto exit;
+
+	/* Only port in the rule is this port. Don't re insert */
+	if (fdb.port_mask == port_mask)
+		goto exit;
+
+	/* Remove port from port mask */
+	fdb.port_mask &= ~port_mask;
+
+	qca8k_fdb_write(priv, vid, fdb.port_mask, mac, fdb.aging);
+	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
+
+exit:
+	mutex_unlock(&priv->reg_mutex);
+	return ret;
+}
+
+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;
+	reg |= cmd;
+	reg |= FIELD_PREP(QCA8K_VTU_FUNC1_VID_MASK, vid);
+
+	/* Write the function register triggering the table access */
+	ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC1, reg);
+	if (ret)
+		return ret;
+
+	/* wait for completion */
+	ret = qca8k_busy_wait(priv, QCA8K_REG_VTU_FUNC1, QCA8K_VTU_FUNC1_BUSY);
+	if (ret)
+		return ret;
+
+	/* Check for table full violation when adding an entry */
+	if (cmd == QCA8K_VLAN_LOAD) {
+		ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC1, &reg);
+		if (ret < 0)
+			return ret;
+		if (reg & QCA8K_VTU_FUNC1_FULL)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid, bool untagged)
+{
+	u32 reg;
+	int ret;
+
+	/* We do the right thing with VLAN 0 and treat it as untagged while
+	 * preserving the tag on egress.
+	 */
+	if (vid == 0)
+		return 0;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
+	if (ret < 0)
+		goto out;
+
+	ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC0, &reg);
+	if (ret < 0)
+		goto out;
+	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
+	reg &= ~QCA8K_VTU_FUNC0_EG_MODE_PORT_MASK(port);
+	if (untagged)
+		reg |= QCA8K_VTU_FUNC0_EG_MODE_PORT_UNTAG(port);
+	else
+		reg |= QCA8K_VTU_FUNC0_EG_MODE_PORT_TAG(port);
+
+	ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC0, reg);
+	if (ret)
+		goto out;
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
+
+out:
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+static int
+qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
+{
+	u32 reg, mask;
+	int ret, i;
+	bool del;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
+	if (ret < 0)
+		goto out;
+
+	ret = regmap_read(priv->regmap, QCA8K_REG_VTU_FUNC0, &reg);
+	if (ret < 0)
+		goto out;
+	reg &= ~QCA8K_VTU_FUNC0_EG_MODE_PORT_MASK(port);
+	reg |= QCA8K_VTU_FUNC0_EG_MODE_PORT_NOT(port);
+
+	/* Check if we're the last member to be removed */
+	del = true;
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		mask = QCA8K_VTU_FUNC0_EG_MODE_PORT_NOT(i);
+
+		if ((reg & mask) != mask) {
+			del = false;
+			break;
+		}
+	}
+
+	if (del) {
+		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
+	} else {
+		ret = regmap_write(priv->regmap, QCA8K_REG_VTU_FUNC0, reg);
+		if (ret)
+			goto out;
+		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
+	}
+
+out:
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+int qca8k_mib_init(struct qca8k_priv *priv)
+{
+	int ret;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
+				 QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
+				 FIELD_PREP(QCA8K_MIB_FUNC, 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;
+
+	ret = regmap_set_bits(priv->regmap, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
+	if (ret)
+		goto exit;
+
+	ret = regmap_write(priv->regmap, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
+
+exit:
+	mutex_unlock(&priv->reg_mutex);
+	return ret;
+}
+
+void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
+{
+	u32 mask = QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
+
+	/* Port 0 and 6 have no internal PHY */
+	if (port > 0 && port < 6)
+		mask |= QCA8K_PORT_STATUS_LINK_AUTO;
+
+	if (enable)
+		regmap_set_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
+	else
+		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
+}
+
+void qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data)
+{
+	const struct qca8k_match_data *match_data;
+	struct qca8k_priv *priv = ds->priv;
+	int i;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	match_data = of_device_get_match_data(priv->dev);
+
+	for (i = 0; i < match_data->mib_count; i++)
+		strncpy(data + i * ETH_GSTRING_LEN, ar8327_mib[i].name,
+			ETH_GSTRING_LEN);
+}
+
+void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
+			     uint64_t *data)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	const struct qca8k_match_data *match_data;
+	const struct qca8k_mib_desc *mib;
+	u32 reg, i, val;
+	u32 hi = 0;
+	int ret;
+
+	if (priv->mgmt_master && priv->autocast_mib &&
+	    priv->autocast_mib(ds, port, data) > 0)
+		return;
+
+	match_data = of_device_get_match_data(priv->dev);
+
+	for (i = 0; i < match_data->mib_count; i++) {
+		mib = &ar8327_mib[i];
+		reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
+
+		ret = regmap_read(priv->regmap, reg, &val);
+		if (ret < 0)
+			continue;
+
+		if (mib->size == 2) {
+			ret = regmap_read(priv->regmap, reg + 4, &hi);
+			if (ret < 0)
+				continue;
+		}
+
+		data[i] = val;
+		if (mib->size == 2)
+			data[i] |= (u64)hi << 32;
+	}
+}
+
+int qca8k_get_sset_count(struct dsa_switch *ds, int port, int sset)
+{
+	const struct qca8k_match_data *match_data;
+	struct qca8k_priv *priv = ds->priv;
+
+	if (sset != ETH_SS_STATS)
+		return 0;
+
+	match_data = of_device_get_match_data(priv->dev);
+
+	return match_data->mib_count;
+}
+
+int 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);
+	u32 reg;
+	int ret;
+
+	mutex_lock(&priv->reg_mutex);
+	ret = regmap_read(priv->regmap, QCA8K_REG_EEE_CTRL, &reg);
+	if (ret < 0)
+		goto exit;
+
+	if (eee->eee_enabled)
+		reg |= lpi_en;
+	else
+		reg &= ~lpi_en;
+	ret = regmap_write(priv->regmap, QCA8K_REG_EEE_CTRL, reg);
+
+exit:
+	mutex_unlock(&priv->reg_mutex);
+	return ret;
+}
+
+int qca8k_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
+{
+	/* Nothing to do on the port's MAC */
+	return 0;
+}
+
+void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	u32 stp_state;
+
+	switch (state) {
+	case BR_STATE_DISABLED:
+		stp_state = QCA8K_PORT_LOOKUP_STATE_DISABLED;
+		break;
+	case BR_STATE_BLOCKING:
+		stp_state = QCA8K_PORT_LOOKUP_STATE_BLOCKING;
+		break;
+	case BR_STATE_LISTENING:
+		stp_state = QCA8K_PORT_LOOKUP_STATE_LISTENING;
+		break;
+	case BR_STATE_LEARNING:
+		stp_state = QCA8K_PORT_LOOKUP_STATE_LEARNING;
+		break;
+	case BR_STATE_FORWARDING:
+	default:
+		stp_state = QCA8K_PORT_LOOKUP_STATE_FORWARD;
+		break;
+	}
+
+	regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+			   QCA8K_PORT_LOOKUP_STATE_MASK, stp_state);
+}
+
+int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
+			   struct dsa_bridge bridge,
+			   bool *tx_fwd_offload,
+			   struct netlink_ext_ack *extack)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	int port_mask, cpu_port;
+	int i, ret;
+
+	cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
+	port_mask = BIT(cpu_port);
+
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		if (dsa_is_cpu_port(ds, i))
+			continue;
+		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+			continue;
+		/* Add this port to the portvlan mask of the other ports
+		 * in the bridge
+		 */
+		ret = regmap_set_bits(priv->regmap,
+				      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 */
+	ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+				 QCA8K_PORT_LOOKUP_MEMBER, port_mask);
+
+	return ret;
+}
+
+void qca8k_port_bridge_leave(struct dsa_switch *ds, int port,
+			     struct dsa_bridge bridge)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	int cpu_port, i;
+
+	cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
+
+	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		if (dsa_is_cpu_port(ds, i))
+			continue;
+		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+			continue;
+		/* Remove this port to the portvlan mask of the other ports
+		 * in the bridge
+		 */
+		regmap_clear_bits(priv->regmap,
+				  QCA8K_PORT_LOOKUP_CTRL(i),
+				  BIT(port));
+	}
+
+	/* Set the cpu port to be the only one in the portvlan mask of
+	 * this port
+	 */
+	regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+			   QCA8K_PORT_LOOKUP_MEMBER, BIT(cpu_port));
+}
+
+void qca8k_port_fast_age(struct dsa_switch *ds, int port)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	mutex_lock(&priv->reg_mutex);
+	qca8k_fdb_access(priv, QCA8K_FDB_FLUSH_PORT, port);
+	mutex_unlock(&priv->reg_mutex);
+}
+
+int qca8k_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
+{
+	struct qca8k_priv *priv = ds->priv;
+	unsigned int secs = msecs / 1000;
+	u32 val;
+
+	/* AGE_TIME reg is set in 7s step */
+	val = secs / 7;
+
+	/* Handle case with 0 as val to NOT disable
+	 * learning
+	 */
+	if (!val)
+		val = 1;
+
+	return regmap_update_bits(priv->regmap, QCA8K_REG_ATU_CTRL, QCA8K_ATU_AGE_TIME_MASK,
+				  QCA8K_ATU_AGE_TIME(val));
+}
+
+int qca8k_port_enable(struct dsa_switch *ds, int port,
+		      struct phy_device *phy)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+
+	qca8k_port_set_status(priv, port, 1);
+	priv->port_enabled_map |= BIT(port);
+
+	if (dsa_is_user_port(ds, port))
+		phy_support_asym_pause(phy);
+
+	return 0;
+}
+
+void 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_enabled_map &= ~BIT(port);
+}
+
+int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
+{
+	struct qca8k_priv *priv = ds->priv;
+	int ret;
+
+	/* We have only have a general MTU setting.
+	 * DSA always set the CPU port's MTU to the largest MTU of the slave
+	 * 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;
+
+	/* To change the MAX_FRAME_SIZE the cpu ports must be off or
+	 * the switch panics.
+	 * Turn off both cpu ports before applying the new value to prevent
+	 * this.
+	 */
+	if (priv->port_enabled_map & BIT(0))
+		qca8k_port_set_status(priv, 0, 0);
+
+	if (priv->port_enabled_map & BIT(6))
+		qca8k_port_set_status(priv, 6, 0);
+
+	/* Include L2 header / FCS length */
+	ret = regmap_write(priv->regmap, QCA8K_MAX_FRAME_SIZE, new_mtu + ETH_HLEN + ETH_FCS_LEN);
+
+	if (priv->port_enabled_map & BIT(0))
+		qca8k_port_set_status(priv, 0, 1);
+
+	if (priv->port_enabled_map & BIT(6))
+		qca8k_port_set_status(priv, 6, 1);
+
+	return ret;
+}
+
+int qca8k_port_max_mtu(struct dsa_switch *ds, int port)
+{
+	return QCA8K_MAX_MTU;
+}
+
+int qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
+			  u16 port_mask, u16 vid)
+{
+	/* Set the vid to the port vlan id if no vid is set */
+	if (!vid)
+		vid = QCA8K_PORT_VID_DEF;
+
+	return qca8k_fdb_add(priv, addr, port_mask, vid,
+			     QCA8K_ATU_STATUS_STATIC);
+}
+
+int qca8k_port_fdb_add(struct dsa_switch *ds, int port,
+		       const unsigned char *addr, u16 vid,
+		       struct dsa_db db)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	return qca8k_port_fdb_insert(priv, addr, port_mask, vid);
+}
+
+int qca8k_port_fdb_del(struct dsa_switch *ds, int port,
+		       const unsigned char *addr, u16 vid,
+		       struct dsa_db db)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	if (!vid)
+		vid = QCA8K_PORT_VID_DEF;
+
+	return qca8k_fdb_del(priv, addr, port_mask, vid);
+}
+
+int qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
+			dsa_fdb_dump_cb_t *cb, void *data)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	struct qca8k_fdb _fdb = { 0 };
+	int cnt = QCA8K_NUM_FDB_RECORDS;
+	bool is_static;
+	int ret = 0;
+
+	mutex_lock(&priv->reg_mutex);
+	while (cnt-- && !qca8k_fdb_next(priv, &_fdb, port)) {
+		if (!_fdb.aging)
+			break;
+		is_static = (_fdb.aging == QCA8K_ATU_STATUS_STATIC);
+		ret = cb(_fdb.mac, _fdb.vid, is_static, data);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&priv->reg_mutex);
+
+	return 0;
+}
+
+int qca8k_port_mdb_add(struct dsa_switch *ds, int port,
+		       const struct switchdev_obj_port_mdb *mdb,
+		       struct dsa_db db)
+{
+	struct qca8k_priv *priv = ds->priv;
+	const u8 *addr = mdb->addr;
+	u16 vid = mdb->vid;
+
+	return qca8k_fdb_search_and_insert(priv, BIT(port), addr, vid);
+}
+
+int qca8k_port_mdb_del(struct dsa_switch *ds, int port,
+		       const struct switchdev_obj_port_mdb *mdb,
+		       struct dsa_db db)
+{
+	struct qca8k_priv *priv = ds->priv;
+	const u8 *addr = mdb->addr;
+	u16 vid = mdb->vid;
+
+	return qca8k_fdb_search_and_del(priv, BIT(port), addr, vid);
+}
+
+int qca8k_port_mirror_add(struct dsa_switch *ds, int port,
+			  struct dsa_mall_mirror_tc_entry *mirror,
+			  bool ingress, struct netlink_ext_ack *extack)
+{
+	struct qca8k_priv *priv = ds->priv;
+	int monitor_port, ret;
+	u32 reg, val;
+
+	/* Check for existent entry */
+	if ((ingress ? priv->mirror_rx : priv->mirror_tx) & BIT(port))
+		return -EEXIST;
+
+	ret = regmap_read(priv->regmap, QCA8K_REG_GLOBAL_FW_CTRL0, &val);
+	if (ret)
+		return ret;
+
+	/* QCA83xx can have only one port set to mirror mode.
+	 * Check that the correct port is requested and return error otherwise.
+	 * When no mirror port is set, the values is set to 0xF
+	 */
+	monitor_port = FIELD_GET(QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM, val);
+	if (monitor_port != 0xF && monitor_port != mirror->to_local_port)
+		return -EEXIST;
+
+	/* Set the monitor port */
+	val = FIELD_PREP(QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM,
+			 mirror->to_local_port);
+	ret = regmap_update_bits(priv->regmap, QCA8K_REG_GLOBAL_FW_CTRL0,
+				 QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM, val);
+	if (ret)
+		return ret;
+
+	if (ingress) {
+		reg = QCA8K_PORT_LOOKUP_CTRL(port);
+		val = QCA8K_PORT_LOOKUP_ING_MIRROR_EN;
+	} else {
+		reg = QCA8K_REG_PORT_HOL_CTRL1(port);
+		val = QCA8K_PORT_HOL_CTRL1_EG_MIRROR_EN;
+	}
+
+	ret = regmap_update_bits(priv->regmap, reg, val, val);
+	if (ret)
+		return ret;
+
+	/* Track mirror port for tx and rx to decide when the
+	 * mirror port has to be disabled.
+	 */
+	if (ingress)
+		priv->mirror_rx |= BIT(port);
+	else
+		priv->mirror_tx |= BIT(port);
+
+	return 0;
+}
+
+void qca8k_port_mirror_del(struct dsa_switch *ds, int port,
+			   struct dsa_mall_mirror_tc_entry *mirror)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg, val;
+	int ret;
+
+	if (mirror->ingress) {
+		reg = QCA8K_PORT_LOOKUP_CTRL(port);
+		val = QCA8K_PORT_LOOKUP_ING_MIRROR_EN;
+	} else {
+		reg = QCA8K_REG_PORT_HOL_CTRL1(port);
+		val = QCA8K_PORT_HOL_CTRL1_EG_MIRROR_EN;
+	}
+
+	ret = regmap_clear_bits(priv->regmap, reg, val);
+	if (ret)
+		goto err;
+
+	if (mirror->ingress)
+		priv->mirror_rx &= ~BIT(port);
+	else
+		priv->mirror_tx &= ~BIT(port);
+
+	/* No port set to send packet to mirror port. Disable mirror port */
+	if (!priv->mirror_rx && !priv->mirror_tx) {
+		val = FIELD_PREP(QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM, 0xF);
+		ret = regmap_update_bits(priv->regmap, QCA8K_REG_GLOBAL_FW_CTRL0,
+					 QCA8K_GLOBAL_FW_CTRL0_MIRROR_PORT_NUM, val);
+		if (ret)
+			goto err;
+	}
+err:
+	dev_err(priv->dev, "Failed to del mirror port from %d", port);
+}
+
+int 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) {
+		ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+					 QCA8K_PORT_LOOKUP_VLAN_MODE_MASK,
+					 QCA8K_PORT_LOOKUP_VLAN_MODE_SECURE);
+	} else {
+		ret = regmap_update_bits(priv->regmap, QCA8K_PORT_LOOKUP_CTRL(port),
+					 QCA8K_PORT_LOOKUP_VLAN_MODE_MASK,
+					 QCA8K_PORT_LOOKUP_VLAN_MODE_NONE);
+	}
+
+	return ret;
+}
+
+int qca8k_port_vlan_add(struct dsa_switch *ds, int port,
+			const struct switchdev_obj_port_vlan *vlan,
+			struct netlink_ext_ack *extack)
+{
+	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+	struct qca8k_priv *priv = ds->priv;
+	int ret;
+
+	ret = qca8k_vlan_add(priv, port, vlan->vid, untagged);
+	if (ret) {
+		dev_err(priv->dev, "Failed to add VLAN to port %d (%d)", port, ret);
+		return ret;
+	}
+
+	if (pvid) {
+		ret = regmap_update_bits(priv->regmap, QCA8K_EGRESS_VLAN(port),
+					 QCA8K_EGREES_VLAN_PORT_MASK(port),
+					 QCA8K_EGREES_VLAN_PORT(port, vlan->vid));
+		if (ret)
+			return ret;
+
+		ret = regmap_write(priv->regmap, QCA8K_REG_PORT_VLAN_CTRL0(port),
+				   QCA8K_PORT_VLAN_CVID(vlan->vid) |
+				   QCA8K_PORT_VLAN_SVID(vlan->vid));
+	}
+
+	return ret;
+}
+
+int 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;
+
+	ret = qca8k_vlan_del(priv, port, vlan->vid);
+	if (ret)
+		dev_err(priv->dev, "Failed to delete VLAN from port %d (%d)", port, ret);
+
+	return ret;
+}
+
+static bool
+qca8k_lag_can_offload(struct dsa_switch *ds, struct dsa_lag lag,
+		      struct netdev_lag_upper_info *info)
+{
+	struct dsa_port *dp;
+	int members = 0;
+
+	if (!lag.id)
+		return false;
+
+	dsa_lag_foreach_port(dp, ds->dst, &lag)
+		/* Includes the port joining the LAG */
+		members++;
+
+	if (members > QCA8K_NUM_PORTS_FOR_LAG)
+		return false;
+
+	if (info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
+		return false;
+
+	if (info->hash_type != NETDEV_LAG_HASH_L2 &&
+	    info->hash_type != NETDEV_LAG_HASH_L23)
+		return false;
+
+	return true;
+}
+
+static int
+qca8k_lag_setup_hash(struct dsa_switch *ds, struct dsa_lag lag,
+		     struct netdev_lag_upper_info *info)
+{
+	struct net_device *lag_dev = lag.dev;
+	struct qca8k_priv *priv = ds->priv;
+	bool unique_lag = true;
+	unsigned int i;
+	u32 hash = 0;
+
+	switch (info->hash_type) {
+	case NETDEV_LAG_HASH_L23:
+		hash |= QCA8K_TRUNK_HASH_SIP_EN;
+		hash |= QCA8K_TRUNK_HASH_DIP_EN;
+		fallthrough;
+	case NETDEV_LAG_HASH_L2:
+		hash |= QCA8K_TRUNK_HASH_SA_EN;
+		hash |= QCA8K_TRUNK_HASH_DA_EN;
+		break;
+	default: /* We should NEVER reach this */
+		return -EOPNOTSUPP;
+	}
+
+	/* Check if we are the unique configured LAG */
+	dsa_lags_foreach_id(i, ds->dst)
+		if (i != lag.id && dsa_lag_by_id(ds->dst, i)) {
+			unique_lag = false;
+			break;
+		}
+
+	/* Hash Mode is global. Make sure the same Hash Mode
+	 * is set to all the 4 possible lag.
+	 * If we are the unique LAG we can set whatever hash
+	 * mode we want.
+	 * To change hash mode it's needed to remove all LAG
+	 * and change the mode with the latest.
+	 */
+	if (unique_lag) {
+		priv->lag_hash_mode = hash;
+	} else if (priv->lag_hash_mode != hash) {
+		netdev_err(lag_dev, "Error: Mismatched Hash Mode across different lag is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	return regmap_update_bits(priv->regmap, QCA8K_TRUNK_HASH_EN_CTRL,
+				  QCA8K_TRUNK_HASH_MASK, hash);
+}
+
+static int
+qca8k_lag_refresh_portmap(struct dsa_switch *ds, int port,
+			  struct dsa_lag lag, bool delete)
+{
+	struct qca8k_priv *priv = ds->priv;
+	int ret, id, i;
+	u32 val;
+
+	/* DSA LAG IDs are one-based, hardware is zero-based */
+	id = lag.id - 1;
+
+	/* Read current port member */
+	ret = regmap_read(priv->regmap, QCA8K_REG_GOL_TRUNK_CTRL0, &val);
+	if (ret)
+		return ret;
+
+	/* Shift val to the correct trunk */
+	val >>= QCA8K_REG_GOL_TRUNK_SHIFT(id);
+	val &= QCA8K_REG_GOL_TRUNK_MEMBER_MASK;
+	if (delete)
+		val &= ~BIT(port);
+	else
+		val |= BIT(port);
+
+	/* Update port member. With empty portmap disable trunk */
+	ret = regmap_update_bits(priv->regmap, QCA8K_REG_GOL_TRUNK_CTRL0,
+				 QCA8K_REG_GOL_TRUNK_MEMBER(id) |
+				 QCA8K_REG_GOL_TRUNK_EN(id),
+				 !val << QCA8K_REG_GOL_TRUNK_SHIFT(id) |
+				 val << QCA8K_REG_GOL_TRUNK_SHIFT(id));
+
+	/* Search empty member if adding or port on deleting */
+	for (i = 0; i < QCA8K_NUM_PORTS_FOR_LAG; i++) {
+		ret = regmap_read(priv->regmap, QCA8K_REG_GOL_TRUNK_CTRL(id), &val);
+		if (ret)
+			return ret;
+
+		val >>= QCA8K_REG_GOL_TRUNK_ID_MEM_ID_SHIFT(id, i);
+		val &= QCA8K_REG_GOL_TRUNK_ID_MEM_ID_MASK;
+
+		if (delete) {
+			/* If port flagged to be disabled assume this member is
+			 * empty
+			 */
+			if (val != QCA8K_REG_GOL_TRUNK_ID_MEM_ID_EN_MASK)
+				continue;
+
+			val &= QCA8K_REG_GOL_TRUNK_ID_MEM_ID_PORT_MASK;
+			if (val != port)
+				continue;
+		} else {
+			/* If port flagged to be enabled assume this member is
+			 * already set
+			 */
+			if (val == QCA8K_REG_GOL_TRUNK_ID_MEM_ID_EN_MASK)
+				continue;
+		}
+
+		/* We have found the member to add/remove */
+		break;
+	}
+
+	/* Set port in the correct port mask or disable port if in delete mode */
+	return regmap_update_bits(priv->regmap, QCA8K_REG_GOL_TRUNK_CTRL(id),
+				  QCA8K_REG_GOL_TRUNK_ID_MEM_ID_EN(id, i) |
+				  QCA8K_REG_GOL_TRUNK_ID_MEM_ID_PORT(id, i),
+				  !delete << QCA8K_REG_GOL_TRUNK_ID_MEM_ID_SHIFT(id, i) |
+				  port << QCA8K_REG_GOL_TRUNK_ID_MEM_ID_SHIFT(id, i));
+}
+
+int qca8k_port_lag_join(struct dsa_switch *ds, int port, struct dsa_lag lag,
+			struct netdev_lag_upper_info *info)
+{
+	int ret;
+
+	if (!qca8k_lag_can_offload(ds, lag, info))
+		return -EOPNOTSUPP;
+
+	ret = qca8k_lag_setup_hash(ds, lag, info);
+	if (ret)
+		return ret;
+
+	return qca8k_lag_refresh_portmap(ds, port, lag, false);
+}
+
+int qca8k_port_lag_leave(struct dsa_switch *ds, int port,
+			 struct dsa_lag lag)
+{
+	return qca8k_lag_refresh_portmap(ds, port, lag, true);
+}
+
+int qca8k_read_switch_id(struct qca8k_priv *priv)
+{
+	const struct qca8k_match_data *data;
+	u32 val;
+	u8 id;
+	int ret;
+
+	/* get the switches ID from the compatible */
+	data = of_device_get_match_data(priv->dev);
+	if (!data)
+		return -ENODEV;
+
+	ret = regmap_read(priv->regmap, QCA8K_REG_MASK_CTRL, &val);
+	if (ret < 0)
+		return -ENODEV;
+
+	id = QCA8K_MASK_CTRL_DEVICE_ID(val);
+	if (id != data->id) {
+		dev_err(priv->dev, "Switch id detected %x but expected %x", id, data->id);
+		return -ENODEV;
+	}
+
+	priv->switch_id = id;
+
+	/* Save revision to communicate to the internal PHY driver */
+	priv->switch_revision = QCA8K_MASK_CTRL_REV_ID(val);
+
+	return 0;
+}
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index a306638a7100..96e726e65185 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -419,4 +419,62 @@ struct qca8k_fdb {
 	u8 mac[6];
 };
 
+/* Common setup function */
+extern const struct qca8k_mib_desc ar8327_mib[];
+extern const struct regmap_access_table qca8k_readable_table;
+
+int qca8k_read_switch_id(struct qca8k_priv *priv);
+int qca8k_mib_init(struct qca8k_priv *priv);
+void qca8k_fdb_flush(struct qca8k_priv *priv);
+void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable);
+
+/* Common ops function */
+void qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data);
+void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data);
+int qca8k_get_sset_count(struct dsa_switch *ds, int port, int sset);
+int qca8k_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *eee);
+int qca8k_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
+void qca8k_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
+int qca8k_port_bridge_join(struct dsa_switch *ds, int port,
+			   struct dsa_bridge bridge,
+			   bool *tx_fwd_offload,
+			   struct netlink_ext_ack *extack);
+void qca8k_port_bridge_leave(struct dsa_switch *ds, int port,
+			     struct dsa_bridge bridge);
+void qca8k_port_fast_age(struct dsa_switch *ds, int port);
+int qca8k_set_ageing_time(struct dsa_switch *ds, unsigned int msecs);
+int qca8k_port_enable(struct dsa_switch *ds, int port, struct phy_device *phy);
+void qca8k_port_disable(struct dsa_switch *ds, int port);
+int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu);
+int qca8k_port_max_mtu(struct dsa_switch *ds, int port);
+int qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr, u16 port_mask, u16 vid);
+int qca8k_port_fdb_add(struct dsa_switch *ds, int port,
+		       const unsigned char *addr, u16 vid,
+		       struct dsa_db db);
+int qca8k_port_fdb_del(struct dsa_switch *ds, int port,
+		       const unsigned char *addr, u16 vid,
+		       struct dsa_db db);
+int qca8k_port_fdb_dump(struct dsa_switch *ds, int port, dsa_fdb_dump_cb_t *cb, void *data);
+int qca8k_port_mdb_add(struct dsa_switch *ds, int port,
+		       const struct switchdev_obj_port_mdb *mdb,
+		       struct dsa_db db);
+int qca8k_port_mdb_del(struct dsa_switch *ds, int port,
+		       const struct switchdev_obj_port_mdb *mdb,
+		       struct dsa_db db);
+int qca8k_port_mirror_add(struct dsa_switch *ds, int port,
+			  struct dsa_mall_mirror_tc_entry *mirror,
+			  bool ingress, struct netlink_ext_ack *extack);
+void qca8k_port_mirror_del(struct dsa_switch *ds, int port,
+			   struct dsa_mall_mirror_tc_entry *mirror);
+int qca8k_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
+			      struct netlink_ext_ack *extack);
+int qca8k_port_vlan_add(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan,
+			struct netlink_ext_ack *extack);
+int qca8k_port_vlan_del(struct dsa_switch *ds, int port,
+			const struct switchdev_obj_port_vlan *vlan);
+int qca8k_port_lag_join(struct dsa_switch *ds, int port, struct dsa_lag lag,
+			struct netdev_lag_upper_info *info);
+int qca8k_port_lag_leave(struct dsa_switch *ds, int port,
+			 struct dsa_lag lag);
+
 #endif /* __QCA8K_H */
-- 
2.36.1

Re: [net-next RFC PATCH 4/4] net: dsa: qca8k: split qca8k in common and 8xxx specific code

[Vladimir Oltean]

On Sat, Jul 16, 2022 at 07:49:58PM +0200, Christian Marangi wrote:
> The qca8k family reg structure is also used in the internal ipq40xx
> switch. Split qca8k common code from specific code for future
> implementation of ipq40xx internal switch based on qca8k.
> 
> While at it also fix minor wrong format for comments and reallign
> function as we had to drop static declaration.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca/Makefile                  |    1 +
>  drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} | 1210 +----------------
>  drivers/net/dsa/qca/qca8k-common.c            | 1174 ++++++++++++++++
>  drivers/net/dsa/qca/qca8k.h                   |   58 +
>  4 files changed, 1245 insertions(+), 1198 deletions(-)
>  rename drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} (64%)
>  create mode 100644 drivers/net/dsa/qca/qca8k-common.c

Sorry, this patch is very difficult to review for correctness.
Could you try to split it to multiple individual function movements?

Re: [net-next RFC PATCH 4/4] net: dsa: qca8k: split qca8k in common and 8xxx specific code

[Christian Marangi]

On Mon, Jul 18, 2022 at 08:21:35PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 16, 2022 at 07:49:58PM +0200, Christian Marangi wrote:
> > The qca8k family reg structure is also used in the internal ipq40xx
> > switch. Split qca8k common code from specific code for future
> > implementation of ipq40xx internal switch based on qca8k.
> > 
> > While at it also fix minor wrong format for comments and reallign
> > function as we had to drop static declaration.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca/Makefile                  |    1 +
> >  drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} | 1210 +----------------
> >  drivers/net/dsa/qca/qca8k-common.c            | 1174 ++++++++++++++++
> >  drivers/net/dsa/qca/qca8k.h                   |   58 +
> >  4 files changed, 1245 insertions(+), 1198 deletions(-)
> >  rename drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} (64%)
> >  create mode 100644 drivers/net/dsa/qca/qca8k-common.c
> 
> Sorry, this patch is very difficult to review for correctness.
> Could you try to split it to multiple individual function movements?

You are right.
Can I split them in category function (bridge function, vlan function,
ATU...) Or you want them even more split? 

-- 
	Ansuel

Re: [net-next RFC PATCH 4/4] net: dsa: qca8k: split qca8k in common and 8xxx specific code

[Vladimir Oltean]

On Mon, Jul 18, 2022 at 07:10:52PM +0200, Christian Marangi wrote:
> On Mon, Jul 18, 2022 at 08:21:35PM +0300, Vladimir Oltean wrote:
> > On Sat, Jul 16, 2022 at 07:49:58PM +0200, Christian Marangi wrote:
> > > The qca8k family reg structure is also used in the internal ipq40xx
> > > switch. Split qca8k common code from specific code for future
> > > implementation of ipq40xx internal switch based on qca8k.
> > > 
> > > While at it also fix minor wrong format for comments and reallign
> > > function as we had to drop static declaration.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/net/dsa/qca/Makefile                  |    1 +
> > >  drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} | 1210 +----------------
> > >  drivers/net/dsa/qca/qca8k-common.c            | 1174 ++++++++++++++++
> > >  drivers/net/dsa/qca/qca8k.h                   |   58 +
> > >  4 files changed, 1245 insertions(+), 1198 deletions(-)
> > >  rename drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} (64%)
> > >  create mode 100644 drivers/net/dsa/qca/qca8k-common.c
> > 
> > Sorry, this patch is very difficult to review for correctness.
> > Could you try to split it to multiple individual function movements?
> 
> You are right.
> Can I split them in category function (bridge function, vlan function,
> ATU...) Or you want them even more split? 

Yes, I think splitting by category is OK as long as the number of
functions being moved at once is trackable by a human being (I'd say at
most 5 functions or so). Use your own judgement while looking at the
output of git format-patch (essentially what a reviewer is looking at).

Re: [net-next RFC PATCH 0/4] net: dsa: qca8k: code split for qca8k

[Christian Marangi]

On Sat, Jul 16, 2022 at 07:49:54PM +0200, Christian Marangi wrote:
> This is posted as an RFC as it does contain changes that depends on a
> regmap patch. The patch is here [1] hoping it will get approved.
> 
> If it will be NACKed, I will have to rework this and revert one of the
> patch that makes use of the new regmap bulk implementation.
>

The regmap patch that this series depends on has been accepted but needs
some time to be put in linux-next. Considering the comments from the
code move, is it urgent to have the changes done or we can wait for the
regmap patch to get applied?

(this was asked from the regmap maintainer so here is the question)

> Anyway, this is needed ad ipq4019 SoC have an internal switch that is
> based on qca8k with very minor changes. The general function is equal.
> 
> Because of this we split the driver to common and specific code.
> 
> As the common function needs to be moved to a different file to be
> reused, we had to convert every remaining user of qca8k_read/write/rmw
> to regmap variant.
> We had also to generilized the special handling for the ethtool_stats
> function that makes use of the autocast mib. (ipq4019 will have a
> different tagger and use mmio so it could be quicker to use mmio instead
> of automib feature)
> And we had to convert the regmap read/write to bulk implementation to
> drop the special function that makes use of it. This will be compatible
> with ipq4019 and at the same time permits normal switch to use the eth
> mgmt way to send the entire ATU table read/write in one go.
> 
> (the bulk implementation could not be done when it was introduced as
> regmap didn't support at times bulk read/write without a bus)
> 
> [1] https://lore.kernel.org/lkml/20220715201032.19507-1-ansuelsmth@gmail.com/
> 
> Christian Marangi (4):
>   net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant
>   net: dsa: qca8k: convert to regmap read/write API
>   net: dsa: qca8k: rework mib autocast handling
>   net: dsa: qca8k: split qca8k in common and 8xxx specific code
> 
>  drivers/net/dsa/qca/Makefile                  |    1 +
>  drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} | 1638 +++--------------
>  drivers/net/dsa/qca/qca8k-common.c            | 1174 ++++++++++++
>  drivers/net/dsa/qca/qca8k.h                   |   61 +
>  4 files changed, 1463 insertions(+), 1411 deletions(-)
>  rename drivers/net/dsa/qca/{qca8k.c => qca8k-8xxx.c} (60%)
>  create mode 100644 drivers/net/dsa/qca/qca8k-common.c
> 
> -- 
> 2.36.1
> 

-- 
	Ansuel

Re: [net-next RFC PATCH 0/4] net: dsa: qca8k: code split for qca8k

[Vladimir Oltean]

On Mon, Jul 18, 2022 at 04:46:20PM +0200, Christian Marangi wrote:
> On Sat, Jul 16, 2022 at 07:49:54PM +0200, Christian Marangi wrote:
> > This is posted as an RFC as it does contain changes that depends on a
> > regmap patch. The patch is here [1] hoping it will get approved.
> > 
> > If it will be NACKed, I will have to rework this and revert one of the
> > patch that makes use of the new regmap bulk implementation.
> >
> 
> The regmap patch that this series depends on has been accepted but needs
> some time to be put in linux-next. Considering the comments from the
> code move, is it urgent to have the changes done or we can wait for the
> regmap patch to get applied?
> 
> (this was asked from the regmap maintainer so here is the question)

If I understand correctly, what you're saying is that the regmap_bulk_read()
change from patch 2/4 (net: dsa: qca8k: convert to regmap read/write API)
won't work correctly without the regmap dependency, and would introduce
a regression in the driver, right?

If so, I would prefer getting the patches merged linearly and not in
parallel, in other words either Mark provides a branch to pull into
net-next or you wait until the merge window opens and then closes, which
means a couple of weeks.

The fact that in linux-next things would work isn't enough, since on
net-next they would still be broken.

> > Anyway, this is needed ad ipq4019 SoC have an internal switch that is
> > based on qca8k with very minor changes. The general function is equal.
> > 
> > Because of this we split the driver to common and specific code.
> > 
> > As the common function needs to be moved to a different file to be
> > reused, we had to convert every remaining user of qca8k_read/write/rmw
> > to regmap variant.
> > We had also to generilized the special handling for the ethtool_stats
> > function that makes use of the autocast mib. (ipq4019 will have a
> > different tagger and use mmio so it could be quicker to use mmio instead
> > of automib feature)
> > And we had to convert the regmap read/write to bulk implementation to
> > drop the special function that makes use of it. This will be compatible
> > with ipq4019 and at the same time permits normal switch to use the eth
> > mgmt way to send the entire ATU table read/write in one go.
> > 
> > (the bulk implementation could not be done when it was introduced as
> > regmap didn't support at times bulk read/write without a bus)
> > 
> > [1] https://lore.kernel.org/lkml/20220715201032.19507-1-ansuelsmth@gmail.com/

Re: [net-next RFC PATCH 0/4] net: dsa: qca8k: code split for qca8k

[Christian Marangi]

On Mon, Jul 18, 2022 at 08:35:04PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 18, 2022 at 04:46:20PM +0200, Christian Marangi wrote:
> > On Sat, Jul 16, 2022 at 07:49:54PM +0200, Christian Marangi wrote:
> > > This is posted as an RFC as it does contain changes that depends on a
> > > regmap patch. The patch is here [1] hoping it will get approved.
> > > 
> > > If it will be NACKed, I will have to rework this and revert one of the
> > > patch that makes use of the new regmap bulk implementation.
> > >
> > 
> > The regmap patch that this series depends on has been accepted but needs
> > some time to be put in linux-next. Considering the comments from the
> > code move, is it urgent to have the changes done or we can wait for the
> > regmap patch to get applied?
> > 
> > (this was asked from the regmap maintainer so here is the question)
> 
> If I understand correctly, what you're saying is that the regmap_bulk_read()
> change from patch 2/4 (net: dsa: qca8k: convert to regmap read/write API)
> won't work correctly without the regmap dependency, and would introduce
> a regression in the driver, right?
>

Yes you are correct.

> If so, I would prefer getting the patches merged linearly and not in
> parallel, in other words either Mark provides a branch to pull into
> net-next or you wait until the merge window opens and then closes, which
> means a couple of weeks.
> 
> The fact that in linux-next things would work isn't enough, since on
> net-next they would still be broken.
> 

Ok, so I have to keep the qca8k special function. Is it a problem if I
keep the function and than later make the conversion when we have the
regmap dependency merged?

> > > Anyway, this is needed ad ipq4019 SoC have an internal switch that is
> > > based on qca8k with very minor changes. The general function is equal.
> > > 
> > > Because of this we split the driver to common and specific code.
> > > 
> > > As the common function needs to be moved to a different file to be
> > > reused, we had to convert every remaining user of qca8k_read/write/rmw
> > > to regmap variant.
> > > We had also to generilized the special handling for the ethtool_stats
> > > function that makes use of the autocast mib. (ipq4019 will have a
> > > different tagger and use mmio so it could be quicker to use mmio instead
> > > of automib feature)
> > > And we had to convert the regmap read/write to bulk implementation to
> > > drop the special function that makes use of it. This will be compatible
> > > with ipq4019 and at the same time permits normal switch to use the eth
> > > mgmt way to send the entire ATU table read/write in one go.
> > > 
> > > (the bulk implementation could not be done when it was introduced as
> > > regmap didn't support at times bulk read/write without a bus)
> > > 
> > > [1] https://lore.kernel.org/lkml/20220715201032.19507-1-ansuelsmth@gmail.com/

-- 
	Ansuel

Re: [net-next RFC PATCH 0/4] net: dsa: qca8k: code split for qca8k

[Vladimir Oltean]

On Mon, Jul 18, 2022 at 07:23:35PM +0200, Christian Marangi wrote:
> Ok, so I have to keep the qca8k special function. Is it a problem if I
> keep the function and than later make the conversion when we have the
> regmap dependency merged?

You mean to ask whether there's any problem if the common qca8k_fdb_read()
calls the specific qca8k_bulk_read as opposed to regmap_bulk_read()?

Well, no, considering that you don't yet support the switch with the
MMIO regmap, the common code is still "common" for the single switch
that the driver supports. You should be able to continue making progress
with qca8k_bulk_read() being called from common code (as long as you
leave a TODO comment or something, that it doesn't really belong there).

Re: [net-next RFC PATCH 0/4] net: dsa: qca8k: code split for qca8k

[Christian Marangi]

On Mon, Jul 18, 2022 at 09:15:59PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 18, 2022 at 07:23:35PM +0200, Christian Marangi wrote:
> > Ok, so I have to keep the qca8k special function. Is it a problem if I
> > keep the function and than later make the conversion when we have the
> > regmap dependency merged?
> 
> You mean to ask whether there's any problem if the common qca8k_fdb_read()
> calls the specific qca8k_bulk_read as opposed to regmap_bulk_read()?
> 
> Well, no, considering that you don't yet support the switch with the
> MMIO regmap, the common code is still "common" for the single switch
> that the driver supports. You should be able to continue making progress
> with qca8k_bulk_read() being called from common code (as long as you
> leave a TODO comment or something, that it doesn't really belong there).

Ok I will leave a TODO comment and switch to the new implemenation when
the regmap implementation will be available. Thanks for the
clarification on how to proceed.

-- 
	Ansuel