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. Regards, Shantur
