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

[Marek Vasut]

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.