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

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

[Christian Lamparter]

The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4.
Based on the System Block Diagram in Section 1.2 of the
QCA8337's datasheet. These PHYs are internally connected
to MACs of PORT 1 - PORT 5. However, neither qca8k's slave
mdio access functions qca8k_phy_read()/qca8k_phy_write()
nor the dsa framework is set up for that.

This version of the patch uses the existing phy-handle
properties of each specified DSA Port in the DT to map
each PORT/MAC to its exposed PHY on the MDIO bus. This
is supported by the current binding document qca8k.txt
as well.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 drivers/net/dsa/qca8k.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a4b6cda38016..6558b7ed855d 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -11,6 +11,7 @@
 #include <linux/netdevice.h>
 #include <net/dsa.h>
 #include <linux/of_net.h>
+#include <linux/of_mdio.h>
 #include <linux/of_platform.h>
 #include <linux/if_bridge.h>
 #include <linux/mdio.h>
@@ -612,20 +613,50 @@ qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
 	qca8k_port_set_status(priv, port, 1);
 }
 
+static int
+qca8k_to_real_phy(struct dsa_switch *ds, int phy)
+{
+	struct device_node *phy_dn, *port_dn;
+	int id;
+
+	if (phy >= ds->num_ports)
+		return -EINVAL;
+
+	port_dn = ds->ports[phy].dn;
+	if (!port_dn)
+		return -EINVAL;
+
+	phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
+	if (!phy_dn)
+		return phy;
+
+	id = of_mdio_parse_addr(ds->dev, phy_dn);
+	of_node_put(phy_dn);
+	return id;
+}
+
 static int
 qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	int realphy = qca8k_to_real_phy(ds, phy);
+
+	if (realphy < 0)
+		return realphy;
 
-	return mdiobus_read(priv->bus, phy, regnum);
+	return mdiobus_read(priv->bus, realphy, regnum);
 }
 
 static int
 qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	int realphy = qca8k_to_real_phy(ds, phy);
+
+	if (realphy < 0)
+		return realphy;
 
-	return mdiobus_write(priv->bus, phy, regnum, val);
+	return mdiobus_write(priv->bus, realphy, regnum, val);
 }
 
 static void
-- 
2.20.1

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

[Florian Fainelli]

On 2/4/19 1:35 PM, Christian Lamparter wrote:
> The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4.
> Based on the System Block Diagram in Section 1.2 of the
> QCA8337's datasheet. These PHYs are internally connected
> to MACs of PORT 1 - PORT 5. However, neither qca8k's slave
> mdio access functions qca8k_phy_read()/qca8k_phy_write()
> nor the dsa framework is set up for that.
> 
> This version of the patch uses the existing phy-handle
> properties of each specified DSA Port in the DT to map
> each PORT/MAC to its exposed PHY on the MDIO bus. This
> is supported by the current binding document qca8k.txt
> as well.

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 cconfiguration.

> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a4b6cda38016..6558b7ed855d 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -11,6 +11,7 @@
>  #include <linux/netdevice.h>
>  #include <net/dsa.h>
>  #include <linux/of_net.h>
> +#include <linux/of_mdio.h>
>  #include <linux/of_platform.h>
>  #include <linux/if_bridge.h>
>  #include <linux/mdio.h>
> @@ -612,20 +613,50 @@ qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
>  	qca8k_port_set_status(priv, port, 1);
>  }
>  
> +static int
> +qca8k_to_real_phy(struct dsa_switch *ds, int phy)
> +{
> +	struct device_node *phy_dn, *port_dn;
> +	int id;
> +
> +	if (phy >= ds->num_ports)
> +		return -EINVAL;
> +
> +	port_dn = ds->ports[phy].dn;
> +	if (!port_dn)
> +		return -EINVAL;
> +
> +	phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
> +	if (!phy_dn)
> +		return phy;
> +
> +	id = of_mdio_parse_addr(ds->dev, phy_dn);
> +	of_node_put(phy_dn);
> +	return id;
> +}
> +
>  static int
>  qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> +	int realphy = qca8k_to_real_phy(ds, phy);
> +
> +	if (realphy < 0)
> +		return realphy;
>  
> -	return mdiobus_read(priv->bus, phy, regnum);
> +	return mdiobus_read(priv->bus, realphy, regnum);
>  }
>  
>  static int
>  qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> +	int realphy = qca8k_to_real_phy(ds, phy);
> +
> +	if (realphy < 0)
> +		return realphy;
>  
> -	return mdiobus_write(priv->bus, phy, regnum, val);
> +	return mdiobus_write(priv->bus, realphy, regnum, val);
>  }
>  
>  static void
> 


-- 
Florian

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

[Andrew Lunn]

On Mon, Feb 04, 2019 at 10:35:55PM +0100, Christian Lamparter wrote:
> The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4.
> Based on the System Block Diagram in Section 1.2 of the
> QCA8337's datasheet. These PHYs are internally connected
> to MACs of PORT 1 - PORT 5.

Hi Christian

Is the off-by-one the problem here?

Thanks
	Andrew

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

[Christian Lamparter]

Hello Andrew and Florian.

I concated both replies into this post.

On Monday, February 4, 2019 11:26:41 PM CET Andrew Lunn wrote:
> On Mon, Feb 04, 2019 at 10:35:55PM +0100, Christian Lamparter wrote:
> > The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4.
> > Based on the System Block Diagram in Section 1.2 of the
> > QCA8337's datasheet. These PHYs are internally connected
> > to MACs of PORT 1 - PORT 5.
> 
> Hi Christian
> 
> Is the off-by-one the problem here?
> 
Yes, this was triggered by a MII_PHYSID1 and MII_PHYSID2 read for a 
non-existent PHY at address >0x5< coming from dsa_register_switch()
during boot.

I added a WARN_ON to qca8k_phy_read() to see from where the reads are
coming from:

[    6.218168] WARNING: CPU: 0 PID: 21 at qca8k_phy_read+0x3c/0x68
[    6.224062] Modules linked in:
[    6.227107] CPU: 0 PID: 21 Comm: kworker/0:1 Tainted: G        W         4.19.16 #0
[    6.234732] Workqueue: events deferred_probe_work_func
[    6.239849] NIP:  c0308528 LR: c0308528 CTR: c0257d30
[    6.244878] REGS: cf485b80 TRAP: 0700   Tainted: G        W          (4.19.16)
[    6.252064] MSR:  00029000 <CE,EE,ME>  CR: 28002222  XER: 00000000
[    6.258224]
[    6.258224] GPR00: c0308528 cf485c30 cf47c000 00000005 00000000 00000215 c09d61e4 c09d3de0
[    6.258224] GPR08: 00021000 c09bfb00 c09bfb00 00000007 24002444 00000000 c003f81c cf466060
[    6.258224] GPR16: ffffffff cf633f60 cf633f6c 00000001 cf633f40 c0589f7c 006080c0 c09bede8
[    6.258224] GPR24: cf633f40 00000000 00000000 fffff000 00000003 cdf21790 00000003 00000005
[    6.292952] NIP [c0308528] qca8k_phy_read+0x3c/0x68
[    6.297808] LR [c0308528] qca8k_phy_read+0x3c/0x68
[    6.302574] Call Trace:
[    6.305013] [cf485c30] [c0308528] qca8k_phy_read+0x3c/0x68 (unreliable)
[    6.311602] [cf485c50] [c0300530] mdiobus_read+0x6c/0x9c
[    6.316894] [cf485c70] [c02ffdcc] get_phy_device+0x188/0x204
[    6.322527] [cf485cd0] [c0300740] mdiobus_scan+0x20/0x160
[    6.327901] [cf485d00] [c0300a3c] __mdiobus_register+0x1bc/0x2a8
[    6.333884] [cf485d30] [c047e690] dsa_register_switch+0x6a0/0x904
[    6.339954] [cf485db0] [c0300dd8] mdio_probe+0x40/0x88
[    6.345070] [cf485dc0] [c026947c] really_probe+0x168/0x300
[    6.350530] [cf485df0] [c0269b44] driver_probe_device+0x410/0x460
[    6.356594] [cf485e10] [c02672cc] bus_for_each_drv+0x5c/0xc0
[    6.362229] [cf485e40] [c0269270] __device_attach+0xa8/0x144
[    6.367862] [cf485e70] [c0268430] bus_probe_device+0x3c/0xc0
[    6.373495] [cf485e90] [c02689dc] deferred_probe_work_func+0x70/0xac
[    6.379821] [cf485eb0] [c003a2c4] process_one_work+0x25c/0x3b0
[    6.385633] [cf485ed0] [c003a708] worker_thread+0x2f0/0x434
[    6.391180] [cf485f10] [c003f950] kthread+0x134/0x138
[    6.396209] [cf485f40] [c000d23c] ret_from_kernel_thread+0x14/0x1c
[    6.402357] Instruction dump:
[    6.405313] 93c10018 7cbe2b78 93e1001c 7c9f2378 93a10014 83a30018 4bfffe4d 3d20c058
[    6.413027] 7c651b78 7fe4fb78 3869c970 4bd4e35d <0fe00000> 80010024 7fc5f378 807d0004
[    6.420916] ---[ end trace 00aeb6767b21cd36 ]---

If I disable port@5 (see qca8k.txt example)
<https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/dsa/qca8k.txt#L103>

then problem went away (but of course, this makes the WAN port useless).

When I looked more into it, I started to notice that the mdiobus_scan
started from address >1< (not 0). It was skipping PHY0 (which does exist!)
and then the extract from qca8k.txt suddenly made sense:

|The integrated switch subnode should be specified according to the binding
|described in dsa/dsa.txt. As the QCA8K switches do not have a N:N mapping of
|port and PHY id, each subnode describing a port needs to have a valid phandle
|referencing the internal PHY connected to it. The CPU port of this switch is
|always port 0.

From what I can tell, the slave mdio port-numbers (i.e 0 - 5/6 on the qca8k)
are being used directly as phy-addresses on the slave-bus. And since the port0
is a cpu port it gets skipped when the ds->phy_mii_mask is created in
dsa_switch_setup():

| ds->phys_mii_mask |= dsa_user_ports(ds);  // 0x3E

<https://elixir.bootlin.com/linux/v5.0-rc5/source/net/dsa/dsa2.c#L350>

However, ds->phys_mii_mask is used to calculate the slave's phy_mask in
dsa_slave_mii_bus_init()

|ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask; // ~0x3E

<https://github.com/torvalds/linux/blob/master/net/dsa/slave.c#L61>

which is then later used by __mdiobus_register() to scan for the
supposedly existing PHY at 0x1 - 0x5.

|	for (i = 0; i < PHY_MAX_ADDR; i++) {
|		if ((bus->phy_mask & (1 << i)) == 0) {
|			struct phy_device *phydev;
|
|			phydev = mdiobus_scan(bus, i);
|			if (IS_ERR(phydev) && (PTR_ERR(phydev) != -ENODEV)) {
|				err = PTR_ERR(phydev);
|				goto error;
|			}
|		}
|	}

So, I'm looking for a way to get this "-1" somewhere and this version
was the best justification I came up with. Because as Florian said,
this is supposed to work for different drivers as well.

---

On Monday, February 4, 2019 11:11:41 PM CET Florian Fainelli wrote:
> On 2/4/19 1:35 PM, Christian Lamparter wrote:
> > The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4.
> > Based on the System Block Diagram in Section 1.2 of the
> > QCA8337's datasheet. These PHYs are internally connected
> > to MACs of PORT 1 - PORT 5. However, neither qca8k's slave
> > mdio access functions qca8k_phy_read()/qca8k_phy_write()
> > nor the dsa framework is set up for that.
> > 
> > This version of the patch uses the existing phy-handle
> > properties of each specified DSA Port in the DT to map
> > each PORT/MAC to its exposed PHY on the MDIO bus. This
> > is supported by the current binding document qca8k.txt
> > as well.
> 
> 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 cconfiguration.

Hm, this sounds to me like the qca8k driver might be cheating a bit. Though,
I can't really tell, I found "stub" translation routines in the mv88e6060
driver called mv88e6060_port_to_phy_addr(). But it just checks the range.
I think the issue here is that qca8k_phy_read/write don't actually operate
on the "internal" mdio-bus of the switch. Instead the mdiobus_read/write
in qca8k_phy_read/write operates on the provided mdio-bus (by the 
ethernet-mac or gpio-mdio, etc...). But let's see what else can be done 
(maybe qca8k_phy_read/write() is just wrong and should be dropped?).

Regards,
Christian

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

[Andrew Lunn]

On Mon, Feb 04, 2019 at 10:35:55PM +0100, Christian Lamparter wrote:
> The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4.
> Based on the System Block Diagram in Section 1.2 of the
> QCA8337's datasheet. These PHYs are internally connected
> to MACs of PORT 1 - PORT 5. However, neither qca8k's slave
> mdio access functions qca8k_phy_read()/qca8k_phy_write()
> nor the dsa framework is set up for that.
> 
> This version of the patch uses the existing phy-handle
> properties of each specified DSA Port in the DT to map
> each PORT/MAC to its exposed PHY on the MDIO bus. This
> is supported by the current binding document qca8k.txt
> as well.

Hi Christian

Looking at Documentation/devicetree/bindings/net/dsa/qca8k.txt 

I think everything you need is already implemented. What problem do
you actually have?

Thanks
	Andrew

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

[Christian Lamparter]

On Tuesday, February 5, 2019 3:45:33 AM CET Andrew Lunn wrote:
> On Mon, Feb 04, 2019 at 10:35:55PM +0100, Christian Lamparter wrote:
> > The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4.
> > Based on the System Block Diagram in Section 1.2 of the
> > QCA8337's datasheet. These PHYs are internally connected
> > to MACs of PORT 1 - PORT 5. However, neither qca8k's slave
> > mdio access functions qca8k_phy_read()/qca8k_phy_write()
> > nor the dsa framework is set up for that.
> > 
> > This version of the patch uses the existing phy-handle
> > properties of each specified DSA Port in the DT to map
> > each PORT/MAC to its exposed PHY on the MDIO bus. This
> > is supported by the current binding document qca8k.txt
> > as well.
> 
> Hi Christian
> 
> Looking at Documentation/devicetree/bindings/net/dsa/qca8k.txt 
> 
> I think everything you need is already implemented. What problem do
> you actually have?

Thankfully, Florian's reply answered the question to me. The problem
has to do with the qca8k's slave qca8k_phy_read and qca8k_phy_write.
Here are the functions:

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

The trick here is that priv->bus is not the internal
bus instead it's set to the SoC's (external) mii bus in qca8k_sw_probe()).
So this isn't a slave bus! And as stated in the qca8k, the the external
and internal PHY bus are not mapped 1:1.

From what I can tell from the datasheet, the QCA8337N does have
dedicated MDIO master control register which is what is
needed here. It's at 0x003C. So these mdiobus_read/writes on the
SoCs MDIO bus would either need to be converted to read and write
to 0x003c... Or the qca8k_phy_read() and qca8k_phy_write could be
dropped all together. I've tested it on the WPQ864 and driver
works without since it was never a real slave bus there to begin with.

Thanks,
Christian
---
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 7e97e620bd44..a26850c888cf 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -620,22 +620,6 @@ qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
 	qca8k_port_set_status(priv, port, 1);
 }
 
-static int
-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);
-}
-
-static int
-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);
-}
-
 static void
 qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data)
 {
@@ -876,8 +860,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,

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

[Andrew Lunn]

> The trick here is that priv->bus is not the internal
> bus instead it's set to the SoC's (external) mii bus in qca8k_sw_probe()).
> So this isn't a slave bus! And as stated in the qca8k, the the external
> and internal PHY bus are not mapped 1:1.
> 
> >From what I can tell from the datasheet, the QCA8337N does have
> dedicated MDIO master control register which is what is
> needed here.

Ah, O.K, i was missing that bit. So yes, export an MDIO bus to Linux,
as the mv88e6xxx does.

   Andrew

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

[Christian Lamparter]

On Tuesday, February 5, 2019 2:09:35 PM CET Andrew Lunn wrote:
> > The trick here is that priv->bus is not the internal
> > bus instead it's set to the SoC's (external) mii bus in qca8k_sw_probe()).
> > So this isn't a slave bus! And as stated in the qca8k, the the external
> > and internal PHY bus are not mapped 1:1.
> > 
> > >From what I can tell from the datasheet, the QCA8337N does have
> > dedicated MDIO master control register which is what is
> > needed here.
> 
> Ah, O.K, i was missing that bit. So yes, export an MDIO bus to Linux,
> as the mv88e6xxx does.

I've attached of what I think is needed for this. Can you please take
a quick look and see if this is matches your expecation?

A nice side-effect of adding support for the internal mdio-bus is that
the chip's mdio is now accessible through its UART interface mode or
through data frames (The QCA8337 registers can be accessed
through special data frames from the CPU port).

As for this version, it boots up fine on the WPQ864 with the QCA8337N:

[    5.799639] libphy: dsa slave smi: probed
[    5.820129] qca8k 37000000.mdio-mii:10 lan1 (uninitialized): PHY [dsa-0.0:01] driver [Generic PHY]
[    5.822418] qca8k 37000000.mdio-mii:10 lan2 (uninitialized): PHY [dsa-0.0:02] driver [Generic PHY]
[    5.830154] qca8k 37000000.mdio-mii:10 lan3 (uninitialized): PHY [dsa-0.0:03] driver [Generic PHY]
[    5.839284] qca8k 37000000.mdio-mii:10 lan4 (uninitialized): PHY [dsa-0.0:04] driver [Generic PHY]
[    5.848048] qca8k 37000000.mdio-mii:10 wan (uninitialized): PHY [dsa-0.0:05] driver [Generic PHY]
[    5.856065] DSA: tree 0 setup

and it works as expected.

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.

Cheers,
Christian

---
From e4951353e5e25f725a777b3a380c16bf55b2b7c1 Mon Sep 17 00:00:00 2001
From: Christian Lamparter <chunkeey@gmail.com>
Date: Fri, 1 Feb 2019 22:54:32 +0100
Subject: [PATCH v2] net: dsa: qca8k: replace slave-bus implementation

The QCA8337 enumerates 5 PHYs on its internal MDIO access:
on address 0, 1, 2, 3 and 4. Based on the System Block
Diagram in Section 1.2 of the QCA8337's datasheet. These PHYs
connected to MACs of Port 1 to 5.

This version of the patch implements the QCA8337 MDIO
access through the switch's MDIO_MASTER register, which
makes it possible to forgo the current port-mapping in
the DT to map each Port.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---

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.
---
 .../devicetree/bindings/net/dsa/qca8k.txt     | 35 ++-----------
 drivers/net/dsa/qca8k.c                       | 50 +++++++++++++++++--
 drivers/net/dsa/qca8k.h                       | 12 +++++
 3 files changed, 62 insertions(+), 35 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a4b6cda38016..dfcfb0791965 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
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)
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index bbcb255c3150..0fcbc2fcfcf1 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -12,10 +12,7 @@ Required properties:
 Subnodes:
 
 The integrated switch subnode should be specified according to the binding
-described in dsa/dsa.txt. As the QCA8K switches do not have a N:N mapping of
-port and PHY id, each subnode describing a port needs to have a valid phandle
-referencing the internal PHY connected to it. The CPU port of this switch is
-always port 0.
+described in dsa/dsa.txt. The CPU port of this switch is always port 0.
 
 A CPU port node has the following optional node:
 
@@ -35,36 +32,17 @@ Example:
 
 
 	&mdio0 {
-		phy_port1: phy@0 {
-			reg = <0>;
-		};
-
-		phy_port2: phy@1 {
-			reg = <1>;
-		};
-
-		phy_port3: phy@2 {
-			reg = <2>;
-		};
-
-		phy_port4: phy@3 {
-			reg = <3>;
-		};
-
-		phy_port5: phy@4 {
-			reg = <4>;
-		};
-
-		switch0@0 {
+		switch@10 {
 			compatible = "qca,qca8337";
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-			reg = <0>;
+			reg = <0x10>;
 
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
+
 				port@0 {
 					reg = <0>;
 					label = "cpu";
@@ -79,31 +57,26 @@ Example:
 				port@1 {
 					reg = <1>;
 					label = "lan1";
-					phy-handle = <&phy_port1>;
 				};
 
 				port@2 {
 					reg = <2>;
 					label = "lan2";
-					phy-handle = <&phy_port2>;
 				};
 
 				port@3 {
 					reg = <3>;
 					label = "lan3";
-					phy-handle = <&phy_port3>;
 				};
 
 				port@4 {
 					reg = <4>;
 					label = "lan4";
-					phy-handle = <&phy_port4>;
 				};
 
 				port@5 {
 					reg = <5>;
 					label = "wan";
-					phy-handle = <&phy_port5>;
 				};
 			};
 		};
-- 
2.20.1

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

[Andrew Lunn]

> 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.

      Andrew

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

[Christian Lamparter]

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.

Regards,
Christian

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

[Florian Fainelli]

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.

> 
> Regards,
> Christian
> 
> 


-- 
Florian

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

[Christian Lamparter]

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 {

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

[Florian Fainelli]

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.

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.

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.

Cheers

> 
> 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 {
> 
> 
> 
> 
> 


-- 
Florian

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

[Florian Fainelli]

On 2/6/19 2:29 PM, 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.
> 
> 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.

Forgot to finish my sentence here. This should read:

Ports that have a 'phy-handle' property will either point to the QCA8k's
internal MDIO bus or an external one, but that will be transparently be
handled during PHY device creation.

Thanks!
-- 
Florian

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

[Christian Lamparter]

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@kernel.org>
    Signed-off-by: David S. Miller <davem@davemloft.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@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..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)