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

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

[Marek Behún]

The MDIO addresses of the internal PHYs on this switch for ports 1-5
have addresses 0-4, not 1-5.

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);
 }
 
 static void
-- 
2.19.2

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

[Florian Fainelli]

+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

> 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);
>  }
>  
>  static void
> 


-- 
Florian

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

[Marek Behun]

Hi,

Oh, I didn't know about Christian's patch.

I shall test on our device tomorrow. If it works, we will use internal
PHY access.

But I think that qca8k_port_to_phy should be used in the external mode
as well. On our device the PHYs are mapped on 0-4 on the master bus even
in that mode.

Marek

On Thu, 21 Mar 2019 11:26:08 -0700
Florian Fainelli <f.fainelli@gmail.com> 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
> 
> > 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);
> >  }
> >  
> >  static void
> >   
> 
> 

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

[Christian Lamparter]

(Bottom post?)
On Thursday, March 21, 2019 8:55:55 PM CET Marek Behun wrote:
> Hi,
> 
> Oh, I didn't know about Christian's patch.
> 
> I shall test on our device tomorrow. If it works, we will use internal
> PHY access.
> 
> But I think that qca8k_port_to_phy should be used in the external mode
> as well. On our device the PHYs are mapped on 0-4 on the master bus even
> in that mode.

Hm, it's not really a "external mode". But let's try one more time. The
idea is that if an external mdio-bus (from the SoC) has already registered
the PHY 0x0 - 0x4 from the QCA8337, the qca8k should not expose the same
PHYs as it's own mdio-bus because then the PHYs end up being registered twice.

If you look at the mdio-bus communications during boot you can definitly see
the funkyness: every PHY gets initialized twice. You can also see this
"duplication" in /sys/class/mdio_bus. In this directory you'll have a mdio-bus
from the SoC and another one dsa-0:0 from the qca8k. If you look into those
you'll notice that their both the same.

As for why this happend. I think I found the culprit in a "missed"
requirement from one of Andrew Lunn's reponses to the initial qca8k patch:
<https://lore.kernel.org/patchwork/patch/715974/#902734>

In this post, he described the Device-Tree dts configuration we use today.
But at the end he requests: 
"and remove the phy_read() and phy_write() functions."

But from what I can tell, this important bit of information was lost
during the night. Because they show up in v2+:
https://lore.kernel.org/patchwork/patch/716989/

(So I guess I have to add Fixes: to my patch as well)
 
Cheers,
Christian

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

[Marek Behun]

> Hm, it's not really a "external mode". But let's try one more time.
> The idea is that if an external mdio-bus (from the SoC) has already
> registered the PHY 0x0 - 0x4 from the QCA8337, the qca8k should not
> expose the same PHYs as it's own mdio-bus because then the PHYs end
> up being registered twice.

Hi,
yes, I understand this bit. What I was talking about was that the MDIO
addresses of internal PHYs are 0 to 4, it does not matter if you access
them via switch or directly. But the current code for direct access is
using addresses 1 to 5, which does not work at all. It should also
substract 1 from the port number.

Marek

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

[Christian Lamparter]

On Friday, March 22, 2019 12:01:20 AM CET Marek Behun wrote:
> > Hm, it's not really a "external mode". But let's try one more time.
> > The idea is that if an external mdio-bus (from the SoC) has already
> > registered the PHY 0x0 - 0x4 from the QCA8337, the qca8k should not
> > expose the same PHYs as it's own mdio-bus because then the PHYs end
> > up being registered twice.
> 
> Hi,
> yes, I understand this bit. What I was talking about was that the MDIO
> addresses of internal PHYs are 0 to 4, it does not matter if you access
> them via switch or directly. But the current code for direct access is
> using addresses 1 to 5, which does not work at all. It should also
> substract 1 from the port number.
> 
I think you clipped the part that explained what's wrong the code:

|If you look at the mdio-bus communications during boot you can definitly see
|the funkyness: every PHY gets initialized twice. You can also see this
|"duplication" in /sys/class/mdio_bus. In this directory you'll have a mdio-bus
|from the SoC and another one dsa-0:0 from the qca8k. If you look into those
|you'll notice that their both the same.
|
|As for why this happend. I think I found the culprit in a "missed"
|requirement from one of Andrew Lunn's reponses to the initial qca8k patch:
|<https://lore.kernel.org/patchwork/patch/715974/#902734>
|
|In this post, he described the Device-Tree dts configuration we use today.
|But at the end he requests: 
|"and remove the phy_read() and phy_write() functions."

TL;DR: the current qca8k_phy_(read|write) are wrong and need to be
removed. 

But you should be able to test this in V4 easily (CC'd you there).
You only need Patch 3/4 (and keep your dts the way it is). Anyway,
that's it for now until tomorrow. 

Cheers,
Christian

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

[Christian Lamparter]

On Thursday, March 21, 2019 8:55:55 PM CET Marek Behun wrote:
> Hi,
> 
> Oh, I didn't know about Christian's patch.
> 
> I shall test on our device tomorrow. If it works, we will use internal
> PHY access.
> 
> But I think that qca8k_port_to_phy should be used in the external mode
> as well. On our device the PHYs are mapped on 0-4 on the master bus even
> in that mode.

Any news regarding your test?

Here's a direct link to the series:
<https://patchwork.ozlabs.org/project/netdev/list/?series=98562>

To test the internal phy you would need 3/4 + 4/4.

Cheers,
Christian

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

[Marek Behun]

> Any news regarding your test?

No, sorry, I fell ill and don't have that specific hardware at home,
only at work. I will probably test it on thursday or friday.

Marek

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

[Christian Lamparter]

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.