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

[Marek Vasut]

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.