On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i...@shantur.com> wrote: > > Thanks Dragan, > > On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsi...@manjaro.org> wrote: > > > > Hello Shantur, > > > > 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. Thanks, Shantur