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@xxxxxxxxxxxx> 
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@xxxxxxxxxxxx>
> > > ---
> > > 
> > >  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@xxxxxxxxxxxxxxx/
> > 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@xxxxxxxxxxxxxxx/

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

Reviewed-By: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>