On Wednesday, February 15, 2017 3:23:08 PM CET Andrew Lunn wrote: > > > > Is the PHY just powered down by chance (BMCR_PWRDN set?) and resetting > > > > it implicitly clears the power down that seems to be what is going on. > > > > > > Yes, the PHY is just in the BMCR_PDOWN state. I can do the same > > > on the WNDR4700, by messing with u-boot: > > Hi Christian > > What happens if you list the PHYs in the device tree, with their PHY > ID. That should avoid it looking for the ID and getting 0xffff > back. It should just probe the correct PHY driver. If the first thing > the drivers probe function does it reset the power down bit, it might > work.
I just tested it. And it didn't work (Same result/error). It fails because the PHYs on the dsa slave-bus are not detected via the device tree method. See <http://lxr.free-electrons.com/source/net/dsa/dsa2.c#L322> |315 if (!ds->slave_mii_bus && ds->ops->phy_read) { |316 ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev); |317 if (!ds->slave_mii_bus) |318 return -ENOMEM; |319 |320 dsa_slave_mii_bus_init(ds); |321 |322 err = mdiobus_register(ds->slave_mii_bus); |323 if (err < 0) |324 return err; |325 } You see that dsa2 just calls mdiobus_register() which will do the PHY discovery with mdiobus_scan() <http://lxr.free-electrons.com/source/drivers/net/phy/mdio_bus.c#L335> which relys on the values from MII_PHYSID1 and MII_PHYSID2. In order to get it work, this mdiobus_register would need to be replaced with of_mdiobus_register(). --- diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 737be6470c7f..5a90ec81040f 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -18,6 +18,7 @@ #include <net/dsa.h> #include <linux/of.h> #include <linux/of_net.h> +#include <linux/of_mdio.h> #include "dsa_priv.h" static LIST_HEAD(dsa_switch_trees); @@ -288,7 +289,8 @@ static void dsa_user_port_unapply(struct dsa_port *port, u32 index, } } -static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds) +static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds, + struct device_node *ports) { struct dsa_port *port; u32 index; @@ -322,7 +324,10 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds) dsa_slave_mii_bus_init(ds); - err = mdiobus_register(ds->slave_mii_bus); + if (!ports) + err = mdiobus_register(ds->slave_mii_bus); + else + err = of_mdiobus_register(ds->slave_mii_bus, ports); if (err < 0) return err; } @@ -383,7 +388,8 @@ static void dsa_ds_unapply(struct dsa_switch_tree *dst, struct dsa_switch *ds) dsa_switch_unregister_notifier(ds); } -static int dsa_dst_apply(struct dsa_switch_tree *dst) +static int dsa_dst_apply(struct dsa_switch_tree *dst, + struct device_node *ports) { struct dsa_switch *ds; u32 index; @@ -394,7 +400,7 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst) if (!ds) continue; - err = dsa_ds_apply(dst, ds); + err = dsa_ds_apply(dst, ds, ports); if (err) return err; } @@ -649,7 +655,7 @@ static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev) struct dsa_chip_data *pdata = dev->platform_data; struct device_node *np = dev->of_node; struct dsa_switch_tree *dst; - struct device_node *ports; + struct device_node *ports = NULL; u32 tree, index; int i, err; @@ -722,7 +728,7 @@ static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev) goto out_del_dst; } - err = dsa_dst_apply(dst); + err = dsa_dst_apply(dst, ports); if (err) { dsa_dst_unapply(dst); goto out_del_dst; --- With this patch applied, the device discovery works as intended: | [ 4.514106] Generic PHY 4ef600c00.ethern:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=4ef600c00.ethern:00, irq=-1) |[ 4.525975] Generic PHY dsa-0.0:01: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:01, irq=-1) |[ 4.536269] Generic PHY dsa-0.0:02: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:02, irq=-1) |[ 4.546562] Generic PHY dsa-0.0:03: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:03, irq=-1) |[ 4.556887] Generic PHY dsa-0.0:04: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:04, irq=-1) >From what I can tell, the PHY works. Although it will ping-pong for a bit: |[ 170.463823] qca8k 4ef600c00.ethern:10 lan1: Link is Down |[ 172.511860] qca8k 4ef600c00.ethern:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx |[ 174.559823] qca8k 4ef600c00.ethern:10 lan1: Link is Down |[ 175.583853] qca8k 4ef600c00.ethern:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx |[ 179.679816] qca8k 4ef600c00.ethern:10 lan1: Link is Down |[ 182.751846] qca8k 4ef600c00.ethern:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx |[ 186.847834] qca8k 4ef600c00.ethern:10 lan1: Link is Down |[ 188.895847] qca8k 4ef600c00.ethern:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx (But it will stay up). As for the patch. It's a bit cheesy because it forces you to specify the compatible property on the "dsa_slave_bus": | mdio { | #address-cells = <1>; | #size-cells = <0>; | ... | phy_port2: phy@1 { /* this phy is PDOWNed */ | compatible = "ethernet-phy-id004d.d034", "ethernet-phy-ieee802.3-c22"; | ^^ this is ignored! | reg = <1>; | }; | ... | switch0@16 { | compatible = "qca,qca8337"; | #address-cells = <1>; | #size-cells = <0>; | reg = <16>; | ports { | #address-cells = <1>; | #size-cells = <0>; | ... | port@2 { | compatible = "ethernet-phy-id004d.d034", "ethernet-phy-ieee802.3-c22"; | // ^^ This compatible string is parsed. However of_mdiobus_register() | // should look up the ignored compatible string from the phy_handle | // property down below. | phy-handle = <&phy_port2>; | // ^^ since this is the real device on the mdiobus. And this is going | // to be tricky since the switch itself it there as well. | reg = <1>; | label = "lan3"; | }; | ... | }; So, anyone: What would be a good solution for this? Thanks, Christian
