[1/2] net: mdiobus: reset deassert delay

The current reset logic only has a delay during assert.
This reuses the delay value as deassert delay to ensure
PHYs are ready for commands. Delays are typically needed
when external hardware slows down reset release with a
RC network. This solution does not need any new device
tree bindings.
It also improves handling of long delays (>20ms) by using
the generic fsleep() for selecting appropriate delay
function.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
 drivers/net/phy/mdio_bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 92ed301919932f777713b9172e525674157e983d

Hi Bruno,

On Tue, Jul 28, 2020 at 6:02 AM Bruno Thomsen <bruno.thomsen@gmail.com> wrote:
>
> The current reset logic only has a delay during assert.
> This reuses the delay value as deassert delay to ensure
> PHYs are ready for commands. Delays are typically needed
> when external hardware slows down reset release with a
> RC network. This solution does not need any new device
> tree bindings.
> It also improves handling of long delays (>20ms) by using
> the generic fsleep() for selecting appropriate delay
> function.

It seems that this patch should be split in two:

One that changes from udelay() to sleep()
and another one that adds the delay after the reset line is de-asserted.

>
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
> ---
>  drivers/net/phy/mdio_bus.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 6ceee82b2839..84d5ab07fe16 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -627,8 +627,9 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>                 bus->reset_gpiod = gpiod;
>
>                 gpiod_set_value_cansleep(gpiod, 1);
> -               udelay(bus->reset_delay_us);
> +               fsleep(bus->reset_delay_us);
>                 gpiod_set_value_cansleep(gpiod, 0);
> +               fsleep(bus->reset_delay_us);

Shouldn't it use the value passed in the reset-deassert-us property instead?

Hi Fabio

Den tir. 28. jul. 2020 kl. 14.32 skrev Fabio Estevam <festevam@gmail.com>:
> On Tue, Jul 28, 2020 at 6:02 AM Bruno Thomsen <bruno.thomsen@gmail.com> wrote:
> >
> > The current reset logic only has a delay during assert.
> > This reuses the delay value as deassert delay to ensure
> > PHYs are ready for commands. Delays are typically needed
> > when external hardware slows down reset release with a
> > RC network. This solution does not need any new device
> > tree bindings.
> > It also improves handling of long delays (>20ms) by using
> > the generic fsleep() for selecting appropriate delay
> > function.
>
> It seems that this patch should be split in two:
>
> One that changes from udelay() to sleep()
> and another one that adds the delay after the reset line is de-asserted.

Okay, I will split it in version 2.

> >  drivers/net/phy/mdio_bus.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index 6ceee82b2839..84d5ab07fe16 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -627,8 +627,9 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> >                 bus->reset_gpiod = gpiod;
> >
> >                 gpiod_set_value_cansleep(gpiod, 1);
> > -               udelay(bus->reset_delay_us);
> > +               fsleep(bus->reset_delay_us);
> >                 gpiod_set_value_cansleep(gpiod, 0);
> > +               fsleep(bus->reset_delay_us);
>
> Shouldn't it use the value passed in the reset-deassert-us property instead?

This place does not use the per PHY reset properties as they are only used
after auto type ID detection. When you need a reset before auto type
ID detection,
it can only be done using the MDIO reset, and it only has
"reset-delay-us" property.
It will be a rather big change if all PHY nodes can choose if reset
should happen
before or after type ID detection.

/Bruno

On Tue, Jul 28, 2020 at 11:02:02AM +0200, Bruno Thomsen wrote:
> The current reset logic only has a delay during assert.
> This reuses the delay value as deassert delay to ensure
> PHYs are ready for commands. Delays are typically needed
> when external hardware slows down reset release with a
> RC network. This solution does not need any new device
> tree bindings.
> It also improves handling of long delays (>20ms) by using
> the generic fsleep() for selecting appropriate delay
> function.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Hi Bruno

In netdev, it is normal to include a [patch 0/2] for a patchset which
explains the big picture. Also, please use [patch net-next ..] to
indicate this is for the net-next tree. If you think these are fixes
for stable, please base them on net, and use [patch net ..], and
include Fixes: tags.

    Andrew

On Tue, Jul 28, 2020 at 09:32:03AM -0300, Fabio Estevam wrote:

> > Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
> > ---
> >  drivers/net/phy/mdio_bus.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index 6ceee82b2839..84d5ab07fe16 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -627,8 +627,9 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> >                 bus->reset_gpiod = gpiod;
> >
> >                 gpiod_set_value_cansleep(gpiod, 1);
> > -               udelay(bus->reset_delay_us);
> > +               fsleep(bus->reset_delay_us);
> >                 gpiod_set_value_cansleep(gpiod, 0);
> > +               fsleep(bus->reset_delay_us);
> 

> Shouldn't it use the value passed in the reset-deassert-us property
> instead?

Hi Fabio

As Bruno pointed out, that property is not relevant here. But i agree
with you in principle. Bruno, please add a new optional property for
the delay after releasing the reset.

     Andrew