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 
[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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 :
> 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 
>> > > > ---
>> > > >
>> > > > 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 = <>;', 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 = <>;

'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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
> > > ---
> > > 
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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> ("").


   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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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> ("").


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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
> > > > ---
> > > > 
> > > > 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 = <>;', 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 = <>;', 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 

 {
status = "okay";

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

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

Andrew
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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> ("").



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 

[...]

MBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
> > ---
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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> ("").

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
> ---
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html