| Message ID | 20220426123329.775-2-du@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: usb251xb: configurable reset delay | expand |
On Tue, Apr 26, 2022 at 12:33:47PM +0000, Daniels Umanovskis wrote: > Next patch implements support for this property > > Signed-off-by: Daniels Umanovskis <du@axentia.se> LGTM. Reviewed-by: Richard Leitner <richard.leitner@skidata.com> > --- > Documentation/devicetree/bindings/usb/usb251xb.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt > index 1a934eab175e..d95c8ae518e7 100644 > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt > @@ -12,6 +12,8 @@ Required properties : > > Optional properties : > - reset-gpios : Should specify the gpio for hub reset > + - reset-delay-us: Specifies delay in microseconds after reset deassert > + on hub power-up. (32 bit, default is 500us) > - vdd-supply : Should specify the phandle to the regulator supplying vdd > - skip-config : Skip Hub configuration, but only send the USB-Attach command > - vendor-id : Set USB Vendor ID of the hub (16 bit, default is 0x0424) > -- > 2.30.2 >
On Tue, Apr 26, 2022 at 12:33:47PM +0000, Daniels Umanovskis wrote: > Next patch implements support for this property Not a great reason why you need this. This patch should stand on its own. My first question is whether this is board specific? If the default or what the reference manual says is 500us, but you have a case needing 600us, why not just change the driver. I don't think this really needs tuning to each board unless the delay becomes noticeable. > > Signed-off-by: Daniels Umanovskis <du@axentia.se> > --- > Documentation/devicetree/bindings/usb/usb251xb.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt > index 1a934eab175e..d95c8ae518e7 100644 > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt > @@ -12,6 +12,8 @@ Required properties : > > Optional properties : > - reset-gpios : Should specify the gpio for hub reset > + - reset-delay-us: Specifies delay in microseconds after reset deassert > + on hub power-up. (32 bit, default is 500us) > - vdd-supply : Should specify the phandle to the regulator supplying vdd > - skip-config : Skip Hub configuration, but only send the USB-Attach command > - vendor-id : Set USB Vendor ID of the hub (16 bit, default is 0x0424) > -- > 2.30.2 > >
On 5/3/22 2:20 AM, Rob Herring wrote: > My first question is whether this is board specific? If the default or > what the reference manual says is 500us, but you have a case needing > 600us, why not just change the driver. I don't think this really needs > tuning to each board unless the delay becomes noticeable. This isn't a board specific issue. I detected the issue on a board we've been using for a long time, it's due to a specific batch of USB2512 hubs. We've had several batches of these hubs that were fine, but more recently received a batch produced in late 2021 and the hubs in this batch do not become responsive to I2C within the expected 500us. I arrived at that by using an oscilloscope to observe how soon after deasserting the reset signal the USB hub is able to respond to I2C. Most of the 2512s we have do that within 500us, the latest batch doesn't.
On Tue, May 03, 2022 at 09:49:36AM +0200, Daniels Umanovskis wrote: > On 5/3/22 2:20 AM, Rob Herring wrote: > > My first question is whether this is board specific? If the default or > > what the reference manual says is 500us, but you have a case needing > > 600us, why not just change the driver. I don't think this really needs > > tuning to each board unless the delay becomes noticeable. > > This isn't a board specific issue. I detected the issue on a board we've > been using for a long time, it's due to a specific batch of USB2512 hubs. > We've had several batches of these hubs that were fine, but more recently > received a batch produced in late 2021 and the hubs in this batch do not > become responsive to I2C within the expected 500us. What's the maximum timeout you've observed? I guess it would be the simpler and "better" approach to just increase the timeout in the driver (if it's not too much above the 500µs). > > I arrived at that by using an oscilloscope to observe how soon after > deasserting the reset signal the USB hub is able to respond to I2C. Most of > the 2512s we have do that within 500us, the latest batch doesn't. > regards;rl
On 5/3/22 11:21 AM, Richard Leitner - SKIDATA wrote: > What's the maximum timeout you've observed? > > I guess it would be the simpler and "better" approach to just increase > the timeout in the driver (if it's not too much above the 500µs). I saw 800-820 us at most, and my initial fix internally was just to increase the sleep duration in the driver. But it's an increase of over 50% and I don't feel it makes sense to change the driver's behavior for thousands of users with properly working chips, hence the configurable timeout for out-of-spec batches like the one we had here. I expect more users to run across such batches in the coming months. In an ideal world, we'd just trash these hubs that should have surely failed factory QA, but with today's component shortage that's an unimaginable luxury... /Daniels
On Tue, May 03, 2022 at 11:41:17AM +0200, Daniels Umanovskis wrote: > On 5/3/22 11:21 AM, Richard Leitner - SKIDATA wrote: > > What's the maximum timeout you've observed? > > > > I guess it would be the simpler and "better" approach to just increase > > the timeout in the driver (if it's not too much above the 500µs). > > I saw 800-820 us at most, and my initial fix internally was just to increase > the sleep duration in the driver. But it's an increase of over 50% and I > don't feel it makes sense to change the driver's behavior for thousands of > users with properly working chips, hence the configurable timeout for > out-of-spec batches like the one we had here. I expect more users to run > across such batches in the coming months. > > In an ideal world, we'd just trash these hubs that should have surely failed > factory QA, but with today's component shortage that's an unimaginable > luxury... The only solution that works here is increase the timeout in the driver. Are you going to tweak the dtb based on what batch the chip is from? No, that's not possible. Having worked in a chip company, I can tell you how they would fix it. Better testing? No, they'd change the documentation. Rob
diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt index 1a934eab175e..d95c8ae518e7 100644 --- a/Documentation/devicetree/bindings/usb/usb251xb.txt +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt @@ -12,6 +12,8 @@ Required properties : Optional properties : - reset-gpios : Should specify the gpio for hub reset + - reset-delay-us: Specifies delay in microseconds after reset deassert + on hub power-up. (32 bit, default is 500us) - vdd-supply : Should specify the phandle to the regulator supplying vdd - skip-config : Skip Hub configuration, but only send the USB-Attach command - vendor-id : Set USB Vendor ID of the hub (16 bit, default is 0x0424)
Next patch implements support for this property Signed-off-by: Daniels Umanovskis <du@axentia.se> --- Documentation/devicetree/bindings/usb/usb251xb.txt | 2 ++ 1 file changed, 2 insertions(+)