[U-Boot] [PATCH v2 1/3] usb: hub: identify the hub-device to usb_hub_reset_devices

[U-Boot] [PATCH v2 1/3] usb: hub: identify the hub-device to usb_hub_reset_devices

[Philipp Tomsich]

When usb_hub_reset_devices is called, it should be passed both an
indicator which hub it should operate on and what port number (local
to that hub) should be reset.

This adds an additional 'hub' parameter to usb_hub_reset_devices.
Existing implementations are changed to simply ignore the new
parameter.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2:
- new in v2

 board/compulab/cm_t54/cm_t54.c | 2 +-
 board/ti/omap5_uevm/evm.c      | 2 +-
 common/usb_hub.c               | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/board/compulab/cm_t54/cm_t54.c b/board/compulab/cm_t54/cm_t54.c
index 31730a4..3e6235a 100644
--- a/board/compulab/cm_t54/cm_t54.c
+++ b/board/compulab/cm_t54/cm_t54.c
@@ -246,7 +246,7 @@ int ehci_hcd_stop(void)
 	return ret;
 }
 
-void usb_hub_reset_devices(int port)
+void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
 {
 	/* The LAN9730 needs to be reset after the port power has been set. */
 	if (port == 3) {
diff --git a/board/ti/omap5_uevm/evm.c b/board/ti/omap5_uevm/evm.c
index 4b25cc2..67242f5 100644
--- a/board/ti/omap5_uevm/evm.c
+++ b/board/ti/omap5_uevm/evm.c
@@ -249,7 +249,7 @@ int ehci_hcd_stop(void)
 	return omap_ehci_hcd_stop();
 }
 
-void usb_hub_reset_devices(int port)
+void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
 {
 	/* The LAN9730 needs to be reset after the port power has been set. */
 	if (port == 3) {
diff --git a/common/usb_hub.c b/common/usb_hub.c
index 325d16d..024dadb 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -57,7 +57,7 @@ struct usb_device_scan {
 
 static LIST_HEAD(usb_scan_list);
 
-__weak void usb_hub_reset_devices(int port)
+__weak void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
 {
 	return;
 }
@@ -853,7 +853,7 @@ static int usb_hub_configure(struct usb_device *dev)
 	 * should occur in the board file of the device.
 	 */
 	for (i = 0; i < dev->maxchild; i++)
-		usb_hub_reset_devices(i + 1);
+		usb_hub_reset_devices(hub, i + 1);
 
 	/*
 	 * Only add the connected USB devices, including potential hubs,
-- 
2.1.4

[U-Boot] [PATCH v2 2/3] rockchip: rk3399-puma: implement usb_hub_reset_devices for puma-rk3399

[Philipp Tomsich]

For some versions of the RK3399-Q7 (at least revisions v1.1 and v1.2
are affected), we need to turn on the power for the port connected to
the on-module USB hub only when the device is probed for the first
time to ensure that the hub does not enter a low-power mode (that
U-Boot's USB stack can't deal with).

Note that this is needed for U-Boot only, as Linux eventually manages
to attach the hub even when it has entered into its low-power state
(when the hub wakes up the next time) after a few seconds.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2:
- moved the power-control logic from xhci.c into the board-specific
  file for the RK3399-Q7, hooking it up via usb_hub_reset_devices

 board/theobroma-systems/puma_rk3399/puma-rk3399.c | 70 ++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
index 2b4988e..7f2dd65 100644
--- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
+++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
@@ -7,12 +7,13 @@
 #include <common.h>
 #include <dm.h>
 #include <misc.h>
+#include <spl.h>
+#include <usb.h>
 #include <dm/pinctrl.h>
 #include <dm/uclass-internal.h>
 #include <asm/setup.h>
 #include <asm/arch/periph.h>
 #include <power/regulator.h>
-#include <spl.h>
 #include <u-boot/sha256.h>
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -158,3 +159,70 @@ void get_board_serial(struct tag_serialnr *serialnr)
 	serialnr->low = (u32)(serial & 0xffffffff);
 }
 #endif
+
+/**
+ * Switch power at an external regulator (for our root hub).
+ *
+ * @param ctrl pointer to the xHCI controller
+ * @param port port number as in the control message (one-based)
+ * @param enable boolean indicating whether to enable or disable power
+ * @return returns 0 on success, an error-code on failure
+ */
+static int board_usb_port_power_set(struct udevice *dev, int port,
+				    bool enable)
+{
+#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(DM_REGULATOR)
+	/* We start counting ports at 0, while USB counts from 1. */
+	int index = port - 1;
+	const char *regname = NULL;
+	struct udevice *regulator;
+	const char *prop = "tsd,usb-port-power";
+	int ret;
+
+	debug("%s: ctrl '%s' port %d enable %s\n", __func__,
+	      dev_read_name(dev), port, enable ? "true" : "false");
+
+	ret = dev_read_string_index(dev, prop, index, &regname);
+	if (ret < 0) {
+		debug("%s: ctrl '%s' port %d: no entry in '%s'\n",
+		      __func__, dev_read_name(dev), port, prop);
+		return ret;
+	}
+
+	ret = regulator_get_by_platname(regname, &regulator);
+	if (ret) {
+		debug("%s: ctrl '%s' port %d: could not get regulator '%s'\n",
+		      __func__, dev_read_name(dev), port, regname);
+		return ret;
+	}
+
+	regulator_set_enable(regulator, enable);
+	return 0;
+#else
+	return -ENOTSUPP;
+#endif
+}
+
+void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
+{
+	struct udevice *dev = hub->pusb_dev->dev;
+	struct udevice *ctrl;
+
+	/* We are only interested in our root-hubs */
+	if (usb_hub_is_root_hub(dev) == false)
+		return;
+
+	ctrl = usb_get_bus(dev);
+	if (!ctrl) {
+		debug("%s: could not retrieve ctrl for hub\n", __func__);
+		return;
+	}
+
+	/*
+	 * To work around an incompatibility between the single-threaded
+	 * USB stack in U-Boot and (a strange low-power mode of) the USB
+	 * hub we have on-module, we need to delay powering on the hub
+	 * until the first time the port is probed.
+	 */
+	board_usb_port_power_set(ctrl, port, true);
+}
-- 
2.1.4

[U-Boot] [PATCH v2 3/3] rockchip: dts: rk3399-puma: add a 'tsd, usb-port-power' stringlist for USB1

[Philipp Tomsich]

USB1 is connected to the on-module USB 3.0 hub and power to the hub
(actually it's a reset signal, modeled as a fixed regulator, that will
be released) should be enabled only during the first probing of the
device to avoid the hub from entering its low-power mode (where it
tries to attach on a fixed interval, but we always miss the timeslot
when U-Boot has the controller listening).

This adds a 'tsd,usb-port-power' stringlist to enable the
infrastructure in the board-specific usb_hub_reset_devices to find and
control the fixed regulator associated with control of the USB hub.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2:
- changed the property name to 'tsd,usb-port-power' to make it a
  vendor-specific property

 arch/arm/dts/rk3399-puma.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/rk3399-puma.dtsi b/arch/arm/dts/rk3399-puma.dtsi
index 96bd4fe..0f5e6ac 100644
--- a/arch/arm/dts/rk3399-puma.dtsi
+++ b/arch/arm/dts/rk3399-puma.dtsi
@@ -544,6 +544,7 @@
 
 &dwc3_typec1 {
 	status = "okay";
+	tsd,usb-port-power = "usbhub_enable";
 };
 
 &vopb {
-- 
2.1.4

[U-Boot] [PATCH v2 1/3] usb: hub: identify the hub-device to usb_hub_reset_devices

[Marek Vasut]

On 11/22/2017 04:57 PM, Philipp Tomsich wrote:
> When usb_hub_reset_devices is called, it should be passed both an
> indicator which hub it should operate on and what port number (local
> to that hub) should be reset.

Why ?

> This adds an additional 'hub' parameter to usb_hub_reset_devices.
> Existing implementations are changed to simply ignore the new
> parameter.
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> 
> ---
> 
> Changes in v2:
> - new in v2
> 
>  board/compulab/cm_t54/cm_t54.c | 2 +-
>  board/ti/omap5_uevm/evm.c      | 2 +-
>  common/usb_hub.c               | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/board/compulab/cm_t54/cm_t54.c b/board/compulab/cm_t54/cm_t54.c
> index 31730a4..3e6235a 100644
> --- a/board/compulab/cm_t54/cm_t54.c
> +++ b/board/compulab/cm_t54/cm_t54.c
> @@ -246,7 +246,7 @@ int ehci_hcd_stop(void)
>  	return ret;
>  }
>  
> -void usb_hub_reset_devices(int port)
> +void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>  {
>  	/* The LAN9730 needs to be reset after the port power has been set. */
>  	if (port == 3) {
> diff --git a/board/ti/omap5_uevm/evm.c b/board/ti/omap5_uevm/evm.c
> index 4b25cc2..67242f5 100644
> --- a/board/ti/omap5_uevm/evm.c
> +++ b/board/ti/omap5_uevm/evm.c
> @@ -249,7 +249,7 @@ int ehci_hcd_stop(void)
>  	return omap_ehci_hcd_stop();
>  }
>  
> -void usb_hub_reset_devices(int port)
> +void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>  {
>  	/* The LAN9730 needs to be reset after the port power has been set. */
>  	if (port == 3) {
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 325d16d..024dadb 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -57,7 +57,7 @@ struct usb_device_scan {
>  
>  static LIST_HEAD(usb_scan_list);
>  
> -__weak void usb_hub_reset_devices(int port)
> +__weak void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>  {
>  	return;
>  }
> @@ -853,7 +853,7 @@ static int usb_hub_configure(struct usb_device *dev)
>  	 * should occur in the board file of the device.
>  	 */
>  	for (i = 0; i < dev->maxchild; i++)
> -		usb_hub_reset_devices(i + 1);
> +		usb_hub_reset_devices(hub, i + 1);
>  
>  	/*
>  	 * Only add the connected USB devices, including potential hubs,
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/3] usb: hub: identify the hub-device to usb_hub_reset_devices

[Dr. Philipp Tomsich]

> On 22 Nov 2017, at 16:59, Marek Vasut <marex@denx.de> wrote:
> 
> On 11/22/2017 04:57 PM, Philipp Tomsich wrote:
>> When usb_hub_reset_devices is called, it should be passed both an
>> indicator which hub it should operate on and what port number (local
>> to that hub) should be reset.
> 
> Why ?

Simply because there will be multiple root hubs in the system, which may
have multiple hubs attached.  And the usb_hub_reset_devices() function
was being called for every hub with just a port-number, but without the
context of what number-space (i.e. hub) that port-number belonged to.

The code in usb_hub previously was:
        /*
         * Reset any devices that may be in a bad state when applying
         * the power.  This is a __weak function.  Resetting of the devices
         * should occur in the board file of the device.
         */
        for (i = 0; i < dev->maxchild; i++)
                usb_hub_reset_devices(i + 1);

So it would call usb_hub_reset_devices(1) for the first port of every
attached hub (including the root-hubs).

I.e. the implementation of usb_hub_reset_devices had no idea what
(hub, port)-combination it was dealing with...

> 
>> This adds an additional 'hub' parameter to usb_hub_reset_devices.
>> Existing implementations are changed to simply ignore the new
>> parameter.
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> 
>> ---
>> 
>> Changes in v2:
>> - new in v2
>> 
>> board/compulab/cm_t54/cm_t54.c | 2 +-
>> board/ti/omap5_uevm/evm.c      | 2 +-
>> common/usb_hub.c               | 4 ++--
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/board/compulab/cm_t54/cm_t54.c b/board/compulab/cm_t54/cm_t54.c
>> index 31730a4..3e6235a 100644
>> --- a/board/compulab/cm_t54/cm_t54.c
>> +++ b/board/compulab/cm_t54/cm_t54.c
>> @@ -246,7 +246,7 @@ int ehci_hcd_stop(void)
>> 	return ret;
>> }
>> 
>> -void usb_hub_reset_devices(int port)
>> +void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>> {
>> 	/* The LAN9730 needs to be reset after the port power has been set. */
>> 	if (port == 3) {
>> diff --git a/board/ti/omap5_uevm/evm.c b/board/ti/omap5_uevm/evm.c
>> index 4b25cc2..67242f5 100644
>> --- a/board/ti/omap5_uevm/evm.c
>> +++ b/board/ti/omap5_uevm/evm.c
>> @@ -249,7 +249,7 @@ int ehci_hcd_stop(void)
>> 	return omap_ehci_hcd_stop();
>> }
>> 
>> -void usb_hub_reset_devices(int port)
>> +void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>> {
>> 	/* The LAN9730 needs to be reset after the port power has been set. */
>> 	if (port == 3) {
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index 325d16d..024dadb 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -57,7 +57,7 @@ struct usb_device_scan {
>> 
>> static LIST_HEAD(usb_scan_list);
>> 
>> -__weak void usb_hub_reset_devices(int port)
>> +__weak void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>> {
>> 	return;
>> }
>> @@ -853,7 +853,7 @@ static int usb_hub_configure(struct usb_device *dev)
>> 	 * should occur in the board file of the device.
>> 	 */
>> 	for (i = 0; i < dev->maxchild; i++)
>> -		usb_hub_reset_devices(i + 1);
>> +		usb_hub_reset_devices(hub, i + 1);
>> 
>> 	/*
>> 	 * Only add the connected USB devices, including potential hubs,
>> 
> 
> 
> -- 
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v2 1/3] usb: hub: identify the hub-device to usb_hub_reset_devices

[Marek Vasut]

On 11/22/2017 05:02 PM, Dr. Philipp Tomsich wrote:
> 
>> On 22 Nov 2017, at 16:59, Marek Vasut <marex@denx.de> wrote:
>>
>> On 11/22/2017 04:57 PM, Philipp Tomsich wrote:
>>> When usb_hub_reset_devices is called, it should be passed both an
>>> indicator which hub it should operate on and what port number (local
>>> to that hub) should be reset.
>>
>> Why ?
> 
> Simply because there will be multiple root hubs in the system, which may
> have multiple hubs attached.  And the usb_hub_reset_devices() function
> was being called for every hub with just a port-number, but without the
> context of what number-space (i.e. hub) that port-number belonged to.
> 
> The code in usb_hub previously was:
>         /*
>          * Reset any devices that may be in a bad state when applying
>          * the power.  This is a __weak function.  Resetting of the devices
>          * should occur in the board file of the device.
>          */
>         for (i = 0; i < dev->maxchild; i++)
>                 usb_hub_reset_devices(i + 1);
> 
> So it would call usb_hub_reset_devices(1) for the first port of every
> attached hub (including the root-hubs).
> 
> I.e. the implementation of usb_hub_reset_devices had no idea what
> (hub, port)-combination it was dealing with...

This should be in the commit message ;-)

>>
>>> This adds an additional 'hub' parameter to usb_hub_reset_devices.
>>> Existing implementations are changed to simply ignore the new
>>> parameter.
>>>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - new in v2
>>>
>>> board/compulab/cm_t54/cm_t54.c | 2 +-
>>> board/ti/omap5_uevm/evm.c      | 2 +-
>>> common/usb_hub.c               | 4 ++--
>>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/board/compulab/cm_t54/cm_t54.c b/board/compulab/cm_t54/cm_t54.c
>>> index 31730a4..3e6235a 100644
>>> --- a/board/compulab/cm_t54/cm_t54.c
>>> +++ b/board/compulab/cm_t54/cm_t54.c
>>> @@ -246,7 +246,7 @@ int ehci_hcd_stop(void)
>>> 	return ret;
>>> }
>>>
>>> -void usb_hub_reset_devices(int port)
>>> +void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>>> {
>>> 	/* The LAN9730 needs to be reset after the port power has been set. */
>>> 	if (port == 3) {
>>> diff --git a/board/ti/omap5_uevm/evm.c b/board/ti/omap5_uevm/evm.c
>>> index 4b25cc2..67242f5 100644
>>> --- a/board/ti/omap5_uevm/evm.c
>>> +++ b/board/ti/omap5_uevm/evm.c
>>> @@ -249,7 +249,7 @@ int ehci_hcd_stop(void)
>>> 	return omap_ehci_hcd_stop();
>>> }
>>>
>>> -void usb_hub_reset_devices(int port)
>>> +void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>>> {
>>> 	/* The LAN9730 needs to be reset after the port power has been set. */
>>> 	if (port == 3) {
>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> index 325d16d..024dadb 100644
>>> --- a/common/usb_hub.c
>>> +++ b/common/usb_hub.c
>>> @@ -57,7 +57,7 @@ struct usb_device_scan {
>>>
>>> static LIST_HEAD(usb_scan_list);
>>>
>>> -__weak void usb_hub_reset_devices(int port)
>>> +__weak void usb_hub_reset_devices(struct usb_hub_device *hub, int port)
>>> {
>>> 	return;
>>> }
>>> @@ -853,7 +853,7 @@ static int usb_hub_configure(struct usb_device *dev)
>>> 	 * should occur in the board file of the device.
>>> 	 */
>>> 	for (i = 0; i < dev->maxchild; i++)
>>> -		usb_hub_reset_devices(i + 1);
>>> +		usb_hub_reset_devices(hub, i + 1);
>>>
>>> 	/*
>>> 	 * Only add the connected USB devices, including potential hubs,
>>>
>>
>>
>> -- 
>> Best regards,
>> Marek Vasut
> 


-- 
Best regards,
Marek Vasut