[net-next PATCH v4 00/13] Multiple improvement for qca8337 switch

[net-next PATCH v4 00/13] Multiple improvement for qca8337 switch

[Ansuel Smith]

This series is the final step of a long process of porting 80+ devices
to use the new qca8k driver instead of the hacky qca one based on never
merged swconfig platform.
Some background to justify all these additions.
QCA used a special binding to declare raw initval to set the swich. I
made a script to convert all these magic values and convert 80+ dts and
scan all the needed "unsupported regs". We find a baseline where we
manage to find the common and used regs so in theory hopefully we don't
have to add anymore things.
We discovered lots of things with this, especially about how differently
qca8327 works compared to qca8337.

In short, we found that qca8327 have some problem with suspend/resume for
their internal phy. It instead sets some dedicated regs that suspend the
phy without setting the standard bit. First 4 patch are to fix this.
There is also a patch about preferring master. This is directly from the
original driver and it seems to be needed to prevent some problem with
the pause frame.

Every ipq806x target sets the mac power sel and this specific reg
regulates the output voltage of the regulator. Without this some
instability can occur.

Some configuration (for some reason) swap mac6 with mac0. We add support
for this.
Also, we discovered that some device doesn't work at all with pll enabled
for sgmii line. In the original code this was based on the switch
revision. In later revision the pll regs were decided based on the switch
type (disabled for qca8327 and enabled for qca8337) but still some
device had that disabled in the initval regs.
Considering we found at least one qca8337 device that required pll
disabled to work (no traffic problem) we decided to introduce a binding
to enable pll and set it only with that.

Lastly, we add support for led open drain that require the power-on-sel
to set. Also, some device have only the power-on-sel set in the initval
so we add also support for that. This is needed for the correct function
of the switch leds.
Qca8327 have a special reg in the pws regs that set it to a reduced
48pin layout. This is needed or the switch doesn't work.

These are all the special configuration we find on all these devices that
are from various targets. Mostly ath79, ipq806x and bcm53xx.

Changes v4:
- Fix typo in SGMII falling edge about using PHY id instead of
  switch id

Changes v3:
- Drop phy patches (proposed separateley)
- Drop special pwr binding. Rework to ipq806x specific
- Better describe compatible and add serial print on switch chip
- Drop mac exchange. Rework falling edge and move it to mac_config
- Add support for port 6 cpu port. Drop hardcoded cpu port to port0
- Improve port stability with sgmii. QCA source have intenal delay also
  for sgmii
- Add warning with pll enabled on wrong configuration

Changes v2:
- Reword Documentation patch to dt-bindings
- Propose first 2 phy patch to net
- Better describe and add hint on how to use all the new
  bindings 
- Rework delay scan function and move to phylink mac_config
- Drop package48 wrong binding
- Introduce support for qca8328 switch
- Fix wrong binding name power-on-sel
- Return error on wrong config with led open drain and 
  ignore-power-on-sel not set

Ansuel Smith (13):
  net: dsa: qca8k: add mac_power_sel support
  net: dsa: qca8k: add support for sgmii falling edge
  dt-bindings: net: dsa: qca8k: Add MAC swap and clock phase properties
  drivers: net: dsa: qca8k: add support for cpu port 6
  dt-bindings: net: dsa: qca8k: Document support for CPU port 6
  net: dsa: qca8k: move rgmii delay detection to phylink mac_config
  net: dsa: qca8k: add explicit SGMII PLL enable
  dt-bindings: net: dsa: qca8k: Document qca,sgmii-enable-pll
  drivers: net: dsa: qca8k: add support for pws config reg
  dt-bindings: net: dsa: qca8k: document open drain binding
  drivers: net: dsa: qca8k: add support for QCA8328
  dt-bindings: net: dsa: qca8k: document support for qca8328
  drivers: net: dsa: qca8k: set internal delay also for sgmii

 .../devicetree/bindings/net/dsa/qca8k.txt     |  38 ++-
 drivers/net/dsa/qca8k.c                       | 285 +++++++++++++-----
 drivers/net/dsa/qca8k.h                       |  21 +-
 3 files changed, 262 insertions(+), 82 deletions(-)

-- 
2.32.0

[net-next PATCH v4 01/13] net: dsa: qca8k: add mac_power_sel support

[Ansuel Smith]

Add missing mac power sel support needed for ipq8064/5 SoC that require
1.8v for the internal regulator port instead of the default 1.5v.
If other device needs this, consider adding a dedicated binding to
support this.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index bda5a9bf4f52..a892b897cd0d 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -950,6 +950,33 @@ qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)
 	return 0;
 }
 
+static int
+qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
+{
+	u32 mask = 0;
+	int ret = 0;
+
+	/* SoC specific settings for ipq8064.
+	 * If more device require this consider adding
+	 * a dedicated binding.
+	 */
+	if (of_machine_is_compatible("qcom,ipq8064"))
+		mask |= QCA8K_MAC_PWR_RGMII0_1_8V;
+
+	/* SoC specific settings for ipq8065 */
+	if (of_machine_is_compatible("qcom,ipq8065"))
+		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);
+	}
+
+	return ret;
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -979,6 +1006,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_mac_pwr_sel(priv);
+	if (ret)
+		return ret;
+
 	/* Enable CPU Port */
 	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index ed3b05ad6745..fc7db94cc0c9 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -100,6 +100,11 @@
 #define   QCA8K_SGMII_MODE_CTRL_PHY			(1 << 22)
 #define   QCA8K_SGMII_MODE_CTRL_MAC			(2 << 22)
 
+/* MAC_PWR_SEL registers */
+#define QCA8K_REG_MAC_PWR_SEL				0x0e4
+#define   QCA8K_MAC_PWR_RGMII1_1_8V			BIT(18)
+#define   QCA8K_MAC_PWR_RGMII0_1_8V			BIT(19)
+
 /* EEE control registers */
 #define QCA8K_REG_EEE_CTRL				0x100
 #define  QCA8K_REG_EEE_CTRL_LPI_EN(_i)			((_i + 1) * 2)
-- 
2.32.0

[net-next PATCH v4 02/13] net: dsa: qca8k: add support for sgmii falling edge

[Ansuel Smith]

Add support for this in the qca8k driver. Also add support for SGMII
rx/tx clock falling edge. This is only present for pad0, pad5 and
pad6 have these bit reserved from Documentation. Add a comment that this
is hardcoded to PAD0 as qca8327/28/34/37 have an unique sgmii line and
setting falling in port0 applies to both configuration with sgmii used
for port0 or port6.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a892b897cd0d..3e4a12d6d61c 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1172,6 +1172,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
+	struct dsa_port *dp;
 	u32 reg, val;
 	int ret;
 
@@ -1240,6 +1241,8 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
+		dp = dsa_to_port(ds, port);
+
 		/* Enable SGMII on the port */
 		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
 
@@ -1274,6 +1277,28 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		}
 
 		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
+
+		/* For qca8327/qca8328/qca8334/qca8338 sgmii is unique and
+		 * falling edge is set writing in the PORT0 PAD reg
+		 */
+		if (priv->switch_id == QCA8K_ID_QCA8327 ||
+		    priv->switch_id == QCA8K_ID_QCA8337)
+			reg = QCA8K_REG_PORT0_PAD_CTRL;
+
+		val = 0;
+
+		/* SGMII Clock phase configuration */
+		if (of_property_read_bool(dp->dn, "qca,sgmii-rxclk-falling-edge"))
+			val |= QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE;
+
+		if (of_property_read_bool(dp->dn, "qca,sgmii-txclk-falling-edge"))
+			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);
 		break;
 	default:
 		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index fc7db94cc0c9..3fded69a6839 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -35,6 +35,9 @@
 #define   QCA8K_MASK_CTRL_DEVICE_ID_MASK		GENMASK(15, 8)
 #define   QCA8K_MASK_CTRL_DEVICE_ID(x)			((x) >> 8)
 #define QCA8K_REG_PORT0_PAD_CTRL			0x004
+#define   QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG		BIT(31)
+#define   QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE	BIT(19)
+#define   QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE	BIT(18)
 #define QCA8K_REG_PORT5_PAD_CTRL			0x008
 #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
 #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
-- 
2.32.0

[net-next PATCH v4 03/13] dt-bindings: net: dsa: qca8k: Add MAC swap and clock phase properties

[Ansuel Smith]

Add names and descriptions of additional PORT0_PAD_CTRL properties.
qca,sgmii-(rx|tx)clk-falling-edge are for setting the respective clock
phase to failling edge.

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

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index 8c73f67c43ca..cc214e655442 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -37,6 +37,10 @@ A CPU port node has the following optional node:
                           managed entity. See
                           Documentation/devicetree/bindings/net/fixed-link.txt
                           for details.
+- qca,sgmii-rxclk-falling-edge: Set the receive clock phase to falling edge.
+                                Mostly used in qca8327 with CPU port 0 set to
+                                sgmii.
+- qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge.
 
 For QCA8K the 'fixed-link' sub-node supports only the following properties:
 
-- 
2.32.0

[net-next PATCH v4 04/13] drivers: net: dsa: qca8k: add support for cpu port 6

[Ansuel Smith]

Currently CPU port is always hardcoded to port 0. This switch have 2 CPU
port. The original intention of this driver seems to be use the
mac06_exchange bit to swap MAC0 with MAC6 in the strange configuration
where device have connected only the CPU port 6. To skip the
introduction of a new binding, rework the driver to address the
secondary CPU port as primary and drop any reference of hardcoded port.
With configuration of mac06 exchange, just skip the definition of port0
and define the CPU port as a secondary. The driver will autoconfigure
the switch to use that as the primary CPU port.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 3e4a12d6d61c..df0a622acdd7 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -977,17 +977,34 @@ qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
 	return ret;
 }
 
+static int qca8k_find_cpu_port(struct dsa_switch *ds)
+{
+	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+
+	/* Find the connected cpu port. Valid port are 0 or 6 */
+	if (dsa_is_cpu_port(ds, 0))
+		return 0;
+
+	dev_dbg(priv->dev, "port 0 is not the CPU port. Checking port 6");
+
+	if (dsa_is_cpu_port(ds, 6))
+		return 6;
+
+	return -EINVAL;
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	u8 cpu_port;
 	int ret, i;
 	u32 mask;
 
-	/* Make sure that port 0 is the cpu port */
-	if (!dsa_is_cpu_port(ds, 0)) {
-		dev_err(priv->dev, "port 0 is not the CPU port");
-		return -EINVAL;
+	cpu_port = qca8k_find_cpu_port(ds);
+	if (cpu_port < 0) {
+		dev_err(priv->dev, "No cpu port configured in both cpu port0 and port6");
+		return cpu_port;
 	}
 
 	mutex_init(&priv->reg_mutex);
@@ -1024,7 +1041,7 @@ qca8k_setup(struct dsa_switch *ds)
 		dev_warn(priv->dev, "mib init failed");
 
 	/* Enable QCA header mode on the cpu port */
-	ret = qca8k_write(priv, QCA8K_REG_PORT_HDR_CTRL(QCA8K_CPU_PORT),
+	ret = qca8k_write(priv, QCA8K_REG_PORT_HDR_CTRL(cpu_port),
 			  QCA8K_PORT_HDR_CTRL_ALL << QCA8K_PORT_HDR_CTRL_TX_S |
 			  QCA8K_PORT_HDR_CTRL_ALL << QCA8K_PORT_HDR_CTRL_RX_S);
 	if (ret) {
@@ -1046,10 +1063,10 @@ qca8k_setup(struct dsa_switch *ds)
 
 	/* Forward all unknown frames to CPU port for Linux processing */
 	ret = qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
-			  BIT(0) << QCA8K_GLOBAL_FW_CTRL1_IGMP_DP_S |
-			  BIT(0) << QCA8K_GLOBAL_FW_CTRL1_BC_DP_S |
-			  BIT(0) << QCA8K_GLOBAL_FW_CTRL1_MC_DP_S |
-			  BIT(0) << QCA8K_GLOBAL_FW_CTRL1_UC_DP_S);
+			  BIT(cpu_port) << QCA8K_GLOBAL_FW_CTRL1_IGMP_DP_S |
+			  BIT(cpu_port) << QCA8K_GLOBAL_FW_CTRL1_BC_DP_S |
+			  BIT(cpu_port) << QCA8K_GLOBAL_FW_CTRL1_MC_DP_S |
+			  BIT(cpu_port) << QCA8K_GLOBAL_FW_CTRL1_UC_DP_S);
 	if (ret)
 		return ret;
 
@@ -1057,7 +1074,7 @@ 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(QCA8K_CPU_PORT),
+			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(cpu_port),
 					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
 			if (ret)
 				return ret;
@@ -1069,7 +1086,7 @@ qca8k_setup(struct dsa_switch *ds)
 
 			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
 					QCA8K_PORT_LOOKUP_MEMBER,
-					BIT(QCA8K_CPU_PORT));
+					BIT(cpu_port));
 			if (ret)
 				return ret;
 
@@ -1578,9 +1595,12 @@ static int
 qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	int port_mask = BIT(QCA8K_CPU_PORT);
+	int port_mask, cpu_port;
 	int i, ret;
 
+	cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
+	port_mask = BIT(cpu_port);
+
 	for (i = 1; i < QCA8K_NUM_PORTS; i++) {
 		if (dsa_to_port(ds, i)->bridge_dev != br)
 			continue;
@@ -1607,7 +1627,9 @@ static void
 qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	int i;
+	int cpu_port, i;
+
+	cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
 
 	for (i = 1; i < QCA8K_NUM_PORTS; i++) {
 		if (dsa_to_port(ds, i)->bridge_dev != br)
@@ -1624,7 +1646,7 @@ qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br)
 	 * this port
 	 */
 	qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
-		  QCA8K_PORT_LOOKUP_MEMBER, BIT(QCA8K_CPU_PORT));
+		  QCA8K_PORT_LOOKUP_MEMBER, BIT(cpu_port));
 }
 
 static int
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 3fded69a6839..5df0f0ef6526 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -24,8 +24,6 @@
 
 #define QCA8K_NUM_FDB_RECORDS				2048
 
-#define QCA8K_CPU_PORT					0
-
 #define QCA8K_PORT_VID_DEF				1
 
 /* Global control registers */
-- 
2.32.0

[net-next PATCH v4 05/13] dt-bindings: net: dsa: qca8k: Document support for CPU port 6

[Ansuel Smith]

The switch now support CPU port to be set 6 instead of be hardcoded to
0. Document support for it and describe logic selection.

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

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index cc214e655442..aeb206556f54 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -29,7 +29,11 @@ the mdio MASTER is used as communication.
 Don't use mixed external and internal mdio-bus configurations, as this is
 not supported by the hardware.
 
-The CPU port of this switch is always port 0.
+This switch support 2 CPU port. Normally and advised configuration is with
+CPU port set to port 0. It is also possible to set the CPU port to port 6
+if the device requires it. The driver will configure the switch to the defined
+port. With both CPU port declared the first CPU port is selected as primary
+and the secondary CPU ignored.
 
 A CPU port node has the following optional node:
 
-- 
2.32.0

[net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

[Ansuel Smith]

Future proof commit. This switch have 2 CPU port and one valid
configuration is first CPU port set to sgmii and second CPU port set to
regmii-id. The current implementation detects delay only for CPU port
zero set to rgmii and doesn't count any delay set in a secondary CPU
port. Drop the current delay scan function and move it to the phylink
mac_config to generilize and implicitly add support for secondary CPU
port set to rgmii-id.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index df0a622acdd7..126f20b0b94c 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -888,68 +888,6 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 	return 0;
 }
 
-static int
-qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)
-{
-	struct device_node *port_dn;
-	phy_interface_t mode;
-	struct dsa_port *dp;
-	u32 val;
-
-	/* CPU port is already checked */
-	dp = dsa_to_port(priv->ds, 0);
-
-	port_dn = dp->dn;
-
-	/* Check if port 0 is set to the correct type */
-	of_get_phy_mode(port_dn, &mode);
-	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
-	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
-	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
-		return 0;
-	}
-
-	switch (mode) {
-	case PHY_INTERFACE_MODE_RGMII_ID:
-	case PHY_INTERFACE_MODE_RGMII_RXID:
-		if (of_property_read_u32(port_dn, "rx-internal-delay-ps", &val))
-			val = 2;
-		else
-			/* Switch regs accept value in ns, convert ps to ns */
-			val = val / 1000;
-
-		if (val > QCA8K_MAX_DELAY) {
-			dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
-			val = 3;
-		}
-
-		priv->rgmii_rx_delay = val;
-		/* Stop here if we need to check only for rx delay */
-		if (mode != PHY_INTERFACE_MODE_RGMII_ID)
-			break;
-
-		fallthrough;
-	case PHY_INTERFACE_MODE_RGMII_TXID:
-		if (of_property_read_u32(port_dn, "tx-internal-delay-ps", &val))
-			val = 1;
-		else
-			/* Switch regs accept value in ns, convert ps to ns */
-			val = val / 1000;
-
-		if (val > QCA8K_MAX_DELAY) {
-			dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
-			val = 3;
-		}
-
-		priv->rgmii_tx_delay = val;
-		break;
-	default:
-		return 0;
-	}
-
-	return 0;
-}
-
 static int
 qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
 {
@@ -1019,10 +957,6 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	ret = qca8k_setup_of_rgmii_delay(priv);
-	if (ret)
-		return ret;
-
 	ret = qca8k_setup_mac_pwr_sel(priv);
 	if (ret)
 		return ret;
@@ -1190,7 +1124,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 {
 	struct qca8k_priv *priv = ds->priv;
 	struct dsa_port *dp;
-	u32 reg, val;
+	u32 reg, val, delay;
 	int ret;
 
 	switch (port) {
@@ -1241,17 +1175,50 @@ 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:
-		/* RGMII_ID needs internal delay. This is enabled through
-		 * PORT5_PAD_CTRL for all ports, rather than individual port
-		 * registers
+		dp = dsa_to_port(ds, port);
+		val = QCA8K_PORT_PAD_RGMII_EN;
+
+		if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+		    state->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+			if (of_property_read_u32(dp->dn, "tx-internal-delay-ps", &delay))
+				delay = 1;
+			else
+				/* Switch regs accept value in ns, convert ps to ns */
+				delay = delay / 1000;
+
+			if (delay > QCA8K_MAX_DELAY) {
+				dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
+				delay = 3;
+			}
+
+			val |= QCA8K_PORT_PAD_RGMII_TX_DELAY(delay) |
+			       QCA8K_PORT_PAD_RGMII_TX_DELAY_EN;
+		}
+
+		if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+		    state->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+			if (of_property_read_u32(dp->dn, "rx-internal-delay-ps", &delay))
+				delay = 2;
+			else
+				/* Switch regs accept value in ns, convert ps to ns */
+				delay = delay / 1000;
+
+			if (delay > QCA8K_MAX_DELAY) {
+				dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
+				delay = 3;
+			}
+
+			val |= QCA8K_PORT_PAD_RGMII_RX_DELAY(delay) |
+			       QCA8K_PORT_PAD_RGMII_RX_DELAY_EN;
+		}
+
+		/* Set RGMII delay based on the selected values */
+		qca8k_write(priv, reg, val);
+
+		/* QCA8337 requires to set rgmii rx delay for all ports.
+		 * This is enabled through PORT5_PAD_CTRL for all ports,
+		 * rather than individual port registers.
 		 */
-		qca8k_write(priv, reg,
-			    QCA8K_PORT_PAD_RGMII_EN |
-			    QCA8K_PORT_PAD_RGMII_TX_DELAY(priv->rgmii_tx_delay) |
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY(priv->rgmii_rx_delay) |
-			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
-		/* QCA8337 requires to set rgmii rx delay */
 		if (priv->switch_id == QCA8K_ID_QCA8337)
 			qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
 				    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 5df0f0ef6526..a790b27bc310 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -259,8 +259,6 @@ struct qca8k_match_data {
 struct qca8k_priv {
 	u8 switch_id;
 	u8 switch_revision;
-	u8 rgmii_tx_delay;
-	u8 rgmii_rx_delay;
 	bool legacy_phy_port_mapping;
 	struct regmap *regmap;
 	struct mii_bus *bus;
-- 
2.32.0

[net-next PATCH v4 07/13] net: dsa: qca8k: add explicit SGMII PLL enable

[Ansuel Smith]

Support enabling PLL on the SGMII CPU port. Some device require this
special configuration or no traffic is transmitted and the switch
doesn't work at all. A dedicated binding is added to the CPU node
port to apply the correct reg on mac config.
Fail to correctly configure sgmii with qca8327 switch and warn if pll is
used on qca8337 with a revision greater than 1.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 126f20b0b94c..afdfbcbbdb52 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1245,8 +1245,20 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (ret)
 			return;
 
-		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
-			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
+		val |= QCA8K_SGMII_EN_SD;
+
+		if (of_property_read_bool(dp->dn, "qca,sgmii-enable-pll")) {
+			if (priv->switch_id == QCA8K_ID_QCA8327) {
+				dev_err(priv->dev, "SGMII PLL should NOT be enabled for qca8327. Aborting enabling");
+				return;
+			}
+
+			if (priv->switch_revision < 2)
+				dev_warn(priv->dev, "SGMII PLL should NOT be enabled for qca8337 with revision 2 or more.");
+
+			val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
+			       QCA8K_SGMII_EN_TX;
+		}
 
 		if (dsa_is_cpu_port(ds, port)) {
 			/* CPU port, we're talking to the CPU MAC, be a PHY */
-- 
2.32.0

[net-next PATCH v4 08/13] dt-bindings: net: dsa: qca8k: Document qca,sgmii-enable-pll

[Ansuel Smith]

Document qca,sgmii-enable-pll binding used in the CPU nodes to
enable SGMII PLL on MAC config.

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

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index aeb206556f54..05a8ddfb5483 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -45,6 +45,16 @@ A CPU port node has the following optional node:
                                 Mostly used in qca8327 with CPU port 0 set to
                                 sgmii.
 - qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge.
+- qca,sgmii-enable-pll  : For SGMII CPU port, explicitly enable PLL, TX and RX
+                          chain along with Signal Detection.
+                          This should NOT be enabled for qca8327. If enabled with
+                          qca8327 the sgmii port won't correctly init and an err
+                          is printed.
+                          This can be required for qca8337 switch with revision 2.
+                          A warning is displayed when used with revision greater
+                          2.
+                          With CPU port set to sgmii and qca8337 it is advised
+                          to set this unless a communication problem is observed.
 
 For QCA8K the 'fixed-link' sub-node supports only the following properties:
 
-- 
2.32.0

[net-next PATCH v4 09/13] drivers: net: dsa: qca8k: add support for pws config reg

[Ansuel Smith]

Some qca8327 switch require to force the ignore of power on sel
strapping. Some switch require to set the led open drain mode in regs
instead of using strapping. While most of the device implements this
using the correct way using pin strapping, there are still some broken
device that require to be set using sw regs.
Introduce a new binding and support these special configuration.
As led open drain require to ignore pin strapping to work, the probe
fails with EINVAL error with incorrect configuration.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index afdfbcbbdb52..5091837cb969 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -931,6 +931,41 @@ static int qca8k_find_cpu_port(struct dsa_switch *ds)
 	return -EINVAL;
 }
 
+static int
+qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
+{
+	struct device_node *node = priv->dev->of_node;
+	u32 val = 0;
+	int ret;
+
+	/* QCA8327 require to set to the correct mode.
+	 * His bigger brother QCA8328 have the 172 pin layout.
+	 * Should be applied by default but we set this just to make sure.
+	 */
+	if (priv->switch_id == QCA8K_ID_QCA8327) {
+		ret = qca8k_rmw(priv, QCA8K_REG_PWS, QCA8327_PWS_PACKAGE148_EN,
+				QCA8327_PWS_PACKAGE148_EN);
+		if (ret)
+			return ret;
+	}
+
+	if (of_property_read_bool(node, "qca,ignore-power-on-sel"))
+		val |= QCA8K_PWS_POWER_ON_SEL;
+
+	if (of_property_read_bool(node, "qca,led-open-drain")) {
+		if (!(val & QCA8K_PWS_POWER_ON_SEL)) {
+			dev_err(priv->dev, "qca,led-open-drain require qca,ignore-power-on-sel to be set.");
+			return -EINVAL;
+		}
+
+		val |= QCA8K_PWS_LED_OPEN_EN_CSR;
+	}
+
+	return qca8k_rmw(priv, QCA8K_REG_PWS,
+			QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL,
+			val);
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -957,6 +992,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_of_pws_reg(priv);
+	if (ret)
+		return ret;
+
 	ret = qca8k_setup_mac_pwr_sel(priv);
 	if (ret)
 		return ret;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index a790b27bc310..535a4515e7b9 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -46,6 +46,12 @@
 #define   QCA8K_MAX_DELAY				3
 #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
 #define QCA8K_REG_PWS					0x010
+#define   QCA8K_PWS_POWER_ON_SEL			BIT(31)
+/* This reg is only valid for QCA832x and toggle the package
+ * type from 176 pin (by default) to 148 pin used on QCA8327
+ */
+#define   QCA8327_PWS_PACKAGE148_EN			BIT(30)
+#define   QCA8K_PWS_LED_OPEN_EN_CSR			BIT(24)
 #define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
 #define QCA8K_REG_MODULE_EN				0x030
 #define   QCA8K_MODULE_EN_MIB				BIT(0)
-- 
2.32.0

[net-next PATCH v4 10/13] dt-bindings: net: dsa: qca8k: document open drain binding

[Ansuel Smith]

Document new binding qca,power_on_sel used to enable Power-on-strapping
select reg and qca,led_open_drain to set led to open drain mode.

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

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index 05a8ddfb5483..71cd45818430 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -13,6 +13,17 @@ Required properties:
 Optional properties:
 
 - reset-gpios: GPIO to be used to reset the whole device
+- qca,ignore-power-on-sel: Ignore power on pin strapping to configure led open
+                           drain or eeprom presence. This is needed for broken
+                           device that have wrong configuration or when the oem
+                           decided to not use pin strapping and fallback to sw
+                           regs.
+- qca,led-open-drain: Set leds to open-drain mode. This require the
+                      qca,ignore-power-on-sel to be set or the driver will fail
+                      to probe. This is needed if the oem doesn't use pin
+                      strapping to set this mode and prefer to set it using sw
+                      regs. The pin strapping related to led open drain mode is
+                      the pin B68 for QCA832x and B49 for QCA833x
 
 Subnodes:
 
-- 
2.32.0

[net-next PATCH v4 11/13] drivers: net: dsa: qca8k: add support for QCA8328

[Ansuel Smith]

QCA8328 switch is the bigger brother of the qca8327. Same regs different
chip. Change the function to set the correct pin layout and introduce a
new match_data to differentiate the 2 switch as they have the same ID
and their internal PHY have the same ID.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 5091837cb969..947346511514 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -935,6 +935,7 @@ static int
 qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
 {
 	struct device_node *node = priv->dev->of_node;
+	const struct qca8k_match_data *data;
 	u32 val = 0;
 	int ret;
 
@@ -943,8 +944,14 @@ qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
 	 * Should be applied by default but we set this just to make sure.
 	 */
 	if (priv->switch_id == QCA8K_ID_QCA8327) {
+		data = of_device_get_match_data(priv->dev);
+
+		/* Set the correct package of 148 pin for QCA8327 */
+		if (data->reduced_package)
+			val |= QCA8327_PWS_PACKAGE148_EN;
+
 		ret = qca8k_rmw(priv, QCA8K_REG_PWS, QCA8327_PWS_PACKAGE148_EN,
-				QCA8327_PWS_PACKAGE148_EN);
+				val);
 		if (ret)
 			return ret;
 	}
@@ -2018,7 +2025,12 @@ static int qca8k_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(qca8k_pm_ops,
 			 qca8k_suspend, qca8k_resume);
 
-static const struct qca8k_match_data qca832x = {
+static const struct qca8k_match_data qca8327 = {
+	.id = QCA8K_ID_QCA8327,
+	.reduced_package = true,
+};
+
+static const struct qca8k_match_data qca8328 = {
 	.id = QCA8K_ID_QCA8327,
 };
 
@@ -2027,7 +2039,8 @@ static const struct qca8k_match_data qca833x = {
 };
 
 static const struct of_device_id qca8k_of_match[] = {
-	{ .compatible = "qca,qca8327", .data = &qca832x },
+	{ .compatible = "qca,qca8327", .data = &qca8327 },
+	{ .compatible = "qca,qca8328", .data = &qca8328 },
 	{ .compatible = "qca,qca8334", .data = &qca833x },
 	{ .compatible = "qca,qca8337", .data = &qca833x },
 	{ /* sentinel */ },
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 535a4515e7b9..c032db5e0d41 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -260,6 +260,7 @@ struct ar8xxx_port_status {
 
 struct qca8k_match_data {
 	u8 id;
+	bool reduced_package;
 };
 
 struct qca8k_priv {
-- 
2.32.0

[net-next PATCH v4 12/13] dt-bindings: net: dsa: qca8k: document support for qca8328

[Ansuel Smith]

QCA8328 is the bigger brother of qca8327. Document the new compatible
binding and add some information to understand the various switch
compatible.

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

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index 71cd45818430..e6b580d815c2 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -3,9 +3,10 @@
 Required properties:
 
 - compatible: should be one of:
-    "qca,qca8327"
-    "qca,qca8334"
-    "qca,qca8337"
+    "qca,qca8328": referenced as AR8328(N)-AK1(A/B) QFN 176 pin package
+    "qca,qca8327": referenced as AR8327(N)-AL1A DR-QFN 148 pin package
+    "qca,qca8334": referenced as QCA8334-AL3C QFN 88 pin package
+    "qca,qca8337": referenced as QCA8337N-AL3(B/C) DR-QFN 148 pin package
 
 - #size-cells: must be 0
 - #address-cells: must be 1
-- 
2.32.0

[net-next PATCH v4 13/13] drivers: net: dsa: qca8k: set internal delay also for sgmii

[Ansuel Smith]

QCA original code report port instability and sa that SGMII also require
to set internal delay. Generalize the rgmii delay function and apply the
advised value if they are not defined in DT.

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

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 947346511514..444894d108ee 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1164,13 +1164,67 @@ qca8k_setup(struct dsa_switch *ds)
 	return 0;
 }
 
+static void
+qca8k_mac_config_setup_internal_delay(struct qca8k_priv *priv, struct dsa_port *dp,
+				      u32 reg, const struct phylink_link_state *state)
+{
+	u32 delay, val = 0;
+	int ret;
+
+	if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    state->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
+	    state->interface == PHY_INTERFACE_MODE_SGMII) {
+		if (of_property_read_u32(dp->dn, "tx-internal-delay-ps", &delay))
+			delay = 1;
+		else
+			/* Switch regs accept value in ns, convert ps to ns */
+			delay = delay / 1000;
+
+		if (delay > QCA8K_MAX_DELAY) {
+			dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
+			delay = 3;
+		}
+
+		val |= QCA8K_PORT_PAD_RGMII_TX_DELAY(delay) |
+			QCA8K_PORT_PAD_RGMII_TX_DELAY_EN;
+	}
+
+	if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    state->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
+	    state->interface == PHY_INTERFACE_MODE_SGMII) {
+		if (of_property_read_u32(dp->dn, "rx-internal-delay-ps", &delay))
+			delay = 2;
+		else
+			/* Switch regs accept value in ns, convert ps to ns */
+			delay = delay / 1000;
+
+		if (delay > QCA8K_MAX_DELAY) {
+			dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
+			delay = 3;
+		}
+
+		val |= QCA8K_PORT_PAD_RGMII_RX_DELAY(delay) |
+			QCA8K_PORT_PAD_RGMII_RX_DELAY_EN;
+	}
+
+	/* Set RGMII delay based on the selected values */
+	ret = qca8k_rmw(priv, reg,
+			QCA8K_PORT_PAD_RGMII_TX_DELAY_MASK |
+			QCA8K_PORT_PAD_RGMII_TX_DELAY_MASK |
+			QCA8K_PORT_PAD_RGMII_RX_DELAY_MASK |
+			QCA8K_PORT_PAD_RGMII_RX_DELAY_EN,
+			val);
+	if (ret)
+		dev_err(priv->dev, "Failed to set internal delay for CPU port %d", dp->index);
+}
+
 static void
 qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
 	struct dsa_port *dp;
-	u32 reg, val, delay;
+	u32 reg, val;
 	int ret;
 
 	switch (port) {
@@ -1222,44 +1276,11 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 		dp = dsa_to_port(ds, port);
-		val = QCA8K_PORT_PAD_RGMII_EN;
-
-		if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-		    state->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
-			if (of_property_read_u32(dp->dn, "tx-internal-delay-ps", &delay))
-				delay = 1;
-			else
-				/* Switch regs accept value in ns, convert ps to ns */
-				delay = delay / 1000;
-
-			if (delay > QCA8K_MAX_DELAY) {
-				dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
-				delay = 3;
-			}
-
-			val |= QCA8K_PORT_PAD_RGMII_TX_DELAY(delay) |
-			       QCA8K_PORT_PAD_RGMII_TX_DELAY_EN;
-		}
 
-		if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-		    state->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
-			if (of_property_read_u32(dp->dn, "rx-internal-delay-ps", &delay))
-				delay = 2;
-			else
-				/* Switch regs accept value in ns, convert ps to ns */
-				delay = delay / 1000;
-
-			if (delay > QCA8K_MAX_DELAY) {
-				dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
-				delay = 3;
-			}
-
-			val |= QCA8K_PORT_PAD_RGMII_RX_DELAY(delay) |
-			       QCA8K_PORT_PAD_RGMII_RX_DELAY_EN;
-		}
+		qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
 
-		/* Set RGMII delay based on the selected values */
-		qca8k_write(priv, reg, val);
+		/* Configure rgmii delay from dp or taking advised values */
+		qca8k_mac_config_setup_internal_delay(priv, dp, reg, state);
 
 		/* QCA8337 requires to set rgmii rx delay for all ports.
 		 * This is enabled through PORT5_PAD_CTRL for all ports,
@@ -1341,6 +1362,13 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 					QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE |
 					QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
 					val);
+
+		/* From original code is reported port instability as SGMII also
+		 * require delay set. Apply advised values here or take them from DT.
+		 */
+		if (state->interface == PHY_INTERFACE_MODE_SGMII)
+			qca8k_mac_config_setup_internal_delay(priv, dp, reg, state);
+
 		break;
 	default:
 		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index c032db5e0d41..92867001cc34 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -39,7 +39,9 @@
 #define QCA8K_REG_PORT5_PAD_CTRL			0x008
 #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
 #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
+#define   QCA8K_PORT_PAD_RGMII_TX_DELAY_MASK		GENMASK(23, 22)
 #define   QCA8K_PORT_PAD_RGMII_TX_DELAY(x)		((x) << 22)
+#define   QCA8K_PORT_PAD_RGMII_RX_DELAY_MASK		GENMASK(21, 20)
 #define   QCA8K_PORT_PAD_RGMII_RX_DELAY(x)		((x) << 20)
 #define	  QCA8K_PORT_PAD_RGMII_TX_DELAY_EN		BIT(25)
 #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN		BIT(24)
-- 
2.32.0

Re: [net-next PATCH v4 02/13] net: dsa: qca8k: add support for sgmii falling edge

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 01:15:45PM +0200, Ansuel Smith wrote:
> Add support for this in the qca8k driver. Also add support for SGMII
> rx/tx clock falling edge. This is only present for pad0, pad5 and
> pad6 have these bit reserved from Documentation. Add a comment that this
> is hardcoded to PAD0 as qca8327/28/34/37 have an unique sgmii line and
> setting falling in port0 applies to both configuration with sgmii used
> for port0 or port6.
> 
> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 25 +++++++++++++++++++++++++
>  drivers/net/dsa/qca8k.h |  3 +++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a892b897cd0d..3e4a12d6d61c 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -1172,6 +1172,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  			 const struct phylink_link_state *state)
>  {
>  	struct qca8k_priv *priv = ds->priv;
> +	struct dsa_port *dp;
>  	u32 reg, val;
>  	int ret;
>  
> @@ -1240,6 +1241,8 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		break;
>  	case PHY_INTERFACE_MODE_SGMII:
>  	case PHY_INTERFACE_MODE_1000BASEX:
> +		dp = dsa_to_port(ds, port);
> +
>  		/* Enable SGMII on the port */
>  		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
>  
> @@ -1274,6 +1277,28 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		}
>  
>  		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
> +
> +		/* For qca8327/qca8328/qca8334/qca8338 sgmii is unique and
> +		 * falling edge is set writing in the PORT0 PAD reg
> +		 */
> +		if (priv->switch_id == QCA8K_ID_QCA8327 ||
> +		    priv->switch_id == QCA8K_ID_QCA8337)
> +			reg = QCA8K_REG_PORT0_PAD_CTRL;
> +
> +		val = 0;
> +
> +		/* SGMII Clock phase configuration */
> +		if (of_property_read_bool(dp->dn, "qca,sgmii-rxclk-falling-edge"))

I would strongly recommend that you stop accessing dp->dn and add your
own device tree parsing function during probe time. It is also a runtime
invariant, there is no reason to read the device tree during each mac_config.

> +			val |= QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE;
> +
> +		if (of_property_read_bool(dp->dn, "qca,sgmii-txclk-falling-edge"))
> +			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);
>  		break;
>  	default:
>  		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index fc7db94cc0c9..3fded69a6839 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -35,6 +35,9 @@
>  #define   QCA8K_MASK_CTRL_DEVICE_ID_MASK		GENMASK(15, 8)
>  #define   QCA8K_MASK_CTRL_DEVICE_ID(x)			((x) >> 8)
>  #define QCA8K_REG_PORT0_PAD_CTRL			0x004
> +#define   QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG		BIT(31)

QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG is not used in this patch.

> +#define   QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE	BIT(19)
> +#define   QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE	BIT(18)
>  #define QCA8K_REG_PORT5_PAD_CTRL			0x008
>  #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
>  #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
> -- 
> 2.32.0
> 

Re: [net-next PATCH v4 03/13] dt-bindings: net: dsa: qca8k: Add MAC swap and clock phase properties

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 01:15:46PM +0200, Ansuel Smith wrote:
> Add names and descriptions of additional PORT0_PAD_CTRL properties.
> qca,sgmii-(rx|tx)clk-falling-edge are for setting the respective clock
> phase to failling edge.
> 
> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> index 8c73f67c43ca..cc214e655442 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> @@ -37,6 +37,10 @@ A CPU port node has the following optional node:
>                            managed entity. See
>                            Documentation/devicetree/bindings/net/fixed-link.txt
>                            for details.
> +- qca,sgmii-rxclk-falling-edge: Set the receive clock phase to falling edge.
> +                                Mostly used in qca8327 with CPU port 0 set to
> +                                sgmii.
> +- qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge.
>  
>  For QCA8K the 'fixed-link' sub-node supports only the following properties:
>  
> -- 
> 2.32.0
> 

Must first document, then use.
Also, would you care converting qca8k.txt to qca8k.yaml?

Re: [net-next PATCH v4 02/13] net: dsa: qca8k: add support for sgmii falling edge

[Ansuel Smith]

On Sun, Oct 10, 2021 at 03:05:26PM +0300, Vladimir Oltean wrote:
> On Sun, Oct 10, 2021 at 01:15:45PM +0200, Ansuel Smith wrote:
> > Add support for this in the qca8k driver. Also add support for SGMII
> > rx/tx clock falling edge. This is only present for pad0, pad5 and
> > pad6 have these bit reserved from Documentation. Add a comment that this
> > is hardcoded to PAD0 as qca8327/28/34/37 have an unique sgmii line and
> > setting falling in port0 applies to both configuration with sgmii used
> > for port0 or port6.
> > 
> > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 25 +++++++++++++++++++++++++
> >  drivers/net/dsa/qca8k.h |  3 +++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index a892b897cd0d..3e4a12d6d61c 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -1172,6 +1172,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  			 const struct phylink_link_state *state)
> >  {
> >  	struct qca8k_priv *priv = ds->priv;
> > +	struct dsa_port *dp;
> >  	u32 reg, val;
> >  	int ret;
> >  
> > @@ -1240,6 +1241,8 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  		break;
> >  	case PHY_INTERFACE_MODE_SGMII:
> >  	case PHY_INTERFACE_MODE_1000BASEX:
> > +		dp = dsa_to_port(ds, port);
> > +
> >  		/* Enable SGMII on the port */
> >  		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> >  
> > @@ -1274,6 +1277,28 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  		}
> >  
> >  		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
> > +
> > +		/* For qca8327/qca8328/qca8334/qca8338 sgmii is unique and
> > +		 * falling edge is set writing in the PORT0 PAD reg
> > +		 */
> > +		if (priv->switch_id == QCA8K_ID_QCA8327 ||
> > +		    priv->switch_id == QCA8K_ID_QCA8337)
> > +			reg = QCA8K_REG_PORT0_PAD_CTRL;
> > +
> > +		val = 0;
> > +
> > +		/* SGMII Clock phase configuration */
> > +		if (of_property_read_bool(dp->dn, "qca,sgmii-rxclk-falling-edge"))
> 
> I would strongly recommend that you stop accessing dp->dn and add your
> own device tree parsing function during probe time. It is also a runtime
> invariant, there is no reason to read the device tree during each mac_config.
>

What would be the correct way to pass all these data? Put all of them in
qca8k_priv? Do we have a cleaner solution? 

> > +			val |= QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE;
> > +
> > +		if (of_property_read_bool(dp->dn, "qca,sgmii-txclk-falling-edge"))
> > +			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);
> >  		break;
> >  	default:
> >  		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index fc7db94cc0c9..3fded69a6839 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -35,6 +35,9 @@
> >  #define   QCA8K_MASK_CTRL_DEVICE_ID_MASK		GENMASK(15, 8)
> >  #define   QCA8K_MASK_CTRL_DEVICE_ID(x)			((x) >> 8)
> >  #define QCA8K_REG_PORT0_PAD_CTRL			0x004
> > +#define   QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG		BIT(31)
> 
> QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG is not used in this patch.
> 
> > +#define   QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE	BIT(19)
> > +#define   QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE	BIT(18)
> >  #define QCA8K_REG_PORT5_PAD_CTRL			0x008
> >  #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
> >  #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
> > -- 
> > 2.32.0
> > 
> 

-- 
	Ansuel

Re: [net-next PATCH v4 03/13] dt-bindings: net: dsa: qca8k: Add MAC swap and clock phase properties

[Ansuel Smith]

On Sun, Oct 10, 2021 at 03:07:28PM +0300, Vladimir Oltean wrote:
> On Sun, Oct 10, 2021 at 01:15:46PM +0200, Ansuel Smith wrote:
> > Add names and descriptions of additional PORT0_PAD_CTRL properties.
> > qca,sgmii-(rx|tx)clk-falling-edge are for setting the respective clock
> > phase to failling edge.
> > 
> > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > index 8c73f67c43ca..cc214e655442 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > @@ -37,6 +37,10 @@ A CPU port node has the following optional node:
> >                            managed entity. See
> >                            Documentation/devicetree/bindings/net/fixed-link.txt
> >                            for details.
> > +- qca,sgmii-rxclk-falling-edge: Set the receive clock phase to falling edge.
> > +                                Mostly used in qca8327 with CPU port 0 set to
> > +                                sgmii.
> > +- qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge.
> >  
> >  For QCA8K the 'fixed-link' sub-node supports only the following properties:
> >  
> > -- 
> > 2.32.0
> > 
> 
> Must first document, then use.
> Also, would you care converting qca8k.txt to qca8k.yaml?

Ow ok, will swap alle the patch.
About converting... Hard task will try but it will be a nightmare. Sad
me.

-- 
	Ansuel

Re: [net-next PATCH v4 04/13] drivers: net: dsa: qca8k: add support for cpu port 6

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 01:15:47PM +0200, Ansuel Smith wrote:
> Currently CPU port is always hardcoded to port 0. This switch have 2 CPU
> port. The original intention of this driver seems to be use the
> mac06_exchange bit to swap MAC0 with MAC6 in the strange configuration
> where device have connected only the CPU port 6. To skip the
> introduction of a new binding, rework the driver to address the
> secondary CPU port as primary and drop any reference of hardcoded port.
> With configuration of mac06 exchange, just skip the definition of port0
> and define the CPU port as a secondary. The driver will autoconfigure
> the switch to use that as the primary CPU port.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Does this really work? What about GLOBAL_FW_CTRL0 bit 10 (CPU_PORT_EN),
which this patch leaves alone, and whose description is:
0 = No CPU connect to switch
1 = CPU is connected to port0
If this bit is set to 1, HEAD_EN of MAC0 is set to 1

I see all of PORT0_HEADER_CTRL - PORT6_HEADER_CTRL have the option of
setting RX_HEADER_MODE and TX_HEADER_MODE.
I just have this doubt: what is port 0 supposed to do when the CPU port
is port 6? Can it be used as a regular user port, attached to an RGMII PHY?

Isn't that use case broken anyway, due to qca8k.c's broken
interpretation of RGMII delay device tree bindings (it always applies
RGMII delays on "rgmii-id", and the PHY will apply them too)?

If I were to trust the documentation, that DSA headers are enabled on
port 0 when the driver does this:

	/* Enable CPU Port */
	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);

doesn't that mean that using port 0 as a user port is double-broken,
since this would implicitly enable DSA headers on it?

Or is the idea of using port 6 as the CPU port to be able to use SGMII,
which is not available on port 0? Jonathan McDowell did some SGMII
configuration for the CPU port in commit f6dadd559886 ("net: dsa: qca8k:
Improve SGMII interface handling"). If the driver supports only port 0
as CPU port, and SGMII is only available on port 6, how did he do it?

On the boards that you have with port 6 as CPU port, what is port 0 used
for? Can port 0 and 6 be CPU ports simultaneously?

I also want to understand what's the use case of this port swap. In the
QCA8337 block diagram I see a dotted-line connection between Port 0 and
the SerDes, presumably this is due to the port swap. But I don't see any
dotted line between the Port 6 and the first GMII/RGMII/MII/RMII MAC.
So what will happen to what software believes is port 6, with the swap
in place?

Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 01:15:49PM +0200, Ansuel Smith wrote:
> Future proof commit. This switch have 2 CPU port and one valid
> configuration is first CPU port set to sgmii and second CPU port set to
> regmii-id. The current implementation detects delay only for CPU port
> zero set to rgmii and doesn't count any delay set in a secondary CPU
> port. Drop the current delay scan function and move it to the phylink
> mac_config to generilize and implicitly add support for secondary CPU
> port set to rgmii-id.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 121 +++++++++++++++-------------------------
>  drivers/net/dsa/qca8k.h |   2 -
>  2 files changed, 44 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index df0a622acdd7..126f20b0b94c 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -888,68 +888,6 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
>  	return 0;
>  }
>  
> -static int
> -qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)

I was actually going to say that since RGMII delays are runtime
invariants, you should move their entire programming to probe time, now
you move device tree parsing to runtime :-/

> -{
> -	struct device_node *port_dn;
> -	phy_interface_t mode;
> -	struct dsa_port *dp;
> -	u32 val;
> -
> -	/* CPU port is already checked */
> -	dp = dsa_to_port(priv->ds, 0);
> -
> -	port_dn = dp->dn;
> -
> -	/* Check if port 0 is set to the correct type */
> -	of_get_phy_mode(port_dn, &mode);
> -	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> -	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> -	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> -		return 0;
> -	}
> -
> -	switch (mode) {
> -	case PHY_INTERFACE_MODE_RGMII_ID:
> -	case PHY_INTERFACE_MODE_RGMII_RXID:

Also, since you touch this area.
There have been tons of discussions on this topic, but I believe that
your interpretation of the RGMII delays is wrong.
Basically a MAC should not apply delays based on the phy-mode string (so
it should treat "rgmii" same as "rgmii-id"), but based on the value of
"rx-internal-delay-ps" and "tx-internal-delay-ps".
The phy-mode is for a PHY to use.

> -		if (of_property_read_u32(port_dn, "rx-internal-delay-ps", &val))
> -			val = 2;
> -		else
> -			/* Switch regs accept value in ns, convert ps to ns */
> -			val = val / 1000;
> -
> -		if (val > QCA8K_MAX_DELAY) {
> -			dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
> -			val = 3;
> -		}
> -
> -		priv->rgmii_rx_delay = val;
> -		/* Stop here if we need to check only for rx delay */
> -		if (mode != PHY_INTERFACE_MODE_RGMII_ID)
> -			break;
> -
> -		fallthrough;
> -	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		if (of_property_read_u32(port_dn, "tx-internal-delay-ps", &val))
> -			val = 1;
> -		else
> -			/* Switch regs accept value in ns, convert ps to ns */
> -			val = val / 1000;
> -
> -		if (val > QCA8K_MAX_DELAY) {
> -			dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
> -			val = 3;
> -		}
> -
> -		priv->rgmii_tx_delay = val;
> -		break;
> -	default:
> -		return 0;
> -	}
> -
> -	return 0;
> -}
> -
>  static int
>  qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
>  {
> @@ -1019,10 +957,6 @@ qca8k_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> -	ret = qca8k_setup_of_rgmii_delay(priv);
> -	if (ret)
> -		return ret;
> -
>  	ret = qca8k_setup_mac_pwr_sel(priv);
>  	if (ret)
>  		return ret;
> @@ -1190,7 +1124,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  {
>  	struct qca8k_priv *priv = ds->priv;
>  	struct dsa_port *dp;
> -	u32 reg, val;
> +	u32 reg, val, delay;
>  	int ret;
>  
>  	switch (port) {
> @@ -1241,17 +1175,50 @@ 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:
> -		/* RGMII_ID needs internal delay. This is enabled through
> -		 * PORT5_PAD_CTRL for all ports, rather than individual port
> -		 * registers
> +		dp = dsa_to_port(ds, port);
> +		val = QCA8K_PORT_PAD_RGMII_EN;
> +
> +		if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +		    state->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> +			if (of_property_read_u32(dp->dn, "tx-internal-delay-ps", &delay))
> +				delay = 1;
> +			else
> +				/* Switch regs accept value in ns, convert ps to ns */
> +				delay = delay / 1000;
> +
> +			if (delay > QCA8K_MAX_DELAY) {
> +				dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
> +				delay = 3;
> +			}
> +
> +			val |= QCA8K_PORT_PAD_RGMII_TX_DELAY(delay) |
> +			       QCA8K_PORT_PAD_RGMII_TX_DELAY_EN;
> +		}
> +
> +		if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +		    state->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> +			if (of_property_read_u32(dp->dn, "rx-internal-delay-ps", &delay))
> +				delay = 2;
> +			else
> +				/* Switch regs accept value in ns, convert ps to ns */
> +				delay = delay / 1000;
> +
> +			if (delay > QCA8K_MAX_DELAY) {
> +				dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
> +				delay = 3;
> +			}
> +
> +			val |= QCA8K_PORT_PAD_RGMII_RX_DELAY(delay) |
> +			       QCA8K_PORT_PAD_RGMII_RX_DELAY_EN;
> +		}
> +
> +		/* Set RGMII delay based on the selected values */
> +		qca8k_write(priv, reg, val);
> +
> +		/* QCA8337 requires to set rgmii rx delay for all ports.
> +		 * This is enabled through PORT5_PAD_CTRL for all ports,
> +		 * rather than individual port registers.
>  		 */
> -		qca8k_write(priv, reg,
> -			    QCA8K_PORT_PAD_RGMII_EN |
> -			    QCA8K_PORT_PAD_RGMII_TX_DELAY(priv->rgmii_tx_delay) |
> -			    QCA8K_PORT_PAD_RGMII_RX_DELAY(priv->rgmii_rx_delay) |
> -			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
> -			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> -		/* QCA8337 requires to set rgmii rx delay */
>  		if (priv->switch_id == QCA8K_ID_QCA8337)
>  			qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
>  				    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 5df0f0ef6526..a790b27bc310 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -259,8 +259,6 @@ struct qca8k_match_data {
>  struct qca8k_priv {
>  	u8 switch_id;
>  	u8 switch_revision;
> -	u8 rgmii_tx_delay;
> -	u8 rgmii_rx_delay;
>  	bool legacy_phy_port_mapping;
>  	struct regmap *regmap;
>  	struct mii_bus *bus;
> -- 
> 2.32.0
> 

Re: [net-next PATCH v4 02/13] net: dsa: qca8k: add support for sgmii falling edge

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 02:10:00PM +0200, Ansuel Smith wrote:
> > I would strongly recommend that you stop accessing dp->dn and add your
> > own device tree parsing function during probe time. It is also a runtime
> > invariant, there is no reason to read the device tree during each mac_config.
> >
>
> What would be the correct way to pass all these data? Put all of them in
> qca8k_priv? Do we have a cleaner solution?

Put in driver private structures, or configure at probe time and never
modify touch again, anything goes.

Re: [net-next PATCH v4 10/13] dt-bindings: net: dsa: qca8k: document open drain binding

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 01:15:53PM +0200, Ansuel Smith wrote:
> Document new binding qca,power_on_sel used to enable Power-on-strapping
> select reg and qca,led_open_drain to set led to open drain mode.

Please spend 5 minutes to proof-read your emails before sending. You
will find tons of spelling mistakes and inconsistencies.

The commit message refers to qca,led_open_drain and qca,power_on_sel
which are not the names added to the documentation.

> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> index 05a8ddfb5483..71cd45818430 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> @@ -13,6 +13,17 @@ Required properties:
>  Optional properties:
>  
>  - reset-gpios: GPIO to be used to reset the whole device
> +- qca,ignore-power-on-sel: Ignore power on pin strapping to configure led open
> +                           drain or eeprom presence. This is needed for broken
> +                           device that have wrong configuration or when the oem

_devices_ that _have_

> +                           decided to not use pin strapping and fallback to sw
> +                           regs.
> +- qca,led-open-drain: Set leds to open-drain mode. This require the

This _requires_

> +                      qca,ignore-power-on-sel to be set or the driver will fail
> +                      to probe. This is needed if the oem doesn't use pin

oem _prefers_

> +                      strapping to set this mode and prefer to set it using sw
> +                      regs. The pin strapping related to led open drain mode is
> +                      the pin B68 for QCA832x and B49 for QCA833x
>  
>  Subnodes:
>  
> -- 
> 2.32.0
> 

Re: [net-next PATCH v4 04/13] drivers: net: dsa: qca8k: add support for cpu port 6

[Ansuel Smith]

On Sun, Oct 10, 2021 at 03:42:43PM +0300, Vladimir Oltean wrote:
> On Sun, Oct 10, 2021 at 01:15:47PM +0200, Ansuel Smith wrote:
> > Currently CPU port is always hardcoded to port 0. This switch have 2 CPU
> > port. The original intention of this driver seems to be use the
> > mac06_exchange bit to swap MAC0 with MAC6 in the strange configuration
> > where device have connected only the CPU port 6. To skip the
> > introduction of a new binding, rework the driver to address the
> > secondary CPU port as primary and drop any reference of hardcoded port.
> > With configuration of mac06 exchange, just skip the definition of port0
> > and define the CPU port as a secondary. The driver will autoconfigure
> > the switch to use that as the primary CPU port.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> 
> Does this really work? What about GLOBAL_FW_CTRL0 bit 10 (CPU_PORT_EN),
> which this patch leaves alone, and whose description is:
> 0 = No CPU connect to switch
> 1 = CPU is connected to port0
> If this bit is set to 1, HEAD_EN of MAC0 is set to 1
>

I tested this with my Netgear r7800 that has a qca8337. I commented cpu
port0 and the switch works correctly.
I think the CPU_PORT_EN enabled cpu port0 that can only be a CPU port
and cpu port6 if it's set in that mode (there are some regs to set it to
CPU mode, PHY mode or BASE-X)

> I see all of PORT0_HEADER_CTRL - PORT6_HEADER_CTRL have the option of
> setting RX_HEADER_MODE and TX_HEADER_MODE.
> I just have this doubt: what is port 0 supposed to do when the CPU port
> is port 6? Can it be used as a regular user port, attached to an RGMII PHY?
> 

Port 0 can only be used as CPU port, nothing else.

> Isn't that use case broken anyway, due to qca8k.c's broken
> interpretation of RGMII delay device tree bindings (it always applies
> RGMII delays on "rgmii-id", and the PHY will apply them too)?
> 

Actually we have dedicated regs for port0 and port6 to set internal
delay. Only qca8337 require additional delay for port1-2-3-4-5 using the
PAD5 reg.

> If I were to trust the documentation, that DSA headers are enabled on
> port 0 when the driver does this:
> 
> 	/* Enable CPU Port */
> 	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
> 			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
> 
> doesn't that mean that using port 0 as a user port is double-broken,
> since this would implicitly enable DSA headers on it?
> 
> Or is the idea of using port 6 as the CPU port to be able to use SGMII,
> which is not available on port 0? Jonathan McDowell did some SGMII
> configuration for the CPU port in commit f6dadd559886 ("net: dsa: qca8k:
> Improve SGMII interface handling"). If the driver supports only port 0
> as CPU port, and SGMII is only available on port 6, how did he do it?
> 

I think the dotted thing in the diagram about sgmii is about the fact
that you can use sgmii for both port0 or port6. (the switch configuration
support only ONE sgmii) We have device that have such configuration
(port0 set to sgmii) without the mac06 exchange bit set.

> On the boards that you have with port 6 as CPU port, what is port 0 used
> for? Can port 0 and 6 be CPU ports simultaneously?

Yes and that's the actual common configuration. 2 CPU port. First
assigned to lan port 1-2-3-4 and second assigned to wan port 5.
DSA currently doesn't support this so everything is handled by cpu port0
(and with this now it can be handled by cpu port6)

> 
> I also want to understand what's the use case of this port swap. In the
> QCA8337 block diagram I see a dotted-line connection between Port 0 and
> the SerDes, presumably this is due to the port swap. But I don't see any
> dotted line between the Port 6 and the first GMII/RGMII/MII/RMII MAC.
> So what will happen to what software believes is port 6, with the swap
> in place?

I'm starting to think that mac swap it's really something used to treat
case where port0 is disconnected and only port6 is connected. They set
the bit to swap them and then everything else will see port6 as port0.
Also the original driver (and from Documentation some specific regs)
looks to be hardcoded to port0 so that can be the user case. But again I
tested with my qca8337 device and a configuration with port6 looks like
to work.
If mac06 exchange is not supported on qca8327 we can safely assume that
such configuration is not supported (port0 MUST be always connected) and
return error on such configuration.
Again with mac swap you would declare everything using port0 so from dsa
side they all see that as port0 and it won't be wrong to treat it like
it's port 0.

-- 
	Ansuel

Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

[Ansuel Smith]

On Sun, Oct 10, 2021 at 03:47:32PM +0300, Vladimir Oltean wrote:
> On Sun, Oct 10, 2021 at 01:15:49PM +0200, Ansuel Smith wrote:
> > Future proof commit. This switch have 2 CPU port and one valid
> > configuration is first CPU port set to sgmii and second CPU port set to
> > regmii-id. The current implementation detects delay only for CPU port
> > zero set to rgmii and doesn't count any delay set in a secondary CPU
> > port. Drop the current delay scan function and move it to the phylink
> > mac_config to generilize and implicitly add support for secondary CPU
> > port set to rgmii-id.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 121 +++++++++++++++-------------------------
> >  drivers/net/dsa/qca8k.h |   2 -
> >  2 files changed, 44 insertions(+), 79 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index df0a622acdd7..126f20b0b94c 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -888,68 +888,6 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
> >  	return 0;
> >  }
> >  
> > -static int
> > -qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)
> 
> I was actually going to say that since RGMII delays are runtime
> invariants, you should move their entire programming to probe time, now
> you move device tree parsing to runtime :-/
> 

The main idea here was to move everything to mac config and scan the DT
node of the current port that is being configured. 

> > -{
> > -	struct device_node *port_dn;
> > -	phy_interface_t mode;
> > -	struct dsa_port *dp;
> > -	u32 val;
> > -
> > -	/* CPU port is already checked */
> > -	dp = dsa_to_port(priv->ds, 0);
> > -
> > -	port_dn = dp->dn;
> > -
> > -	/* Check if port 0 is set to the correct type */
> > -	of_get_phy_mode(port_dn, &mode);
> > -	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> > -	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> > -	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> > -		return 0;
> > -	}
> > -
> > -	switch (mode) {
> > -	case PHY_INTERFACE_MODE_RGMII_ID:
> > -	case PHY_INTERFACE_MODE_RGMII_RXID:
> 
> Also, since you touch this area.
> There have been tons of discussions on this topic, but I believe that
> your interpretation of the RGMII delays is wrong.
> Basically a MAC should not apply delays based on the phy-mode string (so
> it should treat "rgmii" same as "rgmii-id"), but based on the value of
> "rx-internal-delay-ps" and "tx-internal-delay-ps".
> The phy-mode is for a PHY to use.
>

Ok so we can just drop the case and directly check for the
internal-delay-ps presence?

> > -		if (of_property_read_u32(port_dn, "rx-internal-delay-ps", &val))
> > -			val = 2;
> > -		else
> > -			/* Switch regs accept value in ns, convert ps to ns */
> > -			val = val / 1000;
> > -
> > -		if (val > QCA8K_MAX_DELAY) {
> > -			dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
> > -			val = 3;
> > -		}
> > -
> > -		priv->rgmii_rx_delay = val;
> > -		/* Stop here if we need to check only for rx delay */
> > -		if (mode != PHY_INTERFACE_MODE_RGMII_ID)
> > -			break;
> > -
> > -		fallthrough;
> > -	case PHY_INTERFACE_MODE_RGMII_TXID:
> > -		if (of_property_read_u32(port_dn, "tx-internal-delay-ps", &val))
> > -			val = 1;
> > -		else
> > -			/* Switch regs accept value in ns, convert ps to ns */
> > -			val = val / 1000;
> > -
> > -		if (val > QCA8K_MAX_DELAY) {
> > -			dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
> > -			val = 3;
> > -		}
> > -
> > -		priv->rgmii_tx_delay = val;
> > -		break;
> > -	default:
> > -		return 0;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >  static int
> >  qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
> >  {
> > @@ -1019,10 +957,6 @@ qca8k_setup(struct dsa_switch *ds)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = qca8k_setup_of_rgmii_delay(priv);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ret = qca8k_setup_mac_pwr_sel(priv);
> >  	if (ret)
> >  		return ret;
> > @@ -1190,7 +1124,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  {
> >  	struct qca8k_priv *priv = ds->priv;
> >  	struct dsa_port *dp;
> > -	u32 reg, val;
> > +	u32 reg, val, delay;
> >  	int ret;
> >  
> >  	switch (port) {
> > @@ -1241,17 +1175,50 @@ 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:
> > -		/* RGMII_ID needs internal delay. This is enabled through
> > -		 * PORT5_PAD_CTRL for all ports, rather than individual port
> > -		 * registers
> > +		dp = dsa_to_port(ds, port);
> > +		val = QCA8K_PORT_PAD_RGMII_EN;
> > +
> > +		if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +		    state->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> > +			if (of_property_read_u32(dp->dn, "tx-internal-delay-ps", &delay))
> > +				delay = 1;
> > +			else
> > +				/* Switch regs accept value in ns, convert ps to ns */
> > +				delay = delay / 1000;
> > +
> > +			if (delay > QCA8K_MAX_DELAY) {
> > +				dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
> > +				delay = 3;
> > +			}
> > +
> > +			val |= QCA8K_PORT_PAD_RGMII_TX_DELAY(delay) |
> > +			       QCA8K_PORT_PAD_RGMII_TX_DELAY_EN;
> > +		}
> > +
> > +		if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +		    state->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> > +			if (of_property_read_u32(dp->dn, "rx-internal-delay-ps", &delay))
> > +				delay = 2;
> > +			else
> > +				/* Switch regs accept value in ns, convert ps to ns */
> > +				delay = delay / 1000;
> > +
> > +			if (delay > QCA8K_MAX_DELAY) {
> > +				dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
> > +				delay = 3;
> > +			}
> > +
> > +			val |= QCA8K_PORT_PAD_RGMII_RX_DELAY(delay) |
> > +			       QCA8K_PORT_PAD_RGMII_RX_DELAY_EN;
> > +		}
> > +
> > +		/* Set RGMII delay based on the selected values */
> > +		qca8k_write(priv, reg, val);
> > +
> > +		/* QCA8337 requires to set rgmii rx delay for all ports.
> > +		 * This is enabled through PORT5_PAD_CTRL for all ports,
> > +		 * rather than individual port registers.
> >  		 */
> > -		qca8k_write(priv, reg,
> > -			    QCA8K_PORT_PAD_RGMII_EN |
> > -			    QCA8K_PORT_PAD_RGMII_TX_DELAY(priv->rgmii_tx_delay) |
> > -			    QCA8K_PORT_PAD_RGMII_RX_DELAY(priv->rgmii_rx_delay) |
> > -			    QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
> > -			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> > -		/* QCA8337 requires to set rgmii rx delay */
> >  		if (priv->switch_id == QCA8K_ID_QCA8337)
> >  			qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
> >  				    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index 5df0f0ef6526..a790b27bc310 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -259,8 +259,6 @@ struct qca8k_match_data {
> >  struct qca8k_priv {
> >  	u8 switch_id;
> >  	u8 switch_revision;
> > -	u8 rgmii_tx_delay;
> > -	u8 rgmii_rx_delay;
> >  	bool legacy_phy_port_mapping;
> >  	struct regmap *regmap;
> >  	struct mii_bus *bus;
> > -- 
> > 2.32.0
> > 
> 

-- 
	Ansuel

Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

[Andrew Lunn]

> > -{
> > -	struct device_node *port_dn;
> > -	phy_interface_t mode;
> > -	struct dsa_port *dp;
> > -	u32 val;
> > -
> > -	/* CPU port is already checked */
> > -	dp = dsa_to_port(priv->ds, 0);
> > -
> > -	port_dn = dp->dn;
> > -
> > -	/* Check if port 0 is set to the correct type */
> > -	of_get_phy_mode(port_dn, &mode);
> > -	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> > -	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> > -	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> > -		return 0;
> > -	}
> > -
> > -	switch (mode) {
> > -	case PHY_INTERFACE_MODE_RGMII_ID:
> > -	case PHY_INTERFACE_MODE_RGMII_RXID:
> 
> Also, since you touch this area.
> There have been tons of discussions on this topic, but I believe that
> your interpretation of the RGMII delays is wrong.
> Basically a MAC should not apply delays based on the phy-mode string (so
> it should treat "rgmii" same as "rgmii-id"), but based on the value of
> "rx-internal-delay-ps" and "tx-internal-delay-ps".
> The phy-mode is for a PHY to use.

There is one exception to this, when the MAC is taking the place of a
PHY, i.e. CPU port. You need delays added somewhere, and the mv88e6xxx
driver will look at the phy-mode in this case. And i think in general,
a DSA driver needs this for the CPU port.

       Andrew

Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 05:18:16PM +0200, Andrew Lunn wrote:
> > > -{
> > > -	struct device_node *port_dn;
> > > -	phy_interface_t mode;
> > > -	struct dsa_port *dp;
> > > -	u32 val;
> > > -
> > > -	/* CPU port is already checked */
> > > -	dp = dsa_to_port(priv->ds, 0);
> > > -
> > > -	port_dn = dp->dn;
> > > -
> > > -	/* Check if port 0 is set to the correct type */
> > > -	of_get_phy_mode(port_dn, &mode);
> > > -	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> > > -	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> > > -	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> > > -		return 0;
> > > -	}
> > > -
> > > -	switch (mode) {
> > > -	case PHY_INTERFACE_MODE_RGMII_ID:
> > > -	case PHY_INTERFACE_MODE_RGMII_RXID:
> > 
> > Also, since you touch this area.
> > There have been tons of discussions on this topic, but I believe that
> > your interpretation of the RGMII delays is wrong.
> > Basically a MAC should not apply delays based on the phy-mode string (so
> > it should treat "rgmii" same as "rgmii-id"), but based on the value of
> > "rx-internal-delay-ps" and "tx-internal-delay-ps".
> > The phy-mode is for a PHY to use.
> 
> There is one exception to this, when the MAC is taking the place of a
> PHY, i.e. CPU port. You need delays added somewhere, and the mv88e6xxx
> driver will look at the phy-mode in this case.

Yes, and I don't think it's an actual exception, it's still back-to-back MACs.
It is true on the other hand that some drivers don't seem to behave this way.
Russell has been suggesting that the phy-mode should be treated the same
way regardless of whether a PHY is attached or not. It would be nice to
reach a common agreement so that we know what to suggest people to do.

> And i think in general, a DSA driver needs this for the CPU port.

Not saying it doesn't need it, just saying that the need for the
{rx,tx}-internal-delay-ps MAC property was not recognized at the time
and that's why the vast majority of drivers don't act upon it.
The qca8k driver is somewhat special in that it does parse these new
properties, but at the same time also parse the phy-mode, that is kind
of strange.

Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 03:28:39PM +0200, Ansuel Smith wrote:
> > I was actually going to say that since RGMII delays are runtime
> > invariants, you should move their entire programming to probe time, now
> > you move device tree parsing to runtime :-/
> >
>
> The main idea here was to move everything to mac config and scan the DT
> node of the current port that is being configured.

If you insist on doing static configuration in a phylink callback, sure,
the comment was mostly about not accessing directly this struct dsa_port
member. It might change in the future, and the less refactoring required,
the better.

> > > -{
> > > -	struct device_node *port_dn;
> > > -	phy_interface_t mode;
> > > -	struct dsa_port *dp;
> > > -	u32 val;
> > > -
> > > -	/* CPU port is already checked */
> > > -	dp = dsa_to_port(priv->ds, 0);
> > > -
> > > -	port_dn = dp->dn;
> > > -
> > > -	/* Check if port 0 is set to the correct type */
> > > -	of_get_phy_mode(port_dn, &mode);
> > > -	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> > > -	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> > > -	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> > > -		return 0;
> > > -	}
> > > -
> > > -	switch (mode) {
> > > -	case PHY_INTERFACE_MODE_RGMII_ID:
> > > -	case PHY_INTERFACE_MODE_RGMII_RXID:
> >
> > Also, since you touch this area.
> > There have been tons of discussions on this topic, but I believe that
> > your interpretation of the RGMII delays is wrong.
> > Basically a MAC should not apply delays based on the phy-mode string (so
> > it should treat "rgmii" same as "rgmii-id"), but based on the value of
> > "rx-internal-delay-ps" and "tx-internal-delay-ps".
> > The phy-mode is for a PHY to use.
> >
>
> Ok so we can just drop the case and directly check for the
> internal-delay-ps presence?

Yes, but please consider existing device trees for this driver. I see
qcom-ipq8064-rb3011.dts and imx6dl-yapp4-common.dtsi, and neither use
explicit rx-internal-delay-ps or tx-internal-delay-ps properties. So
changing the driver to look at just those and ignore "rgmii-id" will
break those device trees, which is not pleasant. What would work is to
search first for *-internal-delay-ps, and then revert to determining the
delays based on the phy-mode, for compatibility.

Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

[Ansuel Smith]

On Sun, Oct 10, 2021 at 09:11:07PM +0300, Vladimir Oltean wrote:
> On Sun, Oct 10, 2021 at 03:28:39PM +0200, Ansuel Smith wrote:
> > > I was actually going to say that since RGMII delays are runtime
> > > invariants, you should move their entire programming to probe time, now
> > > you move device tree parsing to runtime :-/
> > >
> >
> > The main idea here was to move everything to mac config and scan the DT
> > node of the current port that is being configured.
> 
> If you insist on doing static configuration in a phylink callback, sure,
> the comment was mostly about not accessing directly this struct dsa_port
> member. It might change in the future, and the less refactoring required,
> the better.
>

I think I will put values in qca8k_priv and configure them in mac_config.

> > > > -{
> > > > -	struct device_node *port_dn;
> > > > -	phy_interface_t mode;
> > > > -	struct dsa_port *dp;
> > > > -	u32 val;
> > > > -
> > > > -	/* CPU port is already checked */
> > > > -	dp = dsa_to_port(priv->ds, 0);
> > > > -
> > > > -	port_dn = dp->dn;
> > > > -
> > > > -	/* Check if port 0 is set to the correct type */
> > > > -	of_get_phy_mode(port_dn, &mode);
> > > > -	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> > > > -	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> > > > -	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> > > > -		return 0;
> > > > -	}
> > > > -
> > > > -	switch (mode) {
> > > > -	case PHY_INTERFACE_MODE_RGMII_ID:
> > > > -	case PHY_INTERFACE_MODE_RGMII_RXID:
> > >
> > > Also, since you touch this area.
> > > There have been tons of discussions on this topic, but I believe that
> > > your interpretation of the RGMII delays is wrong.
> > > Basically a MAC should not apply delays based on the phy-mode string (so
> > > it should treat "rgmii" same as "rgmii-id"), but based on the value of
> > > "rx-internal-delay-ps" and "tx-internal-delay-ps".
> > > The phy-mode is for a PHY to use.
> > >
> >
> > Ok so we can just drop the case and directly check for the
> > internal-delay-ps presence?
> 
> Yes, but please consider existing device trees for this driver. I see
> qcom-ipq8064-rb3011.dts and imx6dl-yapp4-common.dtsi, and neither use
> explicit rx-internal-delay-ps or tx-internal-delay-ps properties. So
> changing the driver to look at just those and ignore "rgmii-id" will
> break those device trees, which is not pleasant. What would work is to
> search first for *-internal-delay-ps, and then revert to determining the
> delays based on the phy-mode, for compatibility.

Ok. Will try to implement something that is not entireley a big complex
condition ahahah.

-- 
	Ansuel

Re: [net-next PATCH v4 01/13] net: dsa: qca8k: add mac_power_sel support

[Vladimir Oltean]

On Sun, Oct 10, 2021 at 01:15:44PM +0200, Ansuel Smith wrote:
> Add missing mac power sel support needed for ipq8064/5 SoC that require
> 1.8v for the internal regulator port instead of the default 1.5v.
> If other device needs this, consider adding a dedicated binding to
> support this.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

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

Re: [net-next PATCH v4 04/13] drivers: net: dsa: qca8k: add support for cpu port 6

[Jonathan McDowell]

On Sun, Oct 10, 2021 at 03:22:49PM +0200, Ansuel Smith wrote:
> On Sun, Oct 10, 2021 at 03:42:43PM +0300, Vladimir Oltean wrote:
> > On Sun, Oct 10, 2021 at 01:15:47PM +0200, Ansuel Smith wrote:
> > > Currently CPU port is always hardcoded to port 0. This switch have 2 CPU
> > > port. The original intention of this driver seems to be use the
> > > mac06_exchange bit to swap MAC0 with MAC6 in the strange configuration
> > > where device have connected only the CPU port 6. To skip the
> > > introduction of a new binding, rework the driver to address the
> > > secondary CPU port as primary and drop any reference of hardcoded port.
> > > With configuration of mac06 exchange, just skip the definition of port0
> > > and define the CPU port as a secondary. The driver will autoconfigure
> > > the switch to use that as the primary CPU port.
...
> > If I were to trust the documentation, that DSA headers are enabled on
> > port 0 when the driver does this:
> > 
> > 	/* Enable CPU Port */
> > 	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
> > 			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
> > 
> > doesn't that mean that using port 0 as a user port is double-broken,
> > since this would implicitly enable DSA headers on it?
> > 
> > Or is the idea of using port 6 as the CPU port to be able to use SGMII,
> > which is not available on port 0? Jonathan McDowell did some SGMII
> > configuration for the CPU port in commit f6dadd559886 ("net: dsa: qca8k:
> > Improve SGMII interface handling"). If the driver supports only port 0
> > as CPU port, and SGMII is only available on port 6, how did he do it?
> > 
> 
> I think the dotted thing in the diagram about sgmii is about the fact
> that you can use sgmii for both port0 or port6. (the switch configuration
> support only ONE sgmii) We have device that have such configuration
> (port0 set to sgmii) without the mac06 exchange bit set.

That's certainly the case for my device; the SGMII connection is treated
as port 0 (and connected to the CPU via that) and then port 6 uses its
own RGMII connection (both port0 + port6 have their own dedicated RGMII
pins on the chip, and then the SGMII is shared and selectable).

J.

-- 
If plugging it in doesn't help, turn it on.