On Wed, Feb 14, 2024 at 9:56 AM Shantur Rathore <i...@shantur.com> wrote: > > On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic <dsi...@manjaro.org> wrote: > > > > Hello Shantur and Marek, > > > > 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 <ma...@denx.de> wrote: > > >>> On 2/12/24 14:41, Shantur Rathore wrote: > > >>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i...@shantur.com> > > >>> > wrote: > > >>> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsi...@manjaro.org> > > >>> >> 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 <dsi...@manjaro.org> > > >>> >>>>> 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 <ma...@denx.de> > > >>> >>>>>>>> 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 <andre.przyw...@arm.com> > > >>> >>>>>>>>>> > > >>> >>>>>>>>>> Signed-off-by: Shantur Rathore <i...@shantur.com> > > >>> >>>>>>>>>> --- > > >>> >>>>>>>>>> > > >>> >>>>>>>>>> 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-...@shantur.com/T/#u > > >>> >>> Link: > > >>> >>> https://lore.kernel.org/u-boot/20240201164604.13315...@donnerap.manchester.arm.com/T/#u > > >>> >>> Signed-off-by: Shantur Rathore <i...@shantur.com> > > >>> >>> Helped-by: Dragan Simic <dsi...@manjaro.org> > > >>> >>> Tested-by: Andre Przywara <andre.przyw...@arm.com> > > >>> >>> Reviewed-by: Dragan Simic <dsi...@manjaro.org> > > >>> >>> ------- >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-...@shantur.com/T/#u > > Link: > > https://lore.kernel.org/u-boot/20240201164604.13315...@donnerap.manchester.arm.com/T/#u > > Signed-off-by: Shantur Rathore <i...@shantur.com> > > Helped-by: Dragan Simic <dsi...@manjaro.org> > > Tested-by: Andre Przywara <andre.przyw...@arm.com> > > Reviewed-by: Dragan Simic <dsi...@manjaro.org> > > ------- >8 ------------- >8 ------------- >8 ------------- >8 ------- > > > > Shantur, I hope you're feeling better. > > Thanks Dragan, V2 is submitted. > I am feeling much better, thanks for asking.
PS : I don't think I could ever explain as well as you did Dragan. Thanks a lot for the help. I will bring another pending matter to your attention on the Linux list now ;P