Subject: Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation

From: Christian Lamparter <[email-masked]>
Date: Tue, 05 Feb 2019 22:08:26 +0100

On Tuesday, February 5, 2019 2:09:35 PM CET Andrew Lunn wrote:
> > The trick here is that priv->bus is not the internal
> > bus instead it's set to the SoC's (external) mii bus in qca8k_sw_probe()).
> > So this isn't a slave bus! And as stated in the qca8k, the the external
> > and internal PHY bus are not mapped 1:1.
> > 
> > >From what I can tell from the datasheet, the QCA8337N does have
> > dedicated MDIO master control register which is what is
> > needed here.
> 
> Ah, O.K, i was missing that bit. So yes, export an MDIO bus to Linux,
> as the mv88e6xxx does.

I've attached of what I think is needed for this. Can you please take
a quick look and see if this is matches your expecation?

A nice side-effect of adding support for the internal mdio-bus is that
the chip's mdio is now accessible through its UART interface mode or
through data frames (The QCA8337 registers can be accessed
through special data frames from the CPU port).

As for this version, it boots up fine on the WPQ864 with the QCA8337N:

[    5.799639] libphy: dsa slave smi: probed
[    5.820129] qca8k 37000000.mdio-mii:10 lan1 (uninitialized): PHY [dsa-0.0:01] driver [Generic PHY]
[    5.822418] qca8k 37000000.mdio-mii:10 lan2 (uninitialized): PHY [dsa-0.0:02] driver [Generic PHY]
[    5.830154] qca8k 37000000.mdio-mii:10 lan3 (uninitialized): PHY [dsa-0.0:03] driver [Generic PHY]
[    5.839284] qca8k 37000000.mdio-mii:10 lan4 (uninitialized): PHY [dsa-0.0:04] driver [Generic PHY]
[    5.848048] qca8k 37000000.mdio-mii:10 wan (uninitialized): PHY [dsa-0.0:05] driver [Generic PHY]
[    5.856065] DSA: tree 0 setup

and it works as expected.

For now, I added the DT binding update to the patch as well.
But if this is indeed the way to go, it'll get a separate patch.

Cheers,
Christian

---
>From e4951353e5e25f725a777b3a380c16bf55b2b7c1 Mon Sep 17 00:00:00 2001
From: Christian Lamparter <[email-masked]>
Date: Fri, 1 Feb 2019 22:54:32 +0100
Subject: [PATCH v2] net: dsa: qca8k: replace slave-bus implementation

The QCA8337 enumerates 5 PHYs on its internal MDIO access:
on address 0, 1, 2, 3 and 4. Based on the System Block
Diagram in Section 1.2 of the QCA8337's datasheet. These PHYs
connected to MACs of Port 1 to 5.

This version of the patch implements the QCA8337 MDIO
access through the switch's MDIO_MASTER register, which
makes it possible to forgo the current port-mapping in
the DT to map each Port.

Signed-off-by: Christian Lamparter <[email-masked]>
---

Changes from v1:
 - drop DT port <-> phy mapping
 - added register definitions for the MDIO control register
 - implemented new slave-mdio bus accessors
 - DT-binding: fix switch's PSEUDO_PHY address. It's 0x10 not 0.
---
 .../devicetree/bindings/net/dsa/qca8k.txt     | 35 ++-----------
 drivers/net/dsa/qca8k.c                       | 50 +++++++++++++++++--
 drivers/net/dsa/qca8k.h                       | 12 +++++
 3 files changed, 62 insertions(+), 35 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a4b6cda38016..dfcfb0791965 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -613,19 +613,61 @@ qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
 }
 
 static int
-qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
+qca8k_port_to_phy(int port)
+{
+	if (port < 1 || port > QCA8K_MDIO_MASTER_MAX_PORTS)
+		return -EINVAL;
+
+	return port - 1;
+}
+
+static int
+qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	u32 val, phy;
+
+	phy = qca8k_port_to_phy(port);
+	if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
+		return -EINVAL;
+
+	val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
+	      QCA8K_MDIO_MASTER_WRITE | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+	      QCA8K_MDIO_MASTER_REG_ADDR(regnum) |
+	      QCA8K_MDIO_MASTER_DATA(data);
 
-	return mdiobus_read(priv->bus, phy, regnum);
+	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+
+	return qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+		QCA8K_MDIO_MASTER_BUSY);
 }
 
+
 static int
-qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
+qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	u32 val, phy;
+
+	phy = qca8k_port_to_phy(port);
+	if (phy < 0 || (regnum < 0 || regnum >= QCA8K_MDIO_MASTER_MAX_REG))
+		return 0xffff;
+
+	val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
+	      QCA8K_MDIO_MASTER_READ | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+	      QCA8K_MDIO_MASTER_REG_ADDR(regnum);
+
+	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
 
-	return mdiobus_write(priv->bus, phy, regnum, val);
+	if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+				  QCA8K_MDIO_MASTER_BUSY)) {
+		return 0xffff;
+	}
+
+	val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
+		QCA8K_MDIO_MASTER_DATA_MASK);
+
+	return val;
 }
 
 static void
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 613fe5c50236..09a1d76b8037 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -48,6 +48,18 @@
 #define   QCA8K_MIB_FLUSH				BIT(24)
 #define   QCA8K_MIB_CPU_KEEP				BIT(20)
 #define   QCA8K_MIB_BUSY				BIT(17)
+#define QCA8K_MDIO_MASTER_CTRL				0x3c
+#define   QCA8K_MDIO_MASTER_BUSY			BIT(31)
+#define   QCA8K_MDIO_MASTER_EN				BIT(30)
+#define   QCA8K_MDIO_MASTER_READ			BIT(27)
+#define   QCA8K_MDIO_MASTER_WRITE			0
+#define   QCA8K_MDIO_MASTER_SUP_PRE			BIT(26)
+#define   QCA8K_MDIO_MASTER_PHY_ADDR(x)			((x) << 21)
+#define   QCA8K_MDIO_MASTER_REG_ADDR(x)			((x) << 16)
+#define   QCA8K_MDIO_MASTER_DATA(x)			(x)
+#define   QCA8K_MDIO_MASTER_DATA_MASK			GENMASK(15, 0)
+#define   QCA8K_MDIO_MASTER_MAX_PORTS			5
+#define   QCA8K_MDIO_MASTER_MAX_REG			32
 #define QCA8K_GOL_MAC_ADDR0				0x60
 #define QCA8K_GOL_MAC_ADDR1				0x64
 #define QCA8K_REG_PORT_STATUS(_i)			(0x07c + (_i) * 4)
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index bbcb255c3150..0fcbc2fcfcf1 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -12,10 +12,7 @@ Required properties:
 Subnodes:
 
 The integrated switch subnode should be specified according to the binding
-described in dsa/dsa.txt. As the QCA8K switches do not have a N:N mapping of
-port and PHY id, each subnode describing a port needs to have a valid phandle
-referencing the internal PHY connected to it. The CPU port of this switch is
-always port 0.
+described in dsa/dsa.txt. The CPU port of this switch is always port 0.
 
 A CPU port node has the following optional node:
 
@@ -35,36 +32,17 @@ Example:

 	&mdio0 {
-		phy_port1: phy@0 {
-			reg = <0>;
-		};
-
-		phy_port2: phy@1 {
-			reg = <1>;
-		};
-
-		phy_port3: phy@2 {
-			reg = <2>;
-		};
-
-		phy_port4: phy@3 {
-			reg = <3>;
-		};
-
-		phy_port5: phy@4 {
-			reg = <4>;
-		};
-
-		switch0@0 {
+		switch@10 {
 			compatible = "qca,qca8337";
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-			reg = <0>;
+			reg = <0x10>;
 
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
+
 				port@0 {
 					reg = <0>;
 					label = "cpu";
@@ -79,31 +57,26 @@ Example:
 				port@1 {
 					reg = <1>;
 					label = "lan1";
-					phy-handle = <&phy_port1>;
 				};
 
 				port@2 {
 					reg = <2>;
 					label = "lan2";
-					phy-handle = <&phy_port2>;
 				};
 
 				port@3 {
 					reg = <3>;
 					label = "lan3";
-					phy-handle = <&phy_port3>;
 				};
 
 				port@4 {
 					reg = <4>;
 					label = "lan4";
-					phy-handle = <&phy_port4>;
 				};
 
 				port@5 {
 					reg = <5>;
 					label = "wan";
-					phy-handle = <&phy_port5>;
 				};
 			};
 		};
-- 
2.20.1