[PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()

[PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()

[Matthias Kaehlcke]

onboard_hub_power_off() currently has a delay after asserting the
reset of the hub. There is already a delay in onboard_hub_power_on()
before de-asserting the reset, which ensures that the reset is
asserted for the required time, so the delay in _power_off() is not
needed.

Skip the reset GPIO check before calling gpiod_set_value_cansleep(),
the function returns early when the GPIO descriptor is NULL.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---

 drivers/usb/misc/onboard_usb_hub.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index d1df153e7f5a..d63c63942af1 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -71,10 +71,7 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
 {
 	int err;
 
-	if (hub->reset_gpio) {
-		gpiod_set_value_cansleep(hub->reset_gpio, 1);
-		fsleep(hub->pdata->reset_us);
-	}
+	gpiod_set_value_cansleep(hub->reset_gpio, 1);
 
 	err = regulator_disable(hub->vdd);
 	if (err) {
-- 
2.37.1.559.g78731f0fdb-goog

Re: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()

[Doug Anderson]

Hi,

On Fri, Aug 5, 2022 at 11:19 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> onboard_hub_power_off() currently has a delay after asserting the
> reset of the hub. There is already a delay in onboard_hub_power_on()
> before de-asserting the reset, which ensures that the reset is
> asserted for the required time, so the delay in _power_off() is not
> needed.
>
> Skip the reset GPIO check before calling gpiod_set_value_cansleep(),
> the function returns early when the GPIO descriptor is NULL.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>
>  drivers/usb/misc/onboard_usb_hub.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

I was trying to figure out what this "reset" was defined to do and I
looked for the device tree bindings. They don't seem to exist. Was
that an oversight?

In any case, I'm not convinced that your patch is correct. Timing
diagrams often show a needed delay between adjusting a reset GPIO and
turning on/off the power. The timing diagrams can sometimes show a
required delay on both sides. I guess at the moment the only user of
this reset GPIO has a symmetric delay, but I can totally expect that
someone could come along and say that they needed 10 ms on one side
and 1 ms on the other side...

Re: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()

[Matthias Kaehlcke]

Hi Doug,

On Fri, Aug 05, 2022 at 12:26:35PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Aug 5, 2022 at 11:19 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > onboard_hub_power_off() currently has a delay after asserting the
> > reset of the hub. There is already a delay in onboard_hub_power_on()
> > before de-asserting the reset, which ensures that the reset is
> > asserted for the required time, so the delay in _power_off() is not
> > needed.
> >
> > Skip the reset GPIO check before calling gpiod_set_value_cansleep(),
> > the function returns early when the GPIO descriptor is NULL.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >
> >  drivers/usb/misc/onboard_usb_hub.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> I was trying to figure out what this "reset" was defined to do and I
> looked for the device tree bindings. They don't seem to exist. Was
> that an oversight?

It's not in the binding of the RTS5411 which I guess you looked at,
because that hub doesn't have a reset line.

The reset functionality was initially added for the TI USB8041, the
binding has the reset, but I found it hasn't landed yet:

https://patchwork.kernel.org/project/linux-usb/patch/20220727093801.687361-1-alexander.stein@ew.tq-group.com/

> In any case, I'm not convinced that your patch is correct. Timing
> diagrams often show a needed delay between adjusting a reset GPIO and
> turning on/off the power. The timing diagrams can sometimes show a
> required delay on both sides. I guess at the moment the only user of
> this reset GPIO has a symmetric delay, but I can totally expect that
> someone could come along and say that they needed 10 ms on one side
> and 1 ms on the other side...

As of now none of the supported hubs (there are only two of them) has
an asymmetric delay. The RTS5411 doesn't have a reset line, and the
TI USB8041 only specifies a power on delay (in my interpretation).

[1] has some discussion between Alexander and me about this second
reset. The patch that added the delay was merged before this
discussion concluded.

If the driver is going to support a hub that needs an additional
reset delay when the hub is powered off I'm totally in favor of
adding that delay, however that isn't currently the case in my
understanding. If you draw a different conclusion from the TI
USB8041 datasheet please let me know.

[1] https://patchwork.kernel.org/project/linux-usb/patch/20220727141117.909361-1-alexander.stein@ew.tq-group.com/

Re: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()

[Doug Anderson]

Hi,

On Fri, Aug 5, 2022 at 12:56 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Doug,
>
> On Fri, Aug 05, 2022 at 12:26:35PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Aug 5, 2022 at 11:19 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > onboard_hub_power_off() currently has a delay after asserting the
> > > reset of the hub. There is already a delay in onboard_hub_power_on()
> > > before de-asserting the reset, which ensures that the reset is
> > > asserted for the required time, so the delay in _power_off() is not
> > > needed.
> > >
> > > Skip the reset GPIO check before calling gpiod_set_value_cansleep(),
> > > the function returns early when the GPIO descriptor is NULL.
> > >
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >
> > >  drivers/usb/misc/onboard_usb_hub.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > I was trying to figure out what this "reset" was defined to do and I
> > looked for the device tree bindings. They don't seem to exist. Was
> > that an oversight?
>
> It's not in the binding of the RTS5411 which I guess you looked at,
> because that hub doesn't have a reset line.
>
> The reset functionality was initially added for the TI USB8041, the
> binding has the reset, but I found it hasn't landed yet:
>
> https://patchwork.kernel.org/project/linux-usb/patch/20220727093801.687361-1-alexander.stein@ew.tq-group.com/

Ah, I see. I actually clicked on the "Link" for the patches that
landed and found that they landed without a bindings patch. I guess it
dropped on v4, which I see you yelled about in v3. ;-)


> > In any case, I'm not convinced that your patch is correct. Timing
> > diagrams often show a needed delay between adjusting a reset GPIO and
> > turning on/off the power. The timing diagrams can sometimes show a
> > required delay on both sides. I guess at the moment the only user of
> > this reset GPIO has a symmetric delay, but I can totally expect that
> > someone could come along and say that they needed 10 ms on one side
> > and 1 ms on the other side...
>
> As of now none of the supported hubs (there are only two of them) has
> an asymmetric delay. The RTS5411 doesn't have a reset line, and the
> TI USB8041 only specifies a power on delay (in my interpretation).
>
> [1] has some discussion between Alexander and me about this second
> reset. The patch that added the delay was merged before this
> discussion concluded.
>
> If the driver is going to support a hub that needs an additional
> reset delay when the hub is powered off I'm totally in favor of
> adding that delay, however that isn't currently the case in my
> understanding. If you draw a different conclusion from the TI
> USB8041 datasheet please let me know.
>
> [1] https://patchwork.kernel.org/project/linux-usb/patch/20220727141117.909361-1-alexander.stein@ew.tq-group.com/

Ah, OK. With this context:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: Re: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()

[Alexander Stein]

Am Freitag, 5. August 2022, 21:56:34 CEST schrieb Matthias Kaehlcke:
> Hi Doug,
> 
> On Fri, Aug 05, 2022 at 12:26:35PM -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Fri, Aug 5, 2022 at 11:19 AM Matthias Kaehlcke <mka@chromium.org> 
wrote:
> > > onboard_hub_power_off() currently has a delay after asserting the
> > > reset of the hub. There is already a delay in onboard_hub_power_on()
> > > before de-asserting the reset, which ensures that the reset is
> > > asserted for the required time, so the delay in _power_off() is not
> > > needed.
> > > 
> > > Skip the reset GPIO check before calling gpiod_set_value_cansleep(),
> > > the function returns early when the GPIO descriptor is NULL.
> > > 
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > > 
> > >  drivers/usb/misc/onboard_usb_hub.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > I was trying to figure out what this "reset" was defined to do and I
> > looked for the device tree bindings. They don't seem to exist. Was
> > that an oversight?
> 
> It's not in the binding of the RTS5411 which I guess you looked at,
> because that hub doesn't have a reset line.
> 
> The reset functionality was initially added for the TI USB8041, the
> binding has the reset, but I found it hasn't landed yet:
> 
> https://patchwork.kernel.org/project/linux-usb/patch/20220727093801.687361-1
> -alexander.stein@ew.tq-group.com/
> > In any case, I'm not convinced that your patch is correct. Timing
> > diagrams often show a needed delay between adjusting a reset GPIO and
> > turning on/off the power. The timing diagrams can sometimes show a
> > required delay on both sides. I guess at the moment the only user of
> > this reset GPIO has a symmetric delay, but I can totally expect that
> > someone could come along and say that they needed 10 ms on one side
> > and 1 ms on the other side...
> 
> As of now none of the supported hubs (there are only two of them) has
> an asymmetric delay. The RTS5411 doesn't have a reset line, and the
> TI USB8041 only specifies a power on delay (in my interpretation).
> 
> [1] has some discussion between Alexander and me about this second
> reset. The patch that added the delay was merged before this
> discussion concluded.
> 
> If the driver is going to support a hub that needs an additional
> reset delay when the hub is powered off I'm totally in favor of
> adding that delay, however that isn't currently the case in my
> understanding. If you draw a different conclusion from the TI
> USB8041 datasheet please let me know.
> 
> [1]
> https://patchwork.kernel.org/project/linux-usb/patch/20220727141117.909361-> 1-alexander.stein@ew.tq-group.com/

I'm OK with removing the delay. As mentioned, it can be added if needed for 
some reason.

Reviewed-By: Alexander Stein <alexander.stein@ew.tq-group.com>