[RESEND] [PATCH v4] sh_eth: update OF PHY registeration

[RESEND] [PATCH v4] sh_eth: update OF PHY registeration

[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 and of_phy_connect() to connect it.

This ensures that any PHYs registered in the device tree are
appropriately connected to the parent devices nodes so that
the PHY drivers can access their OF properties.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

--
v2:
	- allocate mdio->irq array at init time
	- set devdata after succesful mdio registration
v3:
	- do not parse phy->irq in of setup (done by of_mdiobus)
	- use of_phy_connect to connect phy
v4:
	- fix review comments on code
	- reword patch description
	- change if to be two way in mdio init code
---
 drivers/net/ethernet/renesas/sh_eth.c | 57 +++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index a18cbe1..e597698 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"
 
@@ -1761,20 +1762,36 @@ static void sh_eth_adjust_link(struct net_device *ndev)
 /* PHY init function */
 static int sh_eth_phy_init(struct net_device *ndev)
 {
+	struct device_node *np = ndev->dev.parent->of_node;
 	struct sh_eth_private *mdp = netdev_priv(ndev);
-	char phy_id[MII_BUS_ID_SIZE + 3];
 	struct phy_device *phydev = NULL;
 
-	snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
-		 mdp->mii_bus->id, mdp->phy_id);
-
 	mdp->link = 0;
 	mdp->speed = 0;
 	mdp->duplex = -1;
 
 	/* Try connect to PHY */
-	phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link,
-			     mdp->phy_interface);
+	if (np) {
+		struct device_node *pn;
+
+		pn = of_parse_phandle(np, "phy-handle", 0);
+		phydev = of_phy_connect(ndev, pn,
+					sh_eth_adjust_link, 0,
+					mdp->phy_interface);
+
+		if (!phydev)
+			phydev = ERR_PTR(-ENOENT);
+	} else {
+		char phy_id[MII_BUS_ID_SIZE + 3];
+
+		snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
+			 mdp->mii_bus->id, mdp->phy_id);
+
+		/* Try connect to PHY */
+		phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link,
+				     mdp->phy_interface);
+	}
+
 	if (IS_ERR(phydev)) {
 		dev_err(&ndev->dev, "phy_connect failed\n");
 		return PTR_ERR(phydev);
@@ -2638,15 +2655,23 @@ static int sh_mdio_init(struct net_device *ndev, int id,
 		goto out_free_bus;
 	}
 
-	for (i = 0; i < PHY_MAX_ADDR; i++)
-		mdp->mii_bus->irq[i] = PHY_POLL;
-	if (pd->phy_irq > 0)
-		mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
+	if (ndev->dev.parent->of_node) {
+		ret = of_mdiobus_register(mdp->mii_bus,
+					  ndev->dev.parent->of_node);
+	} else {
+		for (i = 0; i < PHY_MAX_ADDR; i++)
+			mdp->mii_bus->irq[i] = PHY_POLL;
+		if (pd->phy_irq > 0)
+			mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
 
-	/* register mdio bus */
-	ret = mdiobus_register(mdp->mii_bus);
-	if (ret)
+		/* register mdio bus */
+		ret = mdiobus_register(mdp->mii_bus);
+	}
+
+	if (ret) {
+		dev_err(&ndev->dev, "failed to register mdio\n");
 		goto out_free_bus;
+	}
 
 	dev_set_drvdata(&ndev->dev, mdp->mii_bus);
 
@@ -2719,7 +2744,6 @@ static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
 	struct sh_eth_plat_data *pdata;
-	struct device_node *phy;
 	const char *mac_addr;
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
@@ -2728,11 +2752,6 @@ static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
 
 	pdata->phy_interface = of_get_phy_mode(np);
 
-	phy = of_parse_phandle(np, "phy-handle", 0);
-	if (of_property_read_u32(phy, "reg", &pdata->phy))
-		return NULL;
-	pdata->phy_irq = irq_of_parse_and_map(phy, 0);
-
 	mac_addr = of_get_mac_address(np);
 	if (mac_addr)
 		memcpy(pdata->mac_addr, mac_addr, ETH_ALEN);
-- 
1.8.5.3

Re: [RESEND] [PATCH v4] sh_eth: update OF PHY registeration

[Sergei Shtylyov]

On 02/26/2014 09:23 PM, Ben Dooks wrote:

> 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 and of_phy_connect() to connect it.

    s/it/to PHY/?

> This ensures that any PHYs registered in the device tree are
> appropriately connected to the parent devices nodes so that
> the PHY drivers can access their OF properties.

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

[...]

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index a18cbe1..e597698 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
> @@ -1761,20 +1762,36 @@ static void sh_eth_adjust_link(struct net_device *ndev)
>   /* PHY init function */
>   static int sh_eth_phy_init(struct net_device *ndev)
>   {
> +	struct device_node *np = ndev->dev.parent->of_node;
>   	struct sh_eth_private *mdp = netdev_priv(ndev);
> -	char phy_id[MII_BUS_ID_SIZE + 3];
>   	struct phy_device *phydev = NULL;
>
> -	snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
> -		 mdp->mii_bus->id, mdp->phy_id);
> -
>   	mdp->link = 0;
>   	mdp->speed = 0;
>   	mdp->duplex = -1;
>
>   	/* Try connect to PHY */
> -	phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link,
> -			     mdp->phy_interface);
> +	if (np) {
> +		struct device_node *pn;
> +
> +		pn = of_parse_phandle(np, "phy-handle", 0);
> +		phydev = of_phy_connect(ndev, pn,
> +					sh_eth_adjust_link, 0,

    Hm, that 0 corresponds to 'flags' parameter which is not used. I wonder if 
that was intentional...

> +					mdp->phy_interface);
> +
> +		if (!phydev)
> +			phydev = ERR_PTR(-ENOENT);
> +	} else {
> +		char phy_id[MII_BUS_ID_SIZE + 3];
> +
> +		snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
> +			 mdp->mii_bus->id, mdp->phy_id);
> +
> +		/* Try connect to PHY */

    Now this comment is kinda duplicate... :-)

> +		phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link,
> +				     mdp->phy_interface);
> +	}
> +
>   	if (IS_ERR(phydev)) {
>   		dev_err(&ndev->dev, "phy_connect failed\n");

    I have asked to adjust the message a bit as we now have 2 alternate calls 
that can fail...

>   		return PTR_ERR(phydev);
> @@ -2638,15 +2655,23 @@ static int sh_mdio_init(struct net_device *ndev, int id,
>   		goto out_free_bus;
>   	}
>
> -	for (i = 0; i < PHY_MAX_ADDR; i++)
> -		mdp->mii_bus->irq[i] = PHY_POLL;
> -	if (pd->phy_irq > 0)
> -		mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
> +	if (ndev->dev.parent->of_node) {
> +		ret = of_mdiobus_register(mdp->mii_bus,
> +					  ndev->dev.parent->of_node);
> +	} else {
> +		for (i = 0; i < PHY_MAX_ADDR; i++)
> +			mdp->mii_bus->irq[i] = PHY_POLL;
> +		if (pd->phy_irq > 0)
> +			mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
>
> -	/* register mdio bus */
> -	ret = mdiobus_register(mdp->mii_bus);
> -	if (ret)
> +		/* register mdio bus */
> +		ret = mdiobus_register(mdp->mii_bus);
> +	}

    That looks much better, thanks.

> +

    Although I'd have avoided the empty line...

> +	if (ret) {
> +		dev_err(&ndev->dev, "failed to register mdio\n");

    That function generally avoids error messages, no? Although all errors are 
-ENOMEM type and that's supposed to cause kernel to loudly curse anyway... 
Well, let it be if you want.

>   		goto out_free_bus;
> +	}

WBR, Sergei

Re: [RESEND] [PATCH v4] sh_eth: update OF PHY registeration

[Florian Fainelli]

2014-02-26 11:47 GMT-08:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
[snip]

>> +               pn = of_parse_phandle(np, "phy-handle", 0);
>> +               phydev = of_phy_connect(ndev, pn,
>> +                                       sh_eth_adjust_link, 0,
>
>
>    Hm, that 0 corresponds to 'flags' parameter which is not used. I wonder
> if that was intentional...

This is a left-over from when we needed to set phydev->dev_flags, but
this is no longer necessary although of_phy_connect() kept the
argument. Setting phydev->dev_flags should be done before calling
phy_start(), so far we only have a two users in-tree: tg3.c and some
mach-pxa code doing that.

>
>
>> +                                       mdp->phy_interface);
>> +
>> +               if (!phydev)
>> +                       phydev = ERR_PTR(-ENOENT);

Sounds like this could be moved to of_phy_connect(), although not
necessarily as part of this patchset, also, we would need to update
the callers, but this would be more consistent.
-- 
Florian

Re: [RESEND] [PATCH v4] sh_eth: update OF PHY registeration

[Ben Dooks]

On 26/02/14 19:47, Sergei Shtylyov wrote:
> On 02/26/2014 09:23 PM, Ben Dooks wrote:
>
>> 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 and of_phy_connect() to connect it.
>
>     s/it/to PHY/?


Ok, will change.

>> This ensures that any PHYs registered in the device tree are
>> appropriately connected to the parent devices nodes so that
>> the PHY drivers can access their OF properties.
>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>
> [...]
>
>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c
>> index a18cbe1..e597698 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> [...]
>> @@ -1761,20 +1762,36 @@ static void sh_eth_adjust_link(struct
>> net_device *ndev)
>>   /* PHY init function */
>>   static int sh_eth_phy_init(struct net_device *ndev)
>>   {
>> +    struct device_node *np = ndev->dev.parent->of_node;
>>       struct sh_eth_private *mdp = netdev_priv(ndev);
>> -    char phy_id[MII_BUS_ID_SIZE + 3];
>>       struct phy_device *phydev = NULL;
>>
>> -    snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
>> -         mdp->mii_bus->id, mdp->phy_id);
>> -
>>       mdp->link = 0;
>>       mdp->speed = 0;
>>       mdp->duplex = -1;
>>
>>       /* Try connect to PHY */
>> -    phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link,
>> -                 mdp->phy_interface);
>> +    if (np) {
>> +        struct device_node *pn;
>> +
>> +        pn = of_parse_phandle(np, "phy-handle", 0);
>> +        phydev = of_phy_connect(ndev, pn,
>> +                    sh_eth_adjust_link, 0,
>
>     Hm, that 0 corresponds to 'flags' parameter which is not used. I
> wonder if that was intentional...

This can be left to a separate patch to fix.

>
>> +                    mdp->phy_interface);
>> +
>> +        if (!phydev)
>> +            phydev = ERR_PTR(-ENOENT);
>> +    } else {
>> +        char phy_id[MII_BUS_ID_SIZE + 3];
>> +
>> +        snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
>> +             mdp->mii_bus->id, mdp->phy_id);
>> +
>> +        /* Try connect to PHY */
>
>     Now this comment is kinda duplicate... :-)

Bah, got rid of it.

>> +        phydev = phy_connect(ndev, phy_id, sh_eth_adjust_link,
>> +                     mdp->phy_interface);
>> +    }
>> +
>>       if (IS_ERR(phydev)) {
>>           dev_err(&ndev->dev, "phy_connect failed\n");
>
>     I have asked to adjust the message a bit as we now have 2 alternate
> calls that can fail...

ok, changed to "failed to connect phy"

>
>>           return PTR_ERR(phydev);
>> @@ -2638,15 +2655,23 @@ static int sh_mdio_init(struct net_device
>> *ndev, int id,
>>           goto out_free_bus;
>>       }
>>
>> -    for (i = 0; i < PHY_MAX_ADDR; i++)
>> -        mdp->mii_bus->irq[i] = PHY_POLL;
>> -    if (pd->phy_irq > 0)
>> -        mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
>> +    if (ndev->dev.parent->of_node) {
>> +        ret = of_mdiobus_register(mdp->mii_bus,
>> +                      ndev->dev.parent->of_node);
>> +    } else {
>> +        for (i = 0; i < PHY_MAX_ADDR; i++)
>> +            mdp->mii_bus->irq[i] = PHY_POLL;
>> +        if (pd->phy_irq > 0)
>> +            mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
>>
>> -    /* register mdio bus */
>> -    ret = mdiobus_register(mdp->mii_bus);
>> -    if (ret)
>> +        /* register mdio bus */
>> +        ret = mdiobus_register(mdp->mii_bus);
>> +    }
>
>     That looks much better, thanks.
>
>> +
>
>     Although I'd have avoided the empty line...
>
>> +    if (ret) {
>> +        dev_err(&ndev->dev, "failed to register mdio\n");
>
>     That function generally avoids error messages, no? Although all
> errors are -ENOMEM type and that's supposed to cause kernel to loudly
> curse anyway... Well, let it be if you want.

mdiobus_register() or of_mdiobus_register() could return any error,
but i've moved the error print to the caller as this is useful to
know.

Thanks for the reviews, hopefully the next round will work.


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