[PATCH] common: usb-hub: Reset hub port before scanning
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect. Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
With this patch, we explicitly reset the port while powering up hub This resets the port for hubs without port power control and has no effect on hubs with port power control as the port is still off.
Before this patch AMicro AM8180 based NVME to USB adapter won't be detected as a USB3.0 Mass Storage device but with this it works as expected.
Tested working after this patch: 1. AMicro AM8180 based NVME to USB Adapter 2. Kingston DataTraveler 3.0 3. GenesysLogic USB3.0 Hub
The drives were tested while connected directly and via the hub. --- common/usb_hub.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 85c0822d8b..06fe436add 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
debug("enabling power on all ports\n"); for (i = 0; i < dev->maxchild; i++) { + usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET); + debug("Reset : port %d returns %lX\n", i + 1, dev->status); usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER); - debug("port %d returns %lX\n", i + 1, dev->status); + debug("PowerOn : port %d returns %lX\n", i + 1, dev->status); }
#ifdef CONFIG_SANDBOX
+Simon +Trini
On Fri, Nov 10, 2023 at 2:13 PM Shantur Rathore i@shantur.com wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect. Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
With this patch, we explicitly reset the port while powering up hub This resets the port for hubs without port power control and has no effect on hubs with port power control as the port is still off.
Before this patch AMicro AM8180 based NVME to USB adapter won't be detected as a USB3.0 Mass Storage device but with this it works as expected.
Tested working after this patch:
- AMicro AM8180 based NVME to USB Adapter
- Kingston DataTraveler 3.0
- GenesysLogic USB3.0 Hub
The drives were tested while connected directly and via the hub.
common/usb_hub.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 85c0822d8b..06fe436add 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
debug("enabling power on all ports\n"); for (i = 0; i < dev->maxchild; i++) {
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);debug("Reset : port %d returns %lX\n", i + 1, dev->status); usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
debug("port %d returns %lX\n", i + 1, dev->status);
debug("PowerOn : port %d returns %lX\n", i + 1, dev->status); }#ifdef CONFIG_SANDBOX
2.40.1
+Patrice +Patrick from get_maintainer script
On Mon, Nov 13, 2023 at 1:28 PM Shantur Rathore i@shantur.com wrote:
+Simon +Trini
On Fri, Nov 10, 2023 at 2:13 PM Shantur Rathore i@shantur.com wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect. Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
With this patch, we explicitly reset the port while powering up hub This resets the port for hubs without port power control and has no effect on hubs with port power control as the port is still off.
Before this patch AMicro AM8180 based NVME to USB adapter won't be detected as a USB3.0 Mass Storage device but with this it works as expected.
Tested working after this patch:
- AMicro AM8180 based NVME to USB Adapter
- Kingston DataTraveler 3.0
- GenesysLogic USB3.0 Hub
The drives were tested while connected directly and via the hub.
common/usb_hub.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 85c0822d8b..06fe436add 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
debug("enabling power on all ports\n"); for (i = 0; i < dev->maxchild; i++) {
usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);debug("Reset : port %d returns %lX\n", i + 1, dev->status); usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
debug("port %d returns %lX\n", i + 1, dev->status);
debug("PowerOn : port %d returns %lX\n", i + 1, dev->status); }#ifdef CONFIG_SANDBOX
2.40.1
Hi Marek,
What is the point of this growing CC list ?
Apologies, this is my first time with email based patch management. I was reading articles on best practices and first one mentioned to cc people with recent commits, second one mentioned to use get_maintainer script to cc people and hence multiple emails.
Regards, Shantur
On 11/15/23 09:26, Shantur Rathore wrote:
Hi Marek,
Hi,
What is the point of this growing CC list ?
Apologies, this is my first time with email based patch management. I was reading articles on best practices and first one mentioned to cc people with recent commits, second one mentioned to use get_maintainer script to cc people and hence multiple emails.
No worries, I'll have a look in a couple of days, sorry for the delay.
On 11/10/23 15:13, Shantur Rathore wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
OK
Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
This ^ part needs clarification.
Which devices are in incorrect state, the ones connected to the hub downstream facing ports ?
With this patch, we explicitly reset the port while powering up hub This resets the port for hubs without port power control and has no effect on hubs with port power control as the port is still off.
Should common/usb_hub.c usb_hub_port_connect_change() trigger the reset?
Could it be you do not get a connect change event ?
Before this patch AMicro AM8180 based NVME to USB adapter won't be detected as a USB3.0 Mass Storage device but with this it works as expected.
Do you have HUB power control ?
I recall there was some companion hub discussion recently.
Hi Marek,
On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut marex@denx.de wrote:
On 11/10/23 15:13, Shantur Rathore wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
OK
Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
This ^ part needs clarification.
Which devices are in incorrect state, the ones connected to the hub downstream facing ports ?
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.
With this patch, we explicitly reset the port while powering up hub This resets the port for hubs without port power control and has no effect on hubs with port power control as the port is still off.
Should common/usb_hub.c usb_hub_port_connect_change() trigger the reset?
Could it be you do not get a connect change event ?
Yes, that's correct there is no connect change event while scanning the port.
Before this patch AMicro AM8180 based NVME to USB adapter won't be detected as a USB3.0 Mass Storage device but with this it works as expected.
Do you have HUB power control ?
I recall there was some companion hub discussion recently.
The onboard ports don't have separate power control, only the regulator as explained above.
Kind regards, Shantur
On Sun, Nov 19, 2023 at 11:03 PM Shantur Rathore i@shantur.com wrote:
Hi Marek,
On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut marex@denx.de wrote:
On 11/10/23 15:13, Shantur Rathore wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
OK
Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
This ^ part needs clarification.
Which devices are in incorrect state, the ones connected to the hub downstream facing ports ?
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.
With this patch, we explicitly reset the port while powering up hub This resets the port for hubs without port power control and has no effect on hubs with port power control as the port is still off.
Should common/usb_hub.c usb_hub_port_connect_change() trigger the reset?
Could it be you do not get a connect change event ?
Yes, that's correct there is no connect change event while scanning the port.
Before this patch AMicro AM8180 based NVME to USB adapter won't be detected as a USB3.0 Mass Storage device but with this it works as expected.
Do you have HUB power control ?
I recall there was some companion hub discussion recently.
The onboard ports don't have separate power control, only the regulator as explained above.
Kind regards, Shantur
PS:
If the usb device is kept disconnected on init but connected after 'usb start' while U-boot is scanning usb buses but hasn't scanned the bus the device is connected to, the usb storage device is detected.
Also, I just tried - https://patchwork.ozlabs.org/project/uboot/list/?series=379807 and it doesn't fix the issue.
On 11/20/23 00:03, Shantur Rathore wrote:
Hi Marek,
Hi,
On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut marex@denx.de wrote:
On 11/10/23 15:13, Shantur Rathore wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
OK
Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
This ^ part needs clarification.
Which devices are in incorrect state, the ones connected to the hub downstream facing ports ?
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 ?
[...]
Hi Marek,
On Thu, Nov 23, 2023 at 12:06 AM Marek Vasut marex@denx.de wrote:
On 11/20/23 00:03, Shantur Rathore wrote:
Hi Marek,
Hi,
On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut marex@denx.de wrote:
On 11/10/23 15:13, Shantur Rathore wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
OK
Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
This ^ part needs clarification.
Which devices are in incorrect state, the ones connected to the hub downstream facing ports ?
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 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.
Kind regards, Shantur
On 11/23/23 12:24, Shantur Rathore wrote:
Hi Marek,
On Thu, Nov 23, 2023 at 12:06 AM Marek Vasut marex@denx.de wrote:
On 11/20/23 00:03, Shantur Rathore wrote:
Hi Marek,
Hi,
On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut marex@denx.de wrote:
On 11/10/23 15:13, Shantur Rathore wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
OK
Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
This ^ part needs clarification.
Which devices are in incorrect state, the ones connected to the hub downstream facing ports ?
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
- 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 ?
- 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.
Hi Marek,
On Thu, Nov 23, 2023 at 9:07 PM Marek Vasut marex@denx.de wrote:
On 11/23/23 12:24, Shantur Rathore wrote:
Hi Marek,
On Thu, Nov 23, 2023 at 12:06 AM Marek Vasut marex@denx.de wrote:
On 11/20/23 00:03, Shantur Rathore wrote:
Hi Marek,
Hi,
On Sun, Nov 19, 2023 at 8:53 PM Marek Vasut marex@denx.de wrote:
On 11/10/23 15:13, Shantur Rathore wrote:
Currently when a hub is turned on, all the ports are powered on. This works well for hubs which have individual power control.
For the hubs without individual power control this has no effect.
OK
Mostly in these scenarios the hub port is powered before the USB controller is enabled, this can lead to some devices in unexpected state.
This ^ part needs clarification.
Which devices are in incorrect state, the ones connected to the hub downstream facing ports ?
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
- 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.
- 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. 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 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.
Kind regards, Shantur
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
- 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 ?
- 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.
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
- 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.
- 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.
Kind regards, Shantur
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
- 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.
- 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 .
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
- 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.
- 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? 1. Leave regulator for typec as it was 2. disable regulator-always-on and fix phy here.
Kind regards, Shantur
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.
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
On 12/4/23 13:10, Shantur Rathore wrote:
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 > > 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?
- 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.
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.
Hi Marek,
On Tue, Dec 5, 2023 at 5:40 AM Marek Vasut marex@denx.de wrote:
On 12/4/23 13:10, Shantur Rathore wrote:
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 >> >> 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?
- 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.
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
On 12/8/23 01:16, Shantur Rathore wrote:
Hi Marek,
On Tue, Dec 5, 2023 at 5:40 AM Marek Vasut marex@denx.de wrote:
On 12/4/23 13:10, Shantur Rathore wrote:
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 >>> >>> 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?
- 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.
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
participants (2)
-
Marek Vasut -
Shantur Rathore