Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address

[Christian Lamparter <chunkeey@gmail.com>]

On Thursday, March 21, 2019 7:26:08 PM CET Florian Fainelli wrote:
> +Christian,
> 
> On 3/21/19 11:23 AM, Marek Behún wrote:
> > The MDIO addresses of the internal PHYs on this switch for ports 1-5
> > have addresses 0-4, not 1-5.
> > 
> 
> Can you provide a Fixes: tag for this? Your change will conflicts with
> Christian's patch series here:
> 
> http://patchwork.ozlabs.org/project/netdev/list/?series=98063
http://patchwork.ozlabs.org/patch/1058655/ (link to the patch)

So, Yes and no ;)

We both have the same idea:

+static int
+qca8k_port_to_phy(int port)
+{
+	if (port < 1 || port > QCA8K_MDIO_MASTER_MAX_PORTS)
+		return -EINVAL;
+
+	return port - 1;
+}

I plan to sent v4 tomorrow, since I need to test it on the device first.

> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: Florian Fainelli <f.fainelli@gmail.com>
> > Cc: Michal Vokáč <vokac.m@gmail.com>
> > Cc: John Crispin <john@phrozen.org>
> > Cc: Wei Yongjun <weiyongjun1@huawei.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index cdcde7f8e0b2..eb199193cc3b 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -625,7 +625,7 @@ qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
> >  {
> >  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> >  
> > -	return mdiobus_read(priv->bus, phy, regnum);
> > +	return mdiobus_read(priv->bus, phy - 1, regnum);
> >  }
> >  
> >  static int
> > @@ -633,7 +633,7 @@ qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
> >  {
> >  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> >  
> > -	return mdiobus_write(priv->bus, phy, regnum, val);
> > +	return mdiobus_write(priv->bus, phy - 1, regnum, val);
> >  }

Word of Warning: The priv->bus is pointing to the external mdio-bus that
the QCA8337 is connected to, so in my case I noticed that the *same* PHYs
are registered twice. The first time on the external mdio, which is fine
since we need that for the phy-handle of the "ports" node and a s second
time by the slave-mdio that net/dsa/slave.c provides. In my case I found
Florian's explanation in
https://patchwork.ozlabs.org/patch/1036309/#2084184
very useful:

|I don't think you should have to do any of this translation, because you
|can do a couple of things with DSA/Device Tree:
|
|- you can not provide a phy-handle property at all, in which case, the
|core DSA layer assumes that the PHY is part of the switch's internal
|MDIO bus which is implictly created by dsa_slave_mii_bus_create()
|
|- you can specify a phy-handle property and then the PHY device tree
|node can be placed pretty much anywhere in Device Tree, including on a
|separate MDIO bus Device Tre node which is "external" to the switch
|
|In either case, the PHY device's MDIO bus parent and its address are
|taken care of by drivers/of/of_mdio.c. You can look at mx88e6xxx for how
|it deals with its internal vs. external MDIO bus controller and that
|driver is used on a wide variety of configuration.