usb: Increasing delay for USB [chromiumos/third_party/u-boot : chromeos-v2011.12]

Vivek Gautam (Code Review)

unread,

Mar 7, 2012, 10:10:34 AM3/7/12

to

Vivek Gautam has uploaded a new change for review.

Change subject: usb: Increasing delay for USB
......................................................................

usb: Increasing delay for USB

This increases the timeout and delay since some USB sticks
take a long time after system reset.

Signed-off-by: Vivek Gautam <gautam...@samsung.com>
---
M drivers/usb/host/ehci-hcd.c
1 file changed, 7 insertions(+), 2 deletions(-)

git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/u-boot refs/changes/81/17481/1
--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vincent Palatin (Code Review)

unread,

Mar 7, 2012, 12:26:57 PM3/7/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, alim....@samsung.com, Banajit Goswami

Vincent Palatin has posted comments on this change.

Change subject: usb: Increasing delay for USB
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(2 inline comments)

Please do not blindly increase the timeouts which are for most of them at or over the USB spec value and this will harm all platforms.
Either the hardware controller is broken, then put a special condition for it, or there is some missing check/timing somewhere else, please investigate it.

....................................................
File drivers/usb/host/ehci-hcd.c
Line 481: timeout *= 80;
The delay in this version of U-Boot is already 5s for bulk endpoint and 1000ms for control endpoint (twice as much as the specification value). So the problem probably lies somewhere else.

Line 805: wait_ms(20);
this penalizes the overal boot speed from USB.
Please investigate where the delay is actually needed and why.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Gerrit-Reviewer: Abhilash Kesavan <a.ke...@samsung.com>
Gerrit-Reviewer: Banajit Goswami <bana...@samsung.com>
Gerrit-Reviewer: Che-Liang Chiou <clc...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Hatim Ali R.V <hati...@samsung.com>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Simon Glass <s...@chromium.org>
Gerrit-Reviewer: Tom Wai-Hong Tam <wai...@chromium.org>
Gerrit-Reviewer: Vincent Palatin <vpal...@chromium.org>
Gerrit-Reviewer: alim....@samsung.com

Che-Liang Chiou (Code Review)

unread,

Mar 7, 2012, 5:52:51 PM3/7/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, alim....@samsung.com, Banajit Goswami

Che-Liang Chiou has posted comments on this change.

Change subject: usb: Increasing delay for USB
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

Yes, please investigate the root cause.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Che-Liang Chiou (Code Review)

unread,

Mar 7, 2012, 7:09:14 PM3/7/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, alim....@samsung.com, Banajit Goswami

Che-Liang Chiou has posted comments on this change.

Change subject: usb: Increasing delay for USB
......................................................................

Patch Set 1: I would prefer that you didn't submit this

Hit the wrong button...

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Simon Glass (Code Review)

unread,

Mar 8, 2012, 12:52:33 AM3/8/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, alim....@samsung.com, Banajit Goswami

Simon Glass has posted comments on this change.

Change subject: usb: Increasing delay for USB
......................................................................

Patch Set 1: I would prefer that you didn't submit this

--

To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Anonymous Coward (Code Review)

unread,

Mar 9, 2012, 8:07:45 AM3/9/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, alim....@samsung.com, Banajit Goswami

pad...@samsung.com has posted comments on this change.

Change subject: usb: Increasing delay for USB
......................................................................

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Gerrit-Reviewer: Abhilash Kesavan <a.ke...@samsung.com>
Gerrit-Reviewer: Banajit Goswami <bana...@samsung.com>
Gerrit-Reviewer: Che-Liang Chiou <clc...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Hatim Ali R.V <hati...@samsung.com>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Simon Glass <s...@chromium.org>
Gerrit-Reviewer: Tom Wai-Hong Tam <wai...@chromium.org>
Gerrit-Reviewer: Vincent Palatin <vpal...@chromium.org>
Gerrit-Reviewer: alim....@samsung.com

Gerrit-Reviewer: pad...@samsung.com

Vivek Gautam (Code Review)

unread,

Mar 10, 2012, 1:55:39 AM3/10/12

to Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Vivek Gautam has posted comments on this change.

Change subject: usb: Increasing delay for USB
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File drivers/usb/host/ehci-hcd.c
Line 481: timeout *= 80;

Debugging presently on this. Will come back to you as soon as possible...

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Gerrit-Reviewer: Abhilash Kesavan <a.ke...@samsung.com>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>

Gerrit-Reviewer: Banajit Goswami <bana...@samsung.com>
Gerrit-Reviewer: Che-Liang Chiou <clc...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Hatim Ali R.V <hati...@samsung.com>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Simon Glass <s...@chromium.org>
Gerrit-Reviewer: Tom Wai-Hong Tam <wai...@chromium.org>
Gerrit-Reviewer: Vincent Palatin <vpal...@chromium.org>

Gerrit-Reviewer: Vivek Gautam <gautam...@samsung.com>

Simon Glass (Code Review)

unread,

Mar 24, 2012, 1:45:56 AM3/24/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Simon Glass has posted comments on this change.

Change subject: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

(1 inline comment)

Good work!

Please add a gen: tag indicating that it will be upstreamed to U-Boot non-architecture code.

....................................................
File drivers/usb/host/ehci-hcd.c
Line 461: ping_token:
Can we use

do {

} while ()

please instead of the goto?

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vincent Palatin (Code Review)

unread,

Mar 27, 2012, 1:14:45 PM3/27/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Vincent Palatin has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 4: I would prefer that you didn't submit this

(4 inline comments)

IMO The PING protocol is implemented in hardware in the EHCI controller (you can probably see it with a protocol analyzer without this CL)
The improvement seen with CL is probably the side effect of adding an infinite retry on timeout (which has bitten us in the past by freezing the boot when defective devices are plugged or the controller got a none recoverable error).

....................................................
File drivers/usb/host/ehci-hcd.c
Line 496: printf("EHCI timed out on TD - token=%#x\n", token);
if we consider this code path to be the normal NAK/PING flow, we should not unconditionally display an error message.

Line 502: * After a device NAKs an OUT transaction host sends
we should explicitly check we are in the right condition to need a PING : ie QH.EPS == High speed and PID_code == OUT endpoint

Line 508: token |= ((toggle << 31) | (1 << 0));
I don't think this is how the PING mechanism is working on an EHCI controller.
The wording of the EHCI spec is not fully clear, but IMO
the hardware controller is responsible for setting the next state of the PING state machine (ie setting the bit 0 to 1 when we were in OUT state and we receive a NAK or NYET)
The software is only supposed to initialize it (usually reset it) before the first transaction.

Line 513: } while (token & 0x80);
If we are getting a real timeout, I'm under the impression we are going to loop forever here.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vivek Gautam (Code Review)

unread,

Mar 30, 2012, 1:04:30 AM3/30/12

to Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Vivek Gautam has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 4: (1 inline comment)

Thanks for review. I tested using a protocol analyzer but noticed that during Request sense Bulk OUT, a NYET is recieved and host waits for almost 600ms before resetting the device.
When i rechecked this with linux kernel 3.0 running on the target board, host was sending a PING in the same situation on receiving a NYET.

....................................................
File drivers/usb/host/ehci-hcd.c

Line 508: token |= ((toggle << 31) | (1 << 0));

Hi Mr Palatin
Thanks for the review. But i can see in the linux kernel source (drivers/usb/host/ehci-q.c) that the host controller issues PING (setting the bit to 1) with qTD token for Bulk transfer. Please suggest its possibility over here.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Simon Glass (Code Review)

unread,

Mar 30, 2012, 2:09:06 PM3/30/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Simon Glass has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 4: Looks good to me, approved

(1 inline comment)

LGTM with nit

....................................................
File drivers/usb/host/ehci-hcd.c
Line 496: printf("EHCI timed out on TD - token=%#x\n", token);

If we have the (or a) root cause of this now, perhaps we should change this to debug()

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Hatim Ali R.V (Code Review)

unread,

Mar 30, 2012, 2:32:46 PM3/30/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Alim Akhtar, Banajit Goswami

Hatim Ali R.V has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 4: Looks good to me, but someone else must approve

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vincent Palatin (Code Review)

unread,

Mar 30, 2012, 2:43:52 PM3/30/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Vincent Palatin has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 4: (1 inline comment)

....................................................
File drivers/usb/host/ehci-hcd.c
Line 508: token |= ((toggle << 31) | (1 << 0));

I still fail to find that code in the upstream Linux kernel.
Can you point me at the exact code in drivers/usb/host/ehci-q.c ?

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vivek Gautam (Code Review)

unread,

Mar 31, 2012, 4:37:47 AM3/31/12

to Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Vivek Gautam has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 4: (1 inline comment)

....................................................
File drivers/usb/host/ehci-hcd.c
Line 508: token |= ((toggle << 31) | (1 << 0));

Please refer to the function "qh_update ()" function in the same file. The root call of this function lies in the ehci-host.c where a call to "submit_sync()" reach down to this function.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Prashanth Godrehal (Code Review)

unread,

Mar 31, 2012, 9:00:21 AM3/31/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Prashanth Godrehal has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 5: Looks good to me, but someone else must approve

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 5

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vivek Gautam (Code Review)

unread,

Apr 2, 2012, 5:32:49 AM4/2/12

to Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Vivek Gautam has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 5: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 5

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Simon Glass (Code Review)

unread,

Apr 2, 2012, 6:49:59 PM4/2/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Simon Glass has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 5: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 5

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vincent Palatin (Code Review)

unread,

Apr 2, 2012, 7:03:05 PM4/2/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Vincent Palatin has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 4: (1 inline comment)

sorry, I still don't see it.

....................................................
File drivers/usb/host/ehci-hcd.c
Line 508: token |= ((toggle << 31) | (1 << 0));

There is neither "submit_sync" nor "ehci-host.c" in upstream, I suppose you are referring to "submit_async" and "ehci-hcd.c".
In qh_update() function, the code is only doing :
hw->hw_token &= QTD_STS_PING
is there something else setting the ping bit in the token ?

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vincent Palatin (Code Review)

unread,

Apr 2, 2012, 7:07:03 PM4/2/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Vincent Palatin has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 5: I would prefer that you didn't submit this

(1 inline comment)

my comments on patchset #4 are not addressed.

....................................................
File drivers/usb/host/ehci-hcd.c

Line 513: } while (token & 0x80);

as already commented in patchset #4, this will loop forever and block the boot if a bad/unsupported USB device is plugged to the device.
That will trigger unnecessary RMA in the field.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 5

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Prashanth Godrehal (Code Review)

unread,

Apr 6, 2012, 7:41:09 AM4/6/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Hatim Ali R.V, Alim Akhtar, Banajit Goswami

Prashanth Godrehal has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 5:

For Vincent comment, I suggest we can add some counter check for exit option if that solves the purpose of infinite loop. Vivek, is that something we can do?

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 5

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Simon Glass (Code Review)

unread,

Apr 8, 2012, 11:17:45 PM4/8/12

to Hatim Ali R.V, Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Abhilash Kesavan, Prashanth Godrehal, Alim Akhtar, Banajit Goswami

Simon Glass has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 6: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File drivers/usb/host/ehci-hcd.c
Line 515: } while ((token & 0x80) && (count < 50));
How long before this times out please?

I would prefer a global timeout, like:

loop_timeout = get_timer(0)
do {
...
} while ((token & 0x80) && get_timer(loop_timeout) < 5 * 1000)

for a 5s timeout

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vincent Palatin (Code Review)

unread,

Apr 11, 2012, 11:34:47 AM4/11/12

to Hatim Ali R.V, Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Alim Akhtar, Banajit Goswami, Thomas Abraham

Vincent Palatin has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 7: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File drivers/usb/host/ehci-hcd.c
Line 515: } while ((token & 0x80) && (get_timer(loop_timeout) < 300*1000));
that's a 5 minutes timeout (ie 300 seconds) ?
Nothing requires that amount of time and it might freeze the user machine at boot for that duration...

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Gerrit-Reviewer: Abhilash Kesavan <a.ke...@samsung.com>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Banajit Goswami <bana...@samsung.com>
Gerrit-Reviewer: Che-Liang Chiou <clc...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Hatim Ali R.V <hati...@samsung.com>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Simon Glass <s...@chromium.org>

Gerrit-Reviewer: Thomas Abraham <thom...@samsung.com>

Vivek Gautam (Code Review)

unread,

Apr 15, 2012, 2:43:24 AM4/15/12

to Hatim Ali R.V, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Alim Akhtar, Banajit Goswami, Thomas Abraham

Vivek Gautam has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 7: (1 inline comment)

....................................................
File drivers/usb/host/ehci-hcd.c
Line 515: } while ((token & 0x80) && (get_timer(loop_timeout) < 300*1000));

the code "get_timer()" is written in the file "arch/arm/cpu/armv7/s5p-common/timer.c" in such a way that it returns the number of PWM timer ticks relative to a base value. So for each iteration, while verifying the while condition, only the number of timer ticks are compared to timeout value.
If lets suppose for the first iteration the get_timer would have returned 2000 ticks, the next time it would have crossed 8000(say). So the delay does not become 5 minutes effectively.
Can you please confirm me on this point if i am right or not?
And please suggest the appropriate change of code.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vincent Palatin (Code Review)

unread,

Apr 16, 2012, 11:39:51 AM4/16/12

to Hatim Ali R.V, Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Alim Akhtar, Banajit Goswami, Thomas Abraham

Vincent Palatin has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 7: (1 inline comment)

....................................................
File drivers/usb/host/ehci-hcd.c
Line 515: } while ((token & 0x80) && (get_timer(loop_timeout) < 300*1000));
> the code "get_timer()" is written in the file
> "arch/arm/cpu/armv7/s5p-common/timer.c" in such a way
> that it returns the number of PWM timer ticks
> relative to a base value.

that's indeed the convention for all platform inside U-Boot.

> So for each iteration, while verifying the while condition, only
> the number of timer ticks are compared to timeout value. If
> lets suppose for the first iteration the get_timer would have
> returned 2000 ticks, the next time it would have crossed
> 8000(say). So the delay does not become 5 minutes
> effectively. Can you please confirm me on this point if i am
> right or not?

Sure it returns the number of ticks relative to the timeout baseline store in loop_timeout.
By the way, we are using 1ms tick, so here 300*1000 ticks = 300*1000ms = 300s = 5 minutes.
As a consequence, the loop will exit 5 minutes after we did "loop_timeout = get_timer(0)"

> And please suggest the appropriate change of code.

My concern is the value of the timeout not the code.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vivek Gautam (Code Review)

unread,

Apr 21, 2012, 3:27:14 AM4/21/12

to Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami, Thomas Abraham

Vivek Gautam has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8: (1 inline comment)

....................................................
File drivers/usb/host/ehci-hcd.c
Line 515: } while ((token & 0x80) && (get_timer(loop_timeout) < 60*1000));
timeout is optimized to 1 minute keeping in mind few usb sticks which may take more time.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 8

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Prashanth Godrehal (Code Review)

unread,

Apr 23, 2012, 10:39:43 PM4/23/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Hatim Ali R.V, Alim Akhtar, Banajit Goswami, Thomas Abraham

Prashanth Godrehal has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 8

Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>

Vivek Gautam (Code Review)

unread,

Apr 24, 2012, 5:44:46 AM4/24/12

to Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami, Thomas Abraham

Vivek Gautam has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8: Verified

Banajit Goswami (Code Review)

unread,

Apr 25, 2012, 10:36:18 AM4/25/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Thomas Abraham

Banajit Goswami has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8: Looks good to me, but someone else must approve

Hi Vincent, does it look good now?

Vadim Bendebury (Code Review)

unread,

May 3, 2012, 2:12:37 PM5/3/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami, Thomas Abraham, Vadim Bendebury

Vadim Bendebury has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8:

This patch does not fix the problem with USB stick discovery on the x86 platforms...

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 8
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>
Gerrit-Reviewer: Abhilash Kesavan <a.ke...@samsung.com>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Banajit Goswami <bana...@samsung.com>
Gerrit-Reviewer: Che-Liang Chiou <clc...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Hatim Ali R.V <hati...@samsung.com>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Simon Glass <s...@chromium.org>
Gerrit-Reviewer: Thomas Abraham <thom...@samsung.com>
Gerrit-Reviewer: Tom Wai-Hong Tam <wai...@chromium.org>

Gerrit-Reviewer: Vadim Bendebury <vbe...@chromium.org>

Simon Glass (Code Review)

unread,

May 3, 2012, 7:40:55 PM5/3/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami, Thomas Abraham

Simon Glass has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8:

Please confirm if this is fixed by https://gerrit.chromium.org/gerrit/#change,21830

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a92cd17ef7a974d8083b4c953c5bf4069e0c78
Gerrit-PatchSet: 8
Gerrit-Project: chromiumos/third_party/u-boot
Gerrit-Branch: chromeos-v2011.12
Gerrit-Owner: Vivek Gautam <gautam...@samsung.com>
Gerrit-Reviewer: Abhilash Kesavan <a.ke...@samsung.com>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Banajit Goswami <bana...@samsung.com>
Gerrit-Reviewer: Che-Liang Chiou <clc...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Hatim Ali R.V <hati...@samsung.com>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Simon Glass <s...@chromium.org>
Gerrit-Reviewer: Thomas Abraham <thom...@samsung.com>
Gerrit-Reviewer: Tom Wai-Hong Tam <wai...@chromium.org>

Simon Glass (Code Review)

unread,

May 15, 2012, 9:33:21 PM5/15/12

to Vivek Gautam, Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami, Thomas Abraham

Simon Glass has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8:

ping!

Vivek Gautam (Code Review)

unread,

May 16, 2012, 12:13:45 PM5/16/12

to Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami, Thomas Abraham

Vivek Gautam has posted comments on this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8: No score

This patch from Vadim Bendebury does not fix the issue for us.
However, we have identified the problem with get_timer() api for s5p. get_timer() is supposed to give miliseconds resolution, but that does not seems to be happening, and since the ehci-host controller makes use of get_timer() few slow devices did not seems to be responding within that time. I'll get back with the fix for this.

Vivek Gautam (Code Review)

unread,

May 23, 2012, 1:37:17 PM5/23/12

to Vincent Palatin, Tom Wai-Hong Tam, Doug Anderson, Che-Liang Chiou, Simon Glass, Abhilash Kesavan, Prashanth Godrehal, Hatim Ali R.V, Alim Akhtar, Banajit Goswami, Thomas Abraham

Vivek Gautam has abandoned this change.

Change subject: gen: usb: ehci: Add PING protocol support in EHCI HCD
......................................................................

Patch Set 8: Abandoned

This patch no longer needed. Timer is already fixed and that solves the problem of usb detection. So abandoning this patch.

--
To view, visit https://gerrit.chromium.org/gerrit/17481
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: abandon

Reply all

Reply to author

Forward