| Message ID | 1461854802-8142-1-git-send-email-nathan.sullivan@ni.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 28/04/2016 16:46, Nathan Sullivan a écrit : > Since of_mdiobus_register and mdiobus_register will scan automatically, > do not manually scan for PHY devices in the macb ethernet driver. Doing > so will result in many nonexistent PHYs on the MDIO bus if the MDIO > lines are floating or grounded, such as when they are not used. > > Signed-off-by: Nathan Sullivan <nathan.sullivan@ni.com> Well, as explained in the commit message that added this feature and in the comment, if no phy is specified in the DT we end up without phy... There are AT91 platforms which lack specification for the phy node in the DT. So, I don't know if there is a better way to deal with this case but I see this removal as risky. Bye, > --- > drivers/net/ethernet/cadence/macb.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c > index 48a7d7d..6506b4e 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -424,7 +424,7 @@ static int macb_mii_init(struct macb *bp) > { > struct macb_platform_data *pdata; > struct device_node *np; > - int err = -ENXIO, i; > + int err = -ENXIO; > > /* Enable management port */ > macb_writel(bp, NCR, MACB_BIT(MPE)); > @@ -450,23 +450,6 @@ static int macb_mii_init(struct macb *bp) > if (np) { > /* try dt phy registration */ > err = of_mdiobus_register(bp->mii_bus, np); > - > - /* fallback to standard phy registration if no phy were > - found during dt phy registration */ > - if (!err && !phy_find_first(bp->mii_bus)) { > - for (i = 0; i < PHY_MAX_ADDR; i++) { > - struct phy_device *phydev; > - > - phydev = mdiobus_scan(bp->mii_bus, i); > - if (IS_ERR(phydev)) { > - err = PTR_ERR(phydev); > - break; > - } > - } > - > - if (err) > - goto err_out_unregister_bus; > - } > } else { > if (pdata) > bp->mii_bus->phy_mask = pdata->phy_mask; >
On Thu, Apr 28, 2016 at 05:44:14PM +0200, Nicolas Ferre wrote: > Le 28/04/2016 16:46, Nathan Sullivan a écrit : > > Since of_mdiobus_register and mdiobus_register will scan automatically, > > do not manually scan for PHY devices in the macb ethernet driver. Doing > > so will result in many nonexistent PHYs on the MDIO bus if the MDIO > > lines are floating or grounded, such as when they are not used. > > > > Signed-off-by: Nathan Sullivan <nathan.sullivan@ni.com> > > Well, as explained in the commit message that added this feature and in > the comment, if no phy is specified in the DT we end up without phy... > > There are AT91 platforms which lack specification for the phy node in > the DT. So, I don't know if there is a better way to deal with this case > but I see this removal as risky. > > Bye, > > Nicolas Ferre Hmm, are AT91 platforms special in this regard? As far as I can tell, this driver (macb) and Marvell PXA are the only ethernet drivers that call mdiobus_scan directly, and PXA does it on a known address. I do see that there are trees that use macb and don't have a phy listed, which is unfortunate. Another way to fix our issue would be to consider all 0x0s a bad ID in mdiobus_scan, so grounded MDIO lines do not get PHYs scanned. Or we could add a DT property to disable the manual scan. I'm not sure what the correct solution is, do you have a preference?
> Hmm, are AT91 platforms special in this regard? As far as I can tell, this > driver (macb) and Marvell PXA are the only ethernet drivers that call > mdiobus_scan directly, and PXA does it on a known address. I do see that there > are trees that use macb and don't have a phy listed, which is unfortunate. How it is supposed to work is that you do one of two things: 1) Your device tree does not have an mdio node. In this case, you call mdiobus_register() and it will perform a scan of the bus, and find the phys. 2) Your device tree does have an MDIO node, and you list your PHYs. Having an MDIO node and not listing the PHYs is broken... There are however, a few broken device trees around, and a few drivers have workarounds. e.g. davinci_mdio.c /* register the mii bus * Create PHYs from DT only in case if PHY child nodes are explicitly * defined to support backward compatibility with DTs which assume that * Davinci MDIO will always scan the bus for PHYs detection. */ if (dev->of_node && of_get_child_count(dev->of_node)) { data->skip_scan = true; ret = of_mdiobus_register(data->bus, dev->of_node); } else { ret = mdiobus_register(data->bus); } You probably need to do the same for AT91, count the number of children, and it if it zero, fall back to the non-DT way. It would also be good to print a warning to get people to fix their device tree. Andrew
On Thu, Apr 28, 2016 at 06:32:07PM +0200, Andrew Lunn wrote: > > Hmm, are AT91 platforms special in this regard? As far as I can tell, this > > driver (macb) and Marvell PXA are the only ethernet drivers that call > > mdiobus_scan directly, and PXA does it on a known address. I do see that there > > are trees that use macb and don't have a phy listed, which is unfortunate. > > How it is supposed to work is that you do one of two things: > > 1) Your device tree does not have an mdio node. In this case, you call > mdiobus_register() and it will perform a scan of the bus, and find the > phys. > > 2) Your device tree does have an MDIO node, and you list your PHYs. > > Having an MDIO node and not listing the PHYs is broken... > > There are however, a few broken device trees around, and a few drivers > have workarounds. e.g. davinci_mdio.c > > /* register the mii bus > * Create PHYs from DT only in case if PHY child nodes are explicitly > * defined to support backward compatibility with DTs which assume that > * Davinci MDIO will always scan the bus for PHYs detection. > */ > if (dev->of_node && of_get_child_count(dev->of_node)) { > data->skip_scan = true; > ret = of_mdiobus_register(data->bus, dev->of_node); > } else { > ret = mdiobus_register(data->bus); > } > > You probably need to do the same for AT91, count the number of > children, and it if it zero, fall back to the non-DT way. It would > also be good to print a warning to get people to fix their device > tree. > > Andrew I agree that is a valid fix for AT91, however it won't solve our problem, since we have no children on the second ethernet MAC in our devices' device trees. I'm starting to feel like our second MAC shouldn't even really register the MDIO bus since it isn't being used - maybe adding a DT property to not have a bus is a better option?
> I agree that is a valid fix for AT91, however it won't solve our problem, since > we have no children on the second ethernet MAC in our devices' device trees. I'm > starting to feel like our second MAC shouldn't even really register the MDIO bus > since it isn't being used - maybe adding a DT property to not have a bus is a > better option? status = "disabled" would be the unusual way. Andrew
On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote: > > I agree that is a valid fix for AT91, however it won't solve our problem, since > > we have no children on the second ethernet MAC in our devices' device trees. I'm > > starting to feel like our second MAC shouldn't even really register the MDIO bus > > since it isn't being used - maybe adding a DT property to not have a bus is a > > better option? > > status = "disabled" > > would be the unusual way. > > Andrew Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on the MDIO bus of the first MAC. So, the second MAC is used for ethernet but not for MDIO, and so it does not have any PHYs under its DT node. It would be nice if there were a way to tell macb not to bother with MDIO for the second MAC, since that's handled by the first MAC. I guess a good longer-term solution to all these problems would be to treat the MAC and MDIO as seperate devices, like davinci seems to be doing.
On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote: > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote: > > > I agree that is a valid fix for AT91, however it won't solve our problem, since > > > we have no children on the second ethernet MAC in our devices' device trees. I'm > > > starting to feel like our second MAC shouldn't even really register the MDIO bus > > > since it isn't being used - maybe adding a DT property to not have a bus is a > > > better option? > > > > status = "disabled" > > > > would be the unusual way. > > > > Andrew > > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on the MDIO > bus of the first MAC. So, the second MAC is used for ethernet but not for MDIO, > and so it does not have any PHYs under its DT node. It would be nice if there > were a way to tell macb not to bother with MDIO for the second MAC, since that's > handled by the first MAC. Yes, exactly, add support for status = "disabled" in the mdio node. > I guess a good longer-term solution to all these problems would be to treat the > MAC and MDIO as seperate devices, like davinci seems to be doing. A few others do this as well, e.g. most Marvell devices. Andrew
On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote: > On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote: > > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote: > > > > I agree that is a valid fix for AT91, however it won't solve our problem, since > > > > we have no children on the second ethernet MAC in our devices' device trees. I'm > > > > starting to feel like our second MAC shouldn't even really register the MDIO bus > > > > since it isn't being used - maybe adding a DT property to not have a bus is a > > > > better option? > > > > > > status = "disabled" > > > > > > would be the unusual way. > > > > > > Andrew > > > > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on the MDIO > > bus of the first MAC. So, the second MAC is used for ethernet but not for MDIO, > > and so it does not have any PHYs under its DT node. It would be nice if there > > were a way to tell macb not to bother with MDIO for the second MAC, since that's > > handled by the first MAC. > > Yes, exactly, add support for status = "disabled" in the mdio node. Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively: the node representing the mdio bus is the same node which represents the macb instance itself. Setting 'status = "disabled"' on this node will just prevent the probing of the macb instance. Josh
On Thu, Apr 28, 2016 at 04:03:57PM -0500, Josh Cartwright wrote: > On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote: > > On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote: > > > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote: > > > > > I agree that is a valid fix for AT91, however it won't solve our problem, since > > > > > we have no children on the second ethernet MAC in our devices' device trees. I'm > > > > > starting to feel like our second MAC shouldn't even really register the MDIO bus > > > > > since it isn't being used - maybe adding a DT property to not have a bus is a > > > > > better option? > > > > > > > > status = "disabled" > > > > > > > > would be the unusual way. > > > > > > > > Andrew > > > > > > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on the MDIO > > > bus of the first MAC. So, the second MAC is used for ethernet but not for MDIO, > > > and so it does not have any PHYs under its DT node. It would be nice if there > > > were a way to tell macb not to bother with MDIO for the second MAC, since that's > > > handled by the first MAC. > > > > Yes, exactly, add support for status = "disabled" in the mdio node. > > Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively: > the node representing the mdio bus is the same node which represents the > macb instance itself. Setting 'status = "disabled"' on this node will > just prevent the probing of the macb instance. :-( It is very common to have an mdio node within the MAC node, for example imx6sx-sdb.dtsi &fec1 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_enet1>; phy-supply = <®_enet_3v3>; phy-mode = "rgmii"; phy-handle = <ðphy1>; status = "okay"; mdio { #address-cells = <1>; #size-cells = <0>; ethphy1: ethernet-phy@1 { reg = <1>; }; ethphy2: ethernet-phy@2 { reg = <2>; }; }; }; &fec2 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_enet2>; phy-mode = "rgmii"; phy-handle = <ðphy2>; status = "okay"; }; This even has the two phys on one bus, as you described... Andrew
On Thu, Apr 28, 2016 at 11:23:15PM +0200, Andrew Lunn wrote: > On Thu, Apr 28, 2016 at 04:03:57PM -0500, Josh Cartwright wrote: > > On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote: > > > On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote: > > > > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote: > > > > > > I agree that is a valid fix for AT91, however it won't solve our problem, since > > > > > > we have no children on the second ethernet MAC in our devices' device trees. I'm > > > > > > starting to feel like our second MAC shouldn't even really register the MDIO bus > > > > > > since it isn't being used - maybe adding a DT property to not have a bus is a > > > > > > better option? > > > > > > > > > > status = "disabled" > > > > > > > > > > would be the unusual way. > > > > > > > > > > Andrew > > > > > > > > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on the MDIO > > > > bus of the first MAC. So, the second MAC is used for ethernet but not for MDIO, > > > > and so it does not have any PHYs under its DT node. It would be nice if there > > > > were a way to tell macb not to bother with MDIO for the second MAC, since that's > > > > handled by the first MAC. > > > > > > Yes, exactly, add support for status = "disabled" in the mdio node. > > > > Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively: > > the node representing the mdio bus is the same node which represents the > > macb instance itself. Setting 'status = "disabled"' on this node will > > just prevent the probing of the macb instance. > > :-( > > It is very common to have an mdio node within the MAC node, for example imx6sx-sdb.dtsi Okay, I think that makes sense. I think, then, perhaps the solution to our problem is to: 1. Modify the macb driver to support an 'mdio' node. (And adjust the binding document accordingly). If the node is found, it's used for of_mdiobus_register() w/o any of the manual scan madness. 2. For backwards compatibility, in the case where an 'mdio' node does not exist, leave the existing behavior the way it is now (of_mdiobus_register() followed by manual scan) [perhaps warn of deprecation as well?] 3. Update binding docs to reflect the above. In this way, for our usecase, the 'status = "disabled"' in the newly created 'mdio' node isn't necessary. It's sufficient for the node to exist and be empty. > &fec1 { > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_enet1>; > phy-supply = <®_enet_3v3>; > phy-mode = "rgmii"; > phy-handle = <ðphy1>; > status = "okay"; > > mdio { > #address-cells = <1>; > #size-cells = <0>; > > ethphy1: ethernet-phy@1 { > reg = <1>; > }; > > ethphy2: ethernet-phy@2 { > reg = <2>; > }; > }; > }; > > &fec2 { > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_enet2>; > phy-mode = "rgmii"; > phy-handle = <ðphy2>; > status = "okay"; > }; > > This even has the two phys on one bus, as you described... Yep...looks nearly exactly the same case. Thanks, Josh
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index 48a7d7d..6506b4e 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -424,7 +424,7 @@ static int macb_mii_init(struct macb *bp) { struct macb_platform_data *pdata; struct device_node *np; - int err = -ENXIO, i; + int err = -ENXIO; /* Enable management port */ macb_writel(bp, NCR, MACB_BIT(MPE)); @@ -450,23 +450,6 @@ static int macb_mii_init(struct macb *bp) if (np) { /* try dt phy registration */ err = of_mdiobus_register(bp->mii_bus, np); - - /* fallback to standard phy registration if no phy were - found during dt phy registration */ - if (!err && !phy_find_first(bp->mii_bus)) { - for (i = 0; i < PHY_MAX_ADDR; i++) { - struct phy_device *phydev; - - phydev = mdiobus_scan(bp->mii_bus, i); - if (IS_ERR(phydev)) { - err = PTR_ERR(phydev); - break; - } - } - - if (err) - goto err_out_unregister_bus; - } } else { if (pdata) bp->mii_bus->phy_mask = pdata->phy_mask;
Since of_mdiobus_register and mdiobus_register will scan automatically, do not manually scan for PHY devices in the macb ethernet driver. Doing so will result in many nonexistent PHYs on the MDIO bus if the MDIO lines are floating or grounded, such as when they are not used. Signed-off-by: Nathan Sullivan <nathan.sullivan@ni.com> --- drivers/net/ethernet/cadence/macb.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)