From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2E712C48BEB for ; Wed, 14 Feb 2024 10:23:22 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 73C0487DD5; Wed, 14 Feb 2024 11:23:20 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=manjaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=manjaro.org header.i=@manjaro.org header.b="TkgCu69R"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 395EF87DF3; Wed, 14 Feb 2024 11:23:19 +0100 (CET) Received: from mail.manjaro.org (mail.manjaro.org [IPv6:2a01:4f8:c0c:51f3::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 799B787C6D for ; Wed, 14 Feb 2024 11:23:16 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=manjaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=dsimic@manjaro.org MIME-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=manjaro.org; s=2021; t=1707906195; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wS4dTCukWabCop4LjE1W9Ofw709pfGGHp2r3yPHvh4A=; b=TkgCu69RnysgrHhM23efa7//fGgma8C7OlYBhaFDGZENtBXEwsqDyfTvaSHB1VnYHQztcU evUCA2SqkYwvLu7T1TICBBajrnYvP2iZwWzeVv+d3nBoh3zT9yYON+HMGzrrimEggzUGrQ 3UN0bG1ytWIHUGWJ4O5le+oD+tTYmTODS1C9VNYKHW1JRNSgILuEud61zh6VhzOmQHfLna PM7ldNZyPctyJ9xojJipMRpts8K2vq/lj6AjNKTu93Sbn7jINPXFNbaCnyddzcxgGzs1SJ vsfSKfSiskvjzuffBlWBpcpKinTuclVnQV7THOIXn1e9lv9B4BSkb++TLLo0lA== Date: Wed, 14 Feb 2024 11:23:14 +0100 From: Dragan Simic To: Shantur Rathore Cc: Andre Przywara , Marek Vasut , u-boot@lists.denx.de, Hector Martin , Simon Glass , Tom Rini Subject: Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub In-Reply-To: References: <20240207102327.35125-1-i@shantur.com> <32db6dba-d515-4077-9d72-d77a7f431335@denx.de> <93f69779-3197-4ede-bb78-f2512ec30fc1@denx.de> <24f25359ce213370214bceeabc55e1df@manjaro.org> <3aa6998e3d9fd97cd4ba729f7656f467@manjaro.org> <20240214020413.662bfac6@minigeek.lan> Message-ID: X-Sender: dsimic@manjaro.org Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Authentication-Results: ORIGINATING; auth=pass smtp.auth=dsimic@manjaro.org smtp.mailfrom=dsimic@manjaro.org X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On 2024-02-14 11:01, Shantur Rathore wrote: > On Wed, Feb 14, 2024 at 9:56 AM Shantur Rathore wrote: >> On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic >> wrote: >> > On 2024-02-14 04:18, Dragan Simic wrote: >> > > On 2024-02-14 03:04, Andre Przywara wrote: >> > >> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut wrote: >> > >>> On 2/12/24 14:41, Shantur Rathore wrote: >> > >>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore wrote: >> > >>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic wrote: >> > >>> >>> On 2024-02-08 15:17, Dragan Simic wrote: >> > >>> >>>> On 2024-02-08 15:10, Shantur Rathore wrote: >> > >>> >>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic >> > >>> >>>>> wrote: >> > >>> >>>>>> On 2024-02-08 14:33, Marek Vasut wrote: >> > >>> >>>>>>> On 2/8/24 12:30, Shantur Rathore wrote: >> > >>> >>>>>>>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut wrote: >> > >>> >>>>>>>>> On 2/7/24 11:23, Shantur Rathore wrote: >> > >>> >>>>>>>>>> USB 3.0 spec requires hub to reset device while >> > >>> >>>>>>>>>> enumeration. Some USB 2.0 hubs / devices don't >> > >>> >>>>>>>>>> handle this well and after implementation of >> > >>> >>>>>>>>>> reset some USB 2.0 disks weren't detected on >> > >>> >>>>>>>>>> Allwinner based boards. >> > >>> >>>>>>>>>> >> > >>> >>>>>>>>>> Resetting only when hub is USB 3.0 fixes it. >> > >>> >>>>>>>>> >> > >>> >>>>>>>>> It would be good to include as many details about the faulty hardware >> > >>> >>>>>>>>> in >> > >>> >>>>>>>>> the commit message as possible, so that when someone else runs into >> > >>> >>>>>>>>> this, they would have all that information available. >> > >>> >>>>>>>>> >> > >>> >>>>>>>>>> Tested-by: Andre Przywara >> > >>> >>>>>>>>>> >> > >>> >>>>>>>>>> Signed-off-by: Shantur Rathore >> > >>> >>>>>>>>>> --- >> > >>> >>>>>>>>>> >> > >>> >>>>>>>>>> common/usb_hub.c | 6 ++++-- >> > >>> >>>>>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >> > >>> >>>>>>>>>> >> > >>> >>>>>>>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c >> > >>> >>>>>>>>>> index 3fb7e14d10..2e054eb935 100644 >> > >>> >>>>>>>>>> --- a/common/usb_hub.c >> > >>> >>>>>>>>>> +++ b/common/usb_hub.c >> > >>> >>>>>>>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct >> > >>> >>>>>>>>>> usb_hub_device *hub) >> > >>> >>>>>>>>>> >> > >>> >>>>>>>>>> debug("enabling power on all ports\n"); >> > >>> >>>>>>>>>> for (i = 0; i < dev->maxchild; i++) { >> > >>> >>>>>>>>>> - usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET); >> > >>> >>>>>>>>>> - debug("Reset : port %d returns %lX\n", i + 1, >> > >>> >>>>>>>>>> dev->status); >> > >>> >>>>>>>>>> + if (usb_hub_is_superspeed(dev)) { >> > >>> >>>>>>>>> >> > >>> >>>>>>>>> Should this condition be "all which are lower than superspeed" >> > >>> >>>>>>>>> instead , >> > >>> >>>>>>>>> so when the next generation of USB comes, this problem won't trigger >> > >>> >>>>>>>>> ? >> > >>> >>>>>>>>> >> > >>> >>>>>>>>> What does Linux do btw ? >> > >>> >>>>>>>> >> > >>> >>>>>>>> As of now Linux checks if the hub is superspeed >> > >>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859 >> > >>> >>>>>>>> >> > >>> >>>>>>>> which is >> > >>> >>>>>>>> return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; // >> > >>> >>>>>>>> USB_HUB_PR_SS = 3 >> > >>> >>>>>>>> >> > >>> >>>>>>>> This holds true for newer SuperSpeedPlus hubs as well. >> > >>> >>>>>>>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155 >> > >>> >>>>>>>> >> > >>> >>>>>>>> We can change the check to be bDeviceProtocol > 2 but who knows if >> > >>> >>>>>>>> things change in the newer version of spec. >> > >>> >>>>>>>> I am open to suggestions. >> > >>> >>>>>>> >> > >>> >>>>>>> Please just include the ^ in the commit description. Use link to >> > >>> >>>>>>> git.kernel.org , not some mirror . This is extremely useful >> > >>> >>>>>>> information and, well, you already wrote the V2 commit message >> > >>> >>>>>>> addition in this answer. >> > >>> >>>>>> >> > >>> >>>>>> Shantur, if that would be easier or quicker for you, I can write >> > >>> >>>>>> a quite detailed patch description for you, in exchange for a >> > >>> >>>>>> "Helped-by" tag in the v2 patch submission. :) >> > >>> >>>>> >> > >>> >>>>> That would be really kind of you Dragan. >> > >>> >>>> >> > >>> >>>> Sure, I'll write the summary and send it over. >> > >>> >>>> >> > >>> >>>>> I am down with the flu so that would really help me as my brain is >> > >>> >>>>> working at 15% capacity. >> > >>> >>>> >> > >>> >>>> Oh, I'm really sorry to hear that. :( I hope you'll get better >> > >>> >>>> soon, and I know very well what's it like; I've also been sick >> > >>> >>>> recently, as a result of some kind of flu that unfortunately found >> > >>> >>>> its way into my lungs, and it took me about a month to get back >> > >>> >>>> to about 90% of my usual mental capacity. I'm still not back to >> > >>> >>>> exactly 100%. :/ >> > >>> >>>> >> > >>> >>>> I really hope you'll recover much faster. >> > >>> >>> >> > >>> >>> I hope you're feeling better. >> > >>> >>> >> > >>> >>> Below are the patch subject and description that I prepared for you, >> > >>> >>> please have a look. >> > >>> >>> >> > >>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 ------- >> > >>> >>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only >> > >>> >>> >> > >>> >>> Additional testing of the changes introduced in commit 33e06dcbe57a >> > >>> >>> ("common: >> > >>> >>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0 >> > >>> >>> flash >> > >>> >> >> > >>> >> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port. >> > >>> >> >> > >>> >>> drives didn't work in U-Boot on some Allwinner SoCs that support USB 2.0 >> > >>> >>> only. >> > >>> >>> More precisely, some tested USB 3.0 flash drives failed to be detected >> > >>> >>> and >> > >>> >>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which supports USB >> > >>> >>> 2.0 >> > >>> >>> only, while the same USB flash drives worked just fine on a Pine64 H64 >> > >>> >>> with >> > >>> >>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0. >> > >>> >>> >> > >>> >>> Resetting USB 3.0 hubs only has been tested to work as expected, >> > >>> >>> resolving >> > >>> >>> the previous issues on the Allwinner H616, while not introducing any new >> > >>> >>> issues on other Allwinner SoCs. Thus, let's fix it that way. >> > >>> >>> >> > >>> >>> According to the USB 3.0 specification, resetting a USB 3.0 port is >> > >>> >>> required >> > >>> >>> when an attached USB device transitions between different states, such >> > >>> >>> as >> > >>> >>> when it resumes from suspend. Though, the Linux kernel performs >> > >>> >>> additional >> > >>> >>> USB 3.0 port resets upon initial USB device attachment, presumably to >> > >>> >>> ensure >> > >>> >>> proper state of the USB 3.0 hub port and proper USB mode negotiation >> > >>> >>> during >> > >>> >>> the initial USB device attachment and enumeration. >> > >>> >>> >> > >>> >>> Such USB port resets don't seem to exist for USB 2.0 hubs, according the >> > >>> >>> USB >> > >>> >>> 2.0 specification. The resets seem to be added to the USB 3.0 >> > >>> >>> specification >> > >>> >>> as part of the port and device mode negotiation. >> > >>> >>> >> > >>> >>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as >> > >>> >>> visible >> > >>> >>> in file drivers/usb/core/hub.c in the kernel source, line 2859. This >> > >>> >>> check >> > >>> >>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well, >> > >>> >>> which >> > >>> >>> hopefully makes it future proof. >> > >>> >>> >> > >>> >>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning") >> > >>> >>> Link: >> > >>> >>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u >> > >>> >>> Link: >> > >>> >>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u >> > >>> >>> Signed-off-by: Shantur Rathore >> > >>> >>> Helped-by: Dragan Simic >> > >>> >>> Tested-by: Andre Przywara >> > >>> >>> Reviewed-by: Dragan Simic >> > >>> >>> ------- >8 ------------- >8 ------------- >8 ------------- >8 ------- >> > >>> >> >> > >>> >> Regards, >> > >>> >> Shantur >> > >>> > >> > >>> > Is this description acceptable to you Marek. >> > >>> >> > >>> Please send a V2 patch . If possible, include the device information >> > >>> as >> > >>> reported by Andre, esp. which USB stick triggered it, including USB >> > >>> IDs, >> > >>> this is important for future reference and in case someone has >> > >>> similar >> > >>> failure. >> > >> >> > >> So the USB 2.0 stick is some no-name, unlabelled and super cheap one, >> > >> I >> > >> think we bought a pack of it, just for boot-strapping machines. The >> > >> USB >> > >> ID of "abcd:1234" kind of gives away that this is bogus AF. >> > >> The USB 3.0 stick is a PNY 32GB one, the USB ID is: >> > >> 1f75:0917 Innostor Technology Corporation IS917 Mass storage >> > >> >> > >> Hope that helps. >> > > >> > > Thank you for replying. I'll include the available information into >> > > the revised commit description and send it over a bit later. >> > >> > Here are the revised commit subject and description, please have a look >> > and let me know if further improvements are needed. >> > >> > ------- >8 ------------- >8 ------------- >8 ------------- >8 ------- >> > [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only >> > >> > Additional testing of the changes introduced in commit 33e06dcbe57a >> > ("common: >> > usb-hub: Reset hub port before scanning") revealed that some USB 2.0 and >> > 3.0 >> > flash drives didn't work in U-Boot on some Allwinner SoCs that support >> > USB >> > 2.0 interfaces only. More precisely, some of the tested USB 2.0 and 3.0 >> > flash drives failed to be detected and work on an OrangePi Zero 3, based >> > on >> > the Allwinner H616 SoC that supports USB 2.0 only, while the same USB >> > flash >> > drives worked just fine on a Pine64 H64, based on the Allwinner H6 SoC >> > that >> > supports both USB 2.0 and USB 3.0 interfaces. >> > >> > The USB ID of the above-mentioned USB 3.0 flash drive that failed to >> > work is >> > 1f75:0917 (Innostor Technology Corporation IS917 Mass storage), it is 32 >> > GB >> > in size and sold under the PNY brand. The mentioned USB 2.0 drive is >> > some >> > inexpensive no-name drive with an invalid USB ID. >> > >> > Resetting USB 3.0 hubs only, which this patch introduces to the USB hub >> > resets, has been tested to work as expected, resolving the identified >> > issues >> > on the Allwinner H616, while not introducing any new issues on other >> > tested >> > Allwinner SoCs. Thus, let's fix it that way. >> > >> > According to the USB 3.0 specification, resetting a USB 3.0 port is >> > required >> > when an attached USB device transitions between different states, such >> > as >> > when it resumes from suspend. Though, the Linux kernel performs >> > additional >> > USB 3.0 port resets upon initial USB device attachment, as visible in >> > commit >> > 07194ab7be63 ("USB: Reset USB 3.0 devices on (re)discovery") in the >> > kernel >> > source, to ensure proper state of the USB 3.0 hub port and proper USB >> > mode >> > negotiation during the initial USB device attachment and enumeration. >> > >> > These additional types of USB port resets don't exist for USB 2.0 hubs, >> > according the USB 2.0 specification. The resets seem to be added to the >> > USB >> > 3.0 specification as part of the port and device mode negotiation. >> > >> > The Linux kernel resets USB 3.0 (i.e. SuperSpeed) hubs only, as visible >> > in >> > commit 10d674a82e55 ("USB: When hot reset for USB3 fails, try warm >> > reset.") >> > in the kernel source. The check for SuperSpeed hubs is performed in a >> > way >> > that also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as >> > well, >> > which hopefully makes it future proof. >> > >> > Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning") >> > Link: >> > https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u >> > Link: >> > https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u >> > Signed-off-by: Shantur Rathore >> > Helped-by: Dragan Simic >> > Tested-by: Andre Przywara >> > Reviewed-by: Dragan Simic >> > ------- >8 ------------- >8 ------------- >8 ------------- >8 ------- >> > >> > Shantur, I hope you're feeling better. >> >> Thanks Dragan, V2 is submitted. >> I am feeling much better, thanks for asking. It's good to hear that you're feeling better! > PS : I don't think I could ever explain as well as you did Dragan. > Thanks a lot for the help. I'm glad to help. :) We're here to work together by combining our skills and strengths. > I will bring another pending matter to your attention on the Linux list > now ;P Is it about the USB regulators on the RockPro64? :) I haven't forgotten that, and I expect to work on that rather soon.