[PATCH] net: davinci_mdio: add GPIO reset logic

[PATCH] net: davinci_mdio: add GPIO reset logic

[Roger Quadros]

Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY like not being detected by the mdio bus
or link not functional. To work around these boards have
a GPIO connected to the PHY's reset pin.

Implement GPIO reset handling for such cases.

[1] - am572x-idk, am571x-idk, a437x-idk.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 .../devicetree/bindings/net/davinci-mdio.txt       |  2 +
 drivers/net/ethernet/ti/davinci_mdio.c             | 68 +++++++++++++++++++---
 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/davinci-mdio.txt b/Documentation/devicetree/bindings/net/davinci-mdio.txt
index 621156c..fd6ebe7 100644
--- a/Documentation/devicetree/bindings/net/davinci-mdio.txt
+++ b/Documentation/devicetree/bindings/net/davinci-mdio.txt
@@ -12,6 +12,8 @@ Required properties:
 
 Optional properties:
 - ti,hwmods		: Must be "davinci_mdio"
+- reset-gpios		: array of GPIO specifier for PHY hardware reset control
+- reset-delay-us	: reset assertion time [in microseconds]
 
 Note: "ti,hwmods" field is used to fetch the base address and irq
 resources from TI, omap hwmod data base during device registration.
diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index 33df340..c6f9e55 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -40,6 +40,9 @@
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of_gpio.h>
 
 /*
  * This timeout definition is a worst-case ultra defensive measure against
@@ -53,6 +56,8 @@
 
 #define DEF_OUT_FREQ		2200000		/* 2.2 MHz */
 
+#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
+
 struct davinci_mdio_of_param {
 	int autosuspend_delay_ms;
 };
@@ -104,6 +109,9 @@ struct davinci_mdio_data {
 	 */
 	bool		skip_scan;
 	u32		clk_div;
+	struct gpio_desc **gpio_reset;
+	int		num_gpios;
+	int		reset_delay_us;
 };
 
 static void davinci_mdio_init_clk(struct davinci_mdio_data *data)
@@ -142,6 +150,20 @@ static void davinci_mdio_enable(struct davinci_mdio_data *data)
 	__raw_writel(data->clk_div | CONTROL_ENABLE, &data->regs->control);
 }
 
+static void __davinci_gpio_reset(struct davinci_mdio_data *data)
+{
+	int i;
+
+	for (i = 0; i < data->num_gpios; i++) {
+		if (!data->gpio_reset[i])
+			continue;
+
+		gpiod_set_value_cansleep(data->gpio_reset[i], 1);
+		udelay(data->reset_delay_us);
+		gpiod_set_value_cansleep(data->gpio_reset[i], 0);
+	}
+}
+
 static int davinci_mdio_reset(struct mii_bus *bus)
 {
 	struct davinci_mdio_data *data = bus->priv;
@@ -317,20 +339,50 @@ static int davinci_mdio_write(struct mii_bus *bus, int phy_id,
 }
 
 #if IS_ENABLED(CONFIG_OF)
-static int davinci_mdio_probe_dt(struct mdio_platform_data *data,
-			 struct platform_device *pdev)
+static int davinci_mdio_probe_dt(struct davinci_mdio_data *data,
+				 struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
 	u32 prop;
-
-	if (!node)
-		return -EINVAL;
+	int error;
+	int i;
+	struct gpio_desc *gpiod;
 
 	if (of_property_read_u32(node, "bus_freq", &prop)) {
 		dev_err(&pdev->dev, "Missing bus_freq property in the DT.\n");
-		return -EINVAL;
+		data->pdata = default_pdata;
+	} else {
+		data->pdata.bus_freq = prop;
+	}
+
+	i = of_gpio_named_count(node, "reset-gpios");
+	if (i > 0) {
+		data->num_gpios = i;
+		data->gpio_reset = devm_kcalloc(&pdev->dev, i,
+						sizeof(struct gpio_desc *),
+						GFP_KERNEL);
+		if (!data->gpio_reset)
+			return -ENOMEM;
+
+		for (i = 0; i < data->num_gpios; i++) {
+			gpiod = devm_gpiod_get_index(&pdev->dev, "reset", i,
+						     GPIOD_OUT_LOW);
+			if (IS_ERR(gpiod)) {
+				error = PTR_ERR(gpiod);
+				if (error == -ENOENT)
+					continue;
+				else
+					return error;
+			}
+			data->gpio_reset[i] = gpiod;
+		}
+
+		if (of_property_read_u32(node, "reset-delay-us",
+					 &data->reset_delay_us))
+			data->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+
+		__davinci_gpio_reset(data);
 	}
-	data->bus_freq = prop;
 
 	return 0;
 }
@@ -372,7 +424,7 @@ static int davinci_mdio_probe(struct platform_device *pdev)
 	if (dev->of_node) {
 		const struct of_device_id	*of_id;
 
-		ret = davinci_mdio_probe_dt(&data->pdata, pdev);
+		ret = davinci_mdio_probe_dt(data, pdev);
 		if (ret)
 			return ret;
 		snprintf(data->bus->id, MII_BUS_ID_SIZE, "%s", pdev->name);
-- 
2.7.4

Re: [PATCH] net: davinci_mdio: add GPIO reset logic

[Andrew Lunn]

On Wed, Apr 05, 2017 at 11:33:57AM +0300, Roger Quadros wrote:
> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY like not being detected by the mdio bus
> or link not functional. To work around these boards have
> a GPIO connected to the PHY's reset pin.
> 
> Implement GPIO reset handling for such cases.
> 
> [1] - am572x-idk, am571x-idk, a437x-idk.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> ---
>  .../devicetree/bindings/net/davinci-mdio.txt       |  2 +
>  drivers/net/ethernet/ti/davinci_mdio.c             | 68 +++++++++++++++++++---
>  2 files changed, 62 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/davinci-mdio.txt b/Documentation/devicetree/bindings/net/davinci-mdio.txt
> index 621156c..fd6ebe7 100644
> --- a/Documentation/devicetree/bindings/net/davinci-mdio.txt
> +++ b/Documentation/devicetree/bindings/net/davinci-mdio.txt
> @@ -12,6 +12,8 @@ Required properties:
>  
>  Optional properties:
>  - ti,hwmods		: Must be "davinci_mdio"
> +- reset-gpios		: array of GPIO specifier for PHY hardware reset control
> +- reset-delay-us	: reset assertion time [in microseconds]
>  
>  Note: "ti,hwmods" field is used to fetch the base address and irq
>  resources from TI, omap hwmod data base during device registration.
> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
> index 33df340..c6f9e55 100644
> --- a/drivers/net/ethernet/ti/davinci_mdio.c
> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
> @@ -40,6 +40,9 @@
>  #include <linux/of_device.h>
>  #include <linux/of_mdio.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_gpio.h>
>  
>  /*
>   * This timeout definition is a worst-case ultra defensive measure against
> @@ -53,6 +56,8 @@
>  
>  #define DEF_OUT_FREQ		2200000		/* 2.2 MHz */
>  
> +#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
> +
>  struct davinci_mdio_of_param {
>  	int autosuspend_delay_ms;
>  };
> @@ -104,6 +109,9 @@ struct davinci_mdio_data {
>  	 */
>  	bool		skip_scan;
>  	u32		clk_div;
> +	struct gpio_desc **gpio_reset;
> +	int		num_gpios;
> +	int		reset_delay_us;
>  };
>  
>  static void davinci_mdio_init_clk(struct davinci_mdio_data *data)
> @@ -142,6 +150,20 @@ static void davinci_mdio_enable(struct davinci_mdio_data *data)
>  	__raw_writel(data->clk_div | CONTROL_ENABLE, &data->regs->control);
>  }
>  
> +static void __davinci_gpio_reset(struct davinci_mdio_data *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < data->num_gpios; i++) {
> +		if (!data->gpio_reset[i])
> +			continue;
> +
> +		gpiod_set_value_cansleep(data->gpio_reset[i], 1);
> +		udelay(data->reset_delay_us);
> +		gpiod_set_value_cansleep(data->gpio_reset[i], 0);
> +	}
> +}

Do you really need more than one GPIO? A single gpio would make all
this code a lot simpler.

     Andrew

Re: [PATCH] net: davinci_mdio: add GPIO reset logic

[Roger Quadros]

Hi,

On 05/04/17 18:03, Andrew Lunn wrote:
> On Wed, Apr 05, 2017 at 11:33:57AM +0300, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY like not being detected by the mdio bus
>> or link not functional. To work around these boards have
>> a GPIO connected to the PHY's reset pin.
>>
>> Implement GPIO reset handling for such cases.
>>
>> [1] - am572x-idk, am571x-idk, a437x-idk.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> ---
>>  .../devicetree/bindings/net/davinci-mdio.txt       |  2 +
>>  drivers/net/ethernet/ti/davinci_mdio.c             | 68 +++++++++++++++++++---
>>  2 files changed, 62 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/davinci-mdio.txt b/Documentation/devicetree/bindings/net/davinci-mdio.txt
>> index 621156c..fd6ebe7 100644
>> --- a/Documentation/devicetree/bindings/net/davinci-mdio.txt
>> +++ b/Documentation/devicetree/bindings/net/davinci-mdio.txt
>> @@ -12,6 +12,8 @@ Required properties:
>>  
>>  Optional properties:
>>  - ti,hwmods		: Must be "davinci_mdio"
>> +- reset-gpios		: array of GPIO specifier for PHY hardware reset control
>> +- reset-delay-us	: reset assertion time [in microseconds]
>>  
>>  Note: "ti,hwmods" field is used to fetch the base address and irq
>>  resources from TI, omap hwmod data base during device registration.
>> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
>> index 33df340..c6f9e55 100644
>> --- a/drivers/net/ethernet/ti/davinci_mdio.c
>> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
>> @@ -40,6 +40,9 @@
>>  #include <linux/of_device.h>
>>  #include <linux/of_mdio.h>
>>  #include <linux/pinctrl/consumer.h>
>> +#include <linux/gpio.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/of_gpio.h>
>>  
>>  /*
>>   * This timeout definition is a worst-case ultra defensive measure against
>> @@ -53,6 +56,8 @@
>>  
>>  #define DEF_OUT_FREQ		2200000		/* 2.2 MHz */
>>  
>> +#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
>> +
>>  struct davinci_mdio_of_param {
>>  	int autosuspend_delay_ms;
>>  };
>> @@ -104,6 +109,9 @@ struct davinci_mdio_data {
>>  	 */
>>  	bool		skip_scan;
>>  	u32		clk_div;
>> +	struct gpio_desc **gpio_reset;
>> +	int		num_gpios;
>> +	int		reset_delay_us;
>>  };
>>  
>>  static void davinci_mdio_init_clk(struct davinci_mdio_data *data)
>> @@ -142,6 +150,20 @@ static void davinci_mdio_enable(struct davinci_mdio_data *data)
>>  	__raw_writel(data->clk_div | CONTROL_ENABLE, &data->regs->control);
>>  }
>>  
>> +static void __davinci_gpio_reset(struct davinci_mdio_data *data)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < data->num_gpios; i++) {
>> +		if (!data->gpio_reset[i])
>> +			continue;
>> +
>> +		gpiod_set_value_cansleep(data->gpio_reset[i], 1);
>> +		udelay(data->reset_delay_us);
>> +		gpiod_set_value_cansleep(data->gpio_reset[i], 0);
>> +	}
>> +}
> 
> Do you really need more than one GPIO? A single gpio would make all
> this code a lot simpler.
> 

Yes we need. Some of our boards have separate GPIO RESET lines for
different PHYs on the same MDIO bus.

cheers,
-roger

Re: [PATCH] net: davinci_mdio: add GPIO reset logic

[Andrew Lunn]

> > Do you really need more than one GPIO? A single gpio would make all
> > this code a lot simpler.
> > 
> 
> Yes we need. Some of our boards have separate GPIO RESET lines for
> different PHYs on the same MDIO bus.

If you have a one-to-one mapping of GPIO and PHY, you should really be
modelling that differently. You want to be able to reset just a single
PHY, i.e. make it part of the PHY driver, or maybe the PHY core.

     Andrew

Re: [PATCH] net: davinci_mdio: add GPIO reset logic

[Roger Quadros]

On 06/04/17 15:05, Andrew Lunn wrote:
>>> Do you really need more than one GPIO? A single gpio would make all
>>> this code a lot simpler.
>>>
>>
>> Yes we need. Some of our boards have separate GPIO RESET lines for
>> different PHYs on the same MDIO bus.
> 
> If you have a one-to-one mapping of GPIO and PHY, you should really be
> modelling that differently. You want to be able to reset just a single
> PHY, i.e. make it part of the PHY driver, or maybe the PHY core.
> 
>      Andrew
> 


I'm not sure how it would be modelled. We have never had the need to reset
just one PHY on the MDIO bus.
Some boards have a single RESET line to multiple PHYs whereas others have individual
RESET lines. In all cases we just want to do the RESET once per boot.

cheers,
-roger

Re: [PATCH] net: davinci_mdio: add GPIO reset logic

[David Miller]

From: Roger Quadros <rogerq@ti.com>
Date: Wed, 5 Apr 2017 11:33:57 +0300

> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY like not being detected by the mdio bus
> or link not functional. To work around these boards have
> a GPIO connected to the PHY's reset pin.
> 
> Implement GPIO reset handling for such cases.
> 
> [1] - am572x-idk, am571x-idk, a437x-idk.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>

I have not seen a resolution in this discussion.

My understanding is that there are several cases (single MDIO bus whose
reset does a reset on all that MDIO bus's PHYs, etc.) and it's unclear
how to handle all such cases cleanly.

Re: [PATCH] net: davinci_mdio: add GPIO reset logic

[Andrew Lunn]

On Sat, Apr 08, 2017 at 06:55:45AM -0700, David Miller wrote:
> From: Roger Quadros <rogerq@ti.com>
> Date: Wed, 5 Apr 2017 11:33:57 +0300
> 
> > Some boards [1] leave the PHYs at an invalid state
> > during system power-up or reset thus causing unreliability
> > issues with the PHY like not being detected by the mdio bus
> > or link not functional. To work around these boards have
> > a GPIO connected to the PHY's reset pin.
> > 
> > Implement GPIO reset handling for such cases.
> > 
> > [1] - am572x-idk, am571x-idk, a437x-idk.
> > 
> > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> 
> I have not seen a resolution in this discussion.
> 
> My understanding is that there are several cases (single MDIO bus whose
> reset does a reset on all that MDIO bus's PHYs, etc.) and it's unclear
> how to handle all such cases cleanly.

I see it falling into two cases.

1) We have a GPIO which resets one PHY. In this case, the GPIO is a
PHY property, it should be documented in
Documentation/devicetree/bindings/net/phy.txt. Hopefully there is
nothing PHY driver specific here, so all the handling can be placed in
the core PHY code.

2) We have one or more GPIOs which reset more than one PHY. In this
case, the GPIOs are MDIO bus properties. Again, there should not be
anything which is MDIO bus driver specific, so all the handling can be
placed in the core MDIO bus code.

       Andrew

Re: [PATCH] net: davinci_mdio: add GPIO reset logic

[Florian Fainelli]

On 04/08/2017 08:10 AM, Andrew Lunn wrote:
> On Sat, Apr 08, 2017 at 06:55:45AM -0700, David Miller wrote:
>> From: Roger Quadros <rogerq@ti.com>
>> Date: Wed, 5 Apr 2017 11:33:57 +0300
>>
>>> Some boards [1] leave the PHYs at an invalid state
>>> during system power-up or reset thus causing unreliability
>>> issues with the PHY like not being detected by the mdio bus
>>> or link not functional. To work around these boards have
>>> a GPIO connected to the PHY's reset pin.
>>>
>>> Implement GPIO reset handling for such cases.
>>>
>>> [1] - am572x-idk, am571x-idk, a437x-idk.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>
>> I have not seen a resolution in this discussion.
>>
>> My understanding is that there are several cases (single MDIO bus whose
>> reset does a reset on all that MDIO bus's PHYs, etc.) and it's unclear
>> how to handle all such cases cleanly.
> 
> I see it falling into two cases.
> 
> 1) We have a GPIO which resets one PHY. In this case, the GPIO is a
> PHY property, it should be documented in
> Documentation/devicetree/bindings/net/phy.txt. Hopefully there is
> nothing PHY driver specific here, so all the handling can be placed in
> the core PHY code.

I suspect we would have to release the PHY GPIO reset line within a
mii_bus::reset callback, which occurs before the PHY registers are read.
There is this chicken and egg problem where you can't probe for a PHY
unless you can successfully read from it, and you can't do the PHY reset
in the PHY drivers' probe function unless you were able to find a PHY
device.

NB: you can work around that by using an Ethernet PHY device compatible
string in Device Tree that already has the PHY OUI specified, although
that usually does not scale to many different boards/designs.

> 
> 2) We have one or more GPIOs which reset more than one PHY. In this
> case, the GPIOs are MDIO bus properties. Again, there should not be
> anything which is MDIO bus driver specific, so all the handling can be
> placed in the core MDIO bus code.

Agreed.

I would do something like:

- if the MDIO bus node has a "gpio" reset property, use it and release
the device from reset
- for each available child node:
	- if the PHY/MDIO device's node have a "gpio" reset property use it and
release the PHYs from reset.

All of this should probably be placed somewhere in drivers/of/of_mdio.c
and deal with conditional GPIO/RESET controller support.
-- 
Florian

Re: [PATCH] net: davinci_mdio: add GPIO reset logic

[Roger Quadros]

On 08/04/17 18:18, Florian Fainelli wrote:
> 
> 
> On 04/08/2017 08:10 AM, Andrew Lunn wrote:
>> On Sat, Apr 08, 2017 at 06:55:45AM -0700, David Miller wrote:
>>> From: Roger Quadros <rogerq@ti.com>
>>> Date: Wed, 5 Apr 2017 11:33:57 +0300
>>>
>>>> Some boards [1] leave the PHYs at an invalid state
>>>> during system power-up or reset thus causing unreliability
>>>> issues with the PHY like not being detected by the mdio bus
>>>> or link not functional. To work around these boards have
>>>> a GPIO connected to the PHY's reset pin.
>>>>
>>>> Implement GPIO reset handling for such cases.
>>>>
>>>> [1] - am572x-idk, am571x-idk, a437x-idk.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>>
>>> I have not seen a resolution in this discussion.
>>>
>>> My understanding is that there are several cases (single MDIO bus whose
>>> reset does a reset on all that MDIO bus's PHYs, etc.) and it's unclear
>>> how to handle all such cases cleanly.
>>
>> I see it falling into two cases.
>>
>> 1) We have a GPIO which resets one PHY. In this case, the GPIO is a
>> PHY property, it should be documented in
>> Documentation/devicetree/bindings/net/phy.txt. Hopefully there is
>> nothing PHY driver specific here, so all the handling can be placed in
>> the core PHY code.
> 
> I suspect we would have to release the PHY GPIO reset line within a
> mii_bus::reset callback, which occurs before the PHY registers are read.
> There is this chicken and egg problem where you can't probe for a PHY
> unless you can successfully read from it, and you can't do the PHY reset
> in the PHY drivers' probe function unless you were able to find a PHY
> device.

+1
This is the exact problem we were facing so the reset has to be done at
MDIO bus level, before PHY probe.

> 
> NB: you can work around that by using an Ethernet PHY device compatible
> string in Device Tree that already has the PHY OUI specified, although
> that usually does not scale to many different boards/designs.
> 
>>
>> 2) We have one or more GPIOs which reset more than one PHY. In this
>> case, the GPIOs are MDIO bus properties. Again, there should not be
>> anything which is MDIO bus driver specific, so all the handling can be
>> placed in the core MDIO bus code.
> 
> Agreed.
> 
> I would do something like:
> 
> - if the MDIO bus node has a "gpio" reset property, use it and release
> the device from reset
> - for each available child node:
> 	- if the PHY/MDIO device's node have a "gpio" reset property use it and
> release the PHYs from reset.
> 
> All of this should probably be placed somewhere in drivers/of/of_mdio.c
> and deal with conditional GPIO/RESET controller support.
> 

I agree with this proposal. Just need to spend some time to rework this patch.

cheers,
-roger

[PATCH] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.

Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.

In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.

[1] - am572x-idk, am571x-idk, a437x-idk

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
 drivers/of/of_mdio.c       |  4 ++++
 include/linux/phy.h        |  5 +++++
 3 files changed, 35 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..25fda2b 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
+#include <linux/of_gpio.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
@@ -43,6 +46,8 @@
 
 #include "mdio-boardinfo.h"
 
+#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
+
 int mdiobus_register_device(struct mdio_device *mdiodev)
 {
 	if (mdiodev->bus->mdio_map[mdiodev->addr])
@@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 {
 	struct mdio_device *mdiodev;
 	int i, err;
+	struct gpio_desc *gpiod;
 
 	if (NULL == bus || NULL == bus->name ||
 	    NULL == bus->read || NULL == bus->write)
@@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	if (bus->reset)
 		bus->reset(bus);
 
+	/* de-assert bus level PHY GPIO resets */
+	for (i = 0; i < bus->num_reset_gpios; i++) {
+		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+					     GPIOD_OUT_LOW);
+		if (IS_ERR(gpiod)) {
+			err = PTR_ERR(gpiod);
+			if (err != -ENOENT) {
+				pr_err("mii_bus %s couldn't get reset GPIO\n",
+				       bus->id);
+				return err;
+			}
+		} else {
+			gpiod_set_value_cansleep(gpiod, 1);
+			if (!bus->reset_delay_us)
+				bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+			udelay(bus->reset_delay_us);
+			gpiod_set_value_cansleep(gpiod, 0);
+		}
+	}
+
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		if ((bus->phy_mask & (1 << i)) == 0) {
 			struct phy_device *phydev;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..83a62e4 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -221,6 +221,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 	mdio->dev.of_node = np;
 
+	/* Get bus level PHY reset GPIO details */
+	of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+	mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios");
+
 	/* Register the MDIO bus */
 	rc = mdiobus_register(mdio);
 	if (rc)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 43a7748..80a6574 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -217,6 +217,11 @@ struct mii_bus {
 	 * matching its address
 	 */
 	int irq[PHY_MAX_ADDR];
+
+	/* GPIO reset pulse width in uS */
+	int reset_delay_us;
+	/* Number of reset GPIOs */
+	int num_reset_gpios;
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
 
-- 
2.7.4

Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.

[Andrew Lunn]

On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
> 
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
> 
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
> 
> [1] - am572x-idk, am571x-idk, a437x-idk
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
>  drivers/of/of_mdio.c       |  4 ++++
>  include/linux/phy.h        |  5 +++++
>  3 files changed, 35 insertions(+)

Hi Roger

Thanks for making this generic.

Please add device tree binding documentation. I think that actually
means you have to document MDIO in general, since there currently is
not a binding document.

> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index fa7d51f..25fda2b 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -22,8 +22,11 @@
>  #include <linux/init.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/of_device.h>
>  #include <linux/of_mdio.h>
> +#include <linux/of_gpio.h>
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
>  #include <linux/skbuff.h>
> @@ -43,6 +46,8 @@
>  
>  #include "mdio-boardinfo.h"
>  
> +#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
> +
>  int mdiobus_register_device(struct mdio_device *mdiodev)
>  {
>  	if (mdiodev->bus->mdio_map[mdiodev->addr])
> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>  {
>  	struct mdio_device *mdiodev;
>  	int i, err;
> +	struct gpio_desc *gpiod;
>  
>  	if (NULL == bus || NULL == bus->name ||
>  	    NULL == bus->read || NULL == bus->write)
> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>  	if (bus->reset)
>  		bus->reset(bus);
>  
> +	/* de-assert bus level PHY GPIO resets */
> +	for (i = 0; i < bus->num_reset_gpios; i++) {
> +		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
> +					     GPIOD_OUT_LOW);
> +		if (IS_ERR(gpiod)) {
> +			err = PTR_ERR(gpiod);
> +			if (err != -ENOENT) {
> +				pr_err("mii_bus %s couldn't get reset GPIO\n",
> +				       bus->id);
> +				return err;
> +			}
> +		} else {
> +			gpiod_set_value_cansleep(gpiod, 1);


> +			if (!bus->reset_delay_us)
> +				bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;

Maybe do this once, where you read the device tree property.


> +			udelay(bus->reset_delay_us);
> +			gpiod_set_value_cansleep(gpiod, 0);
> +		}
> +	}
> +
>  	for (i = 0; i < PHY_MAX_ADDR; i++) {
>  		if ((bus->phy_mask & (1 << i)) == 0) {
>  			struct phy_device *phydev;
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 0b29798..83a62e4 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -221,6 +221,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  
>  	mdio->dev.of_node = np;
>  
> +	/* Get bus level PHY reset GPIO details */
> +	of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);

If the property does not exist, it is guaranteed mdio->reset_delay_us
is not changed. So you can set it to the default value before, then do
this call.

     Andrew

Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

Hi,

On 19/04/17 14:39, Andrew Lunn wrote:
> On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY which manifests as PHY not being detected
>> or link not functional. To fix this, these PHYs need to be RESET
>> via a GPIO connected to the PHY's RESET pin.
>>
>> Some boards have a single GPIO controlling the PHY RESET pin of all
>> PHYs on the bus whereas some others have separate GPIOs controlling
>> individual PHY RESETs.
>>
>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>> as the PHY will not probe till its reset is de-asserted.
>> So do the RESET de-assertion in the MDIO bus driver.
>>
>> [1] - am572x-idk, am571x-idk, a437x-idk
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
>>  drivers/of/of_mdio.c       |  4 ++++
>>  include/linux/phy.h        |  5 +++++
>>  3 files changed, 35 insertions(+)
> 
> Hi Roger
> 
> Thanks for making this generic.
> 
> Please add device tree binding documentation. I think that actually
> means you have to document MDIO in general, since there currently is
> not a binding document.

OK.

> 
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index fa7d51f..25fda2b 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -22,8 +22,11 @@
>>  #include <linux/init.h>
>>  #include <linux/delay.h>
>>  #include <linux/device.h>
>> +#include <linux/gpio.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/of_device.h>
>>  #include <linux/of_mdio.h>
>> +#include <linux/of_gpio.h>
>>  #include <linux/netdevice.h>
>>  #include <linux/etherdevice.h>
>>  #include <linux/skbuff.h>
>> @@ -43,6 +46,8 @@
>>  
>>  #include "mdio-boardinfo.h"
>>  
>> +#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
>> +
>>  int mdiobus_register_device(struct mdio_device *mdiodev)
>>  {
>>  	if (mdiodev->bus->mdio_map[mdiodev->addr])
>> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>>  {
>>  	struct mdio_device *mdiodev;
>>  	int i, err;
>> +	struct gpio_desc *gpiod;
>>  
>>  	if (NULL == bus || NULL == bus->name ||
>>  	    NULL == bus->read || NULL == bus->write)
>> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>>  	if (bus->reset)
>>  		bus->reset(bus);
>>  
>> +	/* de-assert bus level PHY GPIO resets */
>> +	for (i = 0; i < bus->num_reset_gpios; i++) {
>> +		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
>> +					     GPIOD_OUT_LOW);
>> +		if (IS_ERR(gpiod)) {
>> +			err = PTR_ERR(gpiod);
>> +			if (err != -ENOENT) {
>> +				pr_err("mii_bus %s couldn't get reset GPIO\n",
>> +				       bus->id);
>> +				return err;
>> +			}
>> +		} else {
>> +			gpiod_set_value_cansleep(gpiod, 1);
> 
> 
>> +			if (!bus->reset_delay_us)
>> +				bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
> 
> Maybe do this once, where you read the device tree property.

I was thinking from point of view that GPIO RESET code should work even without
device tree. Although I'm not sure if there would be any users or not.

> 
> 
>> +			udelay(bus->reset_delay_us);
>> +			gpiod_set_value_cansleep(gpiod, 0);
>> +		}
>> +	}
>> +
>>  	for (i = 0; i < PHY_MAX_ADDR; i++) {
>>  		if ((bus->phy_mask & (1 << i)) == 0) {
>>  			struct phy_device *phydev;
>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>> index 0b29798..83a62e4 100644
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -221,6 +221,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>>  
>>  	mdio->dev.of_node = np;
>>  
>> +	/* Get bus level PHY reset GPIO details */
>> +	of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
> 
> If the property does not exist, it is guaranteed mdio->reset_delay_us
> is not changed. So you can set it to the default value before, then do
> this call.
> 
>      Andrew
> 

cheers,
-roger

Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.

[Andrew Lunn]

On Wed, Apr 19, 2017 at 02:56:48PM +0300, Roger Quadros wrote:
> Hi,
> 
> On 19/04/17 14:39, Andrew Lunn wrote:
> > On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
> >> Some boards [1] leave the PHYs at an invalid state
> >> during system power-up or reset thus causing unreliability
> >> issues with the PHY which manifests as PHY not being detected
> >> or link not functional. To fix this, these PHYs need to be RESET
> >> via a GPIO connected to the PHY's RESET pin.
> >>
> >> Some boards have a single GPIO controlling the PHY RESET pin of all
> >> PHYs on the bus whereas some others have separate GPIOs controlling
> >> individual PHY RESETs.
> >>
> >> In both cases, the RESET de-assertion cannot be done in the PHY driver
> >> as the PHY will not probe till its reset is de-asserted.
> >> So do the RESET de-assertion in the MDIO bus driver.
> >>
> >> [1] - am572x-idk, am571x-idk, a437x-idk
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> ---
> >>  drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
> >>  drivers/of/of_mdio.c       |  4 ++++
> >>  include/linux/phy.h        |  5 +++++
> >>  3 files changed, 35 insertions(+)
> > 
> > Hi Roger
> > 
> > Thanks for making this generic.
> > 
> > Please add device tree binding documentation. I think that actually
> > means you have to document MDIO in general, since there currently is
> > not a binding document.
> 
> OK.
> 
> > 
> >> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> >> index fa7d51f..25fda2b 100644
> >> --- a/drivers/net/phy/mdio_bus.c
> >> +++ b/drivers/net/phy/mdio_bus.c
> >> @@ -22,8 +22,11 @@
> >>  #include <linux/init.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/device.h>
> >> +#include <linux/gpio.h>
> >> +#include <linux/gpio/consumer.h>
> >>  #include <linux/of_device.h>
> >>  #include <linux/of_mdio.h>
> >> +#include <linux/of_gpio.h>
> >>  #include <linux/netdevice.h>
> >>  #include <linux/etherdevice.h>
> >>  #include <linux/skbuff.h>
> >> @@ -43,6 +46,8 @@
> >>  
> >>  #include "mdio-boardinfo.h"
> >>  
> >> +#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
> >> +
> >>  int mdiobus_register_device(struct mdio_device *mdiodev)
> >>  {
> >>  	if (mdiodev->bus->mdio_map[mdiodev->addr])
> >> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> >>  {
> >>  	struct mdio_device *mdiodev;
> >>  	int i, err;
> >> +	struct gpio_desc *gpiod;
> >>  
> >>  	if (NULL == bus || NULL == bus->name ||
> >>  	    NULL == bus->read || NULL == bus->write)
> >> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> >>  	if (bus->reset)
> >>  		bus->reset(bus);
> >>  
> >> +	/* de-assert bus level PHY GPIO resets */
> >> +	for (i = 0; i < bus->num_reset_gpios; i++) {
> >> +		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
> >> +					     GPIOD_OUT_LOW);
> >> +		if (IS_ERR(gpiod)) {
> >> +			err = PTR_ERR(gpiod);
> >> +			if (err != -ENOENT) {
> >> +				pr_err("mii_bus %s couldn't get reset GPIO\n",
> >> +				       bus->id);
> >> +				return err;
> >> +			}
> >> +		} else {
> >> +			gpiod_set_value_cansleep(gpiod, 1);
> > 
> > 
> >> +			if (!bus->reset_delay_us)
> >> +				bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
> > 
> > Maybe do this once, where you read the device tree property.
> 
> I was thinking from point of view that GPIO RESET code should work even without
> device tree. Although I'm not sure if there would be any users or not.

Hi Roger

I don't see how this would work. What would devm_gpiod_get_index()
return? Something from ACPI? But then there would be something
equivalent for getting the delay.

Lets keep it simple and OF only. If there is a real need for something
in addition to OF, it can be added later.

   Andrew

Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

On 19/04/17 16:38, Andrew Lunn wrote:
> On Wed, Apr 19, 2017 at 02:56:48PM +0300, Roger Quadros wrote:
>> Hi,
>>
>> On 19/04/17 14:39, Andrew Lunn wrote:
>>> On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
>>>> Some boards [1] leave the PHYs at an invalid state
>>>> during system power-up or reset thus causing unreliability
>>>> issues with the PHY which manifests as PHY not being detected
>>>> or link not functional. To fix this, these PHYs need to be RESET
>>>> via a GPIO connected to the PHY's RESET pin.
>>>>
>>>> Some boards have a single GPIO controlling the PHY RESET pin of all
>>>> PHYs on the bus whereas some others have separate GPIOs controlling
>>>> individual PHY RESETs.
>>>>
>>>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>>>> as the PHY will not probe till its reset is de-asserted.
>>>> So do the RESET de-assertion in the MDIO bus driver.
>>>>
>>>> [1] - am572x-idk, am571x-idk, a437x-idk
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>  drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
>>>>  drivers/of/of_mdio.c       |  4 ++++
>>>>  include/linux/phy.h        |  5 +++++
>>>>  3 files changed, 35 insertions(+)
>>>
>>> Hi Roger
>>>
>>> Thanks for making this generic.
>>>
>>> Please add device tree binding documentation. I think that actually
>>> means you have to document MDIO in general, since there currently is
>>> not a binding document.
>>
>> OK.
>>
>>>
>>>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>>>> index fa7d51f..25fda2b 100644
>>>> --- a/drivers/net/phy/mdio_bus.c
>>>> +++ b/drivers/net/phy/mdio_bus.c
>>>> @@ -22,8 +22,11 @@
>>>>  #include <linux/init.h>
>>>>  #include <linux/delay.h>
>>>>  #include <linux/device.h>
>>>> +#include <linux/gpio.h>
>>>> +#include <linux/gpio/consumer.h>
>>>>  #include <linux/of_device.h>
>>>>  #include <linux/of_mdio.h>
>>>> +#include <linux/of_gpio.h>
>>>>  #include <linux/netdevice.h>
>>>>  #include <linux/etherdevice.h>
>>>>  #include <linux/skbuff.h>
>>>> @@ -43,6 +46,8 @@
>>>>  
>>>>  #include "mdio-boardinfo.h"
>>>>  
>>>> +#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
>>>> +
>>>>  int mdiobus_register_device(struct mdio_device *mdiodev)
>>>>  {
>>>>  	if (mdiodev->bus->mdio_map[mdiodev->addr])
>>>> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>>>>  {
>>>>  	struct mdio_device *mdiodev;
>>>>  	int i, err;
>>>> +	struct gpio_desc *gpiod;
>>>>  
>>>>  	if (NULL == bus || NULL == bus->name ||
>>>>  	    NULL == bus->read || NULL == bus->write)
>>>> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>>>>  	if (bus->reset)
>>>>  		bus->reset(bus);
>>>>  
>>>> +	/* de-assert bus level PHY GPIO resets */
>>>> +	for (i = 0; i < bus->num_reset_gpios; i++) {
>>>> +		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
>>>> +					     GPIOD_OUT_LOW);
>>>> +		if (IS_ERR(gpiod)) {
>>>> +			err = PTR_ERR(gpiod);
>>>> +			if (err != -ENOENT) {
>>>> +				pr_err("mii_bus %s couldn't get reset GPIO\n",
>>>> +				       bus->id);
>>>> +				return err;
>>>> +			}
>>>> +		} else {
>>>> +			gpiod_set_value_cansleep(gpiod, 1);
>>>
>>>
>>>> +			if (!bus->reset_delay_us)
>>>> +				bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
>>>
>>> Maybe do this once, where you read the device tree property.
>>
>> I was thinking from point of view that GPIO RESET code should work even without
>> device tree. Although I'm not sure if there would be any users or not.
> 
> Hi Roger
> 
> I don't see how this would work. What would devm_gpiod_get_index()
> return? Something from ACPI? But then there would be something

Yes or just lookup tables based on platform data.

> equivalent for getting the delay.

I'm not sure about delay part.

> 
> Lets keep it simple and OF only. If there is a real need for something
> in addition to OF, it can be added later.

OK. I'll revise the patch based on your comments.

cheers,
-roger

[PATCH v2] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.

Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.

In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.

[1] - am572x-idk, am571x-idk, a437x-idk

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v2:
- add device tree binding document (mdio.txt)
- specify default reset delay in of_mdio.c instead of mdio_bus.c

 Documentation/devicetree/bindings/net/mdio.txt | 20 ++++++++++++++++++++
 drivers/net/phy/mdio_bus.c                     | 22 ++++++++++++++++++++++
 drivers/of/of_mdio.c                           |  7 +++++++
 include/linux/phy.h                            |  5 +++++
 4 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mdio.txt

diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
new file mode 100644
index 0000000..6e703d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio.txt
@@ -0,0 +1,20 @@
+Common MDIO bus properties.
+
+These are generic properties that can apply to any MDIO bus.
+
+Optional properties:
+- reset-gpios: List of one or more GPIOs that control the RESET lines
+  of the PHYs on that MDIO bus.
+- reset-delay-us: RESET pulse width as per PHY datasheet.
+
+Example :
+
+	davinci_mdio: ethernet@0x5c030000 {
+		compatible = "ti,davinci_mdio";
+		ti,hwmods = "davinci_mdio";
+		reg = <0x5c030000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+		reset-delay-us = <2>;   /* PHY datasheet states 1uS min */
+	};
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..b353d99 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
+#include <linux/of_gpio.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
@@ -307,6 +310,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 {
 	struct mdio_device *mdiodev;
 	int i, err;
+	struct gpio_desc *gpiod;
 
 	if (NULL == bus || NULL == bus->name ||
 	    NULL == bus->read || NULL == bus->write)
@@ -333,6 +337,24 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	if (bus->reset)
 		bus->reset(bus);
 
+	/* de-assert bus level PHY GPIO resets */
+	for (i = 0; i < bus->num_reset_gpios; i++) {
+		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+					     GPIOD_OUT_LOW);
+		if (IS_ERR(gpiod)) {
+			err = PTR_ERR(gpiod);
+			if (err != -ENOENT) {
+				pr_err("mii_bus %s couldn't get reset GPIO\n",
+				       bus->id);
+				return err;
+			}
+		} else {
+			gpiod_set_value_cansleep(gpiod, 1);
+			udelay(bus->reset_delay_us);
+			gpiod_set_value_cansleep(gpiod, 0);
+		}
+	}
+
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		if ((bus->phy_mask & (1 << i)) == 0) {
 			struct phy_device *phydev;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..7e4c80f 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,6 +22,8 @@
 #include <linux/of_net.h>
 #include <linux/module.h>
 
+#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
+
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_LICENSE("GPL");
 
@@ -221,6 +223,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 	mdio->dev.of_node = np;
 
+	/* Get bus level PHY reset GPIO details */
+	mdio->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+	of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+	mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios");
+
 	/* Register the MDIO bus */
 	rc = mdiobus_register(mdio);
 	if (rc)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 43a7748..80a6574 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -217,6 +217,11 @@ struct mii_bus {
 	 * matching its address
 	 */
 	int irq[PHY_MAX_ADDR];
+
+	/* GPIO reset pulse width in uS */
+	int reset_delay_us;
+	/* Number of reset GPIOs */
+	int num_reset_gpios;
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
 
-- 
2.7.4

Re: [PATCH v2] mdio_bus: Issue GPIO RESET to PHYs.

[Andrew Lunn]

On Thu, Apr 20, 2017 at 11:39:20AM +0300, Roger Quadros wrote:
> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
> 
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
> 
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.

Hi Roger

Networking patches should include in the subject line which tree they
are for. In this case, net-next. So please make the Subject

[patch v3 net-next] .....

> [1] - am572x-idk, am571x-idk, a437x-idk
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> v2:
> - add device tree binding document (mdio.txt)
> - specify default reset delay in of_mdio.c instead of mdio_bus.c
> 
>  Documentation/devicetree/bindings/net/mdio.txt | 20 ++++++++++++++++++++
>  drivers/net/phy/mdio_bus.c                     | 22 ++++++++++++++++++++++
>  drivers/of/of_mdio.c                           |  7 +++++++
>  include/linux/phy.h                            |  5 +++++
>  4 files changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/mdio.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
> new file mode 100644
> index 0000000..6e703d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mdio.txt
> @@ -0,0 +1,20 @@
> +Common MDIO bus properties.
> +
> +These are generic properties that can apply to any MDIO bus.
> +
> +Optional properties:
> +- reset-gpios: List of one or more GPIOs that control the RESET lines
> +  of the PHYs on that MDIO bus.
> +- reset-delay-us: RESET pulse width as per PHY datasheet.

It would be good to explicitly say that it is in uS as part of the
comment.

Also, please document that we expect a list of child nodes, one per
device on the bus. These should follow the generic phy.txt, or a
device specific binding document.

> +
> +Example :
> +

It would be good to say something like:

This example shows these optional properties, plus other properties
required for the TI Davinci MDIO driver.

Pointing this out may stop people cut/past the ti,hwmods property.

> +	davinci_mdio: ethernet@0x5c030000 {
> +		compatible = "ti,davinci_mdio";
> +		ti,hwmods = "davinci_mdio";
> +		reg = <0x5c030000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> +		reset-delay-us = <2>;   /* PHY datasheet states 1uS min */
> +	};

And please include at least one PHY on the bus.

Sorry for asking for so much in the documentation. That is the problem
with the documentation being missing to start with.

The code looks good now.

    Andrew

Re: [PATCH v2] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

On 20/04/17 16:13, Andrew Lunn wrote:
> On Thu, Apr 20, 2017 at 11:39:20AM +0300, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY which manifests as PHY not being detected
>> or link not functional. To fix this, these PHYs need to be RESET
>> via a GPIO connected to the PHY's RESET pin.
>>
>> Some boards have a single GPIO controlling the PHY RESET pin of all
>> PHYs on the bus whereas some others have separate GPIOs controlling
>> individual PHY RESETs.
>>
>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>> as the PHY will not probe till its reset is de-asserted.
>> So do the RESET de-assertion in the MDIO bus driver.
> 
> Hi Roger
> 
> Networking patches should include in the subject line which tree they
> are for. In this case, net-next. So please make the Subject
> 
> [patch v3 net-next] .....
> 
>> [1] - am572x-idk, am571x-idk, a437x-idk
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> v2:
>> - add device tree binding document (mdio.txt)
>> - specify default reset delay in of_mdio.c instead of mdio_bus.c
>>
>>  Documentation/devicetree/bindings/net/mdio.txt | 20 ++++++++++++++++++++
>>  drivers/net/phy/mdio_bus.c                     | 22 ++++++++++++++++++++++
>>  drivers/of/of_mdio.c                           |  7 +++++++
>>  include/linux/phy.h                            |  5 +++++
>>  4 files changed, 54 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/mdio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>> new file mode 100644
>> index 0000000..6e703d7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>> @@ -0,0 +1,20 @@
>> +Common MDIO bus properties.
>> +
>> +These are generic properties that can apply to any MDIO bus.
>> +
>> +Optional properties:
>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>> +  of the PHYs on that MDIO bus.
>> +- reset-delay-us: RESET pulse width as per PHY datasheet.
> 
> It would be good to explicitly say that it is in uS as part of the
> comment.
> 
> Also, please document that we expect a list of child nodes, one per
> device on the bus. These should follow the generic phy.txt, or a
> device specific binding document.
> 
>> +
>> +Example :
>> +
> 
> It would be good to say something like:
> 
> This example shows these optional properties, plus other properties
> required for the TI Davinci MDIO driver.
> 
> Pointing this out may stop people cut/past the ti,hwmods property.
> 
>> +	davinci_mdio: ethernet@0x5c030000 {
>> +		compatible = "ti,davinci_mdio";
>> +		ti,hwmods = "davinci_mdio";
>> +		reg = <0x5c030000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>> +		reset-delay-us = <2>;   /* PHY datasheet states 1uS min */
>> +	};
> 
> And please include at least one PHY on the bus.
> 
> Sorry for asking for so much in the documentation. That is the problem
> with the documentation being missing to start with.
> 
> The code looks good now.
> 

OK Andrew. I'll post a v3 with your suggestions.

cheers,
-roger

[PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.

Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.

In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.

[1] - am572x-idk, am571x-idk, a437x-idk

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v3:
- added more information in the DT binding document.

v2:
- add device tree binding document (mdio.txt)
- specify default reset delay in of_mdio.c instead of mdio_bus.c

 Documentation/devicetree/bindings/net/mdio.txt | 33 ++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c                     | 22 +++++++++++++++++
 drivers/of/of_mdio.c                           |  7 ++++++
 include/linux/phy.h                            |  5 ++++
 4 files changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mdio.txt

diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
new file mode 100644
index 0000000..3517369
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio.txt
@@ -0,0 +1,33 @@
+Common MDIO bus properties.
+
+These are generic properties that can apply to any MDIO bus.
+
+Optional properties:
+- reset-gpios: List of one or more GPIOs that control the RESET lines
+  of the PHYs on that MDIO bus.
+- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
+
+A list of child nodes, one per device on the bus is expected. These
+should follow the generic phy.txt, or a device specific binding document.
+
+Example :
+This example shows these optional properties, plus other properties
+required for the TI Davinci MDIO driver.
+
+	davinci_mdio: ethernet@0x5c030000 {
+		compatible = "ti,davinci_mdio";
+		reg = <0x5c030000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+		reset-delay-us = <2>;   /* PHY datasheet states 1uS min */
+
+		ethphy0: ethernet-phy@1 {
+			reg = <1>;
+		};
+
+		ethphy1: ethernet-phy@3 {
+			reg = <3>;
+		};
+	};
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..b353d99 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
+#include <linux/of_gpio.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
@@ -307,6 +310,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 {
 	struct mdio_device *mdiodev;
 	int i, err;
+	struct gpio_desc *gpiod;
 
 	if (NULL == bus || NULL == bus->name ||
 	    NULL == bus->read || NULL == bus->write)
@@ -333,6 +337,24 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	if (bus->reset)
 		bus->reset(bus);
 
+	/* de-assert bus level PHY GPIO resets */
+	for (i = 0; i < bus->num_reset_gpios; i++) {
+		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+					     GPIOD_OUT_LOW);
+		if (IS_ERR(gpiod)) {
+			err = PTR_ERR(gpiod);
+			if (err != -ENOENT) {
+				pr_err("mii_bus %s couldn't get reset GPIO\n",
+				       bus->id);
+				return err;
+			}
+		} else {
+			gpiod_set_value_cansleep(gpiod, 1);
+			udelay(bus->reset_delay_us);
+			gpiod_set_value_cansleep(gpiod, 0);
+		}
+	}
+
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		if ((bus->phy_mask & (1 << i)) == 0) {
 			struct phy_device *phydev;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..7e4c80f 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,6 +22,8 @@
 #include <linux/of_net.h>
 #include <linux/module.h>
 
+#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
+
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_LICENSE("GPL");
 
@@ -221,6 +223,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 	mdio->dev.of_node = np;
 
+	/* Get bus level PHY reset GPIO details */
+	mdio->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+	of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+	mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios");
+
 	/* Register the MDIO bus */
 	rc = mdiobus_register(mdio);
 	if (rc)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 43a7748..80a6574 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -217,6 +217,11 @@ struct mii_bus {
 	 * matching its address
 	 */
 	int irq[PHY_MAX_ADDR];
+
+	/* GPIO reset pulse width in uS */
+	int reset_delay_us;
+	/* Number of reset GPIOs */
+	int num_reset_gpios;
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
 
-- 
2.7.4

Re: [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Andrew Lunn]

On Thu, Apr 20, 2017 at 05:11:53PM +0300, Roger Quadros wrote:
> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
> 
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
> 
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
> 
> [1] - am572x-idk, am571x-idk, a437x-idk
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

Hi Roger

Thanks for doing a generic solutions and the MDIO DT documentation.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Florian Fainelli]

Hi Roger,

On 04/20/2017 07:11 AM, Roger Quadros wrote:
> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
> 
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
> 
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
> 
> [1] - am572x-idk, am571x-idk, a437x-idk
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

A few comments on the binding and the code, sorry for this late review.

> +Example :
> +This example shows these optional properties, plus other properties
> +required for the TI Davinci MDIO driver.
> +
> +	davinci_mdio: ethernet@0x5c030000 {
> +		compatible = "ti,davinci_mdio";
> +		reg = <0x5c030000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> +		reset-delay-us = <2>;   /* PHY datasheet states 1uS min */

us is micro seconds, uS is micro siemens.

> +
> +		ethphy0: ethernet-phy@1 {
> +			reg = <1>;
> +		};
> +
> +		ethphy1: ethernet-phy@3 {
> +			reg = <3>;
> +		};
> +	};

>  
> +	/* de-assert bus level PHY GPIO resets */
> +	for (i = 0; i < bus->num_reset_gpios; i++) {
> +		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
> +					     GPIOD_OUT_LOW);
> +		if (IS_ERR(gpiod)) {
> +			err = PTR_ERR(gpiod);
> +			if (err != -ENOENT) {
> +				pr_err("mii_bus %s couldn't get reset GPIO\n",
> +				       bus->id);

Could we use dev_err(&bus->dev) here to better identify which MDIO bus
is returning the problem?

> +				return err;

Should we somehow "unwind" the reset lines we were able to successfully
take out of reset and therefore put back into reset state? How about
mdiobus_unregister()? Should we have similar code there, if not for
correctness to be more power efficient?

> +			}
> +		} else {
> +			gpiod_set_value_cansleep(gpiod, 1);
> +			udelay(bus->reset_delay_us);
> +			gpiod_set_value_cansleep(gpiod, 0);

Does that work even if the polarity of the reset line is active low?

Thanks!
-- 
Florian

Re: [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Andrew Lunn]

> > +			gpiod_set_value_cansleep(gpiod, 1);
> > +			udelay(bus->reset_delay_us);
> > +			gpiod_set_value_cansleep(gpiod, 0);
> 
> Does that work even if the polarity of the reset line is active low?

Hi Florian

Yes, it does. The gpiod_ API takes care of that, if you set the flag
GPIO_ACTIVE_LOW in the device tree blob. This is one of the
improvements over the gpio_ API.

		Andrew

Re: [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

Hi Florian,

On 21/04/17 04:23, Florian Fainelli wrote:
> Hi Roger,
> 
> On 04/20/2017 07:11 AM, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY which manifests as PHY not being detected
>> or link not functional. To fix this, these PHYs need to be RESET
>> via a GPIO connected to the PHY's RESET pin.
>>
>> Some boards have a single GPIO controlling the PHY RESET pin of all
>> PHYs on the bus whereas some others have separate GPIOs controlling
>> individual PHY RESETs.
>>
>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>> as the PHY will not probe till its reset is de-asserted.
>> So do the RESET de-assertion in the MDIO bus driver.
>>
>> [1] - am572x-idk, am571x-idk, a437x-idk
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> A few comments on the binding and the code, sorry for this late review.

No problem at all.
> 
>> +Example :
>> +This example shows these optional properties, plus other properties
>> +required for the TI Davinci MDIO driver.
>> +
>> +	davinci_mdio: ethernet@0x5c030000 {
>> +		compatible = "ti,davinci_mdio";
>> +		reg = <0x5c030000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>> +		reset-delay-us = <2>;   /* PHY datasheet states 1uS min */
> 
> us is micro seconds, uS is micro siemens.

OK.

> 
>> +
>> +		ethphy0: ethernet-phy@1 {
>> +			reg = <1>;
>> +		};
>> +
>> +		ethphy1: ethernet-phy@3 {
>> +			reg = <3>;
>> +		};
>> +	};
> 
>>  
>> +	/* de-assert bus level PHY GPIO resets */
>> +	for (i = 0; i < bus->num_reset_gpios; i++) {
>> +		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
>> +					     GPIOD_OUT_LOW);
>> +		if (IS_ERR(gpiod)) {
>> +			err = PTR_ERR(gpiod);
>> +			if (err != -ENOENT) {
>> +				pr_err("mii_bus %s couldn't get reset GPIO\n",
>> +				       bus->id);
> 
> Could we use dev_err(&bus->dev) here to better identify which MDIO bus
> is returning the problem?

Sure.
> 
>> +				return err;
> 
> Should we somehow "unwind" the reset lines we were able to successfully
> take out of reset and therefore put back into reset state? How about
> mdiobus_unregister()? Should we have similar code there, if not for
> correctness to be more power efficient?

Al right.

> 
>> +			}
>> +		} else {
>> +			gpiod_set_value_cansleep(gpiod, 1);
>> +			udelay(bus->reset_delay_us);
>> +			gpiod_set_value_cansleep(gpiod, 0);
> 
> Does that work even if the polarity of the reset line is active low?
> 

Yes. The polarity needs to be specified at DT as explained by Andrew already.

cheers,
-roger

[PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.

Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.

In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.

[1] - am572x-idk, am571x-idk, a437x-idk

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v4:
- use dev_err() instead of pr_err()
- put PHYs back in RESET on failure or mdiobus_unregister()
- typo fixes uS -> us

v3:
- added more information in the DT binding document.

v2:
- add device tree binding document (mdio.txt)
- specify default reset delay in of_mdio.c instead of mdio_bus.c

 Documentation/devicetree/bindings/net/mdio.txt | 33 ++++++++++++++++++
 drivers/net/phy/mdio_bus.c                     | 47 ++++++++++++++++++++++++++
 drivers/of/of_mdio.c                           |  7 ++++
 include/linux/phy.h                            |  7 ++++
 4 files changed, 94 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mdio.txt

diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
new file mode 100644
index 0000000..4ffbbac
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio.txt
@@ -0,0 +1,33 @@
+Common MDIO bus properties.
+
+These are generic properties that can apply to any MDIO bus.
+
+Optional properties:
+- reset-gpios: List of one or more GPIOs that control the RESET lines
+  of the PHYs on that MDIO bus.
+- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
+
+A list of child nodes, one per device on the bus is expected. These
+should follow the generic phy.txt, or a device specific binding document.
+
+Example :
+This example shows these optional properties, plus other properties
+required for the TI Davinci MDIO driver.
+
+	davinci_mdio: ethernet@0x5c030000 {
+		compatible = "ti,davinci_mdio";
+		reg = <0x5c030000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
+
+		ethphy0: ethernet-phy@1 {
+			reg = <1>;
+		};
+
+		ethphy1: ethernet-phy@3 {
+			reg = <3>;
+		};
+	};
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..de927f7 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
+#include <linux/of_gpio.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
@@ -307,6 +310,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 {
 	struct mdio_device *mdiodev;
 	int i, err;
+	struct gpio_desc *gpiod;
 
 	if (NULL == bus || NULL == bus->name ||
 	    NULL == bus->read || NULL == bus->write)
@@ -333,6 +337,35 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	if (bus->reset)
 		bus->reset(bus);
 
+	/* de-assert bus level PHY GPIO resets */
+	if (bus->num_reset_gpios > 0) {
+		bus->reset_gpiod = devm_kcalloc(&bus->dev,
+						 bus->num_reset_gpios,
+						 sizeof(struct gpio_desc *),
+						 GFP_KERNEL);
+		if (!bus->reset_gpiod)
+			return -ENOMEM;
+	}
+
+	for (i = 0; i < bus->num_reset_gpios; i++) {
+		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+					     GPIOD_OUT_LOW);
+		if (IS_ERR(gpiod)) {
+			err = PTR_ERR(gpiod);
+			if (err != -ENOENT) {
+				dev_err(&bus->dev,
+					"mii_bus %s couldn't get reset GPIO\n",
+					bus->id);
+				return err;
+			}
+		} else {
+			bus->reset_gpiod[i] = gpiod;
+			gpiod_set_value_cansleep(gpiod, 1);
+			udelay(bus->reset_delay_us);
+			gpiod_set_value_cansleep(gpiod, 0);
+		}
+	}
+
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		if ((bus->phy_mask & (1 << i)) == 0) {
 			struct phy_device *phydev;
@@ -360,6 +393,13 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 		mdiodev->device_remove(mdiodev);
 		mdiodev->device_free(mdiodev);
 	}
+
+	/* Put PHYs in RESET to save power */
+	for (i = 0; i < bus->num_reset_gpios; i++) {
+		if (bus->reset_gpiod[i])
+			gpiod_set_value_cansleep(bus->reset_gpiod[i], 1);
+	}
+
 	device_del(&bus->dev);
 	return err;
 }
@@ -381,6 +421,13 @@ void mdiobus_unregister(struct mii_bus *bus)
 		mdiodev->device_remove(mdiodev);
 		mdiodev->device_free(mdiodev);
 	}
+
+	/* Put PHYs in RESET to save power */
+	for (i = 0; i < bus->num_reset_gpios; i++) {
+		if (bus->reset_gpiod[i])
+			gpiod_set_value_cansleep(bus->reset_gpiod[i], 1);
+	}
+
 	device_del(&bus->dev);
 }
 EXPORT_SYMBOL(mdiobus_unregister);
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..7e4c80f 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,6 +22,8 @@
 #include <linux/of_net.h>
 #include <linux/module.h>
 
+#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
+
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_LICENSE("GPL");
 
@@ -221,6 +223,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 	mdio->dev.of_node = np;
 
+	/* Get bus level PHY reset GPIO details */
+	mdio->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+	of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+	mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios");
+
 	/* Register the MDIO bus */
 	rc = mdiobus_register(mdio);
 	if (rc)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 43a7748..8ce57f4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -217,6 +217,13 @@ struct mii_bus {
 	 * matching its address
 	 */
 	int irq[PHY_MAX_ADDR];
+
+	/* GPIO reset pulse width in microseconds */
+	int reset_delay_us;
+	/* Number of reset GPIOs */
+	int num_reset_gpios;
+	/* Array of RESET GPIO descriptors */
+	struct gpio_desc **reset_gpiod;
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
 
-- 
2.7.4

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Lars-Peter Clausen]

On 04/21/2017 03:15 PM, Roger Quadros wrote:
> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
> new file mode 100644
> index 0000000..4ffbbac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mdio.txt
> @@ -0,0 +1,33 @@
> +Common MDIO bus properties.
> +
> +These are generic properties that can apply to any MDIO bus.
> +
> +Optional properties:
> +- reset-gpios: List of one or more GPIOs that control the RESET lines
> +  of the PHYs on that MDIO bus.
> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
> +
> +A list of child nodes, one per device on the bus is expected. These
> +should follow the generic phy.txt, or a device specific binding document.
> +
> +Example :
> +This example shows these optional properties, plus other properties
> +required for the TI Davinci MDIO driver.
> +
> +	davinci_mdio: ethernet@0x5c030000 {
> +		compatible = "ti,davinci_mdio";
> +		reg = <0x5c030000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */

If this is the reset line of the PHY shouldn't it be a property of the PHY
node rather than of the MDIO controller node (which might have a reset on
its own)?

> +
> +		ethphy0: ethernet-phy@1 {
> +			reg = <1>;
> +		};
> +
> +		ethphy1: ethernet-phy@3 {
> +			reg = <3>;
> +		};

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Andrew Lunn]

On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
> On 04/21/2017 03:15 PM, Roger Quadros wrote:
> > diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
> > new file mode 100644
> > index 0000000..4ffbbac
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/mdio.txt
> > @@ -0,0 +1,33 @@
> > +Common MDIO bus properties.
> > +
> > +These are generic properties that can apply to any MDIO bus.
> > +
> > +Optional properties:
> > +- reset-gpios: List of one or more GPIOs that control the RESET lines
> > +  of the PHYs on that MDIO bus.
> > +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
> > +
> > +A list of child nodes, one per device on the bus is expected. These
> > +should follow the generic phy.txt, or a device specific binding document.
> > +
> > +Example :
> > +This example shows these optional properties, plus other properties
> > +required for the TI Davinci MDIO driver.
> > +
> > +	davinci_mdio: ethernet@0x5c030000 {
> > +		compatible = "ti,davinci_mdio";
> > +		reg = <0x5c030000 0x1000>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> > +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
> 
> If this is the reset line of the PHY shouldn't it be a property of the PHY
> node rather than of the MDIO controller node (which might have a reset on
> its own)?
> > +
> > +		ethphy0: ethernet-phy@1 {
> > +			reg = <1>;
> > +		};
> > +
> > +		ethphy1: ethernet-phy@3 {
> > +			reg = <3>;
> > +		};

Hi Lars-Peter

We discussed this when the first proposal was made. There are two
cases, to consider.

1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
example, two PHYs.

2) There is one GPIO line per PHY. That is a separate case, and as you
say, the reset line should probably be considered a PHY property, not
an MDIO property. However, it can be messy, since in order to probe
the MDIO bus, you probably need to take the PHY out of reset.

Anyway, this patch addresses the first case, so should be accepted. If
anybody wants to address the second case, they are free to do so.

	Andrew

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

On 24/04/17 02:35, Andrew Lunn wrote:
> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>>> new file mode 100644
>>> index 0000000..4ffbbac
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>> @@ -0,0 +1,33 @@
>>> +Common MDIO bus properties.
>>> +
>>> +These are generic properties that can apply to any MDIO bus.
>>> +
>>> +Optional properties:
>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>> +  of the PHYs on that MDIO bus.
>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>> +
>>> +A list of child nodes, one per device on the bus is expected. These
>>> +should follow the generic phy.txt, or a device specific binding document.
>>> +
>>> +Example :
>>> +This example shows these optional properties, plus other properties
>>> +required for the TI Davinci MDIO driver.
>>> +
>>> +	davinci_mdio: ethernet@0x5c030000 {
>>> +		compatible = "ti,davinci_mdio";
>>> +		reg = <0x5c030000 0x1000>;
>>> +		#address-cells = <1>;
>>> +		#size-cells = <0>;
>>> +
>>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>> +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
>>
>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>> node rather than of the MDIO controller node (which might have a reset on
>> its own)?
>>> +
>>> +		ethphy0: ethernet-phy@1 {
>>> +			reg = <1>;
>>> +		};
>>> +
>>> +		ethphy1: ethernet-phy@3 {
>>> +			reg = <3>;
>>> +		};
> 
> Hi Lars-Peter
> 
> We discussed this when the first proposal was made. There are two
> cases, to consider.
> 
> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
> example, two PHYs.
> 
> 2) There is one GPIO line per PHY. That is a separate case, and as you
> say, the reset line should probably be considered a PHY property, not
> an MDIO property. However, it can be messy, since in order to probe
> the MDIO bus, you probably need to take the PHY out of reset.
> 
> Anyway, this patch addresses the first case, so should be accepted. If
> anybody wants to address the second case, they are free to do so.

Thanks for the explanation Andrew.

For the second case, even if the RESET GPIO property is specified
in the PHY node, the RESET *will* have to be done by the MDIO bus driver
else the PHY might not be probed at all.

Whether we need additional code to just to make the DT look prettier is
questionable and if required can come as a separate patch.

cheers,
-roger

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Florian Fainelli]

On 04/24/2017 02:04 AM, Roger Quadros wrote:
> On 24/04/17 02:35, Andrew Lunn wrote:
>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>>>> new file mode 100644
>>>> index 0000000..4ffbbac
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>>> @@ -0,0 +1,33 @@
>>>> +Common MDIO bus properties.
>>>> +
>>>> +These are generic properties that can apply to any MDIO bus.
>>>> +
>>>> +Optional properties:
>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>>> +  of the PHYs on that MDIO bus.
>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>>> +
>>>> +A list of child nodes, one per device on the bus is expected. These
>>>> +should follow the generic phy.txt, or a device specific binding document.
>>>> +
>>>> +Example :
>>>> +This example shows these optional properties, plus other properties
>>>> +required for the TI Davinci MDIO driver.
>>>> +
>>>> +	davinci_mdio: ethernet@0x5c030000 {
>>>> +		compatible = "ti,davinci_mdio";
>>>> +		reg = <0x5c030000 0x1000>;
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +
>>>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>>> +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
>>>
>>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>>> node rather than of the MDIO controller node (which might have a reset on
>>> its own)?
>>>> +
>>>> +		ethphy0: ethernet-phy@1 {
>>>> +			reg = <1>;
>>>> +		};
>>>> +
>>>> +		ethphy1: ethernet-phy@3 {
>>>> +			reg = <3>;
>>>> +		};
>>
>> Hi Lars-Peter
>>
>> We discussed this when the first proposal was made. There are two
>> cases, to consider.
>>
>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
>> example, two PHYs.
>>
>> 2) There is one GPIO line per PHY. That is a separate case, and as you
>> say, the reset line should probably be considered a PHY property, not
>> an MDIO property. However, it can be messy, since in order to probe
>> the MDIO bus, you probably need to take the PHY out of reset.
>>
>> Anyway, this patch addresses the first case, so should be accepted. If
>> anybody wants to address the second case, they are free to do so.
> 
> Thanks for the explanation Andrew.
> 
> For the second case, even if the RESET GPIO property is specified
> in the PHY node, the RESET *will* have to be done by the MDIO bus driver
> else the PHY might not be probed at all.
> 
> Whether we need additional code to just to make the DT look prettier is
> questionable and if required can come as a separate patch.

Well, it's not about prettier vs. uglier, it's about correct vs.
incorrect. The binding document you propose here is correct for a single
reset line controlling all PHYs, and that's why such a reset line needs
to be placed at the MDIO controller level, because it's a property of
such a node.

If you need to support individual reset lines per-PHY, then there should
be some kind of amendment to the Ethernet PHY Device Tree binding
document which specifies optional reset-gpio properties for these nodes.

Until that happens, I think your v4 is good to go.
-- 
Florian

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[David Miller]

From: Roger Quadros <rogerq@ti.com>
Date: Fri, 21 Apr 2017 16:15:38 +0300

> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
> 
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
> 
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
> 
> [1] - am572x-idk, am571x-idk, a437x-idk
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>

Applied, thanks.

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Lars-Peter Clausen]

On 04/24/2017 11:04 AM, Roger Quadros wrote:
> On 24/04/17 02:35, Andrew Lunn wrote:
>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>>>> new file mode 100644
>>>> index 0000000..4ffbbac
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>>> @@ -0,0 +1,33 @@
>>>> +Common MDIO bus properties.
>>>> +
>>>> +These are generic properties that can apply to any MDIO bus.
>>>> +
>>>> +Optional properties:
>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>>> +  of the PHYs on that MDIO bus.
>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>>> +
>>>> +A list of child nodes, one per device on the bus is expected. These
>>>> +should follow the generic phy.txt, or a device specific binding document.
>>>> +
>>>> +Example :
>>>> +This example shows these optional properties, plus other properties
>>>> +required for the TI Davinci MDIO driver.
>>>> +
>>>> +	davinci_mdio: ethernet@0x5c030000 {
>>>> +		compatible = "ti,davinci_mdio";
>>>> +		reg = <0x5c030000 0x1000>;
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +
>>>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>>> +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
>>>
>>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>>> node rather than of the MDIO controller node (which might have a reset on
>>> its own)?
>>>> +
>>>> +		ethphy0: ethernet-phy@1 {
>>>> +			reg = <1>;
>>>> +		};
>>>> +
>>>> +		ethphy1: ethernet-phy@3 {
>>>> +			reg = <3>;
>>>> +		};
>>
>> Hi Lars-Peter
>>
>> We discussed this when the first proposal was made. There are two
>> cases, to consider.
>>
>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
>> example, two PHYs.
>>
>> 2) There is one GPIO line per PHY. That is a separate case, and as you
>> say, the reset line should probably be considered a PHY property, not
>> an MDIO property. However, it can be messy, since in order to probe
>> the MDIO bus, you probably need to take the PHY out of reset.
>>

But the DT binding documentation says something else "List of one or more
GPIOs that control the RESET lines of the PHYs on that MDIO bus".

>> Anyway, this patch addresses the first case, so should be accepted. If
>> anybody wants to address the second case, they are free to do so.

I think we all know that that's not going to happen. Once there is a working
kludge there is no incentive to do a proper implementation anymore.


> Thanks for the explanation Andrew.
> 
> For the second case, even if the RESET GPIO property is specified
> in the PHY node, the RESET *will* have to be done by the MDIO bus driver
> else the PHY might not be probed at all.

I'm not arguing with that, just that the hardware description should be
truthful to the hardware topology and not to the software topology, i.e. the
implementation details of the Linux kernel in this case. Reset GPIOs are not
the only resource that is connected to the PHY that needs to be enabled
before they can be enumerated. E.g. clocks and regulators fall into the same
realm. And while you might argue that with a on-SoC phy controller node
there wont be any conflicts in regard to the reset-gpios property, this not
so very true for the clocks property.

And MDIO is not really special in this regard, other discoverable buses
(like USB, SDIO, ULPI) have the very same issue. Having a standardized
binding approach where the resources are declared as part as the child child
is preferable in my opinion.

> 
> Whether we need additional code to just to make the DT look prettier is
> questionable and if required can come as a separate patch.

Unfortunately not, once it is merged it can't be changed anymore.

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Florian Fainelli]

On 04/25/2017 09:22 AM, Lars-Peter Clausen wrote:
> On 04/24/2017 11:04 AM, Roger Quadros wrote:
>> On 24/04/17 02:35, Andrew Lunn wrote:
>>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>>>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>>>>> new file mode 100644
>>>>> index 0000000..4ffbbac
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>>>> @@ -0,0 +1,33 @@
>>>>> +Common MDIO bus properties.
>>>>> +
>>>>> +These are generic properties that can apply to any MDIO bus.
>>>>> +
>>>>> +Optional properties:
>>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>>>> +  of the PHYs on that MDIO bus.
>>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>>>> +
>>>>> +A list of child nodes, one per device on the bus is expected. These
>>>>> +should follow the generic phy.txt, or a device specific binding document.
>>>>> +
>>>>> +Example :
>>>>> +This example shows these optional properties, plus other properties
>>>>> +required for the TI Davinci MDIO driver.
>>>>> +
>>>>> +	davinci_mdio: ethernet@0x5c030000 {
>>>>> +		compatible = "ti,davinci_mdio";
>>>>> +		reg = <0x5c030000 0x1000>;
>>>>> +		#address-cells = <1>;
>>>>> +		#size-cells = <0>;
>>>>> +
>>>>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>>>> +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
>>>>
>>>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>>>> node rather than of the MDIO controller node (which might have a reset on
>>>> its own)?
>>>>> +
>>>>> +		ethphy0: ethernet-phy@1 {
>>>>> +			reg = <1>;
>>>>> +		};
>>>>> +
>>>>> +		ethphy1: ethernet-phy@3 {
>>>>> +			reg = <3>;
>>>>> +		};
>>>
>>> Hi Lars-Peter
>>>
>>> We discussed this when the first proposal was made. There are two
>>> cases, to consider.
>>>
>>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
>>> example, two PHYs.
>>>
>>> 2) There is one GPIO line per PHY. That is a separate case, and as you
>>> say, the reset line should probably be considered a PHY property, not
>>> an MDIO property. However, it can be messy, since in order to probe
>>> the MDIO bus, you probably need to take the PHY out of reset.
>>>
> 
> But the DT binding documentation says something else "List of one or more
> GPIOs that control the RESET lines of the PHYs on that MDIO bus".

I agree, it should be defined more strictly as:

"One GPIO that controls the reset line of *all* PHYs populated on that
MDIO bus"

If there are separate lines, these automatically become properties of
the PHY nodes.

> 
>>> Anyway, this patch addresses the first case, so should be accepted. If
>>> anybody wants to address the second case, they are free to do so.
> 
> I think we all know that that's not going to happen. Once there is a working
> kludge there is no incentive to do a proper implementation anymore.
> 
> 
>> Thanks for the explanation Andrew.
>>
>> For the second case, even if the RESET GPIO property is specified
>> in the PHY node, the RESET *will* have to be done by the MDIO bus driver
>> else the PHY might not be probed at all.
> 
> I'm not arguing with that, just that the hardware description should be
> truthful to the hardware topology and not to the software topology, i.e. the
> implementation details of the Linux kernel in this case. Reset GPIOs are not
> the only resource that is connected to the PHY that needs to be enabled
> before they can be enumerated. E.g. clocks and regulators fall into the same
> realm. And while you might argue that with a on-SoC phy controller node
> there wont be any conflicts in regard to the reset-gpios property, this not
> so very true for the clocks property.

Agreed, but with the exception of the unfortunate choice of words here
(single vs. multiple) there is not a really a divergence in how the
shared reset line is represented compared to other similar control
busses, is there?

> 
> And MDIO is not really special in this regard, other discoverable buses
> (like USB, SDIO, ULPI) have the very same issue. Having a standardized
> binding approach where the resources are declared as part as the child child
> is preferable in my opinion.
> 
>>
>> Whether we need additional code to just to make the DT look prettier is
>> questionable and if required can come as a separate patch.
> 
> Unfortunately not, once it is merged it can't be changed anymore.

There are no in tree users yet, so let's get the different things fixed
right now.
-- 
Florian

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

On 25/04/17 19:22, Lars-Peter Clausen wrote:
> On 04/24/2017 11:04 AM, Roger Quadros wrote:
>> On 24/04/17 02:35, Andrew Lunn wrote:
>>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>>>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>>>>> new file mode 100644
>>>>> index 0000000..4ffbbac
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>>>> @@ -0,0 +1,33 @@
>>>>> +Common MDIO bus properties.
>>>>> +
>>>>> +These are generic properties that can apply to any MDIO bus.
>>>>> +
>>>>> +Optional properties:
>>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>>>> +  of the PHYs on that MDIO bus.
>>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>>>> +
>>>>> +A list of child nodes, one per device on the bus is expected. These
>>>>> +should follow the generic phy.txt, or a device specific binding document.
>>>>> +
>>>>> +Example :
>>>>> +This example shows these optional properties, plus other properties
>>>>> +required for the TI Davinci MDIO driver.
>>>>> +
>>>>> +	davinci_mdio: ethernet@0x5c030000 {
>>>>> +		compatible = "ti,davinci_mdio";
>>>>> +		reg = <0x5c030000 0x1000>;
>>>>> +		#address-cells = <1>;
>>>>> +		#size-cells = <0>;
>>>>> +
>>>>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>>>> +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
>>>>
>>>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>>>> node rather than of the MDIO controller node (which might have a reset on
>>>> its own)?
>>>>> +
>>>>> +		ethphy0: ethernet-phy@1 {
>>>>> +			reg = <1>;
>>>>> +		};
>>>>> +
>>>>> +		ethphy1: ethernet-phy@3 {
>>>>> +			reg = <3>;
>>>>> +		};
>>>
>>> Hi Lars-Peter
>>>
>>> We discussed this when the first proposal was made. There are two
>>> cases, to consider.
>>>
>>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
>>> example, two PHYs.
>>>
>>> 2) There is one GPIO line per PHY. That is a separate case, and as you
>>> say, the reset line should probably be considered a PHY property, not
>>> an MDIO property. However, it can be messy, since in order to probe
>>> the MDIO bus, you probably need to take the PHY out of reset.
>>>
> 
> But the DT binding documentation says something else "List of one or more
> GPIOs that control the RESET lines of the PHYs on that MDIO bus".
> 
>>> Anyway, this patch addresses the first case, so should be accepted. If
>>> anybody wants to address the second case, they are free to do so.
> 
> I think we all know that that's not going to happen. Once there is a working
> kludge there is no incentive to do a proper implementation anymore.
> 
> 
>> Thanks for the explanation Andrew.
>>
>> For the second case, even if the RESET GPIO property is specified
>> in the PHY node, the RESET *will* have to be done by the MDIO bus driver
>> else the PHY might not be probed at all.
> 
> I'm not arguing with that, just that the hardware description should be
> truthful to the hardware topology and not to the software topology, i.e. the
> implementation details of the Linux kernel in this case. Reset GPIOs are not
> the only resource that is connected to the PHY that needs to be enabled
> before they can be enumerated. E.g. clocks and regulators fall into the same
> realm. And while you might argue that with a on-SoC phy controller node
> there wont be any conflicts in regard to the reset-gpios property, this not
> so very true for the clocks property.
> 
> And MDIO is not really special in this regard, other discoverable buses
> (like USB, SDIO, ULPI) have the very same issue. Having a standardized
> binding approach where the resources are declared as part as the child child
> is preferable in my opinion.

Good point. I agree now that if PHYs have individual RESET lines, they
should be part of the PHY node.
> 
>>
>> Whether we need additional code to just to make the DT look prettier is
>> questionable and if required can come as a separate patch.
> 
> Unfortunately not, once it is merged it can't be changed anymore.
> 

cheers,
-roger

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Roger Quadros]

On 25/04/17 19:31, Florian Fainelli wrote:
> On 04/25/2017 09:22 AM, Lars-Peter Clausen wrote:
>> On 04/24/2017 11:04 AM, Roger Quadros wrote:
>>> On 24/04/17 02:35, Andrew Lunn wrote:
>>>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>>>>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..4ffbbac
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>>>>> @@ -0,0 +1,33 @@
>>>>>> +Common MDIO bus properties.
>>>>>> +
>>>>>> +These are generic properties that can apply to any MDIO bus.
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>>>>> +  of the PHYs on that MDIO bus.
>>>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>>>>> +
>>>>>> +A list of child nodes, one per device on the bus is expected. These
>>>>>> +should follow the generic phy.txt, or a device specific binding document.
>>>>>> +
>>>>>> +Example :
>>>>>> +This example shows these optional properties, plus other properties
>>>>>> +required for the TI Davinci MDIO driver.
>>>>>> +
>>>>>> +	davinci_mdio: ethernet@0x5c030000 {
>>>>>> +		compatible = "ti,davinci_mdio";
>>>>>> +		reg = <0x5c030000 0x1000>;
>>>>>> +		#address-cells = <1>;
>>>>>> +		#size-cells = <0>;
>>>>>> +
>>>>>> +		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>>>>> +		reset-delay-us = <2>;   /* PHY datasheet states 1us min */
>>>>>
>>>>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>>>>> node rather than of the MDIO controller node (which might have a reset on
>>>>> its own)?
>>>>>> +
>>>>>> +		ethphy0: ethernet-phy@1 {
>>>>>> +			reg = <1>;
>>>>>> +		};
>>>>>> +
>>>>>> +		ethphy1: ethernet-phy@3 {
>>>>>> +			reg = <3>;
>>>>>> +		};
>>>>
>>>> Hi Lars-Peter
>>>>
>>>> We discussed this when the first proposal was made. There are two
>>>> cases, to consider.
>>>>
>>>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
>>>> example, two PHYs.
>>>>
>>>> 2) There is one GPIO line per PHY. That is a separate case, and as you
>>>> say, the reset line should probably be considered a PHY property, not
>>>> an MDIO property. However, it can be messy, since in order to probe
>>>> the MDIO bus, you probably need to take the PHY out of reset.
>>>>
>>
>> But the DT binding documentation says something else "List of one or more
>> GPIOs that control the RESET lines of the PHYs on that MDIO bus".
> 
> I agree, it should be defined more strictly as:
> 
> "One GPIO that controls the reset line of *all* PHYs populated on that
> MDIO bus"

Patch is already in net-next. How can we get this fixed? Should I send a v5?

> 
> If there are separate lines, these automatically become properties of
> the PHY nodes.
> 
>>
>>>> Anyway, this patch addresses the first case, so should be accepted. If
>>>> anybody wants to address the second case, they are free to do so.
>>
>> I think we all know that that's not going to happen. Once there is a working
>> kludge there is no incentive to do a proper implementation anymore.
>>
>>
>>> Thanks for the explanation Andrew.
>>>
>>> For the second case, even if the RESET GPIO property is specified
>>> in the PHY node, the RESET *will* have to be done by the MDIO bus driver
>>> else the PHY might not be probed at all.
>>
>> I'm not arguing with that, just that the hardware description should be
>> truthful to the hardware topology and not to the software topology, i.e. the
>> implementation details of the Linux kernel in this case. Reset GPIOs are not
>> the only resource that is connected to the PHY that needs to be enabled
>> before they can be enumerated. E.g. clocks and regulators fall into the same
>> realm. And while you might argue that with a on-SoC phy controller node
>> there wont be any conflicts in regard to the reset-gpios property, this not
>> so very true for the clocks property.
> 
> Agreed, but with the exception of the unfortunate choice of words here
> (single vs. multiple) there is not a really a divergence in how the
> shared reset line is represented compared to other similar control
> busses, is there?
> 
>>
>> And MDIO is not really special in this regard, other discoverable buses
>> (like USB, SDIO, ULPI) have the very same issue. Having a standardized
>> binding approach where the resources are declared as part as the child child
>> is preferable in my opinion.
>>
>>>
>>> Whether we need additional code to just to make the DT look prettier is
>>> questionable and if required can come as a separate patch.
>>
>> Unfortunately not, once it is merged it can't be changed anymore.
> 
> There are no in tree users yet, so let's get the different things fixed
> right now.
> 

-- 
cheers,
-roger

Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.

[Andrew Lunn]

> Patch is already in net-next. How can we get this fixed? Should I send a v5?

Hi Roger

See the followup patch Florian submitted. Send your Reviewed-by: if
you agree with it.

      Andrew