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

Fri Dec 8 01:16:40 CET 2023

Hi Marek,

On Tue, Dec 5, 2023 at 5:40 AM Marek Vasut <marex at denx.de> wrote:
>
> On 12/4/23 13:10, Shantur Rathore wrote:
> > On Mon, Dec 4, 2023 at 11:05 AM Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 12/4/23 11:59, Shantur Rathore wrote:
> >>> Hi Marek,
> >>>
> >>> On Sun, Dec 3, 2023 at 10:37 PM Marek Vasut <marex at denx.de> wrote:
> >>>>
> >>>> On 12/3/23 22:42, Shantur Rathore wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On Sun, Dec 3, 2023 at 8:42 PM Marek Vasut <marex at denx.de> wrote:
> >>>>>>
> >>>>>> On 11/24/23 01:37, Shantur Rathore wrote:
> >>>>>>> Hi Marek,
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> sorry for the late reply.
> >>>>>>
> >>>>>>>>>>> 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.
> >>>>>>
> >>>>>> How so ?
> >>>>>
> >>>>> Actually, I am wrong. I wasn't sure if there is refcounting for the
> >>>>> regulator subsystem
> >>>>> in linux. just found it has so this is null and void.
> >>>>>
> >>>>>>
> >>>>>>>>> 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.
> >>>>>>
> >>>>>> This is a separate issue from the hard-wired VBus control. I agree the
> >>>>>> issue does exist, I would simply like to avoid conflating the hard-wired
> >>>>>> VBus control with device reset.
> >>>>>>
> >>>>>>> 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
> >>>>>>
> >>>>>> See above
> >>>>>>
> >>>>>>> 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.
> >>>>>>
> >>>>>> I am not disputing the need for USB 3.0 device reset, I believe there
> >>>>>> are two issues here -- one is the hard-wired VBus regulator -- the other
> >>>>>> is this USB 3.0 reset. They should both be fixed.
> >>>>>
> >>>>> Sure, agreed 100%.
> >>>>> Do we need to fix both at the same time?
> >>>>> Both patches seem to be independent.
> >>>>
> >>>> Separate patch for the regulator please .
> >>>
> >>> Thanks, I am working on the regulator patch for rk3399-rockpro64-u-boot but
> >>> there seems to be a bug in the rk3399-rockpro64.dtsi from linux where
> >>> the typec phy
> >>> is configure with wrong regulator and if I do the patch as below
> >>>
> >>> --- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> >>> +++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> >>> @@ -22,6 +22,23 @@
> >>>           };
> >>>    };
> >>>
> >>> +&vcc5v0_host {
> >>> +       /delete-property/ regulator-always-on;
> >>> +};
> >>> +
> >>> +&vcc5v0_typec {
> >>> +       /delete-property/ regulator-always-on;
> >>> +};
> >>> +
> >>> +&vcc5v0_usb {
> >>> +       /delete-property/ regulator-always-on;
> >>> +       /delete-property/ regulator-boot-on;
> >>> +};
> >>>
> >>> The typec port won't work until I correct the issue with
> >>>
> >>> +&u2phy1_host {
> >>> +       phy-supply = <&vcc5v0_typec>;
> >>> +};
> >>> +
> >>>
> >>> What would be acceptable here?
> >>> 1. Leave regulator for typec as it was
> >>> 2. disable regulator-always-on and fix phy here.
> >>
> >> Is there going to be a matching Linux kernel patch with a Linux side fix ?
> >>
> >> If so, include a link to that patch in lore.k.o in the commit message,
> >> and either temporarily fix it up in u-boot.dtsi or backport that patch
> >> to U-Boot DT, either works.
> >
> > So there is no patch in lore.k.o for this and the more I looked the more
> > dts needed fixing related  usb. I would rather do it in one go and submit
> > proper patches to lore.k.o and u-boot.
> >
> > For now, as usb reset is independent of the rockpro64 regulator issue, can
> > you please merge this and I will work on dts fixes separately.
>
> I'll wait for the regulator fix and them merge them both.
>
> This affects too many systems and I'm reluctant to put this into current
> release at rc4, so there should be plenty of time until the next release.

Apologies for the late reply.

I have sent both the patches here
https://patchwork.ozlabs.org/project/uboot/list/?series=385763

Thanks,
Shantur