[PATCH v3] net/macb: add support for resetting PHY using GPIO

[PATCH v3] net/macb: add support for resetting PHY using GPIO

[Gregory CLEMENT]

With device tree it is no more possible to reset the PHY at board
level. Furthermore, doing in the driver allow to power down the PHY when
the network interface is no more used.

This reset can't be done at the PHY driver level. The PHY must be able to
answer to the mii bus scan to let the kernel creating a PHY device.

The patch introduces a new optional property "reset-gpios" at PHY level.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
Hi,

I agree with Sasha to start with a good binding and indeed the reset
is more related to the PHY than to the MAC even if currently we have
to manipulate it at MAC level.

I also followed Russell advice to not use fwnode functions. However
the following code seems to work:

        struct fwnode_handle *phy_node =
		device_get_next_child_node(&pdev->dev, NULL);
        bp->reset_gpio = fwnode_get_named_gpiod(phy_node, "reset-gpios");
        if (IS_ERR)
		bp->reset_gpio = NULL;
        gpiod_set_value(bp->reset_gpio, GPIOD_OUT_HIGH);

Given that it doesn't make the code better I kept the of_get_named_gpio method.

Gregory

Documentation/devicetree/bindings/net/macb.txt |  7 +++++++
 drivers/net/ethernet/cadence/macb.c            | 16 ++++++++++++++++
 drivers/net/ethernet/cadence/macb.h            |  1 +
 3 files changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index b5d7976..38c8e84 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -19,6 +19,9 @@ Required properties:
 	Optional elements: 'tx_clk'
 - clocks: Phandles to input clocks.
 
+Optional properties for PHY child node:
+- reset-gpios : Should specify the gpio for phy reset
+
 Examples:
 
 	macb0: ethernet@fffc4000 {
@@ -29,4 +32,8 @@ Examples:
 		local-mac-address = [3a 0e 03 04 05 06];
 		clock-names = "pclk", "hclk", "tx_clk";
 		clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
+		ethernet-phy@1 {
+			reg = <0x1>;
+			reset-gpios = <&pioE 6 1>;
+		};
 	};
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 88c1e1a..35661aa 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -28,6 +28,7 @@
 #include <linux/phy.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 
@@ -2813,6 +2814,7 @@ static int macb_probe(struct platform_device *pdev)
 					      = macb_clk_init;
 	int (*init)(struct platform_device *) = macb_init;
 	struct device_node *np = pdev->dev.of_node;
+	struct device_node *phy_node;
 	const struct macb_config *macb_config = NULL;
 	struct clk *pclk, *hclk, *tx_clk;
 	unsigned int queue_mask, num_queues;
@@ -2900,6 +2902,16 @@ static int macb_probe(struct platform_device *pdev)
 	else
 		macb_get_hwaddr(bp);
 
+	/* Power up the PHY if there is a GPIO reset */
+	phy_node =  of_get_next_available_child(np, NULL);
+	if (phy_node) {
+		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
+		if (gpio_is_valid(gpio))
+			bp->reset_gpio = gpio_to_desc(gpio);
+		gpiod_set_value(bp->reset_gpio, GPIOD_OUT_HIGH);
+	}
+	of_node_put(phy_node);
+
 	err = of_get_phy_mode(np);
 	if (err < 0) {
 		pdata = dev_get_platdata(&pdev->dev);
@@ -2966,6 +2978,10 @@ static int macb_remove(struct platform_device *pdev)
 		mdiobus_unregister(bp->mii_bus);
 		kfree(bp->mii_bus->irq);
 		mdiobus_free(bp->mii_bus);
+
+		/* Shutdown the PHY if there is a GPIO reset */
+		gpiod_set_value(bp->reset_gpio, GPIOD_OUT_LOW);
+
 		unregister_netdev(dev);
 		clk_disable_unprepare(bp->tx_clk);
 		clk_disable_unprepare(bp->hclk);
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 6e1faea..b6ec421 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -824,6 +824,7 @@ struct macb {
 	unsigned int		dma_burst_length;
 
 	phy_interface_t		phy_interface;
+	struct gpio_desc	*reset_gpio;
 
 	/* AT91RM9200 transmit */
 	struct sk_buff *skb;			/* holds skb until xmit interrupt completes */
-- 
2.5.0

Re: [PATCH v3] net/macb: add support for resetting PHY using GPIO

[Richard Cochran]

On Wed, Dec 16, 2015 at 07:31:30PM +0100, Gregory CLEMENT wrote:
> +Optional properties for PHY child node:
> +- reset-gpios : Should specify the gpio for phy reset

reset-gpios plural or reset-gpio singular?

> +
>  Examples:
>  
>  	macb0: ethernet@fffc4000 {
> @@ -29,4 +32,8 @@ Examples:
>  		local-mac-address = [3a 0e 03 04 05 06];
>  		clock-names = "pclk", "hclk", "tx_clk";
>  		clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
> +		ethernet-phy@1 {
> +			reg = <0x1>;
> +			reset-gpios = <&pioE 6 1>;

Did you mean "gpioE" ?

Thanks,
Richard

Re: [PATCH v3] net/macb: add support for resetting PHY using GPIO

[Gregory CLEMENT]

Hi Richard,
 
 On mer., déc. 16 2015, Richard Cochran <richardcochran@gmail.com> wrote:

> On Wed, Dec 16, 2015 at 07:31:30PM +0100, Gregory CLEMENT wrote:
>> +Optional properties for PHY child node:
>> +- reset-gpios : Should specify the gpio for phy reset
>
> reset-gpios plural or reset-gpio singular?

The bindings name is plural but it handle only one gpio.

See:
https://lkml.org/lkml/2015/12/9/623

>
>> +
>>  Examples:
>>  
>>  	macb0: ethernet@fffc4000 {
>> @@ -29,4 +32,8 @@ Examples:
>>  		local-mac-address = [3a 0e 03 04 05 06];
>>  		clock-names = "pclk", "hclk", "tx_clk";
>>  		clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
>> +		ethernet-phy@1 {
>> +			reg = <0x1>;
>> +			reset-gpios = <&pioE 6 1>;
>
> Did you mean "gpioE" ?

No, I really mean pioE: it is the name used for the at91 SoCs.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v3] net/macb: add support for resetting PHY using GPIO

[Arnd Bergmann]

On Wednesday 16 December 2015 19:31:30 Gregory CLEMENT wrote:
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 88c1e1a..35661aa 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -28,6 +28,7 @@
>  #include <linux/phy.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>

Is this the patch that is already in linux-next?

I needed an additional

#include <linux/gpio/consumer.h>

to avoid this build error on randconfig builds without GPIOLIB:

    drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
    drivers/net/ethernet/cadence/macb.c:2908:19: error: implicit declaration of function 'devm_gpiod_get_optional' [-Werror=implicit-function-declaration]
      bp->reset_gpio = devm_gpiod_get_optional(&bp->pdev->dev, "phy-reset",
                       ^
    drivers/net/ethernet/cadence/macb.c:2909:8: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
            GPIOD_OUT_HIGH);
            ^
    drivers/net/ethernet/cadence/macb.c:2909:8: note: each undeclared identifier is reported only once for each function it appears in
    drivers/net/ethernet/cadence/macb.c: In function 'macb_remove':
    drivers/net/ethernet/cadence/macb.c:2979:3: error: implicit declaration of function 'gpiod_set_value' [-Werror=implicit-function-declaration]


	Arnd

Re: [PATCH v3] net/macb: add support for resetting PHY using GPIO

[Gregory CLEMENT]

Hi Arnd,
 
 On mer., déc. 16 2015, Arnd Bergmann <arnd@arndb.de> wrote:

> On Wednesday 16 December 2015 19:31:30 Gregory CLEMENT wrote:
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index 88c1e1a..35661aa 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/phy.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> +#include <linux/of_gpio.h>
>>  #include <linux/of_mdio.h>
>>  #include <linux/of_net.h>
>
> Is this the patch that is already in linux-next?

I've just checked and it is the v2 which is in linux-next. David applied
it on Monday in his next branche but I was not aware of it.

David,

if I remebered well you do not remove patch from yout branch.  So would
you agree to take a follow-up patch on top of 5833e0526820 "net/macb:
add support for resetting PHY using GPIO" ?

I will fix the error found by Arnd and also use a better device tree
binding (more future proof).

Thanks,

Gregory

>
> I needed an additional
>
> #include <linux/gpio/consumer.h>
>
> to avoid this build error on randconfig builds without GPIOLIB:
>
>     drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
>     drivers/net/ethernet/cadence/macb.c:2908:19: error: implicit declaration of function 'devm_gpiod_get_optional' [-Werror=implicit-function-declaration]
>       bp->reset_gpio = devm_gpiod_get_optional(&bp->pdev->dev, "phy-reset",
>                        ^
>     drivers/net/ethernet/cadence/macb.c:2909:8: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
>             GPIOD_OUT_HIGH);
>             ^
>     drivers/net/ethernet/cadence/macb.c:2909:8: note: each undeclared identifier is reported only once for each function it appears in
>     drivers/net/ethernet/cadence/macb.c: In function 'macb_remove':
>     drivers/net/ethernet/cadence/macb.c:2979:3: error: implicit declaration of function 'gpiod_set_value' [-Werror=implicit-function-declaration]
>
>
> 	Arnd

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] net/macb: Update device tree binding for resetting PHY using GPIO

[Gregory CLEMENT]

Instead of being at the MAC level the reset gpio preperty is moved at the
PHY child node level. It is still managed by the MAC, but from the point
of view of the binding it make more sense to be part of the PHY node.

This commit also fixes a build errors if GPIOLIB is not selected.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 Documentation/devicetree/bindings/net/macb.txt |  8 ++++++--
 drivers/net/ethernet/cadence/macb.c            | 15 ++++++++++++---
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index 4a7fb6c..38c8e84 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -19,8 +19,8 @@ Required properties:
 	Optional elements: 'tx_clk'
 - clocks: Phandles to input clocks.
 
-Optional properties:
-- phy-reset-gpios : Should specify the gpio for phy reset
+Optional properties for PHY child node:
+- reset-gpios : Should specify the gpio for phy reset
 
 Examples:
 
@@ -32,4 +32,8 @@ Examples:
 		local-mac-address = [3a 0e 03 04 05 06];
 		clock-names = "pclk", "hclk", "tx_clk";
 		clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
+		ethernet-phy@1 {
+			reg = <0x1>;
+			reset-gpios = <&pioE 6 1>;
+		};
 	};
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 71fbda3..12370dd 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -19,6 +19,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
@@ -28,6 +29,7 @@
 #include <linux/phy.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 
@@ -2813,6 +2815,7 @@ static int macb_probe(struct platform_device *pdev)
 					      = macb_clk_init;
 	int (*init)(struct platform_device *) = macb_init;
 	struct device_node *np = pdev->dev.of_node;
+	struct device_node *phy_node;
 	const struct macb_config *macb_config = NULL;
 	struct clk *pclk, *hclk, *tx_clk;
 	unsigned int queue_mask, num_queues;
@@ -2901,8 +2904,14 @@ static int macb_probe(struct platform_device *pdev)
 		macb_get_hwaddr(bp);
 
 	/* Power up the PHY if there is a GPIO reset */
-	bp->reset_gpio = devm_gpiod_get_optional(&bp->pdev->dev, "phy-reset",
-						 GPIOD_OUT_HIGH);
+	phy_node =  of_get_next_available_child(np, NULL);
+	if (phy_node) {
+		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
+		if (gpio_is_valid(gpio))
+			bp->reset_gpio = gpio_to_desc(gpio);
+		gpiod_set_value(bp->reset_gpio, GPIOD_OUT_HIGH);
+	}
+	of_node_put(phy_node);
 
 	err = of_get_phy_mode(np);
 	if (err < 0) {
@@ -2972,7 +2981,7 @@ static int macb_remove(struct platform_device *pdev)
 		mdiobus_free(bp->mii_bus);
 
 		/* Shutdown the PHY if there is a GPIO reset */
-		gpiod_set_value(bp->reset_gpio, 0);
+		gpiod_set_value(bp->reset_gpio, GPIOD_OUT_LOW);
 
 		unregister_netdev(dev);
 		clk_disable_unprepare(bp->tx_clk);
-- 
2.5.0

Re: [PATCH v3] net/macb: add support for resetting PHY using GPIO

[David Miller]

From: Gregory CLEMENT <gregory.clement@free-electrons.com>
Date: Thu, 17 Dec 2015 09:39:32 +0100

> if I remebered well you do not remove patch from yout branch.  So would
> you agree to take a follow-up patch on top of 5833e0526820 "net/macb:
> add support for resetting PHY using GPIO" ?

Yes.

Re: [PATCH] net/macb: Update device tree binding for resetting PHY using GPIO

[David Miller]

From: Gregory CLEMENT <gregory.clement@free-electrons.com>
Date: Thu, 17 Dec 2015 10:51:04 +0100

> Instead of being at the MAC level the reset gpio preperty is moved at the
> PHY child node level. It is still managed by the MAC, but from the point
> of view of the binding it make more sense to be part of the PHY node.
> 
> This commit also fixes a build errors if GPIOLIB is not selected.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Applied to net-next, thanks.

Please be explicit in your patch postings what tree you are targetting
a change at.