From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Wed, 4 May 2016 18:27:03 +0200 Subject: [U-Boot] [PATCH 7/7] usb: hub: Increase the query delay In-Reply-To: <5729DF88.2060102@denx.de> References: <1462308680-9366-1-git-send-email-marex@denx.de> <1462308680-9366-7-git-send-email-marex@denx.de> <5729ADCC.3030609@denx.de> <5729DF88.2060102@denx.de> Message-ID: <572A22D7.8080902@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> Cc: Chin Liang See >>> Cc: Dinh Nguyen >>> Cc: Hans de Goede >>> Cc: Stefan Roese >>> Cc: Stephen Warren >>> --- >>> 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