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 
>>> 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 
 Signed-off-by: Sekhar Nori 
>>>
>>> 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

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 
>>> 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 
 Signed-off-by: Sekhar Nori 
>>>
>>> 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

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 
>> 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 
>>> Signed-off-by: Sekhar Nori 
>>
>> 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

[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 
>> 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 
>>> Signed-off-by: Sekhar Nori 
>>
>> 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

[Andrew Lunn]

On Sat, Apr 08, 2017 at 06:55:45AM -0700, David Miller wrote:
> From: Roger Quadros 
> 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 
> > Signed-off-by: Sekhar Nori 
> 
> 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

[Andrew Lunn]

On Sat, Apr 08, 2017 at 06:55:45AM -0700, David Miller wrote:
> From: Roger Quadros 
> 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 
> > Signed-off-by: Sekhar Nori 
> 
> 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

[David Miller]

From: Roger Quadros 
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 
> Signed-off-by: Sekhar Nori 

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

[David Miller]

From: Roger Quadros 
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 
> Signed-off-by: Sekhar Nori 

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

[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

[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

[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

[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]

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 
>> Signed-off-by: Sekhar Nori 
>> ---
>>  .../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 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>>  
>>  /*
>>   * This timeout definition is a worst-case ultra defensive measure against
>> @@ -53,6 +56,8 @@
>>  
>>  #define DEF_OUT_FREQ220 /* 2.2 MHz */
>>  
>> +#define DEFAULT_GPIO_RESET_DELAY10  /* in microseconds */
>> +
>>  struct davinci_mdio_of_param {
>>  int autosuspend_delay_ms;
>>  };
>> @@ -104,6 +109,9 @@ struct davinci_mdio_data {
>>   */
>>  boolskip_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, >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

[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 
>> Signed-off-by: Sekhar Nori 
>> ---
>>  .../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 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>>  
>>  /*
>>   * This timeout definition is a worst-case ultra defensive measure against
>> @@ -53,6 +56,8 @@
>>  
>>  #define DEF_OUT_FREQ220 /* 2.2 MHz */
>>  
>> +#define DEFAULT_GPIO_RESET_DELAY10  /* in microseconds */
>> +
>>  struct davinci_mdio_of_param {
>>  int autosuspend_delay_ms;
>>  };
>> @@ -104,6 +109,9 @@ struct davinci_mdio_data {
>>   */
>>  boolskip_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, >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]

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 
> Signed-off-by: Sekhar Nori 
> ---
>  .../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 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  
>  /*
>   * This timeout definition is a worst-case ultra defensive measure against
> @@ -53,6 +56,8 @@
>  
>  #define DEF_OUT_FREQ 220 /* 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 {
>*/
>   boolskip_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, >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

[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 
> Signed-off-by: Sekhar Nori 
> ---
>  .../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 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  
>  /*
>   * This timeout definition is a worst-case ultra defensive measure against
> @@ -53,6 +56,8 @@
>  
>  #define DEF_OUT_FREQ 220 /* 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 {
>*/
>   boolskip_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, >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