Hi Marek, On Tue, Dec 5, 2023 at 5:40 AM Marek Vasut <ma...@denx.de> wrote: > > On 12/4/23 13:10, Shantur Rathore wrote: > > On Mon, Dec 4, 2023 at 11:05 AM Marek Vasut <ma...@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 <ma...@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 <ma...@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