On Mon, Dec 4, 2023 at 11:05 AM Marek Vasut marex@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@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@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.d... >>>> >>>> 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
- 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.
- 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
- 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?
- Leave regulator for typec as it was
- 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