[PATCH net-next] dsa: slave: support phy devices on external MII bus

[PATCH net-next] dsa: slave: support phy devices on external MII bus

[Martin Hundebøll]

When configuring a switch port to use an external phy, the phy is
connected to external switch MII bus:

                    +---------+
 +-----+  MII cpu   | Switch  |  MII ext   +-----+
 | CPU | ---------> |---------| ---------> | PHY |
 +-----+            | MII int |            +-----+
                    +---------+

In the device tree, the configuration looks something like this:

/ {
  /* ... */

  mdio {
    /* phy on MII cpu */
    phy0@0 {
      reg = <0>;
    };

    /* switch on MII cpu */
    switch0@1 {
      reg = <1>;

      ports {
        /* port internal phy */
        port1@1 {
          reg = <1>;
          label = "lan1";
          phy-handle <&switch0phy1>;
        };

        /* port with external phy */
        port0@0 {
          reg = <0>;
          label = "lan0";
          phy-handle <&switch0phy0>;
        };
      };

      /* internal MII */
      mdio {
        switch0phy1@1 {
          reg = <1>;
        };
      };

      /* external MII */
      mdio1 {
        switch0phy0: switch0phy0@0 {
          reg = <0>;
        };
      };
    };
  };

  /* ... */
};

When connecting port0 to switch0phy0, the current dsa code assumes the
phy to be connected to the internal MII bus, and thus fails with

    mv88e6085 f1072004.mdio-mii:02 lan0: no phy at 0
    mv88e6085 f1072004.mdio-mii:02 lan0: failed to connect to phy0: -19
    mvneta f1034000.ethernet eth2: error -19 setting up slave phy
    mv88e6085 f1072004.mdio-mii:02: Failed to create slave 0: -19

Fix this by using the phy of-handle to obtain a reference to the parent
mdio_bus, which is then used to connect the phy.

Signed-off-by: Martin Hundebøll <mnhu@prevas.dk>
---
 net/dsa/slave.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 45f4ea845c07..62ad69a728be 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -976,12 +976,16 @@ static int dsa_slave_fixed_link_update(struct net_device *dev,
 }
 
 /* slave device setup *******************************************************/
-static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
+static int dsa_slave_phy_connect(struct net_device *slave_dev,
+				 struct mii_bus *bus, int addr)
 {
 	struct dsa_slave_priv *p = netdev_priv(slave_dev);
 	struct dsa_switch *ds = p->dp->ds;
 
-	slave_dev->phydev = mdiobus_get_phy(ds->slave_mii_bus, addr);
+	if (!bus)
+		bus = ds->slave_mii_bus;
+
+	slave_dev->phydev = mdiobus_get_phy(bus, addr);
 	if (!slave_dev->phydev) {
 		netdev_err(slave_dev, "no phy at %d\n", addr);
 		return -ENODEV;
@@ -1029,6 +1033,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 
 	if (phy_dn) {
 		int phy_id = of_mdio_parse_addr(&slave_dev->dev, phy_dn);
+		struct mii_bus *bus = of_mdio_find_bus(phy_dn->parent);
 
 		/* If this PHY address is part of phys_mii_mask, which means
 		 * that we need to divert reads and writes to/from it, then we
@@ -1037,7 +1042,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 		 */
 		if (!phy_is_fixed && phy_id >= 0 &&
 		    (ds->phys_mii_mask & (1 << phy_id))) {
-			ret = dsa_slave_phy_connect(slave_dev, phy_id);
+			ret = dsa_slave_phy_connect(slave_dev, bus, phy_id);
 			if (ret) {
 				netdev_err(slave_dev, "failed to connect to phy%d: %d\n", phy_id, ret);
 				of_node_put(phy_dn);
@@ -1061,7 +1066,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 	 * MDIO bus instead
 	 */
 	if (!slave_dev->phydev) {
-		ret = dsa_slave_phy_connect(slave_dev, p->dp->index);
+		ret = dsa_slave_phy_connect(slave_dev, NULL, p->dp->index);
 		if (ret) {
 			netdev_err(slave_dev, "failed to connect to port %d: %d\n",
 				   p->dp->index, ret);
-- 
2.14.2

Re: [PATCH net-next] dsa: slave: support phy devices on external MII bus

[Andrew Lunn]

On Mon, Oct 16, 2017 at 12:45:25PM +0200, Martin Hundebøll wrote:
> When configuring a switch port to use an external phy, the phy is
> connected to external switch MII bus:

Hi Martin

So this is a 6390?

So this used to work. I have a 10G phy connected to the external MII
bus on a 6390. I wonder when this got broken? Supporting phy-handle is
old code, so when i added the external MII i don't think i needed to
change any generic code.

I will take a closer look.

Thanks
  Andrew

Re: [PATCH net-next] dsa: slave: support phy devices on external MII bus

[Andrew Lunn]

>       /* internal MII */
>       mdio {
>         switch0phy1@1 {
>           reg = <1>;
>         };
>       };
> 
>       /* external MII */
>       mdio1 {
>         switch0phy0: switch0phy0@0 {
>           reg = <0>;
>         };

Hi Martin

You are missing a compatible string here. The binding document says:

- mdio?         : Container of PHYs and devices on the external MDIO
                          bus. The node must contains a compatible string of
                          "marvell,mv88e6xxx-mdio-external"

	  Andrew

Re: [PATCH net-next] dsa: slave: support phy devices on external MII bus

[Martin Hundebøll]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

Hi Andrew,

On 2017-10-16 14:40, Andrew Lunn wrote:
>>        /* internal MII */
>>        mdio {
>>          switch0phy1@1 {
>>            reg = <1>;
>>          };
>>        };
>>
>>        /* external MII */
>>        mdio1 {
>>          switch0phy0: switch0phy0@0 {
>>            reg = <0>;
>>          };
> 
> Hi Martin
> 
> You are missing a compatible string here. The binding document says:
> 
> - mdio?         : Container of PHYs and devices on the external MDIO
>                            bus. The node must contains a compatible string of
>                            "marvell,mv88e6xxx-mdio-external"
> 
> 	  Andrew
> 

Yeah, I have it in my full dts file (attached snippet), but decided to 
limit the commit-message version to keep it short(er). Should I update 
the commit message to avoid confusing others?

The issue is really that dsa_slave_phy_connect() always uses the the 
mdio bus associated with struct dsa_switch, even when the phy-handle 
refers to a phy from another mdio bus.

Or am I missing something ?

// Martin

[-- Attachment #2: armada-388-gp.dts --]
[-- Type: audio/vnd.dts, Size: 2311 bytes --]

Re: [PATCH net-next] dsa: slave: support phy devices on external MII bus

[Martin Hundebøll]

On 2017-10-16 14:32, Andrew Lunn wrote:
> On Mon, Oct 16, 2017 at 12:45:25PM +0200, Martin Hundebøll wrote:
>> When configuring a switch port to use an external phy, the phy is
>> connected to external switch MII bus:
> 
> So this is a 6390?

6390X

> So this used to work. I have a 10G phy connected to the external MII
> bus on a 6390. I wonder when this got broken? Supporting phy-handle is
> old code, so when i added the external MII i don't think i needed to
> change any generic code.

I had debug printing verifying that the external phy got registered with 
mdiobus_register_device(), and that dsa_slave_phy_connect() looked in 
the wrong mdio_map[].

// Martin

Re: [PATCH net-next] dsa: slave: support phy devices on external MII bus

[Martin Hundebøll]

On 2017-10-16 14:32, Andrew Lunn wrote:
> So this used to work. I have a 10G phy connected to the external MII
> bus on a 6390. I wonder when this got broken? Supporting phy-handle is
> old code, so when i added the external MII i don't think i needed to
> change any generic code.

It could look like commit cd28a1a9baee7 ('net: dsa: fully divert PHY 
reads/writes if requested') changed the of-case to use the mdio bus 
associated with struct dsa_switch unconditionally.

// Martin

Re: [PATCH net-next] dsa: slave: support phy devices on external MII bus

[Andrew Lunn]

On Mon, Oct 16, 2017 at 03:16:51PM +0200, Martin Hundebøll wrote:
> On 2017-10-16 14:32, Andrew Lunn wrote:
> >So this used to work. I have a 10G phy connected to the external MII
> >bus on a 6390. I wonder when this got broken? Supporting phy-handle is
> >old code, so when i added the external MII i don't think i needed to
> >change any generic code.
> 
> It could look like commit cd28a1a9baee7 ('net: dsa: fully divert PHY
> reads/writes if requested') changed the of-case to use the mdio bus
> associated with struct dsa_switch unconditionally.

Hi Martin

I think ds->phys_mii_mask is playing a role here. I need to add some
debug prints to my setup and see what is happening with my external
10G PHY.

This phy code is just too complex :-(

     Andrew

Re: [PATCH net-next] dsa: slave: support phy devices on external MII bus

[Florian Fainelli]

On October 16, 2017 6:39:43 AM PDT, Andrew Lunn <andrew@lunn.ch> wrote:
>On Mon, Oct 16, 2017 at 03:16:51PM +0200, Martin Hundebøll wrote:
>> On 2017-10-16 14:32, Andrew Lunn wrote:
>> >So this used to work. I have a 10G phy connected to the external MII
>> >bus on a 6390. I wonder when this got broken? Supporting phy-handle
>is
>> >old code, so when i added the external MII i don't think i needed to
>> >change any generic code.
>> 
>> It could look like commit cd28a1a9baee7 ('net: dsa: fully divert PHY
>> reads/writes if requested') changed the of-case to use the mdio bus
>> associated with struct dsa_switch unconditionally.
>
>Hi Martin
>
>I think ds->phys_mii_mask is playing a role here. I need to add some
>debug prints to my setup and see what is happening with my external
>10G PHY.

FWIW, bcm_sf2 uses a mixture of PHYs referenced by phandle and fixed PHY so if this did not work, I would have noticed.

The logic goes like this:

- try to connect to the PHY via phy-handle
- if we have a PHY we are connecting via phy-handle but we need to divert MDIO reads/writes connect using its address on the diverted bus
- connect using a fixed PHY
- finally try using the DSA slave MII bus which would connect to the switch internal PHYs

If 1) fails then that needs investigating as it really should not. Is it somehow possible that your PHY is powered down or something at that time and there is a timing/dependency not well handled?

-- 
Florian