* [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
@ 2019-03-21 18:23 Marek Behún
2019-03-21 18:26 ` Florian Fainelli
0 siblings, 1 reply; 9+ messages in thread
From: Marek Behún @ 2019-03-21 18:23 UTC (permalink / raw)
To: netdev
Cc: Marek Behún, Andrew Lunn, Florian Fainelli,
Michal Vokáč,
John Crispin, Wei Yongjun
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
2019-03-21 18:23 [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address Marek Behún
@ 2019-03-21 18:26 ` Florian Fainelli
2019-03-21 19:55 ` Marek Behun
2019-03-21 19:56 ` Christian Lamparter
0 siblings, 2 replies; 9+ messages in thread
From: Florian Fainelli @ 2019-03-21 18:26 UTC (permalink / raw)
To: Marek Behún, netdev
Cc: Andrew Lunn, Michal Vokáč,
John Crispin, Wei Yongjun, Christian Lamparter
+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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
2019-03-21 18:26 ` Florian Fainelli
@ 2019-03-21 19:55 ` Marek Behun
2019-03-21 22:24 ` Christian Lamparter
2019-03-21 19:56 ` Christian Lamparter
1 sibling, 1 reply; 9+ messages in thread
From: Marek Behun @ 2019-03-21 19:55 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, Andrew Lunn, Michal Vokáč,
John Crispin, Wei Yongjun, Christian Lamparter
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
> >
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
2019-03-21 18:26 ` Florian Fainelli
2019-03-21 19:55 ` Marek Behun
@ 2019-03-21 19:56 ` Christian Lamparter
1 sibling, 0 replies; 9+ messages in thread
From: Christian Lamparter @ 2019-03-21 19:56 UTC (permalink / raw)
To: Florian Fainelli
Cc: Marek Behún, netdev, Andrew Lunn, Michal Vokáč,
John Crispin, Wei Yongjun
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
2019-03-21 19:55 ` Marek Behun
@ 2019-03-21 22:24 ` Christian Lamparter
2019-03-21 23:01 ` Marek Behun
0 siblings, 1 reply; 9+ messages in thread
From: Christian Lamparter @ 2019-03-21 22:24 UTC (permalink / raw)
To: Marek Behun
Cc: Florian Fainelli, netdev, Andrew Lunn, Michal Vokáč,
John Crispin, Wei Yongjun
(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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
2019-03-21 22:24 ` Christian Lamparter
@ 2019-03-21 23:01 ` Marek Behun
2019-03-22 0:05 ` Christian Lamparter
0 siblings, 1 reply; 9+ messages in thread
From: Marek Behun @ 2019-03-21 23:01 UTC (permalink / raw)
To: Christian Lamparter
Cc: Florian Fainelli, netdev, Andrew Lunn, Michal Vokáč,
John Crispin, Wei Yongjun
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
2019-03-21 23:01 ` Marek Behun
@ 2019-03-22 0:05 ` Christian Lamparter
2019-03-25 17:51 ` Christian Lamparter
0 siblings, 1 reply; 9+ messages in thread
From: Christian Lamparter @ 2019-03-22 0:05 UTC (permalink / raw)
To: Marek Behun
Cc: Florian Fainelli, netdev, Andrew Lunn, Michal Vokáč,
John Crispin, Wei Yongjun
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
2019-03-22 0:05 ` Christian Lamparter
@ 2019-03-25 17:51 ` Christian Lamparter
2019-03-25 19:34 ` Marek Behun
0 siblings, 1 reply; 9+ messages in thread
From: Christian Lamparter @ 2019-03-25 17:51 UTC (permalink / raw)
To: Marek Behun
Cc: Florian Fainelli, netdev, Andrew Lunn, Michal Vokáč,
John Crispin, Wei Yongjun
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address
2019-03-25 17:51 ` Christian Lamparter
@ 2019-03-25 19:34 ` Marek Behun
0 siblings, 0 replies; 9+ messages in thread
From: Marek Behun @ 2019-03-25 19:34 UTC (permalink / raw)
To: Christian Lamparter
Cc: Florian Fainelli, netdev, Andrew Lunn, Michal Vokáč,
John Crispin, Wei Yongjun
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-03-25 19:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21 18:23 [PATCH net-next 1/1] net: dsa: qca8k: Fix internal PHY MDIO address Marek Behún
2019-03-21 18:26 ` Florian Fainelli
2019-03-21 19:55 ` Marek Behun
2019-03-21 22:24 ` Christian Lamparter
2019-03-21 23:01 ` Marek Behun
2019-03-22 0:05 ` Christian Lamparter
2019-03-25 17:51 ` Christian Lamparter
2019-03-25 19:34 ` Marek Behun
2019-03-21 19:56 ` Christian Lamparter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.