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 <[email protected]>
---
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
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 <[email protected]>
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
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 <[email protected]>
>
> 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 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
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
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
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(&bp->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 management port */
macb_writel(bp, NCR, MACB_BIT(MPE));
@@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
dev_set_drvdata(&bp->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;
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(&bp->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 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(&bp->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.
Bye,
--
Nicolas Ferre
> 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(&bp->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
> > +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(&bp->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