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

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

[Bruno Thomsen]

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(-)

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);
 	}
 
 	if (bus->reset) {

base-commit: 92ed301919932f777713b9172e525674157e983d
-- 
2.26.2

[PATCH 2/2] dt-bindings: net: mdio: update reset-delay-us description

[Bruno Thomsen]

Property is now used for both assert and deassert delay
so PHYs can be reset before type id auto detection.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
 Documentation/devicetree/bindings/net/mdio.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
index d6a3bf8550eb..8385960e3b3e 100644
--- a/Documentation/devicetree/bindings/net/mdio.yaml
+++ b/Documentation/devicetree/bindings/net/mdio.yaml
@@ -38,6 +38,8 @@ properties:
       RESET pulse width in microseconds. It applies to all MDIO devices
       and must therefore be appropriately determined based on all devices
       requirements (maximum value of all per-device RESET pulse widths).
+      Additional it also provides a reset deassert delay of the same width
+      to ensure devices are ready for communication.
 
   clock-frequency:
     description:
-- 
2.26.2

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

[Fabio Estevam]

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?

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

[Bruno Thomsen]

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

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

[Andrew Lunn]

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

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

[Andrew Lunn]

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