Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On Wed, Feb 14, 2024 at 9:56 AM Shantur Rathore wrote:
>
> On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic 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 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
>
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic 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 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.
> >>> >>>
> >>> >>>
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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 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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Hello Andre,
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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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:
> >>
> >> Thanks Dragan,
> >>
> >> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic 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
> > 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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On 2/13/24 04:59, Dragan Simic wrote:
On 2024-02-13 00:33, Marek Vasut wrote:
On 2/12/24 22:10, Dragan Simic wrote:
On 2024-02-12 21:19, 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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On 2024-02-13 00:33, Marek Vasut wrote:
On 2/12/24 22:10, Dragan Simic wrote:
On 2024-02-12 21:19, 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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On 2/12/24 22:10, Dragan Simic wrote:
Hello Marek, Andre and Shantur,
On 2024-02-12 21:19, 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.
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Hello Marek, Andre and Shantur,
On 2024-02-12 21:19, 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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On 2/12/24 14:41, Shantur Rathore wrote:
On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore wrote:
Thanks Dragan,
On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic 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
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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Hello Shantur,
On 2024-02-12 14:40, 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
s/some USB 3.0 flash/some USB 2.0 and 3.0 flash/
I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
As visible in the message linked below, there was one USB 3.0 flash
drive that didn't work in a USB 2.0 port. Though, you're right, there
was also a USB 2.0 drive that didn't work. Thus, it's perhaps best to
adjust the wording as I suggested above and below.
https://lore.kernel.org/u-boot/20240202001218.63b4e...@minigeek.lan/
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
s/some tested USB 3.0 flash drives/some tested USB 2.0 and 3.0 flash
drives/
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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore wrote:
>
> Thanks Dragan,
>
> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic 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
> > >> 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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Thanks Dragan,
On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic 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
> >> 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
>
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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
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
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
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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.
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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.
I am down with the flu so that would really help me as my brain is
working at 15% capacity.
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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. :)
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On 2/8/24 12:30, Shantur Rathore wrote:
Hi Marek,
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.
Thanks
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Hi Marek,
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.
Kind regards,
Shantur
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
On 2024-02-07 14:14, Dragan Simic wrote:
Hello Shantur,
Please see my notes below.
On 2024-02-07 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.
Tested-by: Andre Przywara
Signed-off-by: Shantur Rathore
Please, add the "Fixes" line available below, reformat the patch
description to the usual 78-character width, and remove unnecessary
"FIX " from the patch subject prefix.
Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
As a side note, such tags can be produced very easily by running
"git log --max-count=1 --pretty=fixes ". I have an alias
like that defined in my ~/.gitconfig.
With these changes applied, please feel free to also add:
Reviewed-by: Dragan Simic
Oops, sorry for the typo... s/anjaro/manjaro/
---
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)) {
+ usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
+ debug("Reset : port %d returns %lX\n", i + 1,
dev->status);
+ }
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
}
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Hello Shantur,
Please see my notes below.
On 2024-02-07 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.
Tested-by: Andre Przywara
Signed-off-by: Shantur Rathore
Please, add the "Fixes" line available below, reformat the patch
description to the usual 78-character width, and remove unnecessary
"FIX " from the patch subject prefix.
Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before scanning")
As a side note, such tags can be produced very easily by running
"git log --max-count=1 --pretty=fixes ". I have an alias
like that defined in my ~/.gitconfig.
With these changes applied, please feel free to also add:
Reviewed-by: Dragan Simic
---
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)) {
+ usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
+ debug("Reset : port %d returns %lX\n", i + 1,
dev->status);
+ }
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
}
Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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 ?
[FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
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.
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)) {
+ usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
+ debug("Reset : port %d returns %lX\n", i + 1,
dev->status);
+ }
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
debug("PowerOn : port %d returns %lX\n", i + 1, dev->status);
}
--
2.40.1
