[PATCH] sh_eth: call of_mdiobus_register() to register phys

[PATCH] sh_eth: call of_mdiobus_register() to register phys

[Ben Dooks]

If the sh_eth device is registered using OF, then the driver
should call of_mdiobus_register() to register any PHYs connected
to the system.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 06970ac..165f0c4 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -40,6 +40,7 @@
 #include <linux/if_vlan.h>
 #include <linux/clk.h>
 #include <linux/sh_eth.h>
+#include <linux/of_mdio.h>
 
 #include "sh_eth.h"
 
@@ -2629,6 +2630,18 @@ static int sh_mdio_init(struct net_device *ndev, int id,
 	snprintf(mdp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
 		 mdp->pdev->name, id);
 
+	if (ndev->dev.parent->of_node) {
+		dev_set_drvdata(&ndev->dev, mdp->mii_bus);
+		ret = of_mdiobus_register(mdp->mii_bus,
+					  ndev->dev.parent->of_node);
+		if (ret != 0) {
+			dev_err(&ndev->dev, "of_mdiobus_register() failed\n");
+			goto out_free_bus;
+		}
+
+		return 0;
+	}
+
 	/* PHY IRQ */
 	mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
 					 sizeof(int) * PHY_MAX_ADDR,
-- 
1.8.5.3

Re: [PATCH] sh_eth: call of_mdiobus_register() to register phys

[Ben Dooks]

On 17/02/14 16:40, Sergei Shtylyov wrote:
> Hello.
>
> On 02/17/2014 06:28 PM, Ben Dooks wrote:
>
>> If the sh_eth device is registered using OF, then the driver
>
>     Which is not supported yet as my DT patch hasn't been merged.
> This patch seems somewhat premature.

I've got your OF patches in my local tree to test with, this
is what I found during that testing.

>> should call of_mdiobus_register() to register any PHYs connected
>> to the system.
>
>     That's not necessary (but good to have).

Well, it is necessary if you then want any PHYS bound to
the device to have their OF information to hand,

>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/net/ethernet/renesas/sh_eth.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>
>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c
>> index 06970ac..165f0c4 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> [...]
>> @@ -2629,6 +2630,18 @@ static int sh_mdio_init(struct net_device
>> *ndev, int id,
>>       snprintf(mdp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
>>            mdp->pdev->name, id);
>>
>> +    if (ndev->dev.parent->of_node) {
>> +        dev_set_drvdata(&ndev->dev, mdp->mii_bus);
>> +        ret = of_mdiobus_register(mdp->mii_bus,
>> +                      ndev->dev.parent->of_node);
>> +        if (ret != 0) {
>> +            dev_err(&ndev->dev, "of_mdiobus_register() failed\n");
>> +            goto out_free_bus;
>> +        }

I should probably only set the drvdata if the
of_mdiobus_register() succeeds.

>> +        return 0;
>> +    }
>> +
>>       /* PHY IRQ */
>>       mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
>>                        sizeof(int) * PHY_MAX_ADDR,
>>
>
>      Hm, I can only hope this works with PHY IRQ in DT mode.
> Would you mind if I include your patch into my Ether DT patch?

You are welcome to include it in your series, but I would like
to keep the credit for finding this.

Also, FYI, for some reason the probe is not finding the correct
IRQ for this. I will have a look later when I get the board back
as to why this is:

net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] sh_eth: call of_mdiobus_register() to register phys

[Sergei Shtylyov]

Hello.

On 02/17/2014 06:28 PM, Ben Dooks wrote:

> If the sh_eth device is registered using OF, then the driver

    Which is not supported yet as my DT patch hasn't been merged.
This patch seems somewhat premature.

> should call of_mdiobus_register() to register any PHYs connected
> to the system.

    That's not necessary (but good to have).

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>   drivers/net/ethernet/renesas/sh_eth.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 06970ac..165f0c4 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
> @@ -2629,6 +2630,18 @@ static int sh_mdio_init(struct net_device *ndev, int id,
>   	snprintf(mdp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
>   		 mdp->pdev->name, id);
>
> +	if (ndev->dev.parent->of_node) {
> +		dev_set_drvdata(&ndev->dev, mdp->mii_bus);
> +		ret = of_mdiobus_register(mdp->mii_bus,
> +					  ndev->dev.parent->of_node);
> +		if (ret != 0) {
> +			dev_err(&ndev->dev, "of_mdiobus_register() failed\n");
> +			goto out_free_bus;
> +		}
> +
> +		return 0;
> +	}
> +
>   	/* PHY IRQ */
>   	mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
>   					 sizeof(int) * PHY_MAX_ADDR,
>

     Hm, I can only hope this works with PHY IRQ in DT mode.
Would you mind if I include your patch into my Ether DT patch?

WBR, Sergei

Re: [Linux-kernel] [PATCH] sh_eth: call of_mdiobus_register() to register phys

[Ben Dooks]

On 17/02/14 15:46, Ben Dooks wrote:
> On 17/02/14 16:40, Sergei Shtylyo

[snip]

>
>>> +        return 0;
>>> +    }
>>> +
>>>       /* PHY IRQ */
>>>       mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
>>>                        sizeof(int) * PHY_MAX_ADDR,
>>>
>>
>>      Hm, I can only hope this works with PHY IRQ in DT mode.
>> Would you mind if I include your patch into my Ether DT patch?
>
> You are welcome to include it in your series, but I would like
> to keep the credit for finding this.
>
> Also, FYI, for some reason the probe is not finding the correct
> IRQ for this. I will have a look later when I get the board back
> as to why this is:
>
> net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI

I've sent a fix for of_mdio to set phy->irq correctly and will
re-send this as a v2 patch shortly.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] sh_eth: call of_mdiobus_register() to register phys

[Ben Dooks]

On 18/02/14 16:38, Sergei Shtylyov wrote:
> Hello.
>
> On 02/17/2014 06:46 PM, Ben Dooks wrote:
>
>>>> If the sh_eth device is registered using OF, then the driver
>
>>>     Which is not supported yet as my DT patch hasn't been merged.
>>> This patch seems somewhat premature.
>
>> I've got your OF patches in my local tree to test with, this
>> is what I found during that testing.
>
>     The issue is that I didn't post my v3 patch to netdev due to
> net-next.git repo being closed at this moment and DaveM not wanting to
> see any patch targeted to it during this time. I've now posted v4 of my
> Ether DT patch to netdev.

Ok, I will look for these tomorrow.

>>>> should call of_mdiobus_register() to register any PHYs connected
>>>> to the system.
>
>>>     That's not necessary (but good to have).
>
>> Well, it is necessary if you then want any PHYS bound to
>> the device to have their OF information to hand,
>
>     Ether DT support worked for me without this fragment, at least.

Yes, it just that the PHY is not being linked to the relevant
OF node. The PHY gets bound, it will not be able to find the
DT info passed.

>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>> ---
>>>>   drivers/net/ethernet/renesas/sh_eth.c | 13 +++++++++++++
>>>>   1 file changed, 13 insertions(+)
>
>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>>>> b/drivers/net/ethernet/renesas/sh_eth.c
>>>> index 06970ac..165f0c4 100644
>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>> [...]
>>>> @@ -2629,6 +2630,18 @@ static int sh_mdio_init(struct net_device
>>>> *ndev, int id,
>>>>       snprintf(mdp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
>>>>            mdp->pdev->name, id);
>>>>
>>>> +    if (ndev->dev.parent->of_node) {
>>>> +        dev_set_drvdata(&ndev->dev, mdp->mii_bus);
>>>> +        ret = of_mdiobus_register(mdp->mii_bus,
>>>> +                      ndev->dev.parent->of_node);
>>>> +        if (ret != 0) {
>>>> +            dev_err(&ndev->dev, "of_mdiobus_register() failed\n");
>>>> +            goto out_free_bus;
>>>> +        }
>
>> I should probably only set the drvdata if the
>> of_mdiobus_register() succeeds.
>
>     Yes. Probably should use *goto* as well since in that case the
> success path would be the same as the existing one.

I will look into that, not the biggest fan of gotos for
success cases.

>>>> +        return 0;
>>>> +    }
>>>> +
>>>>       /* PHY IRQ */
>>>>       mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
>>>>                        sizeof(int) * PHY_MAX_ADDR,
>
>>>      Hm, I can only hope this works with PHY IRQ in DT mode.
>>> Would you mind if I include your patch into my Ether DT patch?
>
>> You are welcome to include it in your series, but I would like
>> to keep the credit for finding this.
>
>     OK.
>
>> Also, FYI, for some reason the probe is not finding the correct
>> IRQ for this. I will have a look later when I get the board back
>> as to why this is:
>
>> net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI
>
>     Thanks for quickly fixing this. I've yet to study of_mdio.c code...
>
> WBR, Sergei

No problem,


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] sh_eth: call of_mdiobus_register() to register phys

[Sergei Shtylyov]

Hello.

On 02/17/2014 06:46 PM, Ben Dooks wrote:

>>> If the sh_eth device is registered using OF, then the driver

>>     Which is not supported yet as my DT patch hasn't been merged.
>> This patch seems somewhat premature.

> I've got your OF patches in my local tree to test with, this
> is what I found during that testing.

    The issue is that I didn't post my v3 patch to netdev due to net-next.git 
repo being closed at this moment and DaveM not wanting to see any patch 
targeted to it during this time. I've now posted v4 of my Ether DT patch to 
netdev.

>>> should call of_mdiobus_register() to register any PHYs connected
>>> to the system.

>>     That's not necessary (but good to have).

> Well, it is necessary if you then want any PHYS bound to
> the device to have their OF information to hand,

    Ether DT support worked for me without this fragment, at least.

>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>> ---
>>>   drivers/net/ethernet/renesas/sh_eth.c | 13 +++++++++++++
>>>   1 file changed, 13 insertions(+)

>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>>> b/drivers/net/ethernet/renesas/sh_eth.c
>>> index 06970ac..165f0c4 100644
>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> [...]
>>> @@ -2629,6 +2630,18 @@ static int sh_mdio_init(struct net_device
>>> *ndev, int id,
>>>       snprintf(mdp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
>>>            mdp->pdev->name, id);
>>>
>>> +    if (ndev->dev.parent->of_node) {
>>> +        dev_set_drvdata(&ndev->dev, mdp->mii_bus);
>>> +        ret = of_mdiobus_register(mdp->mii_bus,
>>> +                      ndev->dev.parent->of_node);
>>> +        if (ret != 0) {
>>> +            dev_err(&ndev->dev, "of_mdiobus_register() failed\n");
>>> +            goto out_free_bus;
>>> +        }

> I should probably only set the drvdata if the
> of_mdiobus_register() succeeds.

    Yes. Probably should use *goto* as well since in that case the success 
path would be the same as the existing one.

>>> +        return 0;
>>> +    }
>>> +
>>>       /* PHY IRQ */
>>>       mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
>>>                        sizeof(int) * PHY_MAX_ADDR,

>>      Hm, I can only hope this works with PHY IRQ in DT mode.
>> Would you mind if I include your patch into my Ether DT patch?

> You are welcome to include it in your series, but I would like
> to keep the credit for finding this.

    OK.

> Also, FYI, for some reason the probe is not finding the correct
> IRQ for this. I will have a look later when I get the board back
> as to why this is:

> net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI

    Thanks for quickly fixing this. I've yet to study of_mdio.c code...

WBR, Sergei

Re: [PATCH] sh_eth: call of_mdiobus_register() to register phys

[Ben Dooks]

On 18/02/14 18:11, Sergei Shtylyov wrote:
> Hello.
>
> On 02/18/2014 07:00 PM, Ben Dooks wrote:
>
>>>>>> If the sh_eth device is registered using OF, then the driver
>
>>>>>     Which is not supported yet as my DT patch hasn't been merged.
>>>>> This patch seems somewhat premature.
>
>>>> I've got your OF patches in my local tree to test with, this
>>>> is what I found during that testing.
>
>>>     The issue is that I didn't post my v3 patch to netdev due to
>>> net-next.git repo being closed at this moment and DaveM not wanting to
>>> see any patch targeted to it during this time. I've now posted v4 of my
>>> Ether DT patch to netdev.
>
>> Ok, I will look for these tomorrow.
>
>     No significant changes there...
>
>>>>>> should call of_mdiobus_register() to register any PHYs connected
>>>>>> to the system.
>
>>>>>     That's not necessary (but good to have).
>
>>>> Well, it is necessary if you then want any PHYS bound to
>>>> the device to have their OF information to hand,
>
>>>     Ether DT support worked for me without this fragment, at least.
>
>> Yes, it just that the PHY is not being linked to the relevant
>> OF node. The PHY gets bound, it will not be able to find the
>> DT info passed.
>
>     With no DT support in the PHY driver, I don't see how it matters.
> Perhaps it has to do with your "init-regs" prop patch though...


Yes, or if we add some other properties to the PHY node to say
how to initialise the registers, such as if the PHY node had.

I am going to look in to adding led<name> initialisers to the
PHY node in case people do not like my init-regs patch.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] sh_eth: call of_mdiobus_register() to register phys

[Sergei Shtylyov]

Hello.

On 02/18/2014 07:00 PM, Ben Dooks wrote:

>>>>> If the sh_eth device is registered using OF, then the driver

>>>>     Which is not supported yet as my DT patch hasn't been merged.
>>>> This patch seems somewhat premature.

>>> I've got your OF patches in my local tree to test with, this
>>> is what I found during that testing.

>>     The issue is that I didn't post my v3 patch to netdev due to
>> net-next.git repo being closed at this moment and DaveM not wanting to
>> see any patch targeted to it during this time. I've now posted v4 of my
>> Ether DT patch to netdev.

> Ok, I will look for these tomorrow.

    No significant changes there...

>>>>> should call of_mdiobus_register() to register any PHYs connected
>>>>> to the system.

>>>>     That's not necessary (but good to have).

>>> Well, it is necessary if you then want any PHYS bound to
>>> the device to have their OF information to hand,

>>     Ether DT support worked for me without this fragment, at least.

> Yes, it just that the PHY is not being linked to the relevant
> OF node. The PHY gets bound, it will not be able to find the
> DT info passed.

    With no DT support in the PHY driver, I don't see how it matters. Perhaps 
it has to do with your "init-regs" prop patch though...

>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>> ---
>>>>>   drivers/net/ethernet/renesas/sh_eth.c | 13 +++++++++++++
>>>>>   1 file changed, 13 insertions(+)

>>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>>>>> b/drivers/net/ethernet/renesas/sh_eth.c
>>>>> index 06970ac..165f0c4 100644
>>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>>> [...]
>>>>> @@ -2629,6 +2630,18 @@ static int sh_mdio_init(struct net_device
>>>>> *ndev, int id,
>>>>>       snprintf(mdp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
>>>>>            mdp->pdev->name, id);
>>>>>
>>>>> +    if (ndev->dev.parent->of_node) {
>>>>> +        dev_set_drvdata(&ndev->dev, mdp->mii_bus);
>>>>> +        ret = of_mdiobus_register(mdp->mii_bus,
>>>>> +                      ndev->dev.parent->of_node);
>>>>> +        if (ret != 0) {
>>>>> +            dev_err(&ndev->dev, "of_mdiobus_register() failed\n");
>>>>> +            goto out_free_bus;
>>>>> +        }

>>> I should probably only set the drvdata if the
>>> of_mdiobus_register() succeeds.

>>     Yes. Probably should use *goto* as well since in that case the
>> success path would be the same as the existing one.

> I will look into that, not the biggest fan of gotos for
> success cases.

    Can also try to use *else* branch to call mdiobus_register()...

>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>>       /* PHY IRQ */
>>>>>       mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
>>>>>                        sizeof(int) * PHY_MAX_ADDR,

[...]

WBR, Sergei