[PATCH v3] common: usb-hub: Reset hub port before scanning

[Andre Przywara]

On Thu, 1 Feb 2024 18:35:28 +0000
Shantur Rathore <i at shantur.com> wrote:

Hi Shantur,

> Hi Andre,
> 
> On Thu, Feb 1, 2024 at 4:46 PM Andre Przywara <andre.przywara at arm.com> wrote:
> >
> > On Thu, 1 Feb 2024 09:39:54 +0000
> > Shantur Rathore <i at shantur.com> wrote:
> >
> > Hi Shantur,
> >  
> > > On Tue, Jan 30, 2024 at 2:01 PM Andre Przywara <andre.przywara at arm.com> wrote:  
> > > >
> > > > On Sat,  9 Dec 2023 18:10:56 +0000
> > > > Shantur Rathore <i at shantur.com> wrote:
> > > >
> > > > Hi,
> > > >  
> > > > > Currently when a hub is turned on, all the ports are powered on.
> > > > > This works well for hubs which have individual power control.
> > > > >
> > > > > For the hubs without individual power control this has no effect.
> > > > > Mostly in these scenarios the hub port is powered before the USB
> > > > > controller is enabled, this can lead to some devices in unexpected
> > > > > state.
> > > > >
> > > > > With this patch, we explicitly reset the port while powering up hub
> > > > > This resets the port for hubs without port power control and has
> > > > > no effect on hubs with port power control as the port is still off.
> > > > >
> > > > > Before this patch AMicro AM8180 based NVME to USB adapter won't be
> > > > > detected as a USB3.0 Mass Storage device but with this it works as
> > > > > expected.
> > > > >
> > > > > Tested working after this patch:
> > > > > 1. AMicro AM8180 based NVME to USB Adapter
> > > > > 2. Kingston DataTraveler 3.0
> > > > > 3. GenesysLogic USB3.0 Hub
> > > > >
> > > > > The drives were tested while connected directly and via the hub.  
> > > >
> > > > so this broke USB operation on some Allwinner boards. The ports still
> > > > enumerate correctly, but no devices are detected. With mainline
> > > > (containing this patch), and a USB stick connected:
> > > > starting USB...
> > > > Bus usb at 5200000: USB EHCI 1.00
> > > > Bus usb at 5200400: USB OHCI 1.0
> > > > scanning bus usb at 5200000 for devices... 1 USB Device(s) found
> > > > scanning bus usb at 5200400 for devices... 1 USB Device(s) found
> > > >        scanning usb for storage devices... 0 Storage Device(s) found
> > > >
> > > > With this patch here reverted:
> > > > starting USB...
> > > > Bus usb at 5200000: USB EHCI 1.00
> > > > Bus usb at 5200400: USB OHCI 1.0
> > > > scanning bus usb at 5200000 for devices... 2 USB Device(s) found
> > > > scanning bus usb at 5200400 for devices... 1 USB Device(s) found
> > > >        scanning usb for storage devices... 1 Storage Device(s) found
> > > >
> > > > I have no clue why this happens, there is no discrete hub anywhere in this
> > > > setup, so I guess the code runs for the root hub here?
> > > > I am attaching a log with DEBUG enabled for common/usb_hub.c, for both
> > > > cases.
> > > >
> > > > Any clues, debug suggestions, or even a fix are warmly welcome.
> > > >  
> > >
> > > Which USB disk are you using? Is it a USB 2.0 disk ?
> > > Can you try with any other USB disk to confirm?  
> >
> > Yes, this was some USB 2.0 memory stick, on an USB 2.0 port.
> > Most sunxi boards only have USB 2.0, but there is one SoC with USB 3.0, I
> > will try some combinations later tonight.
> >
> > Thanks,
> > Andre  
> 
> That would be awesome.
> We need to test
> 
> USB 3.0 and 2.0 ports with USB 3.0 and 2.0 drives.

So I had some mixed and apparently inconsistent results:
- On the Pine-H64 (H6 SoC with USB 3.0 + USB 2.0) it worked with
  mainline, but only after I applied some pending sunxi USB PHY
  regulator patches. This is an unrelated issue, those patches
  are needed to enable USB 3.0 support on that board (shared regulator
  conflict).
  I tested a USB 2.0 and a USB 3.0 drive in both kind of slots, with
  and without the patch.
- On the OrangePi Zero 3 (H616 SoC with USB 2.0 only), the USB 3.0
  drive would not work with mainline. A USB 2.0 drive was fine.
  Applying your patch below fixed that problem, now both drives worked.
- On the OrangePi Zero (H3 with USB 2.0 only), it worked with and
  without the patch below, with both the USB 2.0 and USB 3.0 drive.
- On the BananaPi M64 (A64 with USB 2.0 only and an onboard USB 2.0
  hub) it also worked in all cases: with and without the patch, USB 2.0
  and USB 3.0 drive.

So the good news is: with the patch below it worked in all cases, with
all drives in all slots, on all boards. With current mainline some
drives don't work at least on the board with the H616 SoC.

I cannot really say if that makes sense, and the results above maybe
more per board than per SoC (different VBUS supply), but would
pragmatically tend to use the patch, feel free to add my Tested-by:
when sending:

Tested-by: Andre Przywara <andre.przywara at arm.com>

Just one tiny thing:

> Once you have tested it, can you please try the patch below
> 
>         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(hub)) {

this must be: usb_hub_is_superspeed(dev)

So many thanks for that patch, that seem to have fixed it for me - even
though I don't understand why. But then again I only have superficial
USB knowledge.

Cheers,
Andre


> +                       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);
>         }
> 
> 
> Thanks