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

[Christian Lamparter <chunkeey@gmail.com>]

On Tuesday, February 5, 2019 11:29:36 PM CET Florian Fainelli wrote:
> On 2/5/19 2:12 PM, Christian Lamparter wrote:
> > On Tuesday, February 5, 2019 10:29:34 PM CET Andrew Lunn wrote:
> >>> 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.
> >>
> >> Hi Christian 
> >>
> >> You need to be careful with the DT binding. You need to keep backwards
> >> compatible with it. An old DT blob needs to keep working. I don't
> >> think this is true with this change.
> > 
> > Do you mean because of the 
> > 
> > -               switch0@0 {
> > +               switch@10 {
> >                         compatible = "qca,qca8337";
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >  
> > -                       reg = <0>;
> > +                       reg = <0x10>;
> > 
> > change?
> > 
> > or because I removed the phy-handles?>
> > The reg = <0x10>; will be necessary regardless. Because this
> > is really a bug in the existing binding example and if it is
> > copied it will prevent the qca8k driver from loading. 
> > This is due to a resource conflict, because there will be 
> > already a "phy_port1: phy@0" registered at reg = <0>;
> > So this never worked would have worked.
> 
> That part is fine, it is the removal of the phy-handle properties that
> is possibly a problem, but in hindsight, I do not believe it will be a
> compatibility issue. Lack of "phy-handle" property within the core DSA
> layer means: utilize the switch's internal MDIO bus (ds->slave_mii_bus)
> instance, which you are not removing, you are just changing how the PHYs
> map to port numbers.
> 
Ok, thanks. 

I think I'm almost ready for v2. I have fully addressed the compatibility
issue by forking off the qca8k_switch_ops depending on whenever a phy-handle
property on one of the ports was found or not. If there was no phy-handle the
driver adds the slave-bus accessors to the ops which tells DSA to allocate
the slave bus and allows the phys can be enumerated. If the phy-handles are
found the driver will not have the accessors and DSA will not setup a
redundant/fake bus and this prevents the second/double/duplicated discovery
and enumeration of the same PHYs again.

Cheers,
Christian

Attached is a preview:

---
commit 96bc70b4d6e290c450b9af728d9ca0f6db877f13
Author: Christian Lamparter <chunkeey@gmail.com>
Date:   Fri Feb 1 22:54:32 2019 +0100

    net: dsa: qca8k: extend slave-bus implementations
    
    This patch implements accessors for the QCA8337 MDIO access
    through the MDIO_MASTER register, which makes it possible to
    access the PHYs on slave-bus through the switch. In cases
    where the switch ports are already mapped via external
    "phy-phandles", the internal mdio-bus is disabled in order to
    prevent a duplicated discovery and enumeration of the same
    PHYs.
    
    Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
    ---
    
    Changes from v2:
     - Make it compatible with existing configurations
    
    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.

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a4b6cda38016..2f1b4b0a3507 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
@@ -868,8 +910,6 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.setup			= qca8k_setup,
 	.adjust_link            = qca8k_adjust_link,
 	.get_strings		= qca8k_get_strings,
-	.phy_read		= qca8k_phy_read,
-	.phy_write		= qca8k_phy_write,
 	.get_ethtool_stats	= qca8k_get_ethtool_stats,
 	.get_sset_count		= qca8k_get_sset_count,
 	.get_mac_eee		= qca8k_get_mac_eee,
@@ -884,6 +924,38 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_fdb_dump		= qca8k_port_fdb_dump,
 };
 
+/* Special code to detect DeviceTrees that use the phy-handle
+ * to map external PHYs to the ports. Only if no phy-handle is
+ * detected the slave bus accessors are getting enabled.
+ */
+static int qca8k_detect_slave_bus(struct qca8k_priv *priv)
+{
+	struct device_node *ports, *port;
+	bool found = false;
+
+	ports = of_get_child_by_name(priv->dev->of_node, "ports");
+	if (!ports) {
+		dev_err(priv->dev, "no ports child node found.\n");
+		return -EINVAL;
+	}
+
+	for_each_available_child_of_node(ports, port) {
+		if (of_property_read_bool(port, "phy-handle")) {
+			found = true;
+			break;
+		}
+	}
+
+	if (found) {
+		dev_info(priv->dev, "uses external mdio-bus.\n");
+	} else {
+		priv->ops.phy_read = qca8k_phy_read;
+		priv->ops.phy_write = qca8k_phy_write;
+	}
+
+	return 0;
+}
+
 static int
 qca8k_sw_probe(struct mdio_device *mdiodev)
 {
@@ -912,7 +984,12 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 		return -ENOMEM;
 
 	priv->ds->priv = priv;
-	priv->ds->ops = &qca8k_switch_ops;
+	priv->ops = qca8k_switch_ops;
+	if (qca8k_detect_slave_bus(priv))
+		return -EINVAL;
+
+	priv->ds->ops = &priv->ops;
+
 	mutex_init(&priv->reg_mutex);
 	dev_set_drvdata(&mdiodev->dev, priv);
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 613fe5c50236..38d06661f0a8 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)
@@ -168,6 +180,7 @@ struct qca8k_priv {
 	struct dsa_switch *ds;
 	struct mutex reg_mutex;
 	struct device *dev;
+	struct dsa_switch_ops ops;
 };
 
 struct qca8k_mib_desc {