[PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Andrew Lunn]

Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
the bus' parent." broke finding PHY properties in the MAC device tree
node. The parent device is now the MDIO bus, not the MAC. Use
attached_dev towards the MAC device tree node.

Fixes: 8b63ec1837fa ("phylib: Make PHYs children of their MDIO bus, not the bus' parent.")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---

Compile tested only.

Dinh, please could you test it and report back if it works or not.

 drivers/net/phy/micrel.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 499185eaf413..9e2083a1a5a9 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -25,6 +25,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/phy.h>
+#include <linux/netdevice.h>
 #include <linux/micrel_phy.h>
 #include <linux/of.h>
 #include <linux/clk.h>
@@ -340,8 +341,9 @@ static int ksz9021_config_init(struct phy_device *phydev)
 	const struct device *dev = &phydev->dev;
 	const struct device_node *of_node = dev->of_node;
 
-	if (!of_node && dev->parent->of_node)
-		of_node = dev->parent->of_node;
+	if (!of_node && phydev->attached_dev &&
+	    phydev->attached_dev->dev.of_node)
+		of_node = phydev->attached_dev->dev.of_node;
 
 	if (of_node) {
 		ksz9021_load_values_from_of(phydev, of_node,
-- 
2.5.1

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Dinh Nguyen]

On Fri, 16 Oct 2015, Andrew Lunn wrote:

> Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
> the bus' parent." broke finding PHY properties in the MAC device tree
> node. The parent device is now the MDIO bus, not the MAC. Use
> attached_dev towards the MAC device tree node.
> 
> Fixes: 8b63ec1837fa ("phylib: Make PHYs children of their MDIO bus, not the bus' parent.")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> 
> Compile tested only.
> 
> Dinh, please could you test it and report back if it works or not.
>

This patch did not seem to fix the problem. The following code did seem to
fix the problem:

	if (!of_node && dev->parent->of_node)
-               of_node = dev->parent->of_node;
+       do {
+               of_node = dev->of_node;
+               dev = dev->parent;
+               i++;
+       } while (!of_node && dev);

BR,
Dinh

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Andrew Lunn]

On Fri, Oct 16, 2015 at 05:58:41PM -0500, Dinh Nguyen wrote:
> On Fri, 16 Oct 2015, Andrew Lunn wrote:
> 
> > Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
> > the bus' parent." broke finding PHY properties in the MAC device tree
> > node. The parent device is now the MDIO bus, not the MAC. Use
> > attached_dev towards the MAC device tree node.
> > 
> > Fixes: 8b63ec1837fa ("phylib: Make PHYs children of their MDIO bus, not the bus' parent.")
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > 
> > Compile tested only.
> > 
> > Dinh, please could you test it and report back if it works or not.
> >
> 
> This patch did not seem to fix the problem. The following code did seem to
> fix the problem:
> 
> 	if (!of_node && dev->parent->of_node)
> -               of_node = dev->parent->of_node;
> +       do {
> +               of_node = dev->of_node;
> +               dev = dev->parent;
> +               i++;
> +       } while (!of_node && dev);

This might fix the issue, but it has disadvantages. As i said before,
it allows people to place phy properties into the mdio device node. We
want to be reducing placing you can add phy properties, not adding
more.

Please could you try to debug why my patch did not work. Is
attached_dev null?

	     Andrew

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Dinh Nguyen]

On Fri, 16 Oct 2015, Andrew Lunn wrote:

> On Fri, Oct 16, 2015 at 05:58:41PM -0500, Dinh Nguyen wrote:
> > On Fri, 16 Oct 2015, Andrew Lunn wrote:
> > 
> > > Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
> > > the bus' parent." broke finding PHY properties in the MAC device tree
> > > node. The parent device is now the MDIO bus, not the MAC. Use
> > > attached_dev towards the MAC device tree node.
> > > 
> > > Fixes: 8b63ec1837fa ("phylib: Make PHYs children of their MDIO bus, not the bus' parent.")
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > ---
> > > 
> > > Compile tested only.
> > > 
> > > Dinh, please could you test it and report back if it works or not.
> > >
> > 
> > This patch did not seem to fix the problem. The following code did seem to
> > fix the problem:
> > 
> > 	if (!of_node && dev->parent->of_node)
> > -               of_node = dev->parent->of_node;
> > +       do {
> > +               of_node = dev->of_node;
> > +               dev = dev->parent;
> > +               i++;
> > +       } while (!of_node && dev);
> 
> This might fix the issue, but it has disadvantages. As i said before,
> it allows people to place phy properties into the mdio device node. We
> want to be reducing placing you can add phy properties, not adding
> more.
> 
> Please could you try to debug why my patch did not work. Is
> attached_dev null?
>

Sure, will try to debug. It looks like phydev->attached_dev is valid, but
phydev->attached_dev->dev.of_node is NULL.
 

BR,
Dinh

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Dinh Nguyen]

On Sat, 17 Oct 2015, Dinh Nguyen wrote:

> On Fri, 16 Oct 2015, Andrew Lunn wrote:
> 
> > On Fri, Oct 16, 2015 at 05:58:41PM -0500, Dinh Nguyen wrote:
> > > On Fri, 16 Oct 2015, Andrew Lunn wrote:
> > > 
> > > > Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
> > > > the bus' parent." broke finding PHY properties in the MAC device tree
> > > > node. The parent device is now the MDIO bus, not the MAC. Use
> > > > attached_dev towards the MAC device tree node.
> > > > 
> > > > Fixes: 8b63ec1837fa ("phylib: Make PHYs children of their MDIO bus, not the bus' parent.")
> > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > > ---
> > > > 
> > > > Compile tested only.
> > > > 
> > > > Dinh, please could you test it and report back if it works or not.
> > > >
> > > 
> > > This patch did not seem to fix the problem. The following code did seem to
> > > fix the problem:
> > > 
> > > 	if (!of_node && dev->parent->of_node)
> > > -               of_node = dev->parent->of_node;
> > > +       do {
> > > +               of_node = dev->of_node;
> > > +               dev = dev->parent;
> > > +               i++;
> > > +       } while (!of_node && dev);
> > 
> > This might fix the issue, but it has disadvantages. As i said before,
> > it allows people to place phy properties into the mdio device node. We
> > want to be reducing placing you can add phy properties, not adding
> > more.
> >

I've also tried creating a separate phy node in the DTS and have the EMAC
point the PHY with a 'phy = <&phy0>;', but that also didn't seem to work with
your patch.
 
> 
> Sure, will try to debug. It looks like phydev->attached_dev is valid, but
> phydev->attached_dev->dev.of_node is NULL.
>  
> 
> BR,
> Dinh
> 

BR,
Dinh

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Andrew Lunn]

> I've also tried creating a separate phy node in the DTS and have the EMAC
> point the PHY with a 'phy = <&phy0>;', but that also didn't seem to work with
> your patch.

Do you have the phy node as a child of the mdio node?

Picking a random example arch/arm/boot/dts/kirkwood-rd88f6192.dts 

&mdio {
        status = "okay";

        ethphy0: ethernet-phy@8 {
                reg = <8>;
        };
};

&eth0 {
        status = "okay";
        ethernet0-port@0 {
                phy-handle = <&ethphy0>;
        };
};

	Andrew

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Florian Fainelli]

2015-10-17 8:06 GMT-07:00 Dinh Nguyen <dinguyen@opensource.altera.com>:
> On Sat, 17 Oct 2015, Dinh Nguyen wrote:
>
>> On Fri, 16 Oct 2015, Andrew Lunn wrote:
>>
>> > On Fri, Oct 16, 2015 at 05:58:41PM -0500, Dinh Nguyen wrote:
>> > > On Fri, 16 Oct 2015, Andrew Lunn wrote:
>> > >
>> > > > Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
>> > > > the bus' parent." broke finding PHY properties in the MAC device tree
>> > > > node. The parent device is now the MDIO bus, not the MAC. Use
>> > > > attached_dev towards the MAC device tree node.
>> > > >
>> > > > Fixes: 8b63ec1837fa ("phylib: Make PHYs children of their MDIO bus, not the bus' parent.")
>> > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> > > > ---
>> > > >
>> > > > Compile tested only.
>> > > >
>> > > > Dinh, please could you test it and report back if it works or not.
>> > > >
>> > >
>> > > This patch did not seem to fix the problem. The following code did seem to
>> > > fix the problem:
>> > >
>> > >   if (!of_node && dev->parent->of_node)
>> > > -               of_node = dev->parent->of_node;
>> > > +       do {
>> > > +               of_node = dev->of_node;
>> > > +               dev = dev->parent;
>> > > +               i++;
>> > > +       } while (!of_node && dev);
>> >
>> > This might fix the issue, but it has disadvantages. As i said before,
>> > it allows people to place phy properties into the mdio device node. We
>> > want to be reducing placing you can add phy properties, not adding
>> > more.
>> >
>
> I've also tried creating a separate phy node in the DTS and have the EMAC
> point the PHY with a 'phy = <&phy0>;', but that also didn't seem to work with
> your patch.

If you intend this to be a real phandle to a phy, it should be something like:

phy-handle = <&phy0>;

'phy' is not a valid phandle property that stmmac recognizes

Looking at stmmac and how the various dwmac-* probe the driver,
SET_NETDEV_DEV() is what assigns the network device's device pointer,
and that seems to be done correctly with the proper device argument.
-- 
Florian

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Andrew Lunn]

> Sure, will try to debug. It looks like phydev->attached_dev is valid, but
> phydev->attached_dev->dev.of_node is NULL.

Humm

phydev->attached_dev is a net_device, so should be the mac.  What
device is phydev->attached_dev->dev? Is it not the dev embedded in the
platform_device passed to socfpga_dwmac_probe()?

	Andrew

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Dinh Nguyen]

On Sat, 17 Oct 2015, Andrew Lunn wrote:

> > Sure, will try to debug. It looks like phydev->attached_dev is valid, but
> > phydev->attached_dev->dev.of_node is NULL.
> 
> Humm
> 
> phydev->attached_dev is a net_device, so should be the mac.  What
> device is phydev->attached_dev->dev? Is it not the dev embedded in the
> platform_device passed to socfpga_dwmac_probe()?
>

Yes, it looks like it is, the dev->of_node is valid in socfpga_dwmac_probe(),
but it looks like of_node is getting lost somewhere. 

BR,
Dinh

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Dinh Nguyen]

On Sat, 17 Oct 2015, Dinh Nguyen wrote:

> On Sat, 17 Oct 2015, Andrew Lunn wrote:
> 
> > > Sure, will try to debug. It looks like phydev->attached_dev is valid, but
> > > phydev->attached_dev->dev.of_node is NULL.
> > 
> > Humm
> > 
> > phydev->attached_dev is a net_device, so should be the mac.  What
> > device is phydev->attached_dev->dev? Is it not the dev embedded in the
> > platform_device passed to socfpga_dwmac_probe()?
> >
> 
> Yes, it looks like it is, the dev->of_node is valid in socfpga_dwmac_probe(),
> but it looks like of_node is getting lost somewhere. 
> 

Do you know why this happening? In ksz9021_config_init():

@@ -345,7 +345,11 @@ static int ksz9021_config_init(struct phy_device *phydev)
            phydev->attached_dev->dev.of_node)
                of_node = phydev->attached_dev->dev.of_node;

+	printk("%s %08x\n", __func__, phydev->attached_dev->dev.of_node);
+       printk("%s %08x %08x\n", __func__, phydev->attached_dev->dev, phydev->attached_dev->dev.of_node);


[    1.923311] ksz9021_config_init 00000000
[    1.927224] ksz9021_config_init eedc0210 ee401680

The first printout shows phydev->attached_dev->dev.of_node is NULL. but the
second printout, where I'm also printing out phydev->attached_dev->dev, then
phydev->attached_dev->dev.of_node is not NULL.

BR,
Dinh

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Sergei Shtylyov]

On 10/16/2015 11:49 PM, Andrew Lunn wrote:

> Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
> the bus' parent." broke finding PHY properties in the MAC device tree

    You probably forgot to run checkpatch.pl on this patch, else it would have 
complained about the commit citing style. It's <12-bit SHA1> ("<summary line>").

> node. The parent device is now the MDIO bus, not the MAC. Use
> attached_dev towards the MAC device tree node.
>
> Fixes: 8b63ec1837fa ("phylib: Make PHYs children of their MDIO bus, not the bus' parent.")

    Yeah, exactly like this.

> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
[...]

MBR, Sergei

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Andrew Lunn]

On Sat, Oct 17, 2015 at 02:32:30AM +0300, Sergei Shtylyov wrote:
> On 10/16/2015 11:49 PM, Andrew Lunn wrote:
> 
> >Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
> >the bus' parent." broke finding PHY properties in the MAC device tree
> 
>    You probably forgot to run checkpatch.pl on this patch, else it
> would have complained about the commit citing style. It's <12-bit
> SHA1> ("<summary line>").

Actual, i did, and decided to ignore it. I'm quoting the regression
report, which formats it that way. However i did deliberately use the
correct format for the fixes: line, where it actually matters.

checkpatch is just a guide, not a rigid rule.

	   Andrew

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Sergei Shtylyov]

On 10/17/2015 2:54 AM, Andrew Lunn wrote:

>>> Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
>>> the bus' parent." broke finding PHY properties in the MAC device tree
>>
>>     You probably forgot to run checkpatch.pl on this patch, else it
>> would have complained about the commit citing style. It's <12-bit
>> SHA1> ("<summary line>").

    12-digit, of course. :-)

> Actual, i did, and decided to ignore it. I'm quoting the regression
> report, which formats it that way. However i did deliberately use the
> correct format for the fixes: line, where it actually matters.

> checkpatch is just a guide, not a rigid rule.

    I've had several cases of a maintainer fixing up the commit citing style 
for me (and ruining my precious line filling :-).

MBR, Sergei

Re: [PATCH] net/phy: micrel: Follow attached_dev to get to the MAC device

[Sergei Shtylyov]

On 10/17/2015 2:54 AM, Andrew Lunn wrote:

>>> Commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
>>> the bus' parent." broke finding PHY properties in the MAC device tree
>>
>>     You probably forgot to run checkpatch.pl on this patch, else it
>> would have complained about the commit citing style. It's <12-bit
>> SHA1> ("<summary line>").
>
> Actual, i did, and decided to ignore it. I'm quoting the regression
> report, which formats it that way.

    BTW, I'm seeing no Reported-by: tag in your patch either, is that intended?

> 	   Andrew

MBR, Sergei