Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 5 Aug 2022 12:56:34 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Doug Anderson <dianders@chromium.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alexander Stein <alexander.stein@ew.tq-group.com>,
        Linux USB List <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in
 onboard_hub_power_off()
Message-ID: <Yu118qfzoYcHgJs3@google.com>
References: <20220805111836.1.Id5a4dc0a2c046236116693aa55672295513a0f2a@changeid>
 <CAD=FV=W9VWbvWqdEEY9=OnNSsAnQ+CgQPRifbAu2ixrgPQd54A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAD=FV=W9VWbvWqdEEY9=OnNSsAnQ+CgQPRifbAu2ixrgPQd54A@mail.gmail.com>
Precedence: bulk

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/