[PATCH 0/4] ravb: Add PHY reset support

[PATCH 0/4] ravb: Add PHY reset support

[Geert Uytterhoeven]

	Hi all,

This patch series adds optional PHY reset support during EthernetAVB driver
probe and system resume on the Renesas Salvator-X/XS development boards.

The rationale behind this is twofold:
  1. On Salvator-XS, the enable pin of the regulator providing PHY power
     is connected to PRESETn, and PSCI powers down the SoC during system
     suspend.  Hence a PHY reset is needed to restore network functionality
     after resume.
  2. Linux should not rely on the boot loader having reset the PHY, but
     should reset the PHY during driver probe.

The first two patches are destined for David's net-next tree. They update
the EthernetAVB DT bindings, and add support for resetting the PHY during
system resume.

The last two patches are destined for Simon's renesas tree.  They add
properties to describe the EthernetAVB PHY reset topology to the common
Salvator-X/XS and ULCB DTS files.

Thanks!

Geert Uytterhoeven (4):
  dt-bindings: net: ravb: Document optional reset-gpios property
  ravb: Add optional PHY reset during system resume
  arm64: dts: renesas: salvator-common: Add EthernetAVB PHY reset
  arm64: dts: renesas: ulcb: Add EthernetAVB PHY reset

 Documentation/devicetree/bindings/net/renesas,ravb.txt | 2 ++
 arch/arm64/boot/dts/renesas/salvator-common.dtsi       | 1 +
 arch/arm64/boot/dts/renesas/ulcb.dtsi                  | 1 +
 drivers/net/ethernet/renesas/ravb_main.c               | 9 +++++++++
 4 files changed, 13 insertions(+)

-- 
2.7.4

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/4] dt-bindings: net: ravb: Document optional reset-gpios property

[Geert Uytterhoeven]

The optional "reset-gpios" property (part of the generic MDIO bus
properties) lets us describe the GPIO used for resetting the Ethernet
PHY.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/net/renesas,ravb.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
index c902261893b913f5..4a6ec1ba32d0bf16 100644
--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
@@ -52,6 +52,7 @@ Optional properties:
 			 AVB_LINK signal.
 - renesas,ether-link-active-low: boolean, specify when the AVB_LINK signal is
 				 active-low instead of normal active-high.
+- reset-gpios: see mdio.txt in the same directory.
 
 Example:
 
@@ -99,6 +100,7 @@ Example:
 		pinctrl-0 = <&ether_pins>;
 		pinctrl-names = "default";
 		renesas,no-ether-link;
+		reset-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-- 
2.7.4

Re: [PATCH 1/4] dt-bindings: net: ravb: Document optional reset-gpios property

[Sergei Shtylyov]

Hello!

On 09/28/2017 06:53 PM, Geert Uytterhoeven wrote:

> The optional "reset-gpios" property (part of the generic MDIO bus
> properties) lets us describe the GPIO used for resetting the Ethernet
> PHY.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>   Documentation/devicetree/bindings/net/renesas,ravb.txt | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> index c902261893b913f5..4a6ec1ba32d0bf16 100644
> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> @@ -52,6 +52,7 @@ Optional properties:
>   			 AVB_LINK signal.
>   - renesas,ether-link-active-low: boolean, specify when the AVB_LINK signal is
>   				 active-low instead of normal active-high.
> +- reset-gpios: see mdio.txt in the same directory.

    Sigh, I can only repeat that was a terrible prop name choice -- when 
applied to a MAC node... what reset does it mean? MAC?

MBR, Sergei

Re: [PATCH 1/4] dt-bindings: net: ravb: Document optional reset-gpios property

[Rob Herring]

On Thu, Sep 28, 2017 at 11:07:38PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 09/28/2017 06:53 PM, Geert Uytterhoeven wrote:
> 
> > The optional "reset-gpios" property (part of the generic MDIO bus
> > properties) lets us describe the GPIO used for resetting the Ethernet
> > PHY.
> > 
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >   Documentation/devicetree/bindings/net/renesas,ravb.txt | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > index c902261893b913f5..4a6ec1ba32d0bf16 100644
> > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > @@ -52,6 +52,7 @@ Optional properties:
> >   			 AVB_LINK signal.
> >   - renesas,ether-link-active-low: boolean, specify when the AVB_LINK signal is
> >   				 active-low instead of normal active-high.
> > +- reset-gpios: see mdio.txt in the same directory.
> 
>    Sigh, I can only repeat that was a terrible prop name choice -- when
> applied to a MAC node... what reset does it mean? MAC?

Agreed. This should be in the phy node. Or MDIO at least.

[PATCH 2/4] ravb: Add optional PHY reset during system resume

[Geert Uytterhoeven]

If the optional "reset-gpios" property is specified in DT, the generic
MDIO bus code takes care of resetting the PHY during device probe.
However, the PHY may still have to be reset explicitly after system
resume.

This allows to restore Ethernet operation after resume from s2ram on
Salvator-XS, where the enable pin of the regulator providing PHY power
is connected to PRESETn, and PSCI suspend powers down the SoC.

Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
 drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index fdf30bfa403bf416..96d1d48e302f8c9a 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -19,6 +19,7 @@
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -2268,6 +2269,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct ravb_private *priv = netdev_priv(ndev);
+	struct mii_bus *bus = priv->mii_bus;
 	int ret = 0;
 
 	if (priv->wol_enabled) {
@@ -2302,6 +2304,13 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	 * reopen device if it was running before system suspended.
 	 */
 
+	/* PHY reset */
+	if (bus->reset_gpiod) {
+		gpiod_set_value_cansleep(bus->reset_gpiod, 1);
+		udelay(bus->reset_delay_us);
+		gpiod_set_value_cansleep(bus->reset_gpiod, 0);
+	}
+
 	/* Set AVB config mode */
 	ravb_set_config_mode(ndev);
 
-- 
2.7.4

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

Re: [PATCH 2/4] ravb: Add optional PHY reset during system resume

[Florian Fainelli]

On 09/28/2017 08:53 AM, Geert Uytterhoeven wrote:
> If the optional "reset-gpios" property is specified in DT, the generic
> MDIO bus code takes care of resetting the PHY during device probe.
> However, the PHY may still have to be reset explicitly after system
> resume.
> 
> This allows to restore Ethernet operation after resume from s2ram on
> Salvator-XS, where the enable pin of the regulator providing PHY power
> is connected to PRESETn, and PSCI suspend powers down the SoC.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index fdf30bfa403bf416..96d1d48e302f8c9a 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -19,6 +19,7 @@
>  #include <linux/etherdevice.h>
>  #include <linux/ethtool.h>
>  #include <linux/if_vlan.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -2268,6 +2269,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  {
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct ravb_private *priv = netdev_priv(ndev);
> +	struct mii_bus *bus = priv->mii_bus;
>  	int ret = 0;
>  
>  	if (priv->wol_enabled) {
> @@ -2302,6 +2304,13 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  	 * reopen device if it was running before system suspended.
>  	 */
>  
> +	/* PHY reset */
> +	if (bus->reset_gpiod) {
> +		gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> +		udelay(bus->reset_delay_us);
> +		gpiod_set_value_cansleep(bus->reset_gpiod, 0);
> +	}

This is a clever hack, but unfortunately this is also misusing the MDIO
bus reset line into a PHY reset line. As commented in patch 3, if this
reset line is tied to the PHY, then this should be a PHY property and
you cannot (ab)use the MDIO bus GPIO reset logic anymore...

Should not you also try to manage this reset line during ravb_open() to
achiever better power savings?
-- 
Florian

Re: [PATCH 2/4] ravb: Add optional PHY reset during system resume

[Geert Uytterhoeven]

Hi Florian,

On Thu, Sep 28, 2017 at 7:22 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 09/28/2017 08:53 AM, Geert Uytterhoeven wrote:
>> If the optional "reset-gpios" property is specified in DT, the generic
>> MDIO bus code takes care of resetting the PHY during device probe.
>> However, the PHY may still have to be reset explicitly after system
>> resume.
>>
>> This allows to restore Ethernet operation after resume from s2ram on
>> Salvator-XS, where the enable pin of the regulator providing PHY power
>> is connected to PRESETn, and PSCI suspend powers down the SoC.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index fdf30bfa403bf416..96d1d48e302f8c9a 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/etherdevice.h>
>>  #include <linux/ethtool.h>
>>  #include <linux/if_vlan.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/kernel.h>
>>  #include <linux/list.h>
>>  #include <linux/module.h>
>> @@ -2268,6 +2269,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
>>  {
>>       struct net_device *ndev = dev_get_drvdata(dev);
>>       struct ravb_private *priv = netdev_priv(ndev);
>> +     struct mii_bus *bus = priv->mii_bus;
>>       int ret = 0;
>>
>>       if (priv->wol_enabled) {
>> @@ -2302,6 +2304,13 @@ static int __maybe_unused ravb_resume(struct device *dev)
>>        * reopen device if it was running before system suspended.
>>        */
>>
>> +     /* PHY reset */
>> +     if (bus->reset_gpiod) {
>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 1);
>> +             udelay(bus->reset_delay_us);
>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 0);
>> +     }
>
> This is a clever hack, but unfortunately this is also misusing the MDIO
> bus reset line into a PHY reset line. As commented in patch 3, if this
> reset line is tied to the PHY, then this should be a PHY property and

OK.

> you cannot (ab)use the MDIO bus GPIO reset logic anymore...

And then I should add reset-gpios support to drivers/net/phy/micrel.c?
Or is there already generic code to handle per-PHY reset? I couldn't find it.

> Should not you also try to manage this reset line during ravb_open() to
> achiever better power savings?

I don't know. The Micrel KSZ9031RNXVA datasheet doesn't mention if it's
safe or not to assert reset for a prolonged time.

Thanks!

Gr{oetje,eeting}s,

                        Geert

Re: [PATCH 2/4] ravb: Add optional PHY reset during system resume

[Florian Fainelli]

On 09/28/2017 11:45 AM, Geert Uytterhoeven wrote:
> Hi Florian,
> 
> On Thu, Sep 28, 2017 at 7:22 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 09/28/2017 08:53 AM, Geert Uytterhoeven wrote:
>>> If the optional "reset-gpios" property is specified in DT, the generic
>>> MDIO bus code takes care of resetting the PHY during device probe.
>>> However, the PHY may still have to be reset explicitly after system
>>> resume.
>>>
>>> This allows to restore Ethernet operation after resume from s2ram on
>>> Salvator-XS, where the enable pin of the regulator providing PHY power
>>> is connected to PRESETn, and PSCI suspend powers down the SoC.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index fdf30bfa403bf416..96d1d48e302f8c9a 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -19,6 +19,7 @@
>>>  #include <linux/etherdevice.h>
>>>  #include <linux/ethtool.h>
>>>  #include <linux/if_vlan.h>
>>> +#include <linux/gpio/consumer.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/list.h>
>>>  #include <linux/module.h>
>>> @@ -2268,6 +2269,7 @@ static int __maybe_unused ravb_resume(struct device *dev)
>>>  {
>>>       struct net_device *ndev = dev_get_drvdata(dev);
>>>       struct ravb_private *priv = netdev_priv(ndev);
>>> +     struct mii_bus *bus = priv->mii_bus;
>>>       int ret = 0;
>>>
>>>       if (priv->wol_enabled) {
>>> @@ -2302,6 +2304,13 @@ static int __maybe_unused ravb_resume(struct device *dev)
>>>        * reopen device if it was running before system suspended.
>>>        */
>>>
>>> +     /* PHY reset */
>>> +     if (bus->reset_gpiod) {
>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 1);
>>> +             udelay(bus->reset_delay_us);
>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 0);
>>> +     }
>>
>> This is a clever hack, but unfortunately this is also misusing the MDIO
>> bus reset line into a PHY reset line. As commented in patch 3, if this
>> reset line is tied to the PHY, then this should be a PHY property and
> 
> OK.
> 
>> you cannot (ab)use the MDIO bus GPIO reset logic anymore...
> 
> And then I should add reset-gpios support to drivers/net/phy/micrel.c?
> Or is there already generic code to handle per-PHY reset? I couldn't find it.

There is not such a thing unfortunately, but it would presumably be
called within drivers/net/phy/mdio_bus.c during bus->reset() time
because you need the PHY reset to be deasserted before you can
successfully read/write from the PHY, and if you can't read/write from
the PHY, the MDIO bus layer cannot read the PHY ID, and therefore cannot
match a PHY device with its driver, so things don't work.

NB: you could move this entirely to the Micrel PHY driver if you specify
a compatible string that has a the PHY OUI in it, because that bypasses
the need to match the PHY driver with the PHY device, but this may not
be an acceptable solution for non-DT platforms or other platforms where
the PHY can't be determined based on the board DTS.

I was going to suggest writing some sort of generic helper that walks
the list of child nodes from a MDIO bus device node and deassert reset
lines and enables clocks, but there is absolutely nothing generic about
that. Things like which of the reset should come first, and if there are
multiple, in which order, etc.

> 
>> Should not you also try to manage this reset line during ravb_open() to
>> achiever better power savings?
> 
> I don't know. The Micrel KSZ9031RNXVA datasheet doesn't mention if it's
> safe or not to assert reset for a prolonged time.
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
Florian

Re: [PATCH 2/4] ravb: Add optional PHY reset during system resume

[Sergei Shtylyov]

Hello!

On 09/28/2017 10:21 PM, Florian Fainelli wrote:

>>>> If the optional "reset-gpios" property is specified in DT, the generic
>>>> MDIO bus code takes care of resetting the PHY during device probe.
>>>> However, the PHY may still have to be reset explicitly after system
>>>> resume.
>>>>
>>>> This allows to restore Ethernet operation after resume from s2ram on
>>>> Salvator-XS, where the enable pin of the regulator providing PHY power
>>>> is connected to PRESETn, and PSCI suspend powers down the SoC.
>>>>
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> ---
>>>>   drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index fdf30bfa403bf416..96d1d48e302f8c9a 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>>> @@ -2302,6 +2304,13 @@ static int __maybe_unused ravb_resume(struct device *dev)
>>>>         * reopen device if it was running before system suspended.
>>>>         */
>>>>
>>>> +     /* PHY reset */
>>>> +     if (bus->reset_gpiod) {
>>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 1);
>>>> +             udelay(bus->reset_delay_us);
>>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 0);
>>>> +     }
>>>
>>> This is a clever hack, but unfortunately this is also misusing the MDIO
>>> bus reset line into a PHY reset line. As commented in patch 3, if this
>>> reset line is tied to the PHY, then this should be a PHY property and
>>
>> OK.
>>
>>> you cannot (ab)use the MDIO bus GPIO reset logic anymore...
>>
>> And then I should add reset-gpios support to drivers/net/phy/micrel.c?
>> Or is there already generic code to handle per-PHY reset? I couldn't find it.
> 
> There is not such a thing unfortunately, but it would presumably be

    It's strange you don't remember about my (abandoned) patches to handle 
per=PHY reset GPIOs -- perhaps it's time to unearth them. Here they are:

http://patchwork.ozlabs.org/patch/616495/
http://patchwork.ozlabs.org/patch/616501/

    I had v3 in the works before abandoning this series -- it doesn't apply now.

> called within drivers/net/phy/mdio_bus.c during bus->reset() time
> because you need the PHY reset to be deasserted before you can
> successfully read/write from the PHY, and if you can't read/write from
> the PHY, the MDIO bus layer cannot read the PHY ID, and therefore cannot
> match a PHY device with its driver, so things don't work.

    I did this a bit differently...

[...]

MBR, Sergei

Re: [PATCH 2/4] ravb: Add optional PHY reset during system resume

[Florian Fainelli]

On 09/30/2017 01:23 PM, Sergei Shtylyov wrote:
> Hello!
> 
> On 09/28/2017 10:21 PM, Florian Fainelli wrote:
> 
>>>>> If the optional "reset-gpios" property is specified in DT, the generic
>>>>> MDIO bus code takes care of resetting the PHY during device probe.
>>>>> However, the PHY may still have to be reset explicitly after system
>>>>> resume.
>>>>>
>>>>> This allows to restore Ethernet operation after resume from s2ram on
>>>>> Salvator-XS, where the enable pin of the regulator providing PHY power
>>>>> is connected to PRESETn, and PSCI suspend powers down the SoC.
>>>>>
>>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>> ---
>>>>>   drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++++
>>>>>   1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> index fdf30bfa403bf416..96d1d48e302f8c9a 100644
>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>>>>> @@ -2302,6 +2304,13 @@ static int __maybe_unused ravb_resume(struct
>>>>> device *dev)
>>>>>         * reopen device if it was running before system suspended.
>>>>>         */
>>>>>
>>>>> +     /* PHY reset */
>>>>> +     if (bus->reset_gpiod) {
>>>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 1);
>>>>> +             udelay(bus->reset_delay_us);
>>>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 0);
>>>>> +     }
>>>>
>>>> This is a clever hack, but unfortunately this is also misusing the MDIO
>>>> bus reset line into a PHY reset line. As commented in patch 3, if this
>>>> reset line is tied to the PHY, then this should be a PHY property and
>>>
>>> OK.
>>>
>>>> you cannot (ab)use the MDIO bus GPIO reset logic anymore...
>>>
>>> And then I should add reset-gpios support to drivers/net/phy/micrel.c?
>>> Or is there already generic code to handle per-PHY reset? I couldn't
>>> find it.
>>
>> There is not such a thing unfortunately, but it would presumably be
> 
>    It's strange you don't remember about my (abandoned) patches to
> handle per=PHY reset GPIOs -- perhaps it's time to unearth them. Here
> they are:
> 
> http://patchwork.ozlabs.org/patch/616495/
> http://patchwork.ozlabs.org/patch/616501/
> 
>    I had v3 in the works before abandoning this series -- it doesn't
> apply now.

Should Geert pick-up where you left and address the feedback given in
v2, or do you plan to post a rebased v3?

> 
>> called within drivers/net/phy/mdio_bus.c during bus->reset() time
>> because you need the PHY reset to be deasserted before you can
>> successfully read/write from the PHY, and if you can't read/write from
>> the PHY, the MDIO bus layer cannot read the PHY ID, and therefore cannot
>> match a PHY device with its driver, so things don't work.
> 
>    I did this a bit differently...
> 
> [...]
> 
> MBR, Sergei

-- 
Florian

Re: [PATCH 2/4] ravb: Add optional PHY reset during system resume

[Sergei Shtylyov]

Hello!

On 10/1/2017 7:34 PM, Florian Fainelli wrote:

>>>>>> If the optional "reset-gpios" property is specified in DT, the generic
>>>>>> MDIO bus code takes care of resetting the PHY during device probe.
>>>>>> However, the PHY may still have to be reset explicitly after system
>>>>>> resume.
>>>>>>
>>>>>> This allows to restore Ethernet operation after resume from s2ram on
>>>>>> Salvator-XS, where the enable pin of the regulator providing PHY power
>>>>>> is connected to PRESETn, and PSCI suspend powers down the SoC.
>>>>>>
>>>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>> ---
>>>>>>    drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++++
>>>>>>    1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> index fdf30bfa403bf416..96d1d48e302f8c9a 100644
>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> [...]
>>>>>> @@ -2302,6 +2304,13 @@ static int __maybe_unused ravb_resume(struct
>>>>>> device *dev)he patches
>>>>>>          * reopen device if it was running before system suspended.
>>>>>>          */
>>>>>>
>>>>>> +     /* PHY reset */
>>>>>> +     if (bus->reset_gpiod) {
>>>>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 1);
>>>>>> +             udelay(bus->reset_delay_us);
>>>>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 0);
>>>>>> +     }
>>>>>
>>>>> This is a clever hack, but unfortunately this is also misusing the MDIO
>>>>> bus reset line into a PHY reset line. As commented in patch 3, if this
>>>>> reset line is tied to the PHY, then this should be a PHY property and
>>>>
>>>> OK.
>>>>
>>>>> you cannot (ab)use the MDIO bus GPIO reset logic anymore...
>>>>
>>>> And then I should add reset-gpios support to drivers/net/phy/micrel.c?
>>>> Or is there already generic code to handle per-PHY reset? I couldn't
>>>> find it.
>>>
>>> There is not such a thing unfortunately, but it would presumably be
>>
>>     It's strange you don't remember about my (abandoned) patches to
>> handle per=PHY reset GPIOs -- perhaps it's time to unearth them. Here
>> they are:
>>
>> http://patchwork.ozlabs.org/patch/616495/
>> http://patchwork.ozlabs.org/patch/616501/
>>
>>     I had v3 in the works before abandoning this series -- it doesn't
>> apply now.
> 
> Should Geert pick-up where you left and address the feedback given in
> v2, or do you plan to post a rebased v3?

    I was going to address the rejects in v3 and give the patchset to Geert... 
unfortunately, this took so-o-o long. I'm going to do it today, at last.

[...]

MBR, Sergei

Re: [PATCH 2/4] ravb: Add optional PHY reset during system resume

[Sergei Shtylyov]

[-- Attachment #1: Type: text/plain, Size: 2880 bytes --]

On 10/09/2017 12:37 PM, Sergei Shtylyov wrote:

>>>>>>> If the optional "reset-gpios" property is specified in DT, the generic
>>>>>>> MDIO bus code takes care of resetting the PHY during device probe.
>>>>>>> However, the PHY may still have to be reset explicitly after system
>>>>>>> resume.
>>>>>>>
>>>>>>> This allows to restore Ethernet operation after resume from s2ram on
>>>>>>> Salvator-XS, where the enable pin of the regulator providing PHY power
>>>>>>> is connected to PRESETn, and PSCI suspend powers down the SoC.
>>>>>>>
>>>>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>> ---
>>>>>>>    drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++++
>>>>>>>    1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>> index fdf30bfa403bf416..96d1d48e302f8c9a 100644
>>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> [...]
>>>>>>> @@ -2302,6 +2304,13 @@ static int __maybe_unused ravb_resume(struct
>>>>>>> device *dev)he patches
>>>>>>>          * reopen device if it was running before system suspended.
>>>>>>>          */
>>>>>>>
>>>>>>> +     /* PHY reset */
>>>>>>> +     if (bus->reset_gpiod) {
>>>>>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 1);
>>>>>>> +             udelay(bus->reset_delay_us);
>>>>>>> +             gpiod_set_value_cansleep(bus->reset_gpiod, 0);
>>>>>>> +     }
>>>>>>
>>>>>> This is a clever hack, but unfortunately this is also misusing the MDIO
>>>>>> bus reset line into a PHY reset line. As commented in patch 3, if this
>>>>>> reset line is tied to the PHY, then this should be a PHY property and
>>>>>
>>>>> OK.
>>>>>
>>>>>> you cannot (ab)use the MDIO bus GPIO reset logic anymore...
>>>>>
>>>>> And then I should add reset-gpios support to drivers/net/phy/micrel.c?
>>>>> Or is there already generic code to handle per-PHY reset? I couldn't
>>>>> find it.
>>>>
>>>> There is not such a thing unfortunately, but it would presumably be
>>>
>>>     It's strange you don't remember about my (abandoned) patches to
>>> handle per=PHY reset GPIOs -- perhaps it's time to unearth them. Here
>>> they are:
>>>
>>> http://patchwork.ozlabs.org/patch/616495/
>>> http://patchwork.ozlabs.org/patch/616501/
>>>
>>>     I had v3 in the works before abandoning this series -- it doesn't
>>> apply now.
>>
>> Should Geert pick-up where you left and address the feedback given in
>> v2, or do you plan to post a rebased v3?
> 
>     I was going to address the rejects in v3 and give the patchset to Geert... 
> unfortunately, this took so-o-o long. I'm going to do it today, at last.

    Here you are! Attaching both the patches against net-next.git...
Perhaps I also will be able to return to this subject in the near future...

MBR, Sergei

[-- Attachment #2: phylib-add-device-reset-GPIO-support-v3.patch --]
[-- Type: text/x-patch, Size: 11599 bytes --]

Subject: phylib: add device reset GPIO support

The PHY devices sometimes do have their reset signal (maybe even power
supply?) tied to some GPIO and sometimes it also does happen that a boot
loader does not leave it deasserted. So far this issue has been attacked
from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
the GPIO in question; that solution, when applied to the device trees, led
to adding the PHY reset GPIO properties to the MAC device node, with one
exception: Cadence MACB driver which could handle the "reset-gpios" prop
in a PHY device subnode. I believe that the correct approach is to teach
the 'phylib' to get the MDIO device reset GPIO from the device tree node
corresponding to this device -- which this patch is doing...

Note that I had to modify the AT803x PHY driver as it would stop working
otherwise -- it made use of the reset GPIO for its own purposes...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 3:
- fixed the fwnode_get_named_gpiod() call due to the added parameters (which
  allowed to eliminate gpiod_direction_output() call);
- undeleted one blank line in the AT803x driver;
- resolved rejects, refreshed the patch;
- reworded/reformatted the changelog.

Changes in version 2:
- reformatted the changelog;
- resolved rejects, refreshed the patch.

 Documentation/devicetree/bindings/net/phy.txt |    2 +
 drivers/net/phy/at803x.c                      |   18 ++------------
 drivers/net/phy/mdio_bus.c                    |    4 +++
 drivers/net/phy/mdio_device.c                 |   27 +++++++++++++++++++--
 drivers/net/phy/phy_device.c                  |   33 ++++++++++++++++++++++++--
 drivers/of/of_mdio.c                          |   16 ++++++++++++
 include/linux/mdio.h                          |    3 ++
 include/linux/phy.h                           |    5 +++
 8 files changed, 89 insertions(+), 19 deletions(-)

Index: net-next/Documentation/devicetree/bindings/net/phy.txt
===================================================================
--- net-next.orig/Documentation/devicetree/bindings/net/phy.txt
+++ net-next/Documentation/devicetree/bindings/net/phy.txt
@@ -53,6 +53,8 @@ Optional Properties:
   to ensure the integrated PHY is used. The absence of this property indicates
   the muxers should be configured so that the external PHY is used.
 
+- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
+
 Example:
 
 ethernet-phy@0 {
Index: net-next/drivers/net/phy/at803x.c
===================================================================
--- net-next.orig/drivers/net/phy/at803x.c
+++ net-next/drivers/net/phy/at803x.c
@@ -71,7 +71,6 @@ MODULE_LICENSE("GPL");
 
 struct at803x_priv {
 	bool phy_reset:1;
-	struct gpio_desc *gpiod_reset;
 };
 
 struct at803x_context {
@@ -254,22 +253,11 @@ static int at803x_probe(struct phy_devic
 {
 	struct device *dev = &phydev->mdio.dev;
 	struct at803x_priv *priv;
-	struct gpio_desc *gpiod_reset;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	if (phydev->drv->phy_id != ATH8030_PHY_ID)
-		goto does_not_require_reset_workaround;
-
-	gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(gpiod_reset))
-		return PTR_ERR(gpiod_reset);
-
-	priv->gpiod_reset = gpiod_reset;
-
-does_not_require_reset_workaround:
 	phydev->priv = priv;
 
 	return 0;
@@ -343,14 +331,14 @@ static void at803x_link_change_notify(st
 	 * cannot recover from by software.
 	 */
 	if (phydev->state == PHY_NOLINK) {
-		if (priv->gpiod_reset && !priv->phy_reset) {
+		if (phydev->mdio.reset && !priv->phy_reset) {
 			struct at803x_context context;
 
 			at803x_context_save(phydev, &context);
 
-			gpiod_set_value(priv->gpiod_reset, 1);
+			phy_device_reset(phydev, 1);
 			msleep(1);
-			gpiod_set_value(priv->gpiod_reset, 0);
+			phy_device_reset(phydev, 0);
 			msleep(1);
 
 			at803x_context_restore(phydev, &context);
Index: net-next/drivers/net/phy/mdio_bus.c
===================================================================
--- net-next.orig/drivers/net/phy/mdio_bus.c
+++ net-next/drivers/net/phy/mdio_bus.c
@@ -38,6 +38,7 @@
 #include <linux/phy.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
+#include <linux/gpio/consumer.h>
 
 #include <asm/irq.h>
 
@@ -420,6 +421,9 @@ void mdiobus_unregister(struct mii_bus *
 		if (!mdiodev)
 			continue;
 
+		if (mdiodev->reset)
+			gpiod_put(mdiodev->reset);
+
 		mdiodev->device_remove(mdiodev);
 		mdiodev->device_free(mdiodev);
 	}
Index: net-next/drivers/net/phy/mdio_device.c
===================================================================
--- net-next.orig/drivers/net/phy/mdio_device.c
+++ net-next/drivers/net/phy/mdio_device.c
@@ -12,6 +12,8 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/errno.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -114,6 +116,13 @@ void mdio_device_remove(struct mdio_devi
 }
 EXPORT_SYMBOL(mdio_device_remove);
 
+void mdio_device_reset(struct mdio_device *mdiodev, int value)
+{
+	if (mdiodev->reset)
+		gpiod_set_value(mdiodev->reset, value);
+}
+EXPORT_SYMBOL(mdio_device_reset);
+
 /**
  * mdio_probe - probe an MDIO device
  * @dev: device to probe
@@ -128,9 +137,16 @@ static int mdio_probe(struct device *dev
 	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
 	int err = 0;
 
-	if (mdiodrv->probe)
+	if (mdiodrv->probe) {
+		/* Deassert the reset signal */
+		mdio_device_reset(mdiodev, 0);
+
 		err = mdiodrv->probe(mdiodev);
 
+		/* Assert the reset signal */
+		mdio_device_reset(mdiodev, 1);
+	}
+
 	return err;
 }
 
@@ -140,9 +156,16 @@ static int mdio_remove(struct device *de
 	struct device_driver *drv = mdiodev->dev.driver;
 	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
 
-	if (mdiodrv->remove)
+	if (mdiodrv->remove) {
+		/* Deassert the reset signal */
+		mdio_device_reset(mdiodev, 0);
+
 		mdiodrv->remove(mdiodev);
 
+		/* Assert the reset signal */
+		mdio_device_reset(mdiodev, 1);
+	}
+
 	return 0;
 }
 
Index: net-next/drivers/net/phy/phy_device.c
===================================================================
--- net-next.orig/drivers/net/phy/phy_device.c
+++ net-next/drivers/net/phy/phy_device.c
@@ -632,6 +632,9 @@ int phy_device_register(struct phy_devic
 	if (err)
 		return err;
 
+	/* Deassert the reset signal */
+	phy_device_reset(phydev, 0);
+
 	/* Run all of the fixups for this PHY */
 	err = phy_scan_fixups(phydev);
 	if (err) {
@@ -647,9 +650,15 @@ int phy_device_register(struct phy_devic
 		goto out;
 	}
 
+	/* Assert the reset signal */
+	phy_device_reset(phydev, 1);
+
 	return 0;
 
  out:
+	/* Assert the reset signal */
+	phy_device_reset(phydev, 1);
+
 	mdiobus_unregister_device(&phydev->mdio);
 	return err;
 }
@@ -849,6 +858,9 @@ int phy_init_hw(struct phy_device *phyde
 {
 	int ret = 0;
 
+	/* Deassert the reset signal */
+	phy_device_reset(phydev, 0);
+
 	if (!phydev->drv || !phydev->drv->config_init)
 		return 0;
 
@@ -1126,6 +1138,9 @@ void phy_detach(struct phy_device *phyde
 	put_device(&phydev->mdio.dev);
 	if (ndev_owner != bus->owner)
 		module_put(bus->owner);
+
+	/* Assert the reset signal */
+	phy_device_reset(phydev, 1);
 }
 EXPORT_SYMBOL(phy_detach);
 
@@ -1811,9 +1826,16 @@ static int phy_probe(struct device *dev)
 	/* Set the state to READY by default */
 	phydev->state = PHY_READY;
 
-	if (phydev->drv->probe)
+	if (phydev->drv->probe) {
+		/* Deassert the reset signal */
+		phy_device_reset(phydev, 0);
+
 		err = phydev->drv->probe(phydev);
 
+		/* Assert the reset signal */
+		phy_device_reset(phydev, 1);
+	}
+
 	mutex_unlock(&phydev->lock);
 
 	return err;
@@ -1829,8 +1851,15 @@ static int phy_remove(struct device *dev
 	phydev->state = PHY_DOWN;
 	mutex_unlock(&phydev->lock);
 
-	if (phydev->drv && phydev->drv->remove)
+	if (phydev->drv && phydev->drv->remove) {
+		/* Deassert the reset signal */
+		phy_device_reset(phydev, 0);
+
 		phydev->drv->remove(phydev);
+
+		/* Assert the reset signal */
+		phy_device_reset(phydev, 1);
+	}
 	phydev->drv = NULL;
 
 	return 0;
Index: net-next/drivers/of/of_mdio.c
===================================================================
--- net-next.orig/drivers/of/of_mdio.c
+++ net-next/drivers/of/of_mdio.c
@@ -47,6 +47,7 @@ static int of_get_phy_id(struct device_n
 static void of_mdiobus_register_phy(struct mii_bus *mdio,
 				    struct device_node *child, u32 addr)
 {
+	struct gpio_desc *gpiod;
 	struct phy_device *phy;
 	bool is_c45;
 	int rc;
@@ -55,10 +56,16 @@ static void of_mdiobus_register_phy(stru
 	is_c45 = of_device_is_compatible(child,
 					 "ethernet-phy-ieee802.3-c45");
 
+	/* Deassert the reset signal */
+	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios", 0,
+				       GPIOD_OUT_LOW, "PHY reset");
 	if (!is_c45 && !of_get_phy_id(child, &phy_id))
 		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
 	else
 		phy = get_phy_device(mdio, addr, is_c45);
+	/* Assert the reset signal again */
+	if (!IS_ERR(gpiod))
+		gpiod_set_value(gpiod, 1);
 	if (IS_ERR(phy))
 		return;
 
@@ -78,6 +85,9 @@ static void of_mdiobus_register_phy(stru
 	of_node_get(child);
 	phy->mdio.dev.of_node = child;
 
+	if (!IS_ERR(gpiod))
+		phy->mdio.reset = gpiod;
+
 	/* All data is now stored in the phy struct;
 	 * register it */
 	rc = phy_device_register(phy);
@@ -95,6 +105,7 @@ static void of_mdiobus_register_device(s
 				       struct device_node *child, u32 addr)
 {
 	struct mdio_device *mdiodev;
+	struct gpio_desc *gpiod;
 	int rc;
 
 	mdiodev = mdio_device_create(mdio, addr);
@@ -107,6 +118,11 @@ static void of_mdiobus_register_device(s
 	of_node_get(child);
 	mdiodev->dev.of_node = child;
 
+	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios", 0,
+				       GPIOD_ASIS, "PHY reset");
+	if (!IS_ERR(gpiod))
+		mdiodev->reset = gpiod;
+
 	/* All data is now stored in the mdiodev struct; register it. */
 	rc = mdio_device_register(mdiodev);
 	if (rc) {
Index: net-next/include/linux/mdio.h
===================================================================
--- net-next.orig/include/linux/mdio.h
+++ net-next/include/linux/mdio.h
@@ -12,6 +12,7 @@
 #include <uapi/linux/mdio.h>
 #include <linux/mod_devicetable.h>
 
+struct gpio_desc;
 struct mii_bus;
 
 /* Multiple levels of nesting are possible. However typically this is
@@ -39,6 +40,7 @@ struct mdio_device {
 	/* Bus address of the MDIO device (0-31) */
 	int addr;
 	int flags;
+	struct gpio_desc *reset;
 };
 #define to_mdio_device(d) container_of(d, struct mdio_device, dev)
 
@@ -71,6 +73,7 @@ void mdio_device_free(struct mdio_device
 struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr);
 int mdio_device_register(struct mdio_device *mdiodev);
 void mdio_device_remove(struct mdio_device *mdiodev);
+void mdio_device_reset(struct mdio_device *mdiodev, int value);
 int mdio_driver_register(struct mdio_driver *drv);
 void mdio_driver_unregister(struct mdio_driver *drv);
 int mdio_device_bus_match(struct device *dev, struct device_driver *drv);
Index: net-next/include/linux/phy.h
===================================================================
--- net-next.orig/include/linux/phy.h
+++ net-next/include/linux/phy.h
@@ -847,6 +847,11 @@ static inline int phy_read_status(struct
 	return phydev->drv->read_status(phydev);
 }
 
+static inline void phy_device_reset(struct phy_device *phydev, int value)
+{
+	mdio_device_reset(&phydev->mdio, value);
+}
+
 #define phydev_err(_phydev, format, args...)	\
 	dev_err(&_phydev->mdio.dev, format, ##args)
 

[-- Attachment #3: macb-kill-PHY-reset-code-v3.patch --]
[-- Type: text/x-patch, Size: 2800 bytes --]

Subject: macb: kill PHY reset code

With  the phylib now  being aware of the "reset-gpios"  PHY node property,
there should  be no need to frob the PHY  reset in this driver anymore...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 3:
- resolved rejects due to the file being renamed, refreshed the patch;
- added the code to reset PHY on a probe error;
- edited the patch description.

 drivers/net/ethernet/cadence/macb.h      |    1 -
 drivers/net/ethernet/cadence/macb_main.c |   21 ---------------------
 2 files changed, 22 deletions(-)

Index: net-next/drivers/net/ethernet/cadence/macb.h
===================================================================
--- net-next.orig/drivers/net/ethernet/cadence/macb.h
+++ net-next/drivers/net/ethernet/cadence/macb.h
@@ -1032,7 +1032,6 @@ 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 */
Index: net-next/drivers/net/ethernet/cadence/macb_main.c
===================================================================
--- net-next.orig/drivers/net/ethernet/cadence/macb_main.c
+++ net-next/drivers/net/ethernet/cadence/macb_main.c
@@ -3393,7 +3393,6 @@ static int macb_probe(struct platform_de
 					      = macb_config->clk_init;
 	int (*init)(struct platform_device *) = macb_config->init;
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *phy_node;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL, *rx_clk = NULL;
 	unsigned int queue_mask, num_queues;
 	struct macb_platform_data *pdata;
@@ -3499,18 +3498,6 @@ static int macb_probe(struct platform_de
 	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_direction_output(bp->reset_gpio, 1);
-		}
-	}
-	of_node_put(phy_node);
-
 	err = of_get_phy_mode(np);
 	if (err < 0) {
 		pdata = dev_get_platdata(&pdev->dev);
@@ -3554,10 +3541,6 @@ err_out_unregister_mdio:
 	mdiobus_unregister(bp->mii_bus);
 	mdiobus_free(bp->mii_bus);
 
-	/* Shutdown the PHY if there is a GPIO reset */
-	if (bp->reset_gpio)
-		gpiod_set_value(bp->reset_gpio, 0);
-
 err_out_free_netdev:
 	free_netdev(dev);
 
@@ -3585,10 +3568,6 @@ static int macb_remove(struct platform_d
 		dev->phydev = NULL;
 		mdiobus_free(bp->mii_bus);
 
-		/* Shutdown the PHY if there is a GPIO reset */
-		if (bp->reset_gpio)
-			gpiod_set_value(bp->reset_gpio, 0);
-
 		unregister_netdev(dev);
 		clk_disable_unprepare(bp->tx_clk);
 		clk_disable_unprepare(bp->hclk);

[PATCH 3/4] arm64: dts: renesas: salvator-common: Add EthernetAVB PHY reset

[Geert Uytterhoeven]

Describe the GPIO used to reset the Ethernet PHY for EthernetAVB.
This allows the driver to reset the PHY during probe and after system
resume.

This fixes Ethernet operation after resume from s2ram on Salvator-XS,
where the enable pin of the regulator providing PHY power is connected
to PRESETn, and PSCI powers down the SoC during system suspend.

On Salvator-X, the enable pin is always pulled high, but the driver may
still need to reset the PHY if this wasn't done by the bootloader
before.

Inspired by patches in the BSP for the individual Salvator-X/XS boards
by Kazuya Mizuguchi.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
For proper PHY reset operation during system resume, this depends on
"ravb: Add missing PHY reset during system resume".
However, this patch can be applied independently.
---
 arch/arm64/boot/dts/renesas/salvator-common.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
index ed4a8dfead3c2e58..db00e7c484f76eac 100644
--- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
@@ -296,6 +296,7 @@
 	pinctrl-names = "default";
 	renesas,no-ether-link;
 	phy-handle = <&phy0>;
+	reset-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
 	status = "okay";
 
 	phy0: ethernet-phy@0 {
-- 
2.7.4

Re: [PATCH 3/4] arm64: dts: renesas: salvator-common: Add EthernetAVB PHY reset

[Florian Fainelli]

On 09/28/2017 08:53 AM, Geert Uytterhoeven wrote:
> Describe the GPIO used to reset the Ethernet PHY for EthernetAVB.
> This allows the driver to reset the PHY during probe and after system
> resume.
> 
> This fixes Ethernet operation after resume from s2ram on Salvator-XS,
> where the enable pin of the regulator providing PHY power is connected
> to PRESETn, and PSCI powers down the SoC during system suspend.
> 
> On Salvator-X, the enable pin is always pulled high, but the driver may
> still need to reset the PHY if this wasn't done by the bootloader
> before.
> 
> Inspired by patches in the BSP for the individual Salvator-X/XS boards
> by Kazuya Mizuguchi.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> ---
> For proper PHY reset operation during system resume, this depends on
> "ravb: Add missing PHY reset during system resume".
> However, this patch can be applied independently.
> ---
>  arch/arm64/boot/dts/renesas/salvator-common.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> index ed4a8dfead3c2e58..db00e7c484f76eac 100644
> --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> @@ -296,6 +296,7 @@
>  	pinctrl-names = "default";
>  	renesas,no-ether-link;
>  	phy-handle = <&phy0>;
> +	reset-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
>  	status = "okay";
>  
>  	phy0: ethernet-phy@0 {

This should be a PHY node property, unless this GPIO pin really is
global to the MDIO bus itself.
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] arm64: dts: renesas: ulcb: Add EthernetAVB PHY reset

[Geert Uytterhoeven]

Describe the GPIO used to reset the Ethernet PHY for EthernetAVB.
This allows the driver to reset the PHY during probe and after system
resume.

On ULCB, the enable pin of the regulator providing PHY power is always
pulled high, but the driver may still need to reset the PHY if this
wasn't done by the bootloader before.

Inspired by patches in the BSP for the individual Salvator-X/XS boards
by Kazuya Mizuguchi.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 arch/arm64/boot/dts/renesas/ulcb.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/renesas/ulcb.dtsi b/arch/arm64/boot/dts/renesas/ulcb.dtsi
index dfec9072718b8da1..49cf392fccfb3165 100644
--- a/arch/arm64/boot/dts/renesas/ulcb.dtsi
+++ b/arch/arm64/boot/dts/renesas/ulcb.dtsi
@@ -147,6 +147,7 @@
 	pinctrl-names = "default";
 	renesas,no-ether-link;
 	phy-handle = <&phy0>;
+	reset-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
 	status = "okay";
 
 	phy0: ethernet-phy@0 {
-- 
2.7.4