| Message ID | 20220805111836.1.Id5a4dc0a2c046236116693aa55672295513a0f2a@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1bcafc0498038a5a2ce5a9b888c39f1c32242cec |
| Headers | show |
| Series | usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off() | expand |
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...
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/
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>
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>
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) {
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(-)