[U-Boot] [PATCH 1/7] usb: Don't init pointer to zero, but NULL

[U-Boot] [PATCH 1/7] usb: Don't init pointer to zero, but NULL

[Marek Vasut]

The pointer should always be inited to NULL, not zero (0). These are
two different things and not necessarily equal.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Stephen Warren <swarren@nvidia.com>
---
 common/usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/usb.c b/common/usb.c
index 4d0de4d..63429d4 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1064,7 +1064,7 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
 
 int usb_select_config(struct usb_device *dev)
 {
-	unsigned char *tmpbuf = 0;
+	unsigned char *tmpbuf = NULL;
 	int err;
 
 	err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE);
-- 
2.7.0

[U-Boot] [PATCH 2/7] usb: dwc2: Resend setup packet in all fail cases

[Marek Vasut]

The USB function can respond to a Setup packet with ACK or, in
case it's busy, it can ignore the Setup packet. The USB function
usually gets busy if we hammer it with Control EP transfers too
much (ie. sending multiple Get Descriptor requests in a single
microframe tends to trigger it on certain USB sticks). The DWC2
controller will interpret not receiving an ACK after Setup packet
as XACTERR. Check for this condition and if it happens, retry
sending the Setup packet.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/host/dwc2.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 30b51b3..8d3949e 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -737,6 +737,7 @@ int wait_for_chhltd(struct dwc2_hc_regs *hc_regs, uint32_t *sub, u8 *toggle)
 {
 	int ret;
 	uint32_t hcint, hctsiz;
+	u8 pid = *toggle;
 
 	ret = wait_for_bit(__func__, &hc_regs->hcint, DWC2_HCINT_CHHLTD, true,
 			   1000, false);
@@ -758,6 +759,19 @@ int wait_for_chhltd(struct dwc2_hc_regs *hc_regs, uint32_t *sub, u8 *toggle)
 	if (hcint & (DWC2_HCINT_NAK | DWC2_HCINT_FRMOVRUN))
 		return -EAGAIN;
 
+	/*
+	 * The USB function can respond to a Setup packet with ACK or, in
+	 * case it's busy, it can ignore the Setup packet. The USB function
+	 * usually gets busy if we hammer it with Control EP transfers too
+	 * much (ie. sending multiple Get Descriptor requests in a single
+	 * microframe tends to trigger it on certain USB sticks). The DWC2
+	 * controller will interpret not receiving an ACK after Setup packet
+	 * as XACTERR. Check for this condition and if it happens, retry
+	 * sending the Setup packet.
+	 */
+	if (hcint & DWC2_HCINT_XACTERR && (pid == DWC2_HC_PID_SETUP))
+		return -EAGAIN;
+
 	debug("%s: Error (HCINT=%08x)\n", __func__, hcint);
 	return -EINVAL;
 }
-- 
2.7.0

[U-Boot] [PATCH 2/7] usb: dwc2: Resend setup packet in all fail cases

[Chin Liang See]

On Tue, 2016-05-03 at 22:51 +0200, Marek Vasut wrote:
> The USB function can respond to a Setup packet with ACK or, in
> case it's busy, it can ignore the Setup packet. The USB function
> usually gets busy if we hammer it with Control EP transfers too
> much (ie. sending multiple Get Descriptor requests in a single
> microframe tends to trigger it on certain USB sticks). The DWC2
> controller will interpret not receiving an ACK after Setup packet
> as XACTERR. Check for this condition and if it happens, retry
> sending the Setup packet.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/usb/host/dwc2.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

Reviewed-by: Chin Liang See <clsee@altera.com>
Tested-by: Chin Liang See <clsee@altera.com>

Thanks
Chin Liang

[U-Boot] [PATCH 3/7] usb: dwc2: Throttle the setup packet resending

[Marek Vasut]

Abort the request in case any of the tokens in the packet failed to
complete transfer 10 times. This is a precaution needed so that we
don't end in endless loop when scanning the bus with some braindead
devices.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/host/dwc2.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 8d3949e..27fcf7c 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -993,6 +993,8 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 	u8 pid;
 	/* For CONTROL endpoint pid should start with DATA1 */
 	int status_direction;
+	int againctr = 0;
+	const int againmax = 10;
 
 	if (devnum == priv->root_hub_devnum) {
 		dev->status = 0;
@@ -1003,25 +1005,37 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 
 	/* SETUP stage */
 	pid = DWC2_HC_PID_SETUP;
-	do {
+	againctr = 0;
+	while (true) {
 		ret = chunk_msg(priv, dev, pipe, &pid, 0, setup, 8);
-	} while (ret == -EAGAIN);
-	if (ret)
-		return ret;
+		if (!ret)
+			break;
+		if (ret != -EAGAIN)
+			return ret;
+		if (againctr == againmax)
+			return -EINVAL;
+		againctr++;
+	};
 
 	/* DATA stage */
 	act_len = 0;
 	if (buffer) {
 		pid = DWC2_HC_PID_DATA1;
-		do {
+		againctr = 0;
+		while (true) {
 			ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe),
 					buffer, len);
 			act_len += dev->act_len;
 			buffer += dev->act_len;
 			len -= dev->act_len;
-		} while (ret == -EAGAIN);
-		if (ret)
-			return ret;
+			if (!ret)
+				break;
+			if (ret != -EAGAIN)
+				return ret;
+			if (againctr == againmax)
+				return -EINVAL;
+			againctr++;
+		};
 		status_direction = usb_pipeout(pipe);
 	} else {
 		/* No-data CONTROL always ends with an IN transaction */
@@ -1030,12 +1044,18 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 
 	/* STATUS stage */
 	pid = DWC2_HC_PID_DATA1;
-	do {
+	againctr = 0;
+	while (true) {
 		ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
 				priv->status_buffer, 0);
-	} while (ret == -EAGAIN);
-	if (ret)
-		return ret;
+		if (!ret)
+			break;
+		if (ret != -EAGAIN)
+			return ret;
+		if (againctr == againmax)
+			return -EINVAL;
+		againctr++;
+	};
 
 	dev->act_len = act_len;
 
-- 
2.7.0

[U-Boot] [PATCH 3/7] usb: dwc2: Throttle the setup packet resending

[Chin Liang See]

On Tue, 2016-05-03 at 22:51 +0200, Marek Vasut wrote:
> Abort the request in case any of the tokens in the packet failed to
> complete transfer 10 times. This is a precaution needed so that we
> don't end in endless loop when scanning the bus with some braindead
> devices.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/usb/host/dwc2.c | 44 ++++++++++++++++++++++++++++++++-------
> -----
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 

Reviewed-by: Chin Liang See <clsee@altera.com>
Tested-by: Chin Liang See <clsee@altera.com>

Thanks
Chin Liang

[U-Boot] [PATCH 3/7] usb: dwc2: Throttle the setup packet resending

[Stephen Warren]

On 05/03/2016 02:51 PM, Marek Vasut wrote:
> Abort the request in case any of the tokens in the packet failed to
> complete transfer 10 times. This is a precaution needed so that we
> don't end in endless loop when scanning the bus with some braindead
> devices.

Does this affect USB keyboards when SYS_USB_EVENT_POLL_VIA_CONTROL_EP is 
enabled? IIRC control transactions to HID devices can be held off for 
some duration based on polling intervals, and this patch might abort 
them early?

Or do we typically expect to use interrupt transfers for keyboards, so 
this isn't too relevant (although there are some platforms that enable 
SYS_USB_EVENT_POLL_VIA_CONTROL_EP). Maybe not DWC2 platforms though; I 
didn't check.

[U-Boot] [PATCH 3/7] usb: dwc2: Throttle the setup packet resending

[Marek Vasut]

On 05/04/2016 07:08 PM, Stephen Warren wrote:
> On 05/03/2016 02:51 PM, Marek Vasut wrote:
>> Abort the request in case any of the tokens in the packet failed to
>> complete transfer 10 times. This is a precaution needed so that we
>> don't end in endless loop when scanning the bus with some braindead
>> devices.
> 
> Does this affect USB keyboards when SYS_USB_EVENT_POLL_VIA_CONTROL_EP is
> enabled? IIRC control transactions to HID devices can be held off for
> some duration based on polling intervals, and this patch might abort
> them early?

I didn't try this with keyboard, so I am not quite sure on this one.
Do you have RPi zero or somesuch on which you could try ?

btw are usb 1.1 keyboards supposed to work with DWC2 in U-Boot ?

> Or do we typically expect to use interrupt transfers for keyboards, so
> this isn't too relevant (although there are some platforms that enable
> SYS_USB_EVENT_POLL_VIA_CONTROL_EP). Maybe not DWC2 platforms though; I
> didn't check.

The platforms which enable POLL_VIA_CONTROL_EP are all chipidea otg, so
this should be fine.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/7] usb: dwc2: Throttle the setup packet resending

[Marek Vasut]

On 05/04/2016 07:08 PM, Stephen Warren wrote:
> On 05/03/2016 02:51 PM, Marek Vasut wrote:
>> Abort the request in case any of the tokens in the packet failed to
>> complete transfer 10 times. This is a precaution needed so that we
>> don't end in endless loop when scanning the bus with some braindead
>> devices.
> 
> Does this affect USB keyboards when SYS_USB_EVENT_POLL_VIA_CONTROL_EP is
> enabled? IIRC control transactions to HID devices can be held off for
> some duration based on polling intervals, and this patch might abort
> them early?
> 
> Or do we typically expect to use interrupt transfers for keyboards, so
> this isn't too relevant (although there are some platforms that enable
> SYS_USB_EVENT_POLL_VIA_CONTROL_EP). Maybe not DWC2 platforms though; I
> didn't check.

Hm, I found one keyboard with which I have trouble with 2/7 and 3/7 , so
I will drop them from my USB PR. This means half of my USB sticks will
no longer work again, but at least it won't introduce any regressions.
We need to revisit this for 2016.07. Do you have any ideas how to deal
with this ? I am tempted to just add udelay(125); after each control
transfer.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 4/7] usb: Wait after sending Set Configuration request

[Marek Vasut]

Some devices, like the SanDisk Cruzer Pop need some time to process
the Set Configuration request, so wait a little until they are ready.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Stephen Warren <swarren@nvidia.com>
---
 common/usb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/common/usb.c b/common/usb.c
index 63429d4..205041b 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev)
 			"len %d, status %lX\n", dev->act_len, dev->status);
 		return err;
 	}
+
+	/*
+	 * Wait until the Set Configuration request gets processed by the
+	 * device. This is required by at least SanDisk Cruzer Pop USB 2.0
+	 * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.
+	 */
+	mdelay(10);
+
 	debug("new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
 	      dev->descriptor.iManufacturer, dev->descriptor.iProduct,
 	      dev->descriptor.iSerialNumber);
-- 
2.7.0

[U-Boot] [PATCH 4/7] usb: Wait after sending Set Configuration request

[Stefan Roese]

On 03.05.2016 22:51, Marek Vasut wrote:
> Some devices, like the SanDisk Cruzer Pop need some time to process
> the Set Configuration request, so wait a little until they are ready.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>   common/usb.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/common/usb.c b/common/usb.c
> index 63429d4..205041b 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev)
>   			"len %d, status %lX\n", dev->act_len, dev->status);
>   		return err;
>   	}
> +
> +	/*
> +	 * Wait until the Set Configuration request gets processed by the
> +	 * device. This is required by at least SanDisk Cruzer Pop USB 2.0
> +	 * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.
> +	 */
> +	mdelay(10);
> +
>   	debug("new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
>   	      dev->descriptor.iManufacturer, dev->descriptor.iProduct,
>   	      dev->descriptor.iSerialNumber);
>

As you might have expected, I'm not a fan of adding new delays to
the common USB code. As this negatively affects all platforms. Did
you test these two sticks that require this delay on other platforms
than SoCFPGA? I would be very interested to know, if these keys are
successfully detected without this patch on other platforms. I
don't have access to these USB keys, so I can't test it on my
platforms.

Thanks,
Stefan

[U-Boot] [PATCH 4/7] usb: Wait after sending Set Configuration request

[Chin Liang See]

On Wed, 2016-05-04 at 09:41 +0200, Stefan Roese wrote:
> On 03.05.2016 22:51, Marek Vasut wrote:
> > Some devices, like the SanDisk Cruzer Pop need some time to process
> > the Set Configuration request, so wait a little until they are
> > ready.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Stephen Warren <swarren@nvidia.com>
> > ---
> >   common/usb.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/common/usb.c b/common/usb.c
> > index 63429d4..205041b 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device
> > *dev)
> >   			"len %d, status %lX\n", dev->act_len, dev
> > ->status);
> >   		return err;
> >   	}
> > +
> > +	/*
> > +	 * Wait until the Set Configuration request gets processed
> > by the
> > +	 * device. This is required by at least SanDisk Cruzer Pop
> > USB 2.0
> > +	 * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG
> > controller.
> > +	 */
> > +	mdelay(10);
> > +
> >   	debug("new device strings: Mfr=%d, Product=%d,
> > SerialNumber=%d\n",
> >   	      dev->descriptor.iManufacturer, dev
> > ->descriptor.iProduct,
> >   	      dev->descriptor.iSerialNumber);
> > 
> 
> As you might have expected, I'm not a fan of adding new delays to
> the common USB code. As this negatively affects all platforms. Did
> you test these two sticks that require this delay on other platforms
> than SoCFPGA? I would be very interested to know, if these keys are
> successfully detected without this patch on other platforms. I
> don't have access to these USB keys, so I can't test it on my
> platforms.
> 

Actually this series of patches (include the delay) help for all my
problematic pen drives too. It sound to me these pen drives need time
to process. But at same time, its strange to me that the device didn't
NAK (as I added printout for NAK) to indicate that its busy.

Seems that this issue is noticed at Freescale too. 
http://lists.denx.de/pipermail/u-boot/2015-December/238434.html. Cc'ed
them as they might can share more details on this.

Chin Liang



> Thanks,
> Stefan

[U-Boot] [PATCH 4/7] usb: Wait after sending Set Configuration request

[Stefan Roese]

On 04.05.2016 11:45, Chin Liang See wrote:
> On Wed, 2016-05-04 at 09:41 +0200, Stefan Roese wrote:
>> On 03.05.2016 22:51, Marek Vasut wrote:
>>> Some devices, like the SanDisk Cruzer Pop need some time to process
>>> the Set Configuration request, so wait a little until they are
>>> ready.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Chin Liang See <clsee@altera.com>
>>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Cc: Stefan Roese <sr@denx.de>
>>> Cc: Stephen Warren <swarren@nvidia.com>
>>> ---
>>>    common/usb.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/common/usb.c b/common/usb.c
>>> index 63429d4..205041b 100644
>>> --- a/common/usb.c
>>> +++ b/common/usb.c
>>> @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device
>>> *dev)
>>>    			"len %d, status %lX\n", dev->act_len, dev
>>> ->status);
>>>    		return err;
>>>    	}
>>> +
>>> +	/*
>>> +	 * Wait until the Set Configuration request gets processed
>>> by the
>>> +	 * device. This is required by at least SanDisk Cruzer Pop
>>> USB 2.0
>>> +	 * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG
>>> controller.
>>> +	 */
>>> +	mdelay(10);
>>> +
>>>    	debug("new device strings: Mfr=%d, Product=%d,
>>> SerialNumber=%d\n",
>>>    	      dev->descriptor.iManufacturer, dev
>>> ->descriptor.iProduct,
>>>    	      dev->descriptor.iSerialNumber);
>>>
>>
>> As you might have expected, I'm not a fan of adding new delays to
>> the common USB code. As this negatively affects all platforms. Did
>> you test these two sticks that require this delay on other platforms
>> than SoCFPGA? I would be very interested to know, if these keys are
>> successfully detected without this patch on other platforms. I
>> don't have access to these USB keys, so I can't test it on my
>> platforms.
>>
>
> Actually this series of patches (include the delay) help for all my
> problematic pen drives too. It sound to me these pen drives need time
> to process.

This delay (I'm talking mainly about the 1000ms delay per USB hub
in patch 7/7) does not seem to be necessary on other platforms.

Chin, could you please test again without this patch 7/7 but
with "usb_pgood_delay" set to 1000? And let us know if this
also fixes all the problems with your problematic pen drives?
That would be very helpful.

Thanks,
Stefan

[U-Boot] [PATCH 4/7] usb: Wait after sending Set Configuration request

[Chin Liang See]

On Wed, 2016-05-04 at 12:00 +0200, Stefan Roese wrote:
> On 04.05.2016 11:45, Chin Liang See wrote:
> > On Wed, 2016-05-04 at 09:41 +0200, Stefan Roese wrote:
> > > On 03.05.2016 22:51, Marek Vasut wrote:
> > > > Some devices, like the SanDisk Cruzer Pop need some time to
> > > > process
> > > > the Set Configuration request, so wait a little until they are
> > > > ready.
> > > > 
> > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > Cc: Chin Liang See <clsee@altera.com>
> > > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > > Cc: Stefan Roese <sr@denx.de>
> > > > Cc: Stephen Warren <swarren@nvidia.com>
> > > > ---
> > > >    common/usb.c | 8 ++++++++
> > > >    1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/common/usb.c b/common/usb.c
> > > > index 63429d4..205041b 100644
> > > > --- a/common/usb.c
> > > > +++ b/common/usb.c
> > > > @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device
> > > > *dev)
> > > >    			"len %d, status %lX\n", dev
> > > > ->act_len, dev
> > > > ->status);
> > > >    		return err;
> > > >    	}
> > > > +
> > > > +	/*
> > > > +	 * Wait until the Set Configuration request gets
> > > > processed
> > > > by the
> > > > +	 * device. This is required by at least SanDisk Cruzer
> > > > Pop
> > > > USB 2.0
> > > > +	 * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG
> > > > controller.
> > > > +	 */
> > > > +	mdelay(10);
> > > > +
> > > >    	debug("new device strings: Mfr=%d, Product=%d,
> > > > SerialNumber=%d\n",
> > > >    	      dev->descriptor.iManufacturer, dev
> > > > ->descriptor.iProduct,
> > > >    	      dev->descriptor.iSerialNumber);
> > > > 
> > > 
> > > As you might have expected, I'm not a fan of adding new delays to
> > > the common USB code. As this negatively affects all platforms.
> > > Did
> > > you test these two sticks that require this delay on other
> > > platforms
> > > than SoCFPGA? I would be very interested to know, if these keys
> > > are
> > > successfully detected without this patch on other platforms. I
> > > don't have access to these USB keys, so I can't test it on my
> > > platforms.
> > > 
> > 
> > Actually this series of patches (include the delay) help for all my
> > problematic pen drives too. It sound to me these pen drives need
> > time
> > to process.
> 
> This delay (I'm talking mainly about the 1000ms delay per USB hub
> in patch 7/7) does not seem to be necessary on other platforms.
> 
> Chin, could you please test again without this patch 7/7 but
> with "usb_pgood_delay" set to 1000? And let us know if this
> also fixes all the problems with your problematic pen drives?
> That would be very helpful.
> 

Yup, it works for my problematic pen drives. This apply to both with
and without setting the usb_pgood_delay to 1000.

Thanks
Chin Liang

> Thanks,
> Stefan

[U-Boot] [PATCH 4/7] usb: Wait after sending Set Configuration request

[Sriram Dash]

>-----Original Message-----
>From: Chin Liang See [mailto:clsee at altera.com]
>Sent: Wednesday, May 04, 2016 3:15 PM
>To: Stefan Roese <sr@denx.de>; Marek Vasut <marex@denx.de>; u-
>boot at lists.denx.de
>Cc: Dinh Nguyen <dinguyen@opensource.altera.com>; Hans de Goede
><hdegoede@redhat.com>; Stephen Warren <swarren@nvidia.com>;
>sriram.dash at freescale.com; rajesh.bhagat at freescale.com
>Subject: Re: [PATCH 4/7] usb: Wait after sending Set Configuration request
>
>On Wed, 2016-05-04 at 09:41 +0200, Stefan Roese wrote:
>> On 03.05.2016 22:51, Marek Vasut wrote:
>> > Some devices, like the SanDisk Cruzer Pop need some time to process
>> > the Set Configuration request, so wait a little until they are
>> > ready.
>> >
>> > Signed-off-by: Marek Vasut <marex@denx.de>
>> > Cc: Chin Liang See <clsee@altera.com>
>> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>> > Cc: Hans de Goede <hdegoede@redhat.com>
>> > Cc: Stefan Roese <sr@denx.de>
>> > Cc: Stephen Warren <swarren@nvidia.com>
>> > ---
>> >   common/usb.c | 8 ++++++++
>> >   1 file changed, 8 insertions(+)
>> >
>> > diff --git a/common/usb.c b/common/usb.c index 63429d4..205041b
>> > 100644
>> > --- a/common/usb.c
>> > +++ b/common/usb.c
>> > @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device
>> > *dev)
>> >   			"len %d, status %lX\n", dev->act_len, dev
>> > ->status);
>> >   		return err;
>> >   	}
>> > +
>> > +	/*
>> > +	 * Wait until the Set Configuration request gets processed
>> > by the
>> > +	 * device. This is required by at least SanDisk Cruzer Pop
>> > USB 2.0
>> > +	 * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG
>> > controller.
>> > +	 */
>> > +	mdelay(10);
>> > +
>> >   	debug("new device strings: Mfr=%d, Product=%d,
>> > SerialNumber=%d\n",
>> >   	      dev->descriptor.iManufacturer, dev
>> > ->descriptor.iProduct,
>> >   	      dev->descriptor.iSerialNumber);
>> >
>>
>> As you might have expected, I'm not a fan of adding new delays to the
>> common USB code. As this negatively affects all platforms. Did you
>> test these two sticks that require this delay on other platforms than
>> SoCFPGA? I would be very interested to know, if these keys are
>> successfully detected without this patch on other platforms. I don't
>> have access to these USB keys, so I can't test it on my platforms.
>>
>
>Actually this series of patches (include the delay) help for all my problematic pen
>drives too. It sound to me these pen drives need time to process. But at same time,
>its strange to me that the device didn't NAK (as I added printout for NAK) to indicate
>that its busy.
>
>Seems that this issue is noticed at Freescale too.
>http://lists.denx.de/pipermail/u-boot/2015-December/238434.html. Cc'ed them as
>they might can share more details on this.
>

Hello,

In our case, for some??USB 2.0 stick(notably : Sandisk Cruzer blade 4 GB) on USB 3.0 controller, 
we were getting timeouts in get descriptor string, and were not able to retrieve the manufacturer,
product id , serial number.

Later we discovered, they needed a little more time (~ 2 ms) after setting configuration. However,
as per the usb set address delay, we choose 10 ms delay(to support even slower devices).

Sriram

>Chin Liang
>
>
>
>> Thanks,
>> Stefan

[U-Boot] [PATCH 4/7] usb: Wait after sending Set Configuration request

[Marek Vasut]

On 05/04/2016 09:41 AM, Stefan Roese wrote:
> On 03.05.2016 22:51, Marek Vasut wrote:
>> Some devices, like the SanDisk Cruzer Pop need some time to process
>> the Set Configuration request, so wait a little until they are ready.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Chin Liang See <clsee@altera.com>
>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Stephen Warren <swarren@nvidia.com>
>> ---
>>   common/usb.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/common/usb.c b/common/usb.c
>> index 63429d4..205041b 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev)
>>               "len %d, status %lX\n", dev->act_len, dev->status);
>>           return err;
>>       }
>> +
>> +    /*
>> +     * Wait until the Set Configuration request gets processed by the
>> +     * device. This is required by at least SanDisk Cruzer Pop USB 2.0
>> +     * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.
>> +     */
>> +    mdelay(10);
>> +
>>       debug("new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
>>             dev->descriptor.iManufacturer, dev->descriptor.iProduct,
>>             dev->descriptor.iSerialNumber);
>>
> 
> As you might have expected, I'm not a fan of adding new delays to
> the common USB code. As this negatively affects all platforms. Did
> you test these two sticks that require this delay on other platforms
> than SoCFPGA?

Yes, on intel. Many microframes pass between these control EP requests,
while on the dwc2 in u-boot, quite a few of these requests end in the
same microframe because the code is fast, which is what the stick does
not like.

> I would be very interested to know, if these keys are
> successfully detected without this patch on other platforms. I
> don't have access to these USB keys, so I can't test it on my
> platforms.
> 
> Thanks,
> Stefan


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 5/7] usb: Assure Get Descriptor request is in separate microframe

[Marek Vasut]

The Kingston DT Ultimate USB 3.0 stick is sensitive to this first
Get Descriptor request and if the request is not in a separate
microframe, the stick refuses to operate. Add slight delay, which
is enough for one microframe to pass on any USB spec revision.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Stephen Warren <swarren@nvidia.com>
---
 common/usb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/common/usb.c b/common/usb.c
index 205041b..8d9efe5 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1077,6 +1077,14 @@ int usb_select_config(struct usb_device *dev)
 	le16_to_cpus(&dev->descriptor.idProduct);
 	le16_to_cpus(&dev->descriptor.bcdDevice);
 
+	/*
+	 * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
+	 * about this first Get Descriptor request. If there are any other
+	 * requests in the first microframe, the stick crashes. Wait about
+	 * one microframe duration here (1mS for USB 1.x , 125uS for USB 2.0).
+	 */
+	mdelay(1);
+
 	/* only support for one config for now */
 	err = usb_get_configuration_len(dev, 0);
 	if (err >= 0) {
-- 
2.7.0

[U-Boot] [PATCH 5/7] usb: Assure Get Descriptor request is in separate microframe

[Stefan Roese]

On 03.05.2016 22:51, Marek Vasut wrote:
> The Kingston DT Ultimate USB 3.0 stick is sensitive to this first
> Get Descriptor request and if the request is not in a separate
> microframe, the stick refuses to operate. Add slight delay, which
> is enough for one microframe to pass on any USB spec revision.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>   common/usb.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/common/usb.c b/common/usb.c
> index 205041b..8d9efe5 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1077,6 +1077,14 @@ int usb_select_config(struct usb_device *dev)
>   	le16_to_cpus(&dev->descriptor.idProduct);
>   	le16_to_cpus(&dev->descriptor.bcdDevice);
>
> +	/*
> +	 * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
> +	 * about this first Get Descriptor request. If there are any other
> +	 * requests in the first microframe, the stick crashes. Wait about
> +	 * one microframe duration here (1mS for USB 1.x , 125uS for USB 2.0).
> +	 */
> +	mdelay(1);
> +
>   	/* only support for one config for now */
>   	err = usb_get_configuration_len(dev, 0);
>   	if (err >= 0) {
>

Again my question, if this problem also occurs on other platforms
with this USB key. A 1ms delay is not really a big deal, but its
my general feeling that we should manifest such changes by testing
on different platforms.

Thanks,
Stefan

[U-Boot] [PATCH 5/7] usb: Assure Get Descriptor request is in separate microframe

[Marek Vasut]

On 05/04/2016 10:03 AM, Stefan Roese wrote:
> On 03.05.2016 22:51, Marek Vasut wrote:
>> The Kingston DT Ultimate USB 3.0 stick is sensitive to this first
>> Get Descriptor request and if the request is not in a separate
>> microframe, the stick refuses to operate. Add slight delay, which
>> is enough for one microframe to pass on any USB spec revision.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Chin Liang See <clsee@altera.com>
>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Stephen Warren <swarren@nvidia.com>
>> ---
>>   common/usb.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/common/usb.c b/common/usb.c
>> index 205041b..8d9efe5 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -1077,6 +1077,14 @@ int usb_select_config(struct usb_device *dev)
>>       le16_to_cpus(&dev->descriptor.idProduct);
>>       le16_to_cpus(&dev->descriptor.bcdDevice);
>>
>> +    /*
>> +     * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
>> +     * about this first Get Descriptor request. If there are any other
>> +     * requests in the first microframe, the stick crashes. Wait about
>> +     * one microframe duration here (1mS for USB 1.x , 125uS for USB
>> 2.0).
>> +     */
>> +    mdelay(1);
>> +
>>       /* only support for one config for now */
>>       err = usb_get_configuration_len(dev, 0);
>>       if (err >= 0) {
>>
> 
> Again my question, if this problem also occurs on other platforms
> with this USB key. A 1ms delay is not really a big deal, but its
> my general feeling that we should manifest such changes by testing
> on different platforms.

I tested it by connecting the bus analyzer between the stick and socfpga
and between the stick and x86 host. I wouldn't be able to
come up with this solution. btw. any ehci ends up inserting these
control requests into separate microframes, but we need the mdelay
to also cater for ohci/uhci, which has longer frames (1ms).

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 5/7] usb: Assure Get Descriptor request is in separate microframe

[Stephen Warren]

On 05/03/2016 02:51 PM, Marek Vasut wrote:
> The Kingston DT Ultimate USB 3.0 stick is sensitive to this first
> Get Descriptor request and if the request is not in a separate
> microframe, the stick refuses to operate. Add slight delay, which
> is enough for one microframe to pass on any USB spec revision.

> diff --git a/common/usb.c b/common/usb.c

> +	/*
> +	 * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
> +	 * about this first Get Descriptor request. If there are any other
> +	 * requests in the first microframe, the stick crashes. Wait about
> +	 * one microframe duration here (1mS for USB 1.x , 125uS for USB 2.0).
> +	 */
> +	mdelay(1);

Do we know the connection speed here? If so, we could sleep 1ms for 
USB1.x and 125us for USB2.x, thus reducing any performance impact. 
Still, this is a short delay that I think only happens once per actual 
device so perhaps it isn't worth it.

[U-Boot] [PATCH 6/7] usb: hub: Don't continue on get_port_status failure

[Marek Vasut]

The code shouldn't continue probing the port if get_port_status() failed.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Stephen Warren <swarren@nvidia.com>
---
 common/usb_hub.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 4f59802..0f39c9f 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -402,6 +402,7 @@ static int usb_scan_port(struct usb_device_scan *usb_scan)
 			free(usb_scan);
 			return 0;
 		}
+		return 0;
 	}
 
 	portstatus = le16_to_cpu(portsts->wPortStatus);
-- 
2.7.0

[U-Boot] [PATCH 6/7] usb: hub: Don't continue on get_port_status failure

[Stefan Roese]

On 03.05.2016 22:51, Marek Vasut wrote:
> The code shouldn't continue probing the port if get_port_status() failed.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>   common/usb_hub.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 4f59802..0f39c9f 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -402,6 +402,7 @@ static int usb_scan_port(struct usb_device_scan *usb_scan)
>   			free(usb_scan);
>   			return 0;
>   		}
> +		return 0;
>   	}
>
>   	portstatus = le16_to_cpu(portsts->wPortStatus);
>

Thanks for spotting this:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 6/7] usb: hub: Don't continue on get_port_status failure

[Chin Liang See]

On Wed, 2016-05-04 at 10:05 +0200, Stefan Roese wrote:
> On 03.05.2016 22:51, Marek Vasut wrote:
> > The code shouldn't continue probing the port if get_port_status()
> > failed.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Stephen Warren <swarren@nvidia.com>
> > ---
> >   common/usb_hub.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/common/usb_hub.c b/common/usb_hub.c
> > index 4f59802..0f39c9f 100644
> > --- a/common/usb_hub.c
> > +++ b/common/usb_hub.c
> > @@ -402,6 +402,7 @@ static int usb_scan_port(struct usb_device_scan
> > *usb_scan)
> >   			free(usb_scan);
> >   			return 0;
> >   		}
> > +		return 0;
> >   	}
> > 
> >   	portstatus = le16_to_cpu(portsts->wPortStatus);
> > 
> 
> Thanks for spotting this:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 

Reviewed-by: Chin Liang See <clsee@altera.com>
Tested-by: Chin Liang See <clsee@altera.com>

Thanks
Chin Liang

> Thanks,
> Stefan

[U-Boot] [PATCH 7/7] usb: hub: Increase the query delay

[Marek Vasut]

Increase the query delay, otherwise some sticks are not detected.
The problem shows up on the USB bus analyzer such that the stick
takes longer time to switch from FS mode to HS mode than the code
allows.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Stephen Warren <swarren@nvidia.com>
---
 common/usb_hub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 0f39c9f..6cd274a 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 	 * Do a minimum delay of the larger value of 100ms or pgood_delay
 	 * so that the power can stablize before the devices are queried
 	 */
-	hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
+	hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
 
 	/*
 	 * Record the power-on timeout here. The max. delay (timeout)
-- 
2.7.0

[U-Boot] [PATCH 7/7] usb: hub: Increase the query delay

[Stefan Roese]

On 03.05.2016 22:51, Marek Vasut wrote:
> Increase the query delay, otherwise some sticks are not detected.
> The problem shows up on the USB bus analyzer such that the stick
> takes longer time to switch from FS mode to HS mode than the code
> allows.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>   common/usb_hub.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 0f39c9f..6cd274a 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>   	 * Do a minimum delay of the larger value of 100ms or pgood_delay
>   	 * so that the power can stablize before the devices are queried
>   	 */
> -	hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
> +	hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
>
>   	/*
>   	 * Record the power-on timeout here. The max. delay (timeout)
>

We have touched this part of the delay a number of times in my
USB scanning patch series. I've integrated all very valuable
suggestions from Stephen and Hans and I'm pretty sure that the
current implementation is aligned with the USB spec. And tested
successfully one multiple platforms without regression.
Stephen did some tests on Tegra and Hans on sunxi. I tested
mainly on x86. But I now also tested on Armada XP (see
below).

As mentioned before, the current version causes no problems with
all my USB sticks on the congatec x86 board. Even the 2 ones that
are problematic when connected to the SoCFPGA. They are detected
without any issues on this board. Thats why I assume, that the
real problem here is the DWC driver and not the generic USB
handling code. My feeling is that increasing this initial delay
(before the scanning starts) just papers over the real problem
most likely hidden somewhere in the DWC driver. This is just
a feeling though which I can't prove somehow other than testing
the common USB code on different platforms. And I really have
no deeper knowledge of the DWC driver to manifest this feeling
or even fix this potential problem there.

I've already mentioned this in another mail. My suggestion for
SoCFPGA boards that need to use these problematic USB keys is
to use the already available solution to set "usb_pgood_delay"
to 1000. This effectively does the same as this patch. Without
implying this general 1 second delay per hub (!!!) to all other
platforms that use USB in U-Boot.

To test my suspicions about this being a DWC (SoCFPGA?) only
problem, I've also tested all my current USB sticks including
the 2 problematic ones (on SoCrates) on another ARM platform
(additionally to all my test on x86). I've used the Marvell
Armada XP development board (db-mv784mp-gp) for this. And all
USB sticks are detected without any problems on this platform.

As a result of all this, I would like to have this patch not
applied. As it negatively touches the common USB code to fix
(paper over?) a problem only seen on one platform (AFAIK). And
we already have the solution of this "usb_pgood_delay" that
can be used on SoCFPGA. To manifest this, here again the
numbers for the USB scanning time on x86, without and with
this patch:

Without this patch:
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 1.940 seconds

With this patch:
=> time usb start
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) found

time: 5.421 seconds

Thanks,
Stefan

[U-Boot] [PATCH 7/7] usb: hub: Increase the query delay

[Marek Vasut]

On 05/04/2016 10:07 AM, Stefan Roese wrote:
> On 03.05.2016 22:51, Marek Vasut wrote:
>> Increase the query delay, otherwise some sticks are not detected.
>> The problem shows up on the USB bus analyzer such that the stick
>> takes longer time to switch from FS mode to HS mode than the code
>> allows.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Chin Liang See <clsee@altera.com>
>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Stephen Warren <swarren@nvidia.com>
>> ---
>>   common/usb_hub.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index 0f39c9f..6cd274a 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device
>> *hub)
>>        * Do a minimum delay of the larger value of 100ms or pgood_delay
>>        * so that the power can stablize before the devices are queried
>>        */
>> -    hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
>> +    hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
>>
>>       /*
>>        * Record the power-on timeout here. The max. delay (timeout)
>>
> 
> We have touched this part of the delay a number of times in my
> USB scanning patch series. I've integrated all very valuable
> suggestions from Stephen and Hans and I'm pretty sure that the
> current implementation is aligned with the USB spec.

Sadly, not everyone goes exactly be the USB spec it seems.
Especially the cheap sticks which are the majority in the market.

> And tested
> successfully one multiple platforms without regression.
> Stephen did some tests on Tegra and Hans on sunxi. I tested
> mainly on x86. But I now also tested on Armada XP (see
> below).

All of which use the same EHCI or XHCI controller, correct ?

> As mentioned before, the current version causes no problems with
> all my USB sticks on the congatec x86 board. Even the 2 ones that
> are problematic when connected to the SoCFPGA. They are detected
> without any issues on this board. Thats why I assume, that the
> real problem here is the DWC driver and not the generic USB
> handling code.

The logic here is flawed. If the code works against EHCI controller and
does not work against DWC2 controller, you cannot infer from this that
the DWC2 controller is the problem.

This can also be specific behavior of the EHCI controller, and I in fact
suspect it is, but you cannot decide this without checking the
bus itself.

> My feeling is that increasing this initial delay
> (before the scanning starts) just papers over the real problem
> most likely hidden somewhere in the DWC driver. This is just
> a feeling though which I can't prove somehow other than testing
> the common USB code on different platforms. And I really have
> no deeper knowledge of the DWC driver to manifest this feeling
> or even fix this potential problem there.

The bus analyzer tells me that the stick just takes longer to come out
of FS mode and switch to HS mode when the USB started from reset state
of the controller, I decide to trust what the analyzer shows me instead.

See attached logs.

> I've already mentioned this in another mail. My suggestion for
> SoCFPGA boards that need to use these problematic USB keys is
> to use the already available solution to set "usb_pgood_delay"
> to 1000. This effectively does the same as this patch. Without
> implying this general 1 second delay per hub (!!!) to all other
> platforms that use USB in U-Boot.
> 
> To test my suspicions about this being a DWC (SoCFPGA?) only
> problem, I've also tested all my current USB sticks including
> the 2 problematic ones (on SoCrates) on another ARM platform
> (additionally to all my test on x86). I've used the Marvell
> Armada XP development board (db-mv784mp-gp) for this. And all
> USB sticks are detected without any problems on this platform.

I see, it would be interesting to know what happens on the bus on the
marvell board compared to the socfpga board. For the socfpga, the bus
starts from complete cold state, could it be the marvell does have the
EHCI running when you do your tests ? That would explain why the stick
might be ready much faster on your platform.

> As a result of all this, I would like to have this patch not
> applied. As it negatively touches the common USB code to fix
> (paper over?) a problem only seen on one platform (AFAIK). And
> we already have the solution of this "usb_pgood_delay" that
> can be used on SoCFPGA. To manifest this, here again the
> numbers for the USB scanning time on x86, without and with
> this patch:
> 
> Without this patch:
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
> 
> time: 1.940 seconds
> 
> With this patch:
> => time usb start
> starting USB...
> USB0:   USB EHCI 1.00
> scanning bus 0 for devices... 9 USB Device(s) found
> 
> time: 5.421 seconds

Well that's great, removing some delays makes the code run faster and
breaks some platform. Stefan, I need a solution for this release and
the release is coming very quickly. So far, I see no other proposals
how to fix this issue and it's been reported for well over a month.

As I don't want to have regressions in this release, here are the two
options:
a) I revert "usb: Change power-on / scanning timeout handling"
b) I apply these 7 patches. I am willing to isolate this increase
   of delay with an ifdef to DWC2 controller, but that does NOT
   seem correct according to the analyzer. I would just wait for
   an EHCI controller on which this breaks.

> Thanks,
> Stefan


-- 
Best regards,
Marek Vasut
-------------- next part --------------
A non-text attachment was scrubbed...
Name: trace-dwc2-with-delay.csv
Type: text/csv
Size: 4657 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160504/74350c99/attachment.csv>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: trace-dwc2-without-delay.csv
Type: text/csv
Size: 1066 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160504/74350c99/attachment-0001.csv>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: trace-ehci.csv
Type: text/csv
Size: 4362 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160504/74350c99/attachment-0002.csv>

[U-Boot] [PATCH 7/7] usb: hub: Increase the query delay

[Stefan Roese]

On 04.05.2016 13:39, Marek Vasut wrote:
> On 05/04/2016 10:07 AM, Stefan Roese wrote:
>> On 03.05.2016 22:51, Marek Vasut wrote:
>>> Increase the query delay, otherwise some sticks are not detected.
>>> The problem shows up on the USB bus analyzer such that the stick
>>> takes longer time to switch from FS mode to HS mode than the code
>>> allows.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Chin Liang See <clsee@altera.com>
>>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Cc: Stefan Roese <sr@denx.de>
>>> Cc: Stephen Warren <swarren@nvidia.com>
>>> ---
>>>    common/usb_hub.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> index 0f39c9f..6cd274a 100644
>>> --- a/common/usb_hub.c
>>> +++ b/common/usb_hub.c
>>> @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device
>>> *hub)
>>>         * Do a minimum delay of the larger value of 100ms or pgood_delay
>>>         * so that the power can stablize before the devices are queried
>>>         */
>>> -    hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
>>> +    hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
>>>
>>>        /*
>>>         * Record the power-on timeout here. The max. delay (timeout)
>>>
>>
>> We have touched this part of the delay a number of times in my
>> USB scanning patch series. I've integrated all very valuable
>> suggestions from Stephen and Hans and I'm pretty sure that the
>> current implementation is aligned with the USB spec.
>
> Sadly, not everyone goes exactly be the USB spec it seems.
> Especially the cheap sticks which are the majority in the market.
>
>> And tested
>> successfully one multiple platforms without regression.
>> Stephen did some tests on Tegra and Hans on sunxi. I tested
>> mainly on x86. But I now also tested on Armada XP (see
>> below).
>
> All of which use the same EHCI or XHCI controller, correct ?
>
>> As mentioned before, the current version causes no problems with
>> all my USB sticks on the congatec x86 board. Even the 2 ones that
>> are problematic when connected to the SoCFPGA. They are detected
>> without any issues on this board. Thats why I assume, that the
>> real problem here is the DWC driver and not the generic USB
>> handling code.
>
> The logic here is flawed. If the code works against EHCI controller and
> does not work against DWC2 controller, you cannot infer from this that
> the DWC2 controller is the problem.
>
> This can also be specific behavior of the EHCI controller, and I in fact
> suspect it is, but you cannot decide this without checking the
> bus itself.
>
>> My feeling is that increasing this initial delay
>> (before the scanning starts) just papers over the real problem
>> most likely hidden somewhere in the DWC driver. This is just
>> a feeling though which I can't prove somehow other than testing
>> the common USB code on different platforms. And I really have
>> no deeper knowledge of the DWC driver to manifest this feeling
>> or even fix this potential problem there.
>
> The bus analyzer tells me that the stick just takes longer to come out
> of FS mode and switch to HS mode when the USB started from reset state
> of the controller, I decide to trust what the analyzer shows me instead.
>
> See attached logs.
>
>> I've already mentioned this in another mail. My suggestion for
>> SoCFPGA boards that need to use these problematic USB keys is
>> to use the already available solution to set "usb_pgood_delay"
>> to 1000. This effectively does the same as this patch. Without
>> implying this general 1 second delay per hub (!!!) to all other
>> platforms that use USB in U-Boot.
>>
>> To test my suspicions about this being a DWC (SoCFPGA?) only
>> problem, I've also tested all my current USB sticks including
>> the 2 problematic ones (on SoCrates) on another ARM platform
>> (additionally to all my test on x86). I've used the Marvell
>> Armada XP development board (db-mv784mp-gp) for this. And all
>> USB sticks are detected without any problems on this platform.
>
> I see, it would be interesting to know what happens on the bus on the
> marvell board compared to the socfpga board. For the socfpga, the bus
> starts from complete cold state, could it be the marvell does have the
> EHCI running when you do your tests ? That would explain why the stick
> might be ready much faster on your platform.
>
>> As a result of all this, I would like to have this patch not
>> applied. As it negatively touches the common USB code to fix
>> (paper over?) a problem only seen on one platform (AFAIK). And
>> we already have the solution of this "usb_pgood_delay" that
>> can be used on SoCFPGA. To manifest this, here again the
>> numbers for the USB scanning time on x86, without and with
>> this patch:
>>
>> Without this patch:
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 1.940 seconds
>>
>> With this patch:
>> => time usb start
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 5.421 seconds
>
> Well that's great, removing some delays makes the code run faster and
> breaks some platform. Stefan, I need a solution for this release and
> the release is coming very quickly. So far, I see no other proposals
> how to fix this issue and it's been reported for well over a month.
>
> As I don't want to have regressions in this release, here are the two
> options:
> a) I revert "usb: Change power-on / scanning timeout handling"
> b) I apply these 7 patches. I am willing to isolate this increase
>     of delay with an ifdef to DWC2 controller, but that does NOT
>     seem correct according to the analyzer. I would just wait for
>     an EHCI controller on which this breaks.

You are missing c), use "usb_pgood_delay" here. But okay, please use
option b) for this release. We can always re-visit this issue later
and try to find a "better" way to fix this.

Thanks,
Stefan

[U-Boot] [PATCH 7/7] usb: hub: Increase the query delay

[Stephen Warren]

On 05/03/2016 02:51 PM, Marek Vasut wrote:
> Increase the query delay, otherwise some sticks are not detected.
> The problem shows up on the USB bus analyzer such that the stick
> takes longer time to switch from FS mode to HS mode than the code
> allows.

> diff --git a/common/usb_hub.c b/common/usb_hub.c

> @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>   	 * Do a minimum delay of the larger value of 100ms or pgood_delay
>   	 * so that the power can stablize before the devices are queried
>   	 */
> -	hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
> +	hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);

The comment right above that line of code should be updated to say 1000 
not 100, and to mention why we're using the non-spec value.

[U-Boot] [PATCH] usb: hub: Increase the query delay

[Marek Vasut]

Increase the query delay, otherwise some sticks are not detected.
The problem shows up on the USB bus analyzer such that the stick
takes longer time to switch from FS mode to HS mode than the code
allows when the controller starts from completely off state.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Stephen Warren <swarren@nvidia.com>
---
V2: Special-case this only for DWC2
---
 common/usb_hub.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 0f39c9f..4795ea1 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -144,8 +144,14 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 	/*
 	 * Do a minimum delay of the larger value of 100ms or pgood_delay
 	 * so that the power can stablize before the devices are queried
+	 * DWC2 (and possibly others?) needs longer time to stabilize when
+	 * coming out of cold start. 1 second seems to work reliably.
 	 */
+#ifdef CONFIG_USB_DWC2
+	hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
+#else
 	hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
+#endif
 
 	/*
 	 * Record the power-on timeout here. The max. delay (timeout)
-- 
2.7.0

[U-Boot] [PATCH] usb: hub: Increase the query delay

[Stephen Warren]

On 05/04/2016 03:32 PM, Marek Vasut wrote:
> Increase the query delay, otherwise some sticks are not detected.
> The problem shows up on the USB bus analyzer such that the stick
> takes longer time to switch from FS mode to HS mode than the code
> allows when the controller starts from completely off state.

Acked-by: Stephen Warren <swarren@nvidia.com>

(I suspect this is more to do with the board than the USB controller, 
since the board controls the VBUS power. However, if this works, it's 
probably fine for now.)

[U-Boot] [PATCH] usb: hub: Increase the query delay

[Marek Vasut]

On 05/04/2016 11:34 PM, Stephen Warren wrote:
> On 05/04/2016 03:32 PM, Marek Vasut wrote:
>> Increase the query delay, otherwise some sticks are not detected.
>> The problem shows up on the USB bus analyzer such that the stick
>> takes longer time to switch from FS mode to HS mode than the code
>> allows when the controller starts from completely off state.
> 
> Acked-by: Stephen Warren <swarren@nvidia.com>
> 
> (I suspect this is more to do with the board than the USB controller,
> since the board controls the VBUS power. However, if this works, it's
> probably fine for now.)

For now, maybe, but I am extremely unhappy to have this hack in the
common code.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/7] usb: Don't init pointer to zero, but NULL

[Stefan Roese]

On 03.05.2016 22:51, Marek Vasut wrote:
> The pointer should always be inited to NULL, not zero (0). These are
> two different things and not necessarily equal.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Stephen Warren <swarren@nvidia.com>
> ---
>   common/usb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index 4d0de4d..63429d4 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1064,7 +1064,7 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
>
>   int usb_select_config(struct usb_device *dev)
>   {
> -	unsigned char *tmpbuf = 0;
> +	unsigned char *tmpbuf = NULL;
>   	int err;
>
>   	err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE);
>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 1/7] usb: Don't init pointer to zero, but NULL

[Chin Liang See]

On Wed, 2016-05-04 at 09:36 +0200, Stefan Roese wrote:
> On 03.05.2016 22:51, Marek Vasut wrote:
> > The pointer should always be inited to NULL, not zero (0). These
> > are
> > two different things and not necessarily equal.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Stephen Warren <swarren@nvidia.com>
> > ---
> >   common/usb.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/usb.c b/common/usb.c
> > index 4d0de4d..63429d4 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -1064,7 +1064,7 @@ static int usb_prepare_device(struct
> > usb_device *dev, int addr, bool do_read,
> > 
> >   int usb_select_config(struct usb_device *dev)
> >   {
> > -	unsigned char *tmpbuf = 0;
> > +	unsigned char *tmpbuf = NULL;
> >   	int err;
> > 
> >   	err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE,
> > USB_DT_DEVICE_SIZE);
> > 
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 

Reviewed-by: Chin Liang See <clsee@altera.com>
Tested-by: Chin Liang See <clsee@altera.com>

Thanks
Chin Liang

> Thanks,
> Stefan

[U-Boot] [PATCH 1/7] usb: Don't init pointer to zero, but NULL

[Marek Vasut]

On 05/03/2016 10:51 PM, Marek Vasut wrote:
> The pointer should always be inited to NULL, not zero (0). These are
> two different things and not necessarily equal.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Stephen Warren <swarren@nvidia.com>

1..6 applied, 7 dropped in favor of
usb: dwc2: Add delay to fix the USB detection problem on SoCFPGA

Hopefully there is no more breakage in this release.

Best regards,
Marek Vasut