Message ID | 20171016104525.26810-1-mnhu@prevas.dk |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] dsa: slave: support phy devices on external MII bus | expand |
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
> /* 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
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
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
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
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
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?
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);
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(-)