[PATCH] phy: add support for a reset-gpio specification

[PATCH] phy: add support for a reset-gpio specification

[Uwe Kleine-König]

The framework only asserts (for now) that the reset gpio is not active.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 Documentation/devicetree/bindings/net/phy.txt | 3 +++
 drivers/net/phy/phy_device.c                  | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index bc1c3c8bf8fa..c00a9a894547 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -35,6 +35,8 @@ Optional Properties:
 - broken-turn-around: If set, indicates the PHY device does not correctly
   release the turn around line low at the end of a MDIO transaction.
 
+- reset-gpios: Reference to a GPIO used to reset the phy.
+
 Example:
 
 ethernet-phy@0 {
@@ -42,4 +44,5 @@ ethernet-phy@0 {
 	interrupt-parent = <40000>;
 	interrupts = <35 1>;
 	reg = <0>;
+	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
 };
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e551f3a89cfd..7d666ab47271 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -34,6 +34,7 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
+#include <linux/gpio/consumer.h>
 
 #include <asm/irq.h>
 
@@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
 	struct device_driver *drv = phydev->mdio.dev.driver;
 	struct phy_driver *phydrv = to_phy_driver(drv);
 	int err = 0;
+	struct gpio_descs *reset_gpios;
 
 	phydev->drv = phydrv;
 
+	/* take phy out of reset */
+	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
+						    GPIOD_OUT_LOW);
+	if (IS_ERR(reset_gpios))
+		return PTR_ERR(reset_gpios);
+
 	/* Disable the interrupt if the PHY doesn't support it
 	 * but the interrupt is still a valid one
 	 */
-- 
2.8.0.rc3

--
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] phy: add support for a reset-gpio specification

[Nishanth Menon]

On 05/12/2016 05:00 AM, Uwe Kleine-König wrote:
> The framework only asserts (for now) that the reset gpio is not active.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/net/phy.txt | 3 +++
>  drivers/net/phy/phy_device.c                  | 8 ++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index bc1c3c8bf8fa..c00a9a894547 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -35,6 +35,8 @@ Optional Properties:
>  - broken-turn-around: If set, indicates the PHY device does not correctly
>    release the turn around line low at the end of a MDIO transaction.
>  
> +- reset-gpios: Reference to a GPIO used to reset the phy.
> +
>  Example:
>  
>  ethernet-phy@0 {
> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>  	interrupt-parent = <40000>;
>  	interrupts = <35 1>;
>  	reg = <0>;
> +	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>  };
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index e551f3a89cfd..7d666ab47271 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -34,6 +34,7 @@
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
>  #include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <asm/irq.h>
>  
> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>  	struct device_driver *drv = phydev->mdio.dev.driver;
>  	struct phy_driver *phydrv = to_phy_driver(drv);
>  	int err = 0;
> +	struct gpio_descs *reset_gpios;
>  
>  	phydev->drv = phydrv;
>  
> +	/* take phy out of reset */
> +	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
> +						    GPIOD_OUT_LOW);
> +	if (IS_ERR(reset_gpios))
> +		return PTR_ERR(reset_gpios);
> +
>  	/* Disable the interrupt if the PHY doesn't support it
>  	 * but the interrupt is still a valid one
>  	 */
> 

This looks like the right approach to me at least: I see that TI EVMs
will also benefit with this approach.

-- 
Regards,
Nishanth Menon

Re: [PATCH] phy: add support for a reset-gpio specification

[Roger Quadros]

Hi,

On 12/05/16 16:50, Nishanth Menon wrote:
> On 05/12/2016 05:00 AM, Uwe Kleine-König wrote:
>> The framework only asserts (for now) that the reset gpio is not active.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/net/phy.txt | 3 +++
>>  drivers/net/phy/phy_device.c                  | 8 ++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
>> index bc1c3c8bf8fa..c00a9a894547 100644
>> --- a/Documentation/devicetree/bindings/net/phy.txt
>> +++ b/Documentation/devicetree/bindings/net/phy.txt
>> @@ -35,6 +35,8 @@ Optional Properties:
>>  - broken-turn-around: If set, indicates the PHY device does not correctly
>>    release the turn around line low at the end of a MDIO transaction.
>>  
>> +- reset-gpios: Reference to a GPIO used to reset the phy.
>> +
>>  Example:
>>  
>>  ethernet-phy@0 {
>> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>>  	interrupt-parent = <40000>;
>>  	interrupts = <35 1>;
>>  	reg = <0>;
>> +	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>  };
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index e551f3a89cfd..7d666ab47271 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -34,6 +34,7 @@
>>  #include <linux/io.h>
>>  #include <linux/uaccess.h>
>>  #include <linux/of.h>
>> +#include <linux/gpio/consumer.h>
>>  
>>  #include <asm/irq.h>
>>  
>> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>>  	struct device_driver *drv = phydev->mdio.dev.driver;
>>  	struct phy_driver *phydrv = to_phy_driver(drv);
>>  	int err = 0;
>> +	struct gpio_descs *reset_gpios;
>>  
>>  	phydev->drv = phydrv;
>>  
>> +	/* take phy out of reset */
>> +	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
>> +						    GPIOD_OUT_LOW);
>> +	if (IS_ERR(reset_gpios))
>> +		return PTR_ERR(reset_gpios);
>> +
>>  	/* Disable the interrupt if the PHY doesn't support it
>>  	 * but the interrupt is still a valid one
>>  	 */
>>
> 
> This looks like the right approach to me at least: I see that TI EVMs
> will also benefit with this approach.
> 

Agreed. Although on some of our boards we actually need a RESET pulse
to get the PHY in a sane state. I can send a patch on top for that.

Reviewed-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>

--
cheers,
-roger
--
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] phy: add support for a reset-gpio specification

[Nishanth Menon]

On 05/12/2016 09:02 AM, Roger Quadros wrote:
[...]
>> This looks like the right approach to me at least: I see that TI EVMs
>> will also benefit with this approach.
>>
> 
> Agreed. Although on some of our boards we actually need a RESET pulse
> to get the PHY in a sane state. I can send a patch on top for that.
> 
> Reviewed-by: Roger Quadros <rogerq@ti.com>

There is an interesting thread starting up on Linux-arm[1] as well - I
guess Uwe can comment on direction.

[1] http://marc.info/?t=146303744800003&r=1&w=2


-- 
Regards,
Nishanth Menon

Re: [PATCH] phy: add support for a reset-gpio specification

[Roger Quadros]

On 12/05/16 17:02, Roger Quadros wrote:
> Hi,
> 
> On 12/05/16 16:50, Nishanth Menon wrote:
>> On 05/12/2016 05:00 AM, Uwe Kleine-König wrote:
>>> The framework only asserts (for now) that the reset gpio is not active.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>> ---
>>>  Documentation/devicetree/bindings/net/phy.txt | 3 +++
>>>  drivers/net/phy/phy_device.c                  | 8 ++++++++
>>>  2 files changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
>>> index bc1c3c8bf8fa..c00a9a894547 100644
>>> --- a/Documentation/devicetree/bindings/net/phy.txt
>>> +++ b/Documentation/devicetree/bindings/net/phy.txt
>>> @@ -35,6 +35,8 @@ Optional Properties:
>>>  - broken-turn-around: If set, indicates the PHY device does not correctly
>>>    release the turn around line low at the end of a MDIO transaction.
>>>  
>>> +- reset-gpios: Reference to a GPIO used to reset the phy.
>>> +
>>>  Example:
>>>  
>>>  ethernet-phy@0 {
>>> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>>>  	interrupt-parent = <40000>;
>>>  	interrupts = <35 1>;
>>>  	reg = <0>;
>>> +	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>>  };
>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>> index e551f3a89cfd..7d666ab47271 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>>> @@ -34,6 +34,7 @@
>>>  #include <linux/io.h>
>>>  #include <linux/uaccess.h>
>>>  #include <linux/of.h>
>>> +#include <linux/gpio/consumer.h>
>>>  
>>>  #include <asm/irq.h>
>>>  
>>> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>>>  	struct device_driver *drv = phydev->mdio.dev.driver;
>>>  	struct phy_driver *phydrv = to_phy_driver(drv);
>>>  	int err = 0;
>>> +	struct gpio_descs *reset_gpios;
>>>  
>>>  	phydev->drv = phydrv;
>>>  
>>> +	/* take phy out of reset */
>>> +	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
>>> +						    GPIOD_OUT_LOW);
>>> +	if (IS_ERR(reset_gpios))
>>> +		return PTR_ERR(reset_gpios);
>>> +
>>>  	/* Disable the interrupt if the PHY doesn't support it
>>>  	 * but the interrupt is still a valid one
>>>  	 */
>>>
>>
>> This looks like the right approach to me at least: I see that TI EVMs
>> will also benefit with this approach.
>>
> 
> Agreed. Although on some of our boards we actually need a RESET pulse
> to get the PHY in a sane state. I can send a patch on top for that.
> 

I think I replied too early. Will phy_probe() be called at all if the
PHY is not detected on the MDIO bus?
If not then this approach is not sufficient for some of the TI boards.
e.g. am57xx-idk.
I think the reset needs to be done at MDIO bus level before it enumerates
the PHYs so that we know the PHY is in an enumerable state.

cheers,
-roger

Re: [PATCH] phy: add support for a reset-gpio specification

[Sergei Shtylyov]

On 5/12/2016 5:16 PM, Roger Quadros wrote:

>>>> The framework only asserts (for now) that the reset gpio is not active.
>>>>
>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>> ---
>>>>  Documentation/devicetree/bindings/net/phy.txt | 3 +++
>>>>  drivers/net/phy/phy_device.c                  | 8 ++++++++
>>>>  2 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
>>>> index bc1c3c8bf8fa..c00a9a894547 100644
>>>> --- a/Documentation/devicetree/bindings/net/phy.txt
>>>> +++ b/Documentation/devicetree/bindings/net/phy.txt
>>>> @@ -35,6 +35,8 @@ Optional Properties:
>>>>  - broken-turn-around: If set, indicates the PHY device does not correctly
>>>>    release the turn around line low at the end of a MDIO transaction.
>>>>
>>>> +- reset-gpios: Reference to a GPIO used to reset the phy.
>>>> +
>>>>  Example:
>>>>
>>>>  ethernet-phy@0 {
>>>> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>>>>  	interrupt-parent = <40000>;
>>>>  	interrupts = <35 1>;
>>>>  	reg = <0>;
>>>> +	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>>>  };
>>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>>> index e551f3a89cfd..7d666ab47271 100644
>>>> --- a/drivers/net/phy/phy_device.c
>>>> +++ b/drivers/net/phy/phy_device.c
>>>> @@ -34,6 +34,7 @@
>>>>  #include <linux/io.h>
>>>>  #include <linux/uaccess.h>
>>>>  #include <linux/of.h>
>>>> +#include <linux/gpio/consumer.h>
>>>>
>>>>  #include <asm/irq.h>
>>>>
>>>> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>>>>  	struct device_driver *drv = phydev->mdio.dev.driver;
>>>>  	struct phy_driver *phydrv = to_phy_driver(drv);
>>>>  	int err = 0;
>>>> +	struct gpio_descs *reset_gpios;
>>>>
>>>>  	phydev->drv = phydrv;
>>>>
>>>> +	/* take phy out of reset */
>>>> +	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
>>>> +						    GPIOD_OUT_LOW);
>>>> +	if (IS_ERR(reset_gpios))
>>>> +		return PTR_ERR(reset_gpios);
>>>> +
>>>>  	/* Disable the interrupt if the PHY doesn't support it
>>>>  	 * but the interrupt is still a valid one
>>>>  	 */
>>>
>>> This looks like the right approach to me at least: I see that TI EVMs
>>> will also benefit with this approach.
>>
>> Agreed. Although on some of our boards we actually need a RESET pulse
>> to get the PHY in a sane state. I can send a patch on top for that.
>
> I think I replied too early. Will phy_probe() be called at all if the
> PHY is not detected on the MDIO bus?

    No. What Uwe haven't said is that the PHY ID needs to be specified in the 
device tree for his patch to actually work. ;-)

> If not then this approach is not sufficient for some of the TI boards.
> e.g. am57xx-idk.
> I think the reset needs to be done at MDIO bus level before it enumerates
> the PHYs so that we know the PHY is in an enumerable state.

    Have you seen my patch, [1]?

[1] http://patchwork.ozlabs.org/patch/616495/

> cheers,
> -roger

MBR, Seregi

Re: [PATCH] phy: add support for a reset-gpio specification

[Uwe Kleine-König]

Hello,

On Thu, May 12, 2016 at 05:16:56PM +0300, Roger Quadros wrote:
> On 12/05/16 17:02, Roger Quadros wrote:
> > On 12/05/16 16:50, Nishanth Menon wrote:
> >> On 05/12/2016 05:00 AM, Uwe Kleine-König wrote:
> >>> The framework only asserts (for now) that the reset gpio is not active.
> >>>
> >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>> ---
> >>>  Documentation/devicetree/bindings/net/phy.txt | 3 +++
> >>>  drivers/net/phy/phy_device.c                  | 8 ++++++++
> >>>  2 files changed, 11 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> >>> index bc1c3c8bf8fa..c00a9a894547 100644
> >>> --- a/Documentation/devicetree/bindings/net/phy.txt
> >>> +++ b/Documentation/devicetree/bindings/net/phy.txt
> >>> @@ -35,6 +35,8 @@ Optional Properties:
> >>>  - broken-turn-around: If set, indicates the PHY device does not correctly
> >>>    release the turn around line low at the end of a MDIO transaction.
> >>>  
> >>> +- reset-gpios: Reference to a GPIO used to reset the phy.
> >>> +
> >>>  Example:
> >>>  
> >>>  ethernet-phy@0 {
> >>> @@ -42,4 +44,5 @@ ethernet-phy@0 {
> >>>  	interrupt-parent = <40000>;
> >>>  	interrupts = <35 1>;
> >>>  	reg = <0>;
> >>> +	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
> >>>  };
> >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> >>> index e551f3a89cfd..7d666ab47271 100644
> >>> --- a/drivers/net/phy/phy_device.c
> >>> +++ b/drivers/net/phy/phy_device.c
> >>> @@ -34,6 +34,7 @@
> >>>  #include <linux/io.h>
> >>>  #include <linux/uaccess.h>
> >>>  #include <linux/of.h>
> >>> +#include <linux/gpio/consumer.h>
> >>>  
> >>>  #include <asm/irq.h>
> >>>  
> >>> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
> >>>  	struct device_driver *drv = phydev->mdio.dev.driver;
> >>>  	struct phy_driver *phydrv = to_phy_driver(drv);
> >>>  	int err = 0;
> >>> +	struct gpio_descs *reset_gpios;
> >>>  
> >>>  	phydev->drv = phydrv;
> >>>  
> >>> +	/* take phy out of reset */
> >>> +	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
> >>> +						    GPIOD_OUT_LOW);
> >>> +	if (IS_ERR(reset_gpios))
> >>> +		return PTR_ERR(reset_gpios);
> >>> +
> >>>  	/* Disable the interrupt if the PHY doesn't support it
> >>>  	 * but the interrupt is still a valid one
> >>>  	 */
> >>>
> >>
> >> This looks like the right approach to me at least: I see that TI EVMs
> >> will also benefit with this approach.
> >>
> > 
> > Agreed. Although on some of our boards we actually need a RESET pulse
> > to get the PHY in a sane state. I can send a patch on top for that.
> > 
> 
> I think I replied too early. Will phy_probe() be called at all if the
> PHY is not detected on the MDIO bus?
> If not then this approach is not sufficient for some of the TI boards.
> e.g. am57xx-idk.
> I think the reset needs to be done at MDIO bus level before it enumerates
> the PHYs so that we know the PHY is in an enumerable state.
I have this in the device tree:

&davinci_mdio {
        pinctrl-names = "default", "sleep";
        pinctrl-0 = <&davinci_mdio_default>;
        pinctrl-1 = <&davinci_mdio_sleep>;
        status = "okay";

        phy0: ethernet-phy@0 {
                compatible = "ethernet-phy-id2000.a212", "ethernet-phy-ieee802.3-c22";
                reg = <0>;

                interrupt-parent = <&gpio2>;
                interrupts = <2 8>;

                pinctrl-names = "default";
                pinctrl-0 = <&phy0_pins>;

                reset-gpios = <&gpio2 9 GPIO_ACTIVE_LOW>;
        };

        phy1: ethernet-phy@2 {
                compatible = "ethernet-phy-id2000.a212", "ethernet-phy-ieee802.3-c22";
                reg = <2>;

                interrupt-parent = <&gpio0>;
                interrupts = <2 8>;

                pinctrl-names = "default";
                pinctrl-0 = <&phy1_pins>;

                reset-gpios = <&gpio2 11 GPIO_ACTIVE_LOW>;
        };
};

With the phy id values specified in the compatible the mdio bus is only
accessed when the reset pin was handled.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] phy: add support for a reset-gpio specification

[Sergei Shtylyov]

On 05/12/2016 01:00 PM, Uwe Kleine-König wrote:

> The framework only asserts (for now) that the reset gpio is not active.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/net/phy.txt | 3 +++
>  drivers/net/phy/phy_device.c                  | 8 ++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index bc1c3c8bf8fa..c00a9a894547 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -35,6 +35,8 @@ Optional Properties:
>  - broken-turn-around: If set, indicates the PHY device does not correctly
>    release the turn around line low at the end of a MDIO transaction.
>
> +- reset-gpios: Reference to a GPIO used to reset the phy.
> +
>  Example:
>
>  ethernet-phy@0 {
> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>  	interrupt-parent = <40000>;
>  	interrupts = <35 1>;
>  	reg = <0>;
> +	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
 >  };

    OK, the example has PHY ID specified in the "compatible" prop. It's a 
vital detail, you should mention it somewhere...

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index e551f3a89cfd..7d666ab47271 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
[...]
> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>  	struct device_driver *drv = phydev->mdio.dev.driver;
>  	struct phy_driver *phydrv = to_phy_driver(drv);
>  	int err = 0;
> +	struct gpio_descs *reset_gpios;
>
>  	phydev->drv = phydrv;
>
> +	/* take phy out of reset */
> +	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
> +						    GPIOD_OUT_LOW);
> +	if (IS_ERR(reset_gpios))
> +		return PTR_ERR(reset_gpios);
> +

    It would have been logical to assert back the reset GPIO in phy_remove(), 
no? Also, you'd need to store the GPIO permanently somewhere...

MBR, Sergei

Re: [PATCH] phy: add support for a reset-gpio specification

[Uwe Kleine-König]

Hello Sergei,

On Fri, May 13, 2016 at 12:41:47AM +0300, Sergei Shtylyov wrote:
> On 05/12/2016 01:00 PM, Uwe Kleine-König wrote:
> 
> >The framework only asserts (for now) that the reset gpio is not active.
> >
> >Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >---
> > Documentation/devicetree/bindings/net/phy.txt | 3 +++
> > drivers/net/phy/phy_device.c                  | 8 ++++++++
> > 2 files changed, 11 insertions(+)
> >
> >diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> >index bc1c3c8bf8fa..c00a9a894547 100644
> >--- a/Documentation/devicetree/bindings/net/phy.txt
> >+++ b/Documentation/devicetree/bindings/net/phy.txt
> >@@ -35,6 +35,8 @@ Optional Properties:
> > - broken-turn-around: If set, indicates the PHY device does not correctly
> >   release the turn around line low at the end of a MDIO transaction.
> >
> >+- reset-gpios: Reference to a GPIO used to reset the phy.
> >+
> > Example:
> >
> > ethernet-phy@0 {
> >@@ -42,4 +44,5 @@ ethernet-phy@0 {
> > 	interrupt-parent = <40000>;
> > 	interrupts = <35 1>;
> > 	reg = <0>;
> >+	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
> >  };
> 
>    OK, the example has PHY ID specified in the "compatible" prop. It's a
> vital detail, you should mention it somewhere...

Something like that in the commit log:

	Note that reset is only deasserted when the PHY ID is already
	determined. If your phy doesn't support reading the PHY ID while
	reset is asserted it's recommended to specify it in the
	compatible string. Otherwise the device might fail to probe.

> >diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> >index e551f3a89cfd..7d666ab47271 100644
> >--- a/drivers/net/phy/phy_device.c
> >+++ b/drivers/net/phy/phy_device.c
> [...]
> >@@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
> > 	struct device_driver *drv = phydev->mdio.dev.driver;
> > 	struct phy_driver *phydrv = to_phy_driver(drv);
> > 	int err = 0;
> >+	struct gpio_descs *reset_gpios;
> >
> > 	phydev->drv = phydrv;
> >
> >+	/* take phy out of reset */
> >+	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
> >+						    GPIOD_OUT_LOW);
> >+	if (IS_ERR(reset_gpios))
> >+		return PTR_ERR(reset_gpios);
> >+
> 
>    It would have been logical to assert back the reset GPIO in phy_remove(),

Not necessarily. If you want to save some energy it might make sense. If
you only focus on making the device work, ensuring that reset is not
applied is enough.

> no? Also, you'd need to store the GPIO permanently somewhere...

I need to store it, iff I want to do something with the reset line
later. Currently that's not the case.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] phy: add support for a reset-gpio specification

[Sergei Shtylyov]

Hello.

On 05/13/2016 09:26 AM, Uwe Kleine-König wrote:

>>> The framework only asserts (for now) that the reset gpio is not active.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>> ---
>>> Documentation/devicetree/bindings/net/phy.txt | 3 +++
>>> drivers/net/phy/phy_device.c                  | 8 ++++++++
>>> 2 files changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
>>> index bc1c3c8bf8fa..c00a9a894547 100644
>>> --- a/Documentation/devicetree/bindings/net/phy.txt
>>> +++ b/Documentation/devicetree/bindings/net/phy.txt
>>> @@ -35,6 +35,8 @@ Optional Properties:
>>> - broken-turn-around: If set, indicates the PHY device does not correctly
>>>   release the turn around line low at the end of a MDIO transaction.
>>>
>>> +- reset-gpios: Reference to a GPIO used to reset the phy.
>>> +
>>> Example:
>>>
>>> ethernet-phy@0 {
>>> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>>> 	interrupt-parent = <40000>;
>>> 	interrupts = <35 1>;
>>> 	reg = <0>;
>>> +	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>>  };
>>
>>    OK, the example has PHY ID specified in the "compatible" prop. It's a
>> vital detail, you should mention it somewhere...
>
> Something like that in the commit log:
>
> 	Note that reset is only deasserted when the PHY ID is already
> 	determined. If your phy doesn't support reading the PHY ID while
> 	reset is asserted it's recommended to specify it in the
> 	compatible string. Otherwise the device might fail to probe.

    Yes, that should do.

>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>> index e551f3a89cfd..7d666ab47271 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>> [...]
>>> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>>> 	struct device_driver *drv = phydev->mdio.dev.driver;
>>> 	struct phy_driver *phydrv = to_phy_driver(drv);
>>> 	int err = 0;
>>> +	struct gpio_descs *reset_gpios;
>>>
>>> 	phydev->drv = phydrv;
>>>
>>> +	/* take phy out of reset */
>>> +	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
>>> +						    GPIOD_OUT_LOW);
>>> +	if (IS_ERR(reset_gpios))
>>> +		return PTR_ERR(reset_gpios);
>>> +
>>
>>    It would have been logical to assert back the reset GPIO in phy_remove(),

> Not necessarily. If you want to save some energy it might make sense. If

    Yes, we'd like to do that. Florian (the phylib maintainer) even wanted 
asserting reset during the PM suspended state.

> you only focus on making the device work, ensuring that reset is not
> applied is enough.

>> no? Also, you'd need to store the GPIO permanently somewhere...
>
> I need to store it, iff I want to do something with the reset line
> later. Currently that's not the case.

    Oops, I seem to somewhat muddled up here. Of course, you need to store it 
if you intend to de-assert it later.

> Best regards
> Uwe

MBR, Sergei

Re: [PATCH] phy: add support for a reset-gpio specification

[Rob Herring]

On Thu, May 12, 2016 at 12:00:33PM +0200, Uwe Kleine-König wrote:
> The framework only asserts (for now) that the reset gpio is not active.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/net/phy.txt | 3 +++
>  drivers/net/phy/phy_device.c                  | 8 ++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index bc1c3c8bf8fa..c00a9a894547 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -35,6 +35,8 @@ Optional Properties:
>  - broken-turn-around: If set, indicates the PHY device does not correctly
>    release the turn around line low at the end of a MDIO transaction.
>  
> +- reset-gpios: Reference to a GPIO used to reset the phy.
> +

And also active mode flag should be specified.

>  Example:
>  
>  ethernet-phy@0 {
> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>  	interrupt-parent = <40000>;
>  	interrupts = <35 1>;
>  	reg = <0>;
> +	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>  };
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index e551f3a89cfd..7d666ab47271 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -34,6 +34,7 @@
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
>  #include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <asm/irq.h>
>  
> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>  	struct device_driver *drv = phydev->mdio.dev.driver;
>  	struct phy_driver *phydrv = to_phy_driver(drv);
>  	int err = 0;
> +	struct gpio_descs *reset_gpios;
>  
>  	phydev->drv = phydrv;
>  
> +	/* take phy out of reset */
> +	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
> +						    GPIOD_OUT_LOW);
> +	if (IS_ERR(reset_gpios))
> +		return PTR_ERR(reset_gpios);
> +
>  	/* Disable the interrupt if the PHY doesn't support it
>  	 * but the interrupt is still a valid one
>  	 */
> -- 
> 2.8.0.rc3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] phy: add support for a reset-gpio specification

[David Miller]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Thu, 12 May 2016 12:00:33 +0200

> The framework only asserts (for now) that the reset gpio is not active.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied.

Re: [PATCH] phy: add support for a reset-gpio specification

[Florian Fainelli]

On 05/16/2016 10:23 AM, David Miller wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Date: Thu, 12 May 2016 12:00:33 +0200
> 
>> The framework only asserts (for now) that the reset gpio is not active.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Applied.

Humm that was a little too quick IMHO, there are two concurrent patches
adressing the same problem right now, and Uwe's patch relies on a
specific "quirk" of specifying the Ethernet PHY's full OUI in the Device
Tree compatible string, while Sergei's approach does not require that...
AFAICT, the binding portion of the patch was also still being discussed.

I guess we will have to submit incremental patches now.
-- 
Florian

Re: [PATCH] phy: add support for a reset-gpio specification

[David Miller]

From: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Mon, 16 May 2016 10:29:44 -0700

> On 05/16/2016 10:23 AM, David Miller wrote:
>> From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Date: Thu, 12 May 2016 12:00:33 +0200
>> 
>>> The framework only asserts (for now) that the reset gpio is not active.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> 
>> Applied.
> 
> Humm that was a little too quick IMHO, there are two concurrent patches
> adressing the same problem right now, and Uwe's patch relies on a
> specific "quirk" of specifying the Ethernet PHY's full OUI in the Device
> Tree compatible string, while Sergei's approach does not require that...
> AFAICT, the binding portion of the patch was also still being discussed.
> 
> I guess we will have to submit incremental patches now.

Sorry, I thought this one had been resolved. :-/

There is still a lot of time to correct this, don't worry.
--
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