Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Mon, May 02, 2016 at 12:08:50PM -0700, Florian Fainelli wrote:
> On 02/05/16 11:36, Josh Cartwright wrote:
> > On Fri, Apr 29, 2016 at 02:40:53PM +0200, Nicolas Ferre wrote:
> > [..]
> >>>  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));
> >>> @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
> >>>   dev_set_drvdata(>dev->dev, bp->mii_bus);
> >>>  
> >>>   np = bp->pdev->dev.of_node;
> >>> - 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;
> >>> -
> >>> - err = mdiobus_register(bp->mii_bus);
> >>> - }
> >>> + if (np)
> >>> + err = macb_mii_of_init(bp, np);
> >>> + else
> >>> + err = macb_mii_pdata_init(bp, pdata);
> >>>  
> >>>   if (err)
> >>>   goto err_out_free_mdiobus;
> >>
> >> I'm okay with this. Thanks for having taken the initiative to implement it.
> > 
> > Unfortunately, I don't think it's going to be as straightforward
> > as I originally thought.  Still doable, but more complicated.
> > 
> > In particular, the macb bindings allow for a user to specify a
> > 'reset-gpios' property _at the PHY_ level, which is consumed by the
> > macb to adjust the PHY reset state on remove.
> 
> In fact, not just on remove, anytime there is an opportunity to save
> power (interface down, closed) and putting the PHY into reset is usually
> guaranteed to be saving more power than e.g: a BMCR power down.

I can understand how that might have been a long term goal of managing a
reset GPIO in general, however as it stands in the macb driver the only
callsite where the reset gpio is tweaked macb_remove().

> > My question is: why is the PHY reset GPIO management not the
> > responsibility of the PHY driver/core itself?
> 
> Well, this is actually being worked on at the moment by Sergei, since
> there is not necessarily a reason why PHYLIB can't deal with that:
> 
> https://lkml.org/lkml/2016/4/28/831

Cool, thanks.  I was about to see about implementing this...but since
it's already been done, I'll rebase my set on Sergei's changes.

Thanks,
  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Florian Fainelli]

On 02/05/16 11:36, Josh Cartwright wrote:
> On Fri, Apr 29, 2016 at 02:40:53PM +0200, Nicolas Ferre wrote:
> [..]
>>>  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));
>>> @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
>>> dev_set_drvdata(>dev->dev, bp->mii_bus);
>>>  
>>> np = bp->pdev->dev.of_node;
>>> -   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;
>>> -
>>> -   err = mdiobus_register(bp->mii_bus);
>>> -   }
>>> +   if (np)
>>> +   err = macb_mii_of_init(bp, np);
>>> +   else
>>> +   err = macb_mii_pdata_init(bp, pdata);
>>>  
>>> if (err)
>>> goto err_out_free_mdiobus;
>>
>> I'm okay with this. Thanks for having taken the initiative to implement it.
> 
> Unfortunately, I don't think it's going to be as straightforward
> as I originally thought.  Still doable, but more complicated.
> 
> In particular, the macb bindings allow for a user to specify a
> 'reset-gpios' property _at the PHY_ level, which is consumed by the
> macb to adjust the PHY reset state on remove.

In fact, not just on remove, anytime there is an opportunity to save
power (interface down, closed) and putting the PHY into reset is usually
guaranteed to be saving more power than e.g: a BMCR power down.

> 
> My question is: why is the PHY reset GPIO management not the
> responsibility of the PHY driver/core itself?

Well, this is actually being worked on at the moment by Sergei, since
there is not necessarily a reason why PHYLIB can't deal with that:

https://lkml.org/lkml/2016/4/28/831
-- 
Florian

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Fri, Apr 29, 2016 at 02:40:53PM +0200, Nicolas Ferre wrote:
[..]
> >  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));
> > @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
> > dev_set_drvdata(>dev->dev, bp->mii_bus);
> >  
> > np = bp->pdev->dev.of_node;
> > -   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;
> > -
> > -   err = mdiobus_register(bp->mii_bus);
> > -   }
> > +   if (np)
> > +   err = macb_mii_of_init(bp, np);
> > +   else
> > +   err = macb_mii_pdata_init(bp, pdata);
> >  
> > if (err)
> > goto err_out_free_mdiobus;
> 
> I'm okay with this. Thanks for having taken the initiative to implement it.

Unfortunately, I don't think it's going to be as straightforward
as I originally thought.  Still doable, but more complicated.

In particular, the macb bindings allow for a user to specify a
'reset-gpios' property _at the PHY_ level, which is consumed by the
macb to adjust the PHY reset state on remove.

My question is: why is the PHY reset GPIO management not the
responsibility of the PHY driver/core itself?

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Andrew Lunn]

> > +static int macb_mii_of_init(struct macb *bp, struct device_node *np)
> > +{
> > +   struct device_node *mdio;
> > +   int err, i;
> > +
> > +   mdio = of_get_child_by_name(np, "mdio");
> > +   if (mdio)
> > +   return of_mdiobus_register(bp->mii_bus, mdio);
> > +
> > +   dev_warn(>pdev->dev,
> > +"using deprecated PHY probing mechanism.  Please update device 
> > tree.");
> 
> Do we need to warn here?
> 
> Too bad I was not aware of that earlier, I even updated some of my DTs
> recently with only a phy node without the "mdio" one as parents :-\

It is messy. Unfortunately, there is no binding documentation (yet)
suggesting the right way to do this. And as a result, we have
drivers/device trees doing different things, leading to workarounds
like manually scanning the bus, not listing PHYs in the device tree
and so or falling back to the old methods, etc.

We need to document how we expect this to be done, and then add
warnings in various places to encourage developers to migrate their
device trees to what has been documented.

   Andrew

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Andrew Lunn]

> diff --git a/drivers/net/ethernet/cadence/macb.c 
> b/drivers/net/ethernet/cadence/macb.c
> index eec3200..d843bc9 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -419,11 +419,62 @@ static int macb_mii_probe(struct net_device *dev)
>   return 0;
>  }
>  
> +static int macb_mii_of_init(struct macb *bp, struct device_node *np)
> +{
> + struct device_node *mdio;
> + int err, i;
> +
> + mdio = of_get_child_by_name(np, "mdio");
> + if (mdio)
> + return of_mdiobus_register(bp->mii_bus, mdio);

We want to encourage driver writers to use an mdio subnode inside
there MAC node. So i wounder if this looking for the child and using
it should go into the core code?

Florian: What do you think?

> +
> + dev_warn(>pdev->dev,
> +  "using deprecated PHY probing mechanism.  Please update device 
> tree.");
> +
> + /* try dt phy registration */
> + err = of_mdiobus_register(bp->mii_bus, np);
> + if (err)
> + return err;
> +
> + /* fallback to standard phy registration if no phy were
> +  * found during dt phy registration
> +  */
> + if (!phy_find_first(bp->mii_bus)) {

I would also suggest putting a warning here, saying that PHYs should
be listed in the device tree.

> + 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);

FYI: There is a change making its way through which will mean
mdiobus_scan() will return -ENODEV where there is nothing on the bus
at that address, rather than the current NULL. You will need to adopt
this here.

 Andrew

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Nicolas Ferre]

Le 29/04/2016 14:25, Josh Cartwright a écrit :
> On Thu, Apr 28, 2016 at 07:34:59PM -0500, Josh Cartwright wrote:
>> 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.
> 
> Here's a (only build tested) attempt at implementing a part of this.  I
> macb_mii_init() was getting complicated enough that I lifted out two
> helper functions for the dt/no-dt case.  Sweeping the in-tree
> devicetrees to update them to place phys under an 'mdio' node is still
> to be done.
> 
>   Josh
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c 
> b/drivers/net/ethernet/cadence/macb.c
> index eec3200..d843bc9 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -419,11 +419,62 @@ static int macb_mii_probe(struct net_device *dev)
>   return 0;
>  }
>  
> +static int macb_mii_of_init(struct macb *bp, struct device_node *np)
> +{
> + struct device_node *mdio;
> + int err, i;
> +
> + mdio = of_get_child_by_name(np, "mdio");
> + if (mdio)
> + return of_mdiobus_register(bp->mii_bus, mdio);
> +
> + dev_warn(>pdev->dev,
> +  "using deprecated PHY probing mechanism.  Please update device 
> tree.");

Do we need to warn here?

Too bad I was not aware of that earlier, I even updated some of my DTs
recently with only a phy node without the "mdio" one as parents :-\


> + /* try dt phy registration */
> + err = of_mdiobus_register(bp->mii_bus, np);
> + if (err)
> + return err;
> +
> + /* fallback to standard phy registration if no phy were
> +  * found during dt phy registration
> +  */
> + if (!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;
> + }
> +
> + return err;
> +
> +err_out_unregister_bus:
> + mdiobus_unregister(bp->mii_bus);
> + return err;
> +}
> +
> +static int macb_mii_pdata_init(struct macb *bp,
> +struct macb_platform_data *pdata)
> +{
> + if (pdata)
> + bp->mii_bus->phy_mask = pdata->phy_mask;
> +
> + return mdiobus_register(bp->mii_bus);
> +}
> +
>  static int 

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 07:34:59PM -0500, Josh Cartwright wrote:
> 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.

Here's a (only build tested) attempt at implementing a part of this.  I
macb_mii_init() was getting complicated enough that I lifted out two
helper functions for the dt/no-dt case.  Sweeping the in-tree
devicetrees to update them to place phys under an 'mdio' node is still
to be done.

  Josh

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index eec3200..d843bc9 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -419,11 +419,62 @@ static int macb_mii_probe(struct net_device *dev)
return 0;
 }
 
+static int macb_mii_of_init(struct macb *bp, struct device_node *np)
+{
+   struct device_node *mdio;
+   int err, i;
+
+   mdio = of_get_child_by_name(np, "mdio");
+   if (mdio)
+   return of_mdiobus_register(bp->mii_bus, mdio);
+
+   dev_warn(>pdev->dev,
+"using deprecated PHY probing mechanism.  Please update device 
tree.");
+
+   /* try dt phy registration */
+   err = of_mdiobus_register(bp->mii_bus, np);
+   if (err)
+   return err;
+
+   /* fallback to standard phy registration if no phy were
+* found during dt phy registration
+*/
+   if (!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;
+   }
+
+   return err;
+
+err_out_unregister_bus:
+   mdiobus_unregister(bp->mii_bus);
+   return err;
+}
+
+static int macb_mii_pdata_init(struct macb *bp,
+  struct macb_platform_data *pdata)
+{
+   if (pdata)
+   bp->mii_bus->phy_mask = pdata->phy_mask;
+
+   return mdiobus_register(bp->mii_bus);
+}
+
 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 

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

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.

>  {
> pinctrl-names = "default";
> pinctrl-0 = <_enet1>;
> phy-supply = <_enet_3v3>;
> phy-mode = "rgmii";
> phy-handle = <>;
> status = "okay";
> 
> mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> 
> ethphy1: ethernet-phy@1 {
> reg = <1>;
> };
> 
> ethphy2: ethernet-phy@2 {
> reg = <2>;
> };
> };
> };
> 
>  {
> pinctrl-names = "default";
> pinctrl-0 = <_enet2>;
> phy-mode = "rgmii";
> phy-handle = <>;
> status = "okay";
> };
> 
> This even has the two phys on one bus, as you described...

Yep...looks nearly exactly the same case.

Thanks,
  Josh

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Andrew Lunn]

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

 {
pinctrl-names = "default";
pinctrl-0 = <_enet1>;
phy-supply = <_enet_3v3>;
phy-mode = "rgmii";
phy-handle = <>;
status = "okay";

mdio {
#address-cells = <1>;
#size-cells = <0>;

ethphy1: ethernet-phy@1 {
reg = <1>;
};

ethphy2: ethernet-phy@2 {
reg = <2>;
};
};
};

 {
pinctrl-names = "default";
pinctrl-0 = <_enet2>;
phy-mode = "rgmii";
phy-handle = <>;
status = "okay";
};

This even has the two phys on one bus, as you described...

 Andrew

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

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


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Andrew Lunn]

On Thu, Apr 28, 2016 at 01:03:15PM -0700, Florian Fainelli wrote:
> On 28/04/16 11:59, 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.
> 
> Something like that, just so we do not have to sprinkle tests all other
> the place:
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index b622b33dbf93..2f497790be1b 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -209,6 +209,10 @@ int of_mdiobus_register(struct mii_bus *mdio,
> struct device_node *np)
> bool scanphys = false;
> int addr, rc;
> 
> +   /* Do not continue if the node is disabled */
> +   if (!of_device_is_available(np))
> +   return -EINVAL;
> +
> /* Mask out all PHYs from auto probing.  Instead the PHYs listed in
>  * the device tree are populated after the bus has been
> registered */
> mdio->phy_mask = ~0;

Yes, that looks good.

 Andrew

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Florian Fainelli]

On 28/04/16 11:59, 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.

Something like that, just so we do not have to sprinkle tests all other
the place:

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index b622b33dbf93..2f497790be1b 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -209,6 +209,10 @@ int of_mdiobus_register(struct mii_bus *mdio,
struct device_node *np)
bool scanphys = false;
int addr, rc;

+   /* Do not continue if the node is disabled */
+   if (!of_device_is_available(np))
+   return -EINVAL;
+
/* Mask out all PHYs from auto probing.  Instead the PHYs listed in
 * the device tree are populated after the bus has been
registered */
mdio->phy_mask = ~0;


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

Sometimes the MDIO registers are intertwinned with the Ethernet MAC
register space, which is something you can solve by handing just the
relevant portion of the MDIO register space to a separate driver (though
you need to watch out for two drivers calling request_mem_region on the
same register space).
-- 
Florian

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Andrew Lunn]

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

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Nathan Sullivan]

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.

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Andrew Lunn]

> 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

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Nathan Sullivan]

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?

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Andrew Lunn]

> 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

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Nathan Sullivan]

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

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Nicolas Ferre]

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 

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


-- 
Nicolas Ferre

[PATCH v2] net: macb: do not scan PHYs manually

[Nathan Sullivan]

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 
---
 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;
-- 
1.7.10.4