lkml.org 
[lkml]   [2022]   [Aug]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()
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>

\
 
 \ /
  Last update: 2022-08-06 00:56    [W:0.140 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site