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

[Shantur Rathore]

Hi Marek,

On Thu, Nov 23, 2023 at 9:07 PM Marek Vasut <marex at denx.de> wrote:
>
> On 11/23/23 12:24, Shantur Rathore wrote:
> > Hi Marek,
> >
> > On Thu, Nov 23, 2023 at 12:06 AM Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 11/20/23 00:03, Shantur Rathore wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut <marex at denx.de> wrote:
> >>>>
> >>>> On 11/10/23 15:13, Shantur Rathore wrote:
> >>>>> 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.
> >>>>
> >>>> OK
> >>>>
> >>>>> Mostly in these scenarios the hub port is powered before the USB
> >>>>> controller is enabled, this can lead to some devices in unexpected
> >>>>> state.
> >>>>
> >>>> This ^ part needs clarification.
> >>>>
> >>>> Which devices are in incorrect state, the ones connected to the hub
> >>>> downstream facing ports ?
> >>>>
> >>>
> >>> In my case RockPro64, the power to usb ports onboard is controlled by
> >>> a regulator.
> >>> This regulator is enabled as part of init  as here
> >>> https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/rk3399-rockpro64.dtsi#L177
> >>>
> >>> On init, the usb devices connected to the port are powered up, in my
> >>> case AM8180 (a RTL9210 based ) NVMe to USB enclosure with UAS.
> >>> But the usb controller is only enabled at 'usb start' and by this time
> >>> the device is in some state that it doesn't get detected.
> >>>
> >>> One way to make sure the devices connected to the ports are in an
> >>> initialising state is by issuing a port reset before scanning the
> >>> port.
> >>
> >> Why not remove this regulator-always-on and let the PHY driver turn the
> >> VBUS ON only when it is needed ?
> >>
> >> Wouldn't that solve the problem too AND remove unnecessarily enabled
> >> regulator ?
> >>
> >> [...]
> >
> > Removing the regulator-always-on does make it work but there are few issues
> >
> > 1. regulator is used to control power to multiple ports ( USB3.0 and
> > USB2.0 in RockPro64 ),
> > so this could lead to all ports turning off if a phy resets / turns off power
>
> I vaguely recall seeing some refcounting patch for regulator subsystem,
> would that help ?

I don't think this would be a problem for u-boot, but linux maybe.

>
> > 2. Even with regulator-always-on and without this reset port patch,
> > linux was always
> > able to detect the drive on the port, so there is something missing in u-boot.
> > 3. Looking at usb hub code in Linux, I found that for USB3.0 it tries
> > to reset the port before
> > scanning. The comment says
> >
> > /* Is a USB 3.0 port in the Inactive or Compliance Mode state?
> >   * Port warm reset is required to recover
> >   */
> >
> > i. https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L5736
> > ii. https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2836
> >
> > I believe this isn't implemented in u-boot and hence explicit reset of
> > a port fixes the issue.
>
> I feel this is separate issue and what needs to be fixed first is the
> hard-wired VBus control.

I beg to differ on this, couple of reasons for this

1. The same drive works fine without being reset on the USB2.0 ports - this
confirms that the issue is related to USB3.0 only.
2. Linux is able to detect the same drive on USB3.0 when u-boot fails - this
confirms issue doesn't lie with regulator-always-on
3. The links I pasted earlier mentions that in USB3.0 the ports need reset
and this is done before the port is scanned. Adding the similar reset in u-boot
fixes all with the same regulator-always-on. AFAIK u-boot doesn't handle
this special USB3.0 case and maybe this is why I was finding a few posts
around the drive not being detected in the USB3.0 port in u-boot but works in
a USB2.0 port.

Kind regards,
Shantur