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.