[PATCH/RFC 2/3] ethernet: add a PHY reset GPIO DT binding to sh_eth

[PATCH/RFC 2/3] ethernet: add a PHY reset GPIO DT binding to sh_eth

[Guennadi Liakhovetski]

If an ethernet PHY can be reset by a GPIO, it can be specified in DT. Add
a binding and code to parse it, request the GPIO and take the PHY out of
reset.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: netdev@vger.kernel.org
---
 Documentation/devicetree/bindings/net/sh_ether.txt |    2 ++
 drivers/net/ethernet/renesas/sh_eth.c              |    9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt
index c11e45d..edaf683 100644
--- a/Documentation/devicetree/bindings/net/sh_ether.txt
+++ b/Documentation/devicetree/bindings/net/sh_ether.txt
@@ -12,6 +12,7 @@ Required properties:
 - interrupts:                   Interrupt mapping for the sh_eth interrupt
                                 sources (vector id).
 - phy-mode:              String, operation mode of the PHY interface.
+- phy-reset-gpios:              PHY reset GPIO tuple
 - sh-eth,edmac-endian:          String, endian of sh_eth dmac.
 - sh-eth,register-type:         String, register type of sh_eth.
                                 Please select "gigabit", "fast-sh4" or
@@ -37,6 +38,7 @@ Example (armadillo800eva):
 		reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>;
 		interrupts = <0x500>;
 		phy-mode = "mii";
+		phy-reset-gpios = <&gpio 18 0>;
 		sh-eth,edmac-endian = "little";
 		sh-eth,register-type = "gigabit";
 		sh-eth,phy-id = <0>;
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 1f64848..06035a2 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -20,6 +20,7 @@
  *  the file called "COPYING".
  */
 
+#include <linux/gpio.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
@@ -33,6 +34,7 @@
 #include <linux/netdevice.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/of_platform.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -2376,10 +2378,11 @@ sh_eth_of_get_register_type(struct device_node *np)
 static struct sh_eth_plat_data *
 sh_eth_parse_dt(struct device *dev, struct net_device *ndev)
 {
-	int ret;
+	int ret, gpio;
 	const char *of_str;
 	struct device_node *np = dev->of_node;
 	struct sh_eth_plat_data *pdata;
+	enum of_gpio_flags flags;
 
 	pdata = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data),
 					GFP_KERNEL);
@@ -2420,6 +2423,10 @@ sh_eth_parse_dt(struct device *dev, struct net_device *ndev)
 	else
 		pdata->needs_init = 0;
 
+	gpio = of_get_named_gpio_flags(np, "phy-reset-gpios", 0, &flags);
+	if (gpio_is_valid(gpio) && !devm_gpio_request(dev, gpio, NULL))
+		gpio_direction_output(gpio, !!(flags & OF_GPIO_ACTIVE_LOW));
+
 #ifdef CONFIG_OF_NET
 	if (!is_valid_ether_addr(ndev->dev_addr)) {
 		const char *macaddr = of_get_mac_address(np);
-- 
1.7.2.5

Re: [PATCH/RFC 2/3] ethernet: add a PHY reset GPIO DT binding to sh_eth

[Laurent Pinchart]

Hi Guennadi,

On Thursday 24 January 2013 17:07:32 Guennadi Liakhovetski wrote:
> If an ethernet PHY can be reset by a GPIO, it can be specified in DT. Add
> a binding and code to parse it, request the GPIO and take the PHY out of
> reset.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: netdev@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/net/sh_ether.txt |    2 ++
>  drivers/net/ethernet/renesas/sh_eth.c              |    9 ++++++++-
>  2 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt
> b/Documentation/devicetree/bindings/net/sh_ether.txt index c11e45d..edaf683
> 100644
> --- a/Documentation/devicetree/bindings/net/sh_ether.txt
> +++ b/Documentation/devicetree/bindings/net/sh_ether.txt
> @@ -12,6 +12,7 @@ Required properties:
>  - interrupts:                   Interrupt mapping for the sh_eth interrupt
>                                  sources (vector id).
>  - phy-mode:              String, operation mode of the PHY interface.
> +- phy-reset-gpios:              PHY reset GPIO tuple

If you can't have more than one GPIO here, what about calling it phy-reset-
gpio ?

>  - sh-eth,edmac-endian:          String, endian of sh_eth dmac.
>  - sh-eth,register-type:         String, register type of sh_eth.
>                                  Please select "gigabit", "fast-sh4" or
> @@ -37,6 +38,7 @@ Example (armadillo800eva):
>  		reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>;
>  		interrupts = <0x500>;
>  		phy-mode = "mii";
> +		phy-reset-gpios = <&gpio 18 0>;
>  		sh-eth,edmac-endian = "little";
>  		sh-eth,register-type = "gigabit";
>  		sh-eth,phy-id = <0>;
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> b/drivers/net/ethernet/renesas/sh_eth.c index 1f64848..06035a2 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -20,6 +20,7 @@
>   *  the file called "COPYING".
>   */
> 
> +#include <linux/gpio.h>
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> @@ -33,6 +34,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_platform.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> @@ -2376,10 +2378,11 @@ sh_eth_of_get_register_type(struct device_node *np)
>  static struct sh_eth_plat_data *
>  sh_eth_parse_dt(struct device *dev, struct net_device *ndev)
>  {
> -	int ret;
> +	int ret, gpio;
>  	const char *of_str;
>  	struct device_node *np = dev->of_node;
>  	struct sh_eth_plat_data *pdata;
> +	enum of_gpio_flags flags;
> 
>  	pdata = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data),
>  					GFP_KERNEL);
> @@ -2420,6 +2423,10 @@ sh_eth_parse_dt(struct device *dev, struct net_device
> *ndev) else
>  		pdata->needs_init = 0;
> 
> +	gpio = of_get_named_gpio_flags(np, "phy-reset-gpios", 0, &flags);
> +	if (gpio_is_valid(gpio) && !devm_gpio_request(dev, gpio, NULL))
> +		gpio_direction_output(gpio, !!(flags & OF_GPIO_ACTIVE_LOW));

You could use devm_gpio_request_one() here.

Is there no need to reset the phy at runtime ?

> +
>  #ifdef CONFIG_OF_NET
>  	if (!is_valid_ether_addr(ndev->dev_addr)) {
>  		const char *macaddr = of_get_mac_address(np);
-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 2/3] ethernet: add a PHY reset GPIO DT binding to sh_eth

[Guennadi Liakhovetski]

Hi Laurent

On Fri, 25 Jan 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Thursday 24 January 2013 17:07:32 Guennadi Liakhovetski wrote:
> > If an ethernet PHY can be reset by a GPIO, it can be specified in DT. Add
> > a binding and code to parse it, request the GPIO and take the PHY out of
> > reset.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: devicetree-discuss@lists.ozlabs.org
> > Cc: netdev@vger.kernel.org
> > ---
> >  Documentation/devicetree/bindings/net/sh_ether.txt |    2 ++
> >  drivers/net/ethernet/renesas/sh_eth.c              |    9 ++++++++-
> >  2 files changed, 10 insertions(+), 1 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt
> > b/Documentation/devicetree/bindings/net/sh_ether.txt index c11e45d..edaf683
> > 100644
> > --- a/Documentation/devicetree/bindings/net/sh_ether.txt
> > +++ b/Documentation/devicetree/bindings/net/sh_ether.txt
> > @@ -12,6 +12,7 @@ Required properties:
> >  - interrupts:                   Interrupt mapping for the sh_eth interrupt
> >                                  sources (vector id).
> >  - phy-mode:              String, operation mode of the PHY interface.
> > +- phy-reset-gpios:              PHY reset GPIO tuple
> 
> If you can't have more than one GPIO here, what about calling it phy-reset-
> gpio ?

>From Documentation/devicetree/bindings/gpio/gpio.txt:

<quote>
GPIO properties should be named "[<name>-]gpios".
</quote>

> >  - sh-eth,edmac-endian:          String, endian of sh_eth dmac.
> >  - sh-eth,register-type:         String, register type of sh_eth.
> >                                  Please select "gigabit", "fast-sh4" or
> > @@ -37,6 +38,7 @@ Example (armadillo800eva):
> >  		reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>;
> >  		interrupts = <0x500>;
> >  		phy-mode = "mii";
> > +		phy-reset-gpios = <&gpio 18 0>;
> >  		sh-eth,edmac-endian = "little";
> >  		sh-eth,register-type = "gigabit";
> >  		sh-eth,phy-id = <0>;
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> > b/drivers/net/ethernet/renesas/sh_eth.c index 1f64848..06035a2 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> > @@ -20,6 +20,7 @@
> >   *  the file called "COPYING".
> >   */
> > 
> > +#include <linux/gpio.h>
> >  #include <linux/init.h>
> >  #include <linux/module.h>
> >  #include <linux/kernel.h>
> > @@ -33,6 +34,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_irq.h>
> > @@ -2376,10 +2378,11 @@ sh_eth_of_get_register_type(struct device_node *np)
> >  static struct sh_eth_plat_data *
> >  sh_eth_parse_dt(struct device *dev, struct net_device *ndev)
> >  {
> > -	int ret;
> > +	int ret, gpio;
> >  	const char *of_str;
> >  	struct device_node *np = dev->of_node;
> >  	struct sh_eth_plat_data *pdata;
> > +	enum of_gpio_flags flags;
> > 
> >  	pdata = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data),
> >  					GFP_KERNEL);
> > @@ -2420,6 +2423,10 @@ sh_eth_parse_dt(struct device *dev, struct net_device
> > *ndev) else
> >  		pdata->needs_init = 0;
> > 
> > +	gpio = of_get_named_gpio_flags(np, "phy-reset-gpios", 0, &flags);
> > +	if (gpio_is_valid(gpio) && !devm_gpio_request(dev, gpio, NULL))
> > +		gpio_direction_output(gpio, !!(flags & OF_GPIO_ACTIVE_LOW));
> 
> You could use devm_gpio_request_one() here.

Yes, but then the flag would look uglier, something like

		devm_gpio_request_one(dev, gpio, flags & 
			OF_GPIO_ACTIVE_LOW ? GPIOF_OUT_INIT_HIGH : 
			GPIOF_OUT_INIT_LOW);

Does it really look like an improvement? :)

> Is there no need to reset the phy at runtime ?

No idea, I'm not developing the driver, I'm just porting one specific 
feature from one API to another with no functional changes (apart from 
postponing setting the GPIO).

Thanks
Guennadi

> > +
> >  #ifdef CONFIG_OF_NET
> >  	if (!is_valid_ether_addr(ndev->dev_addr)) {
> >  		const char *macaddr = of_get_mac_address(np);
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC 2/3] ethernet: add a PHY reset GPIO DT binding to sh_eth

[Jason Gunthorpe]

On Fri, Jan 25, 2013 at 11:34:55AM +0100, Guennadi Liakhovetski wrote:

> > Is there no need to reset the phy at runtime ?
> 
> No idea, I'm not developing the driver, I'm just porting one specific 
> feature from one API to another with no functional changes (apart from 
> postponing setting the GPIO).

Generally Linux relies on resetting the phy via the inband MDIO method,
which is what Linux does. It is pretty much never required to reset
via the hard pin - but you do need to generate a robust reset edge on
the reset pin once after power up.

Jason

Re: [PATCH/RFC 2/3] ethernet: add a PHY reset GPIO DT binding to sh_eth

[Guennadi Liakhovetski]

Hi Jason

Sorry for resuming an old thread, but I'd like to get this solved 
eventually.

On Fri, 25 Jan 2013, Jason Gunthorpe wrote:

> On Fri, Jan 25, 2013 at 11:34:55AM +0100, Guennadi Liakhovetski wrote:
> 
> > > Is there no need to reset the phy at runtime ?
> > 
> > No idea, I'm not developing the driver, I'm just porting one specific 
> > feature from one API to another with no functional changes (apart from 
> > postponing setting the GPIO).
> 
> Generally Linux relies on resetting the phy via the inband MDIO method,
> which is what Linux does. It is pretty much never required to reset
> via the hard pin - but you do need to generate a robust reset edge on
> the reset pin once after power up.

This all sounds good. So, is there a preferred way to do that? Where in 
the ethernet framework would you propose to hook up such a reset edge 
generation?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC 2/3] ethernet: add a PHY reset GPIO DT binding to sh_eth

[Laurent Pinchart]

Hi Guennadi,

On Friday 25 January 2013 11:34:55 Guennadi Liakhovetski wrote:
> On Fri, 25 Jan 2013, Laurent Pinchart wrote:
> > On Thursday 24 January 2013 17:07:32 Guennadi Liakhovetski wrote:
> > > If an ethernet PHY can be reset by a GPIO, it can be specified in DT.
> > > Add a binding and code to parse it, request the GPIO and take the PHY
> > > out of reset.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > Cc: devicetree-discuss@lists.ozlabs.org
> > > Cc: netdev@vger.kernel.org
> > > ---
> > > 
> > >  Documentation/devicetree/bindings/net/sh_ether.txt |    2 ++
> > >  drivers/net/ethernet/renesas/sh_eth.c              |    9 ++++++++-
> > >  2 files changed, 10 insertions(+), 1 deletions(-)

[snip]

> > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> > > b/drivers/net/ethernet/renesas/sh_eth.c index 1f64848..06035a2 100644
> > > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > > +++ b/drivers/net/ethernet/renesas/sh_eth.c

[snip]

> > > @@ -2420,6 +2423,10 @@ sh_eth_parse_dt(struct device *dev, struct
> > > net_device *ndev) else
> > > 
> > >  		pdata->needs_init = 0;
> > > 
> > > +	gpio = of_get_named_gpio_flags(np, "phy-reset-gpios", 0, &flags);
> > > +	if (gpio_is_valid(gpio) && !devm_gpio_request(dev, gpio, NULL))
> > > +		gpio_direction_output(gpio, !!(flags & OF_GPIO_ACTIVE_LOW));
> > 
> > You could use devm_gpio_request_one() here.
> 
> Yes, but then the flag would look uglier, something like
> 
> 		devm_gpio_request_one(dev, gpio, flags &
> 			OF_GPIO_ACTIVE_LOW ? GPIOF_OUT_INIT_HIGH :
> 			GPIOF_OUT_INIT_LOW);
> 
> Does it really look like an improvement? :)

It's one less function call, so to me it does :-) Feel free to ignore that 
though.

-- 
Regards,

Laurent Pinchart