[PATCH 0/2] usb: usb251xb: configurable reset delay

[PATCH 0/2] usb: usb251xb: configurable reset delay

[Daniels Umanovskis]

The Microchip USB251xB hub, according to its datasheet, is operational
500us after deasserting reset on startup and can then be attached or
configured.

I ran into a batch of such hubs, of the 2512Bi variant,
that didn't appear to work or only worked sporadically. Debugging the
issue revealed that these specific hubs do not typically manage to
reset within 500us. Instead they become operational 800us
or so after deasserting the RESET_N signal.

This is probably a faulty batch of the chips but making the reset delay
configurable through dt bindings allows these chips to be used.

Daniels Umanovskis (2):
  dt-bindings: usb: usb251xb: add documentation for reset-delay-us
  usb: usb251xb: make power-up reset delay configurable in device tree

 Documentation/devicetree/bindings/usb/usb251xb.txt | 2 ++
 drivers/usb/misc/usb251xb.c                        | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.30.2

[PATCH 1/2] dt-bindings: usb: usb251xb: add documentation for reset-delay-us

[Daniels Umanovskis]

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(+)

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

[PATCH 2/2] usb: usb251xb: make power-up reset delay configurable in device tree

[Daniels Umanovskis]

According to the datasheet, the hub should be operational 500us after
the reset has been deasserted. Some individual circuits have been
observed not to reset within the specified 500us and require a longer
wait for subsequent configuration to succeed.

Signed-off-by: Daniels Umanovskis <du@axentia.se>
---
 drivers/usb/misc/usb251xb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 04c4e3fed094..e287e241ef96 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -115,6 +115,7 @@ struct usb251xb {
 	struct regulator *vdd;
 	u8 skip_config;
 	struct gpio_desc *gpio_reset;
+	u32 reset_delay_us;
 	u16 vendor_id;
 	u16 product_id;
 	u16 device_id;
@@ -278,7 +279,7 @@ static void usb251xb_reset(struct usb251xb *hub)
 	gpiod_set_value_cansleep(hub->gpio_reset, 0);
 
 	/* wait for hub recovery/stabilization */
-	usleep_range(500, 750);	/* >=500us after RESET_N deasserted */
+	fsleep(hub->reset_delay_us);
 
 	i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
 }
@@ -424,6 +425,9 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		return err;
 	}
 
+	if (of_property_read_u32(np, "reset-delay-us", &hub->reset_delay_us))
+		hub->reset_delay_us = 500;
+
 	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
 		hub->vendor_id = USB251XB_DEF_VENDOR_ID;
 
-- 
2.30.2

Re: [PATCH 2/2] usb: usb251xb: make power-up reset delay configurable in device tree

[Greg KH]

On Tue, Apr 26, 2022 at 12:34:13PM +0000, Daniels Umanovskis wrote:
> According to the datasheet, the hub should be operational 500us after
> the reset has been deasserted. Some individual circuits have been
> observed not to reset within the specified 500us and require a longer
> wait for subsequent configuration to succeed.
> 
> Signed-off-by: Daniels Umanovskis <du@axentia.se>
> ---
>  drivers/usb/misc/usb251xb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 04c4e3fed094..e287e241ef96 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -115,6 +115,7 @@ struct usb251xb {
>  	struct regulator *vdd;
>  	u8 skip_config;
>  	struct gpio_desc *gpio_reset;
> +	u32 reset_delay_us;
>  	u16 vendor_id;
>  	u16 product_id;
>  	u16 device_id;
> @@ -278,7 +279,7 @@ static void usb251xb_reset(struct usb251xb *hub)
>  	gpiod_set_value_cansleep(hub->gpio_reset, 0);
>  
>  	/* wait for hub recovery/stabilization */
> -	usleep_range(500, 750);	/* >=500us after RESET_N deasserted */
> +	fsleep(hub->reset_delay_us);
>  
>  	i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
>  }
> @@ -424,6 +425,9 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>  		return err;
>  	}
>  
> +	if (of_property_read_u32(np, "reset-delay-us", &hub->reset_delay_us))
> +		hub->reset_delay_us = 500;

So if this fails the delay is 0?

And what commit id does this fix?

And any reason why you didn't cc: the people you get when running
scripts/get_maintainer.pl on the patch?

thanks,

greg k-h

Re: [PATCH 2/2] usb: usb251xb: make power-up reset delay configurable in device tree

[Daniels Umanovskis]

On 4/26/22 2:46 PM, Greg KH wrote:
> On Tue, Apr 26, 2022 at 12:34:13PM +0000, Daniels Umanovskis wrote:
>> According to the datasheet, the hub should be operational 500us after
>> the reset has been deasserted. Some individual circuits have been
>> observed not to reset within the specified 500us and require a longer
>> wait for subsequent configuration to succeed.
>>
>> Signed-off-by: Daniels Umanovskis <du@axentia.se>
>> ---
>>   drivers/usb/misc/usb251xb.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
>> index 04c4e3fed094..e287e241ef96 100644
>> --- a/drivers/usb/misc/usb251xb.c
>> +++ b/drivers/usb/misc/usb251xb.c
>> @@ -115,6 +115,7 @@ struct usb251xb {
>>   	struct regulator *vdd;
>>   	u8 skip_config;
>>   	struct gpio_desc *gpio_reset;
>> +	u32 reset_delay_us;
>>   	u16 vendor_id;
>>   	u16 product_id;
>>   	u16 device_id;
>> @@ -278,7 +279,7 @@ static void usb251xb_reset(struct usb251xb *hub)
>>   	gpiod_set_value_cansleep(hub->gpio_reset, 0);
>>   
>>   	/* wait for hub recovery/stabilization */
>> -	usleep_range(500, 750);	/* >=500us after RESET_N deasserted */
>> +	fsleep(hub->reset_delay_us);
>>   
>>   	i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
>>   }
>> @@ -424,6 +425,9 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>>   		return err;
>>   	}
>>   
>> +	if (of_property_read_u32(np, "reset-delay-us", &hub->reset_delay_us))
>> +		hub->reset_delay_us = 500;
> So if this fails the delay is 0?
of_property_read_u32 can fail with -EINVAL or -ENODATA, then delay is 
500, unless I'm missing something more obvious.
> And what commit id does this fix?
No Fixes: tag because this isn't a bug or regression, just a new 
workaround for a hardware issue nobody's (presumably) 
encountered/bothered with.
> And any reason why you didn't cc: the people you get when running
> scripts/get_maintainer.pl on the patch?
get_maintainer showed Richard as the maintainer. You showed as a 
supporter, if supporters should also be CCed by default then it's my bad 
- perhaps that should be clarified in submitting-patches.rst.
> thanks,
>
> greg k-h
/Daniels

Re: [PATCH 2/2] usb: usb251xb: make power-up reset delay configurable in device tree

[Greg KH]

On Tue, Apr 26, 2022 at 03:06:34PM +0200, Daniels Umanovskis wrote:
> On 4/26/22 2:46 PM, Greg KH wrote:
> > On Tue, Apr 26, 2022 at 12:34:13PM +0000, Daniels Umanovskis wrote:
> > > According to the datasheet, the hub should be operational 500us after
> > > the reset has been deasserted. Some individual circuits have been
> > > observed not to reset within the specified 500us and require a longer
> > > wait for subsequent configuration to succeed.
> > > 
> > > Signed-off-by: Daniels Umanovskis <du@axentia.se>
> > > ---
> > >   drivers/usb/misc/usb251xb.c | 6 +++++-
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > > index 04c4e3fed094..e287e241ef96 100644
> > > --- a/drivers/usb/misc/usb251xb.c
> > > +++ b/drivers/usb/misc/usb251xb.c
> > > @@ -115,6 +115,7 @@ struct usb251xb {
> > >   	struct regulator *vdd;
> > >   	u8 skip_config;
> > >   	struct gpio_desc *gpio_reset;
> > > +	u32 reset_delay_us;
> > >   	u16 vendor_id;
> > >   	u16 product_id;
> > >   	u16 device_id;
> > > @@ -278,7 +279,7 @@ static void usb251xb_reset(struct usb251xb *hub)
> > >   	gpiod_set_value_cansleep(hub->gpio_reset, 0);
> > >   	/* wait for hub recovery/stabilization */
> > > -	usleep_range(500, 750);	/* >=500us after RESET_N deasserted */
> > > +	fsleep(hub->reset_delay_us);
> > >   	i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
> > >   }
> > > @@ -424,6 +425,9 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
> > >   		return err;
> > >   	}
> > > +	if (of_property_read_u32(np, "reset-delay-us", &hub->reset_delay_us))
> > > +		hub->reset_delay_us = 500;
> > So if this fails the delay is 0?
> of_property_read_u32 can fail with -EINVAL or -ENODATA, then delay is 500,
> unless I'm missing something more obvious.

Ah, no, you are right, sorry, I read this wrong.

greg k-h

Re: [PATCH 1/2] dt-bindings: usb: usb251xb: add documentation for reset-delay-us

[Richard Leitner - SKIDATA]

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
> 

Re: [PATCH 2/2] usb: usb251xb: make power-up reset delay configurable in device tree

[Richard Leitner - SKIDATA]

On Tue, Apr 26, 2022 at 12:34:13PM +0000, Daniels Umanovskis wrote:
> According to the datasheet, the hub should be operational 500us after
> the reset has been deasserted. Some individual circuits have been
> observed not to reset within the specified 500us and require a longer
> wait for subsequent configuration to succeed.
> 
> Signed-off-by: Daniels Umanovskis <du@axentia.se>

LGTM.

Reviewed-by: Richard Leitner <richard.leitner@skidata.com>

> ---
>  drivers/usb/misc/usb251xb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 04c4e3fed094..e287e241ef96 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -115,6 +115,7 @@ struct usb251xb {
>  	struct regulator *vdd;
>  	u8 skip_config;
>  	struct gpio_desc *gpio_reset;
> +	u32 reset_delay_us;
>  	u16 vendor_id;
>  	u16 product_id;
>  	u16 device_id;
> @@ -278,7 +279,7 @@ static void usb251xb_reset(struct usb251xb *hub)
>  	gpiod_set_value_cansleep(hub->gpio_reset, 0);
>  
>  	/* wait for hub recovery/stabilization */
> -	usleep_range(500, 750);	/* >=500us after RESET_N deasserted */
> +	fsleep(hub->reset_delay_us);
>  
>  	i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
>  }
> @@ -424,6 +425,9 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>  		return err;
>  	}
>  
> +	if (of_property_read_u32(np, "reset-delay-us", &hub->reset_delay_us))
> +		hub->reset_delay_us = 500;
> +
>  	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
>  		hub->vendor_id = USB251XB_DEF_VENDOR_ID;
>  
> -- 
> 2.30.2
> 

Re: [PATCH 1/2] dt-bindings: usb: usb251xb: add documentation for reset-delay-us

[Rob Herring]

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
> 
> 

Re: [PATCH 1/2] dt-bindings: usb: usb251xb: add documentation for reset-delay-us

[Daniels Umanovskis]

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.

Re: [PATCH 1/2] dt-bindings: usb: usb251xb: add documentation for reset-delay-us

[Richard Leitner - SKIDATA]

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

Re: [PATCH 1/2] dt-bindings: usb: usb251xb: add documentation for reset-delay-us

[Daniels Umanovskis]

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

Re: [PATCH 1/2] dt-bindings: usb: usb251xb: add documentation for reset-delay-us

[Rob Herring]

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