Date:   Thu, 07 Feb 2019 01:43:46 +0100
From:   Christian Lamparter <chunkeey@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation

On Wednesday, February 6, 2019 11:29:18 PM CET Florian Fainelli wrote:
> On 2/6/19 1:57 PM, Christian Lamparter wrote:
> > 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.
> 
> The logic you have sounds a little too broad since it stops as soon as
> one port is found with a 'phy-handle' property and assumes that the
> parent MDIO bus from which qca8k itself is a child device, is the MDIO
> bus to be used. There are possibly 3 cases:
> 
> 1) All ports using internal/build-in PHYs. In that case, you can either
> not specify a 'phy-handle' property and DSA assumes that they are part
> of the switch's internal MDIO bus. You can also specify a 'phy-handle'
> property that references the internal MDIO bus, although then we also
> expect qca8k to register its internal MDIO bus (ala mv88e6xxx)
> 
> 2) Some ports using internal PHYs, some using external PHYs. Similar
> situation again, ports may, or may not specify a 'phy-handle' property,
> so without a 'phy-handle' property that means the port connects to an
> internal PHY, with a 'phy-handle' it could connect to either internal
> PHY or external PHY
> 
> 3) All ports using external PHYs, in that case, we must have a
> 'phy-handle' for each port to specify where and how they connect to
> their external PHYs.

Oh, sadly the mixed configuration you have envisioned will not work really.
The QCA8K_MDIO_MASTER_EN Bit,which grants access to PHYs through the
MDIO_MASTER register also _disconnects_ the external MDC passthrough to
the internal PHYs. So you get garbage like:

[   17.036963] Generic PHY 37000000.mdio-mii:01: Master/Slave resolution failed, maybe conflicting manual settings?
[   17.116927] Generic PHY 37000000.mdio-mii:02: Master/Slave resolution failed, maybe conflicting manual settings?
[   17.196894] Generic PHY 37000000.mdio-mii:03: Master/Slave resolution failed, maybe conflicting manual settings?
(the PHY reads/write get seemingly stuck/do nothing/strange things).

To pull this partially (so it works for the kernel) off, would require
at least a custom phy driver on top of the dsa switch which would
syncronize the phy register access between the external and internal source.

(I think this would still leave access from userspace in a broken though?!
unless the mdiobus between the qca8k and the SoC can be syncronized as well)

> With respect to your patch, what I would do is register QCA8k's internal
> MDIO bus as a proper mdio bus and use ds->slave_mii_bus as a storage for
> that bus, such that tell the DSA layer: look, here is the internal MDIO
> bus, would you ever find a port that needs to use a PHY in there.
> 
> Then you can still scan each enabled port device, and for each of them,
> populate ds->phys_mii_mask, thus telling DSA exacly which ports are
> using an internal PHY because that would be the ports that do not have a
> 'phy-handle' property. Ports that have a 'phy-handle' property.
> 
> Hope this helps and is clear, if not, I can try to cook a patch for you
> to try, though I don't have you hardware.
Yes, I think I understood, I also tested it (see the implementation below).
But as said it's not that easy.

> Tangential, since you are working on qca8k, it would be great to give
> this driver some TLC and make sure that:
> 
> - bridge w/ and w/o VLAN filtering enabled works
> - multicast snooping works etc.
the driver/switch has much bigger problems :(. For example, my existing
configuration broke (I see RX and TX, but fails to get address by dhcp) due to:

    net: dsa: qca8k: disable delay for RGMII mode
    
    In RGMII mode we should not have any delay in port MAC, so disable
    the delay.
    
    Signed-off-by: Vinod Koul <vkoul@...nel.org>
    Signed-off-by: David S. Miller <davem@...emloft.net>

It would have been great if a proper .phylink_mac_config() was implemented
at that time. But oh well.

Cheers
Christian

---

commit 9db6f01c864494ebf1308e2f001ece92b5e1ae57
Author: Christian Lamparter <chunkeey@...il.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@...il.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..8d7ee4449d13 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -469,6 +469,131 @@ qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 		qca8k_reg_clear(priv, QCA8K_REG_PORT_STATUS(port), mask);
 }
 
+static int
+qca8k_port_to_phy(int port)
+{
+	if (port < 1 || port > QCA8K_MDIO_MASTER_MAX_PORTS)
+		return -EINVAL;
+
+	return port - 1;
+}
+
+static int
+qca8k_mdio_write(struct qca8k_priv *priv, int port, int regnum, u16 data)
+{
+	u32 val, phy;
+	int ret;
+
+	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);
+
+	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+
+	return qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+		QCA8K_MDIO_MASTER_BUSY);
+}
+
+static int
+qca8k_mdio_read(struct qca8k_priv *priv, int port, int regnum)
+{
+	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_READ | QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+	      QCA8K_MDIO_MASTER_REG_ADDR(regnum);
+
+	qca8k_write(priv, QCA8K_MDIO_MASTER_CTRL, val);
+
+	if (qca8k_busy_wait(priv, QCA8K_MDIO_MASTER_CTRL,
+				  QCA8K_MDIO_MASTER_BUSY)) {
+		return -ETIMEDOUT;
+	}
+
+	val = (qca8k_read(priv, QCA8K_MDIO_MASTER_CTRL) &
+		QCA8K_MDIO_MASTER_DATA_MASK);
+
+	return val;
+}
+
+static int
+qca8k_slave_phy_read(struct mii_bus *bus, int addr, int reg)
+{
+	struct qca8k_priv *priv = bus->priv;
+
+	if (priv->ds->phys_mii_mask & BIT(addr)) {
+		int ret = qca8k_mdio_read(priv, addr, reg);
+
+		if (ret >= 0)
+			return ret;
+	}
+
+	return 0xffff;
+}
+
+static int
+qca8k_slave_phy_write(struct mii_bus *bus, int addr, int reg, u16 val)
+{
+	struct qca8k_priv *priv = bus->priv;
+
+	if (priv->ds->phys_mii_mask & BIT(addr))
+		qca8k_mdio_write(priv, addr, reg, val);
+
+	return 0;
+}
+
+static int
+qca8k_setup_mdio_bus(struct qca8k_priv *priv)
+{
+	struct device_node *ports, *port;
+	struct mii_bus *bus;
+	u32 reg, mask = 0;
+	int err;
+
+	bus = devm_mdiobus_alloc_size(priv->dev, sizeof(priv));
+	if (!bus)
+		return -ENOMEM;
+	bus->priv = (void *)priv;
+
+	bus->name = priv->dev->of_node->full_name;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", priv->dev->of_node);
+
+	bus->read = qca8k_slave_phy_read;
+	bus->write = qca8k_slave_phy_write;
+	bus->parent = priv->dev;
+
+	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"))
+			continue;
+
+		err = of_property_read_u32(port, "reg", &reg);
+		if (err)
+			return err;
+
+		if (dsa_is_user_port(priv->ds, reg))
+			mask |= BIT(reg);
+	}
+	bus->phy_mask = ~mask;
+	priv->ds->slave_mii_bus = bus;
+
+	return mdiobus_register(bus);
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -490,6 +615,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (IS_ERR(priv->regmap))
 		pr_warn("regmap initialization failed");
 
+	ret = qca8k_setup_mdio_bus(priv);
+	if (ret)
+		return ret;
+
 	/* Initialize CPU port pad mode (xMII type, delays...) */
 	phy_mode = of_get_phy_mode(ds->ports[QCA8K_CPU_PORT].dn);
 	if (phy_mode < 0) {
@@ -613,19 +742,27 @@ 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_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
 {
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	struct qca8k_priv *priv = ds->priv;
+	int ret = -EIO;
 
-	return mdiobus_read(priv->bus, phy, regnum);
+	if (ds->slave_mii_bus->phy_mask & BIT(port))
+		ret = qca8k_mdio_write(priv, port, regnum, data);
+
+	return ret;
 }
 
 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;
+	struct qca8k_priv *priv = ds->priv;
+	int ret = -EIO;
 
-	return mdiobus_write(priv->bus, phy, regnum, val);
+	if (ds->slave_mii_bus->phy_mask & BIT(port))
+		ret = qca8k_mdio_read(priv, port, regnum);
+
+	return ret;
 }
 
 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)

Powered by blists - more mailing lists