[PATCH v2 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller

[PATCH v2 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller

[Alexander Stein]

The TI USB8041 is a USB 3.0 hub controller with 4 ports.

This initial version of the binding only describes USB related aspects
of the USB8041, it does not cover the option of connecting the controller
as an i2c slave.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v2:
* Removed 'items' from compatible, it's just en enum now
* Rename reset-gpio to reset-gpios
* Use 'items' for reset-gpios
* Adjust description of vdd-supply
* Sorted required list
* Adjusted example

 .../devicetree/bindings/usb/ti,usb8041.yaml   | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml

diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
new file mode 100644
index 000000000000..7fe7416e2b51
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binding for the TI USB8041 USB 3.0 hub controller
+
+maintainers:
+  - Matthias Kaehlcke <mka@chromium.org>
+
+allOf:
+  - $ref: usb-device.yaml#
+
+properties:
+  compatible:
+    enum:
+      - usb451,8140
+      - usb451,8142
+
+  reg: true
+
+  reset-gpios:
+    items:
+      - description: GPIO specifier for GRST# pin.
+
+  vdd-supply:
+    description:
+      "VDD power supply to the hub"
+
+  peer-hub:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to the peer hub on the controller.
+
+required:
+  - compatible
+  - reg
+  - peer-hub
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    usb {
+        dr_mode = "host";
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        /* 2.0 hub on port 1 */
+        hub_2_0: hub@1 {
+          compatible = "usb451,8142";
+          reg = <1>;
+          peer-hub = <&hub_3_0>;
+          reset-gpios = <&gpio1 11 GPIO_ACTIVE_LOW>;
+        };
+
+        /* 3.0 hub on port 2 */
+        hub_3_0: hub@2 {
+          compatible = "usb451,8140";
+          reg = <2>;
+          peer-hub = <&hub_2_0>;
+          reset-gpios = <&gpio1 11 GPIO_ACTIVE_LOW>;
+        };
+    };
-- 
2.25.1

[PATCH v2 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support

[Alexander Stein]

Despite default reset upon probe, release reset line after powering up
the hub and assert reset again before powering down.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v2:
* Use device specific sleep times, if present
* Use fsleep instead of usleep_range

 drivers/usb/misc/onboard_usb_hub.c | 29 +++++++++++++++++++++++++++++
 drivers/usb/misc/onboard_usb_hub.h |  5 +++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 6b9b949d17d3..1dd7f4767def 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -7,6 +7,7 @@
 
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -38,6 +39,8 @@ struct usbdev_node {
 struct onboard_hub {
 	struct regulator *vdd;
 	struct device *dev;
+	const struct onboard_hub_devtype_data *devtype_data;
+	struct gpio_desc *reset_gpio;
 	bool always_powered_in_suspend;
 	bool is_powered_on;
 	bool going_away;
@@ -56,6 +59,11 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
 		return err;
 	}
 
+	if (hub->devtype_data)
+		fsleep(hub->devtype_data->power_stable_time);
+	if (hub->reset_gpio)
+		gpiod_set_value_cansleep(hub->reset_gpio, 0);
+
 	hub->is_powered_on = true;
 
 	return 0;
@@ -65,6 +73,12 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
 {
 	int err;
 
+	if (hub->reset_gpio) {
+		gpiod_set_value_cansleep(hub->reset_gpio, 1);
+		if (hub->devtype_data)
+			fsleep(hub->devtype_data->reset_duration);
+	}
+
 	err = regulator_disable(hub->vdd);
 	if (err) {
 		dev_err(hub->dev, "failed to disable regulator: %d\n", err);
@@ -219,6 +233,7 @@ static void onboard_hub_attach_usb_driver(struct work_struct *work)
 
 static int onboard_hub_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id;
 	struct device *dev = &pdev->dev;
 	struct onboard_hub *hub;
 	int err;
@@ -227,10 +242,24 @@ static int onboard_hub_probe(struct platform_device *pdev)
 	if (!hub)
 		return -ENOMEM;
 
+	of_id = of_match_device(onboard_hub_match, &pdev->dev);
+	if (of_id)
+		hub->devtype_data = of_id->data;
+	else
+		return -ENODEV;
+
 	hub->vdd = devm_regulator_get(dev, "vdd");
 	if (IS_ERR(hub->vdd))
 		return PTR_ERR(hub->vdd);
 
+	hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(hub->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(hub->reset_gpio), "failed to get reset GPIO\n");
+
+	if (hub->devtype_data && hub->reset_gpio)
+		fsleep(hub->devtype_data->reset_duration);
+
 	hub->dev = dev;
 	mutex_init(&hub->lock);
 	INIT_LIST_HEAD(&hub->udev_list);
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
index d3a5b6938582..7e743f4c8aaa 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -6,6 +6,11 @@
 #ifndef _USB_MISC_ONBOARD_USB_HUB_H
 #define _USB_MISC_ONBOARD_USB_HUB_H
 
+struct onboard_hub_devtype_data {
+	unsigned long power_stable_time;	/* power stabilization time in us */
+	unsigned long reset_duration;		/* reset pulse width in us */
+};
+
 static const struct of_device_id onboard_hub_match[] = {
 	{ .compatible = "usbbda,411" },
 	{ .compatible = "usbbda,5411" },
-- 
2.25.1

[PATCH v2 3/3] usb: misc: onboard_usb_hub: Add TI USB8041 hub support

[Alexander Stein]

This is a 4-port 3.0 USB hub.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v2:
* Add devtype data containing waiting times

 drivers/usb/misc/onboard_usb_hub.c | 3 +++
 drivers/usb/misc/onboard_usb_hub.h | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 1dd7f4767def..319b4b1748fb 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -339,6 +339,7 @@ static struct platform_driver onboard_hub_driver = {
 /************************** USB driver **************************/
 
 #define VENDOR_ID_REALTEK	0x0bda
+#define VENDOR_ID_TI		0x0451
 
 /*
  * Returns the onboard_hub platform device that is associated with the USB
@@ -416,6 +417,8 @@ static const struct usb_device_id onboard_hub_id_table[] = {
 	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
 	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
 	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
+	{ USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
+	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
 	{}
 };
 MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
index 7e743f4c8aaa..fcb6a9024bbd 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -11,7 +11,14 @@ struct onboard_hub_devtype_data {
 	unsigned long reset_duration;		/* reset pulse width in us */
 };
 
+static const struct onboard_hub_devtype_data ti_tusb8041_data = {
+	.power_stable_time = 3000,
+	.reset_duration = 3000,
+};
+
 static const struct of_device_id onboard_hub_match[] = {
+	{ .compatible = "usb451,8140", .data = &ti_tusb8041_data, },
+	{ .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
 	{ .compatible = "usbbda,411" },
 	{ .compatible = "usbbda,5411" },
 	{ .compatible = "usbbda,414" },
-- 
2.25.1

Re: [PATCH v2 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller

[Krzysztof Kozlowski]

On 15/07/2022 09:32, Alexander Stein wrote:
> The TI USB8041 is a USB 3.0 hub controller with 4 ports.
> 
> This initial version of the binding only describes USB related aspects
> of the USB8041, it does not cover the option of connecting the controller
> as an i2c slave.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v2:
> * Removed 'items' from compatible, it's just en enum now
> * Rename reset-gpio to reset-gpios
> * Use 'items' for reset-gpios
> * Adjust description of vdd-supply
> * Sorted required list
> * Adjusted example
> 
>  .../devicetree/bindings/usb/ti,usb8041.yaml   | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> new file mode 100644
> index 000000000000..7fe7416e2b51
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for the TI USB8041 USB 3.0 hub controller
> +
> +maintainers:
> +  - Matthias Kaehlcke <mka@chromium.org>
> +
> +allOf:
> +  - $ref: usb-device.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - usb451,8140
> +      - usb451,8142
> +
> +  reg: true
> +
> +  reset-gpios:
> +    items:
> +      - description: GPIO specifier for GRST# pin.
> +
> +  vdd-supply:
> +    description:
> +      "VDD power supply to the hub"

No quotes needed.



Best regards,
Krzysztof

Re: [PATCH v2 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support

[Matthias Kaehlcke]

On Fri, Jul 15, 2022 at 09:32:59AM +0200, Alexander Stein wrote:
> Despite default reset upon probe, release reset line after powering up
> the hub and assert reset again before powering down.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v2:
> * Use device specific sleep times, if present
> * Use fsleep instead of usleep_range
> 
>  drivers/usb/misc/onboard_usb_hub.c | 29 +++++++++++++++++++++++++++++
>  drivers/usb/misc/onboard_usb_hub.h |  5 +++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index 6b9b949d17d3..1dd7f4767def 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -38,6 +39,8 @@ struct usbdev_node {
>  struct onboard_hub {
>  	struct regulator *vdd;
>  	struct device *dev;
> +	const struct onboard_hub_devtype_data *devtype_data;

This kind of device specific data is often call platform data, let's
call the struct 'onboard_hub_pdata' and the field 'pdata'?

> +	struct gpio_desc *reset_gpio;
>  	bool always_powered_in_suspend;
>  	bool is_powered_on;
>  	bool going_away;
> @@ -56,6 +59,11 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
>  		return err;
>  	}
>  
> +	if (hub->devtype_data)

Instead of these checks let's make sure all hubs have pdata. Actually your
onboard_hub_probe() already esnures that the field is assigned.

> +		fsleep(hub->devtype_data->power_stable_time);
> +	if (hub->reset_gpio)
> +		gpiod_set_value_cansleep(hub->reset_gpio, 0);

I would have expected:

  	if (hub->reset_gpio) {
		fsleep(hub->devtype_data->power_stable_time);
		gpiod_set_value_cansleep(hub->reset_gpio, 0);
	}

For the TUSB8041 the 'power_stable_time' (td2 in the datasheet) is "VDD and
VDD33 stable before de-assertion of GRSTz", so no delay without reset.

> +
>  	hub->is_powered_on = true;
>  
>  	return 0;
> @@ -65,6 +73,12 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
>  {
>  	int err;
>  
> +	if (hub->reset_gpio) {
> +		gpiod_set_value_cansleep(hub->reset_gpio, 1);
> +		if (hub->devtype_data)
> +			fsleep(hub->devtype_data->reset_duration);
> +	}
> +
>  	err = regulator_disable(hub->vdd);
>  	if (err) {
>  		dev_err(hub->dev, "failed to disable regulator: %d\n", err);
> @@ -219,6 +233,7 @@ static void onboard_hub_attach_usb_driver(struct work_struct *work)
>  
>  static int onboard_hub_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *of_id;
>  	struct device *dev = &pdev->dev;
>  	struct onboard_hub *hub;
>  	int err;
> @@ -227,10 +242,24 @@ static int onboard_hub_probe(struct platform_device *pdev)
>  	if (!hub)
>  		return -ENOMEM;
>  
> +	of_id = of_match_device(onboard_hub_match, &pdev->dev);
> +	if (of_id)
> +		hub->devtype_data = of_id->data;
> +	else
> +		return -ENODEV;

Please change to:

	if (!of_id)
		return -ENODEV;

	hub->pdata = of_id->data;

With the currently supported Realtek hubs will fail to probe. Please add a pdata for
the RTS541x hubs. The have no reset signal, so the values can be 0.

> +
>  	hub->vdd = devm_regulator_get(dev, "vdd");
>  	if (IS_ERR(hub->vdd))
>  		return PTR_ERR(hub->vdd);
>  
> +	hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(hub->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(hub->reset_gpio), "failed to get reset GPIO\n");
> +
> +	if (hub->devtype_data && hub->reset_gpio)

drop check 'hub->devtype_data' check

> +		fsleep(hub->devtype_data->reset_duration);
> +
>  	hub->dev = dev;
>  	mutex_init(&hub->lock);
>  	INIT_LIST_HEAD(&hub->udev_list);
> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> index d3a5b6938582..7e743f4c8aaa 100644
> --- a/drivers/usb/misc/onboard_usb_hub.h
> +++ b/drivers/usb/misc/onboard_usb_hub.h
> @@ -6,6 +6,11 @@
>  #ifndef _USB_MISC_ONBOARD_USB_HUB_H
>  #define _USB_MISC_ONBOARD_USB_HUB_H
>  
> +struct onboard_hub_devtype_data {
> +	unsigned long power_stable_time;	/* power stabilization time in us */
> +	unsigned long reset_duration;		/* reset pulse width in us */

Let's encode the unit in the field name:

	unsigned long power_stable_us;
	unsigned long reset_us;

> +};

Is it necessary to define this struct in onboard_usb_hub.h? It seems it
could as well be defined in onboard_usb_hub.c.

Re: [PATCH v2 3/3] usb: misc: onboard_usb_hub: Add TI USB8041 hub support

[Matthias Kaehlcke]

On Fri, Jul 15, 2022 at 09:33:00AM +0200, Alexander Stein wrote:
> This is a 4-port 3.0 USB hub.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v2:
> * Add devtype data containing waiting times
> 
>  drivers/usb/misc/onboard_usb_hub.c | 3 +++
>  drivers/usb/misc/onboard_usb_hub.h | 7 +++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index 1dd7f4767def..319b4b1748fb 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -339,6 +339,7 @@ static struct platform_driver onboard_hub_driver = {
>  /************************** USB driver **************************/
>  
>  #define VENDOR_ID_REALTEK	0x0bda
> +#define VENDOR_ID_TI		0x0451
>  
>  /*
>   * Returns the onboard_hub platform device that is associated with the USB
> @@ -416,6 +417,8 @@ static const struct usb_device_id onboard_hub_id_table[] = {
>  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
>  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
>  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
> +	{ USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
> +	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
>  	{}
>  };
>  MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> index 7e743f4c8aaa..fcb6a9024bbd 100644
> --- a/drivers/usb/misc/onboard_usb_hub.h
> +++ b/drivers/usb/misc/onboard_usb_hub.h
> @@ -11,7 +11,14 @@ struct onboard_hub_devtype_data {
>  	unsigned long reset_duration;		/* reset pulse width in us */
>  };
>  
> +static const struct onboard_hub_devtype_data ti_tusb8041_data = {
> +	.power_stable_time = 3000,
> +	.reset_duration = 3000,

Aren't these two values actually the same thing, i.e. the minimum
duration of the reset?

From the data sheet:

  A minimum reset duration of 3 ms is required

  td2: VDD and VDD33 stable before de-assertion of GRSTz (>= 3ms)

My understanding is that td2 is just another expression of the first
requirement.

> +};
> +
>  static const struct of_device_id onboard_hub_match[] = {
> +	{ .compatible = "usb451,8140", .data = &ti_tusb8041_data, },
> +	{ .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
>  	{ .compatible = "usbbda,411" },
>  	{ .compatible = "usbbda,5411" },
>  	{ .compatible = "usbbda,414" },

Ah, now I see why the struct is defined in the .h file, never mind my comment
on the other patch.

Re: [PATCH v2 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller

[Matthias Kaehlcke]

On Fri, Jul 15, 2022 at 09:32:58AM +0200, Alexander Stein wrote:
> The TI USB8041 is a USB 3.0 hub controller with 4 ports.
> 
> This initial version of the binding only describes USB related aspects
> of the USB8041, it does not cover the option of connecting the controller
> as an i2c slave.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v2:
> * Removed 'items' from compatible, it's just en enum now
> * Rename reset-gpio to reset-gpios
> * Use 'items' for reset-gpios
> * Adjust description of vdd-supply
> * Sorted required list
> * Adjusted example
> 
>  .../devicetree/bindings/usb/ti,usb8041.yaml   | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> new file mode 100644
> index 000000000000..7fe7416e2b51
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for the TI USB8041 USB 3.0 hub controller
> +
> +maintainers:
> +  - Matthias Kaehlcke <mka@chromium.org>

Not sure this should be me, just because I'm the author/maintainer of
the onboard_usb_hub driver, which may be used for this hub.

> +
> +allOf:
> +  - $ref: usb-device.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - usb451,8140
> +      - usb451,8142
> +
> +  reg: true
> +
> +  reset-gpios:
> +    items:
> +      - description: GPIO specifier for GRST# pin.
> +
> +  vdd-supply:
> +    description:
> +      "VDD power supply to the hub"
> +
> +  peer-hub:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to the peer hub on the controller.
> +
> +required:
> +  - compatible
> +  - reg
> +  - peer-hub
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    usb {
> +        dr_mode = "host";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        /* 2.0 hub on port 1 */
> +        hub_2_0: hub@1 {
> +          compatible = "usb451,8142";
> +          reg = <1>;
> +          peer-hub = <&hub_3_0>;
> +          reset-gpios = <&gpio1 11 GPIO_ACTIVE_LOW>;
> +        };
> +
> +        /* 3.0 hub on port 2 */
> +        hub_3_0: hub@2 {
> +          compatible = "usb451,8140";
> +          reg = <2>;
> +          peer-hub = <&hub_2_0>;
> +          reset-gpios = <&gpio1 11 GPIO_ACTIVE_LOW>;
> +        };
> +    };
> -- 
> 2.25.1
> 

Re: Re: [PATCH v2 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller

[Alexander Stein]

Hi Matthias,

Am Samstag, 16. Juli 2022, 01:53:12 CEST schrieb Matthias Kaehlcke:
> On Fri, Jul 15, 2022 at 09:32:58AM +0200, Alexander Stein wrote:
> > The TI USB8041 is a USB 3.0 hub controller with 4 ports.
> > 
> > This initial version of the binding only describes USB related aspects
> > of the USB8041, it does not cover the option of connecting the controller
> > as an i2c slave.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v2:
> > * Removed 'items' from compatible, it's just en enum now
> > * Rename reset-gpio to reset-gpios
> > * Use 'items' for reset-gpios
> > * Adjust description of vdd-supply
> > * Sorted required list
> > * Adjusted example
> > 
> >  .../devicetree/bindings/usb/ti,usb8041.yaml   | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml new file mode
> > 100644
> > index 000000000000..7fe7416e2b51
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Binding for the TI USB8041 USB 3.0 hub controller
> > +
> > +maintainers:
> > +  - Matthias Kaehlcke <mka@chromium.org>
> 
> Not sure this should be me, just because I'm the author/maintainer of
> the onboard_usb_hub driver, which may be used for this hub.

Sure, if you are uncomfortable I'll add myself here.

Regards,
Alexander

> > +
> > +allOf:
> > +  - $ref: usb-device.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - usb451,8140
> > +      - usb451,8142
> > +
> > +  reg: true
> > +
> > +  reset-gpios:
> > +    items:
> > +      - description: GPIO specifier for GRST# pin.
> > +
> > +  vdd-supply:
> > +    description:
> > +      "VDD power supply to the hub"
> > +
> > +  peer-hub:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      phandle to the peer hub on the controller.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - peer-hub
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    usb {
> > +        dr_mode = "host";
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        /* 2.0 hub on port 1 */
> > +        hub_2_0: hub@1 {
> > +          compatible = "usb451,8142";
> > +          reg = <1>;
> > +          peer-hub = <&hub_3_0>;
> > +          reset-gpios = <&gpio1 11 GPIO_ACTIVE_LOW>;
> > +        };
> > +
> > +        /* 3.0 hub on port 2 */
> > +        hub_3_0: hub@2 {
> > +          compatible = "usb451,8140";
> > +          reg = <2>;
> > +          peer-hub = <&hub_2_0>;
> > +          reset-gpios = <&gpio1 11 GPIO_ACTIVE_LOW>;
> > +        };
> > +    };

Re: Re: [PATCH v2 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller

[Alexander Stein]

Hi Krzysztof,

Am Freitag, 15. Juli 2022, 10:17:31 CEST schrieb Krzysztof Kozlowski:
> On 15/07/2022 09:32, Alexander Stein wrote:
> > The TI USB8041 is a USB 3.0 hub controller with 4 ports.
> > 
> > This initial version of the binding only describes USB related aspects
> > of the USB8041, it does not cover the option of connecting the controller
> > as an i2c slave.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v2:
> > * Removed 'items' from compatible, it's just en enum now
> > * Rename reset-gpio to reset-gpios
> > * Use 'items' for reset-gpios
> > * Adjust description of vdd-supply
> > * Sorted required list
> > * Adjusted example
> > 
> >  .../devicetree/bindings/usb/ti,usb8041.yaml   | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml new file mode
> > 100644
> > index 000000000000..7fe7416e2b51
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Binding for the TI USB8041 USB 3.0 hub controller
> > +
> > +maintainers:
> > +  - Matthias Kaehlcke <mka@chromium.org>
> > +
> > +allOf:
> > +  - $ref: usb-device.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - usb451,8140
> > +      - usb451,8142
> > +
> > +  reg: true
> > +
> > +  reset-gpios:
> > +    items:
> > +      - description: GPIO specifier for GRST# pin.
> > +
> > +  vdd-supply:
> > +    description:
> > +      "VDD power supply to the hub"
> 
> No quotes needed.

Fixed, thanks.

Regards,
Alexander

Re: Re: [PATCH v2 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support

[Alexander Stein]

Hi Matthias,

Am Freitag, 15. Juli 2022, 21:44:12 CEST schrieb Matthias Kaehlcke:
> On Fri, Jul 15, 2022 at 09:32:59AM +0200, Alexander Stein wrote:
> > Despite default reset upon probe, release reset line after powering up
> > the hub and assert reset again before powering down.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v2:
> > * Use device specific sleep times, if present
> > * Use fsleep instead of usleep_range
> > 
> >  drivers/usb/misc/onboard_usb_hub.c | 29 +++++++++++++++++++++++++++++
> >  drivers/usb/misc/onboard_usb_hub.h |  5 +++++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > b/drivers/usb/misc/onboard_usb_hub.c index 6b9b949d17d3..1dd7f4767def
> > 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.c
> > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > @@ -7,6 +7,7 @@
> > 
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> > 
> > +#include <linux/gpio/consumer.h>
> > 
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > 
> > @@ -38,6 +39,8 @@ struct usbdev_node {
> > 
> >  struct onboard_hub {
> >  
> >  	struct regulator *vdd;
> >  	struct device *dev;
> > 
> > +	const struct onboard_hub_devtype_data *devtype_data;
> 
> This kind of device specific data is often call platform data, let's
> call the struct 'onboard_hub_pdata' and the field 'pdata'?

I took flexcan as a reference, but I do not mind using other names if that is 
preferred.

> > +	struct gpio_desc *reset_gpio;
> > 
> >  	bool always_powered_in_suspend;
> >  	bool is_powered_on;
> >  	bool going_away;
> > 
> > @@ -56,6 +59,11 @@ static int onboard_hub_power_on(struct onboard_hub
> > *hub)
> > 
> >  		return err;
> >  	
> >  	}
> > 
> > +	if (hub->devtype_data)
> 
> Instead of these checks let's make sure all hubs have pdata. Actually your
> onboard_hub_probe() already esnures that the field is assigned.

For Realtek hubs (no platform data yet), of_id->data therefore hub-
>devtype_data is NULL.
But I agree that providing platformdata for all hubs seems reasonable to get 
rid of these checks.

> > +		fsleep(hub->devtype_data->power_stable_time);
> > +	if (hub->reset_gpio)
> > +		gpiod_set_value_cansleep(hub->reset_gpio, 0);
> 
> I would have expected:
> 
>   	if (hub->reset_gpio) {
> 		fsleep(hub->devtype_data->power_stable_time);
> 		gpiod_set_value_cansleep(hub->reset_gpio, 0);
> 	}
> 
> For the TUSB8041 the 'power_stable_time' (td2 in the datasheet) is "VDD and
> VDD33 stable before de-assertion of GRSTz", so no delay without reset.

Your suggestion only works if you control the reset line (GRSTz) by GPIO. If 
this line is controlled by hardware using RC circuits this waiting time still 
has to be respected when power is supplied.
I prefer that the USB hub is properly reset once this function exits. Without 
waiting time the hardware controlled GRSTz might not yet be at a proper level.

> > +
> > 
> >  	hub->is_powered_on = true;
> >  	
> >  	return 0;
> > 
> > @@ -65,6 +73,12 @@ static int onboard_hub_power_off(struct onboard_hub
> > *hub)> 
> >  {
> >  
> >  	int err;
> > 
> > +	if (hub->reset_gpio) {
> > +		gpiod_set_value_cansleep(hub->reset_gpio, 1);
> > +		if (hub->devtype_data)
> > +			fsleep(hub->devtype_data->reset_duration);
> > +	}
> > +
> > 
> >  	err = regulator_disable(hub->vdd);
> >  	if (err) {
> >  	
> >  		dev_err(hub->dev, "failed to disable regulator: %d\n", 
err);
> > 
> > @@ -219,6 +233,7 @@ static void onboard_hub_attach_usb_driver(struct
> > work_struct *work)> 
> >  static int onboard_hub_probe(struct platform_device *pdev)
> >  {
> > 
> > +	const struct of_device_id *of_id;
> > 
> >  	struct device *dev = &pdev->dev;
> >  	struct onboard_hub *hub;
> >  	int err;
> > 
> > @@ -227,10 +242,24 @@ static int onboard_hub_probe(struct platform_device
> > *pdev)> 
> >  	if (!hub)
> >  	
> >  		return -ENOMEM;
> > 
> > +	of_id = of_match_device(onboard_hub_match, &pdev->dev);
> > +	if (of_id)
> > +		hub->devtype_data = of_id->data;
> > +	else
> > +		return -ENODEV;
> 
> Please change to:
> 
> 	if (!of_id)
> 		return -ENODEV;
> 
> 	hub->pdata = of_id->data;
> 
> With the currently supported Realtek hubs will fail to probe. Please add a
> pdata for the RTS541x hubs. The have no reset signal, so the values can be
> 0.

RTS541x hubs should still probe, but their of_id->data is NULL.
Nevertheless providing platform data for all hubs seems reasonable.

> > +
> > 
> >  	hub->vdd = devm_regulator_get(dev, "vdd");
> >  	if (IS_ERR(hub->vdd))
> >  	
> >  		return PTR_ERR(hub->vdd);
> > 
> > +	hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +						  
GPIOD_OUT_HIGH);
> > +	if (IS_ERR(hub->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(hub->reset_gpio), 
"failed to get
> > reset GPIO\n"); +
> > +	if (hub->devtype_data && hub->reset_gpio)
> 
> drop check 'hub->devtype_data' check

With platform data always provided this will be removed.

> > +		fsleep(hub->devtype_data->reset_duration);
> > +
> > 
> >  	hub->dev = dev;
> >  	mutex_init(&hub->lock);
> >  	INIT_LIST_HEAD(&hub->udev_list);
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.h
> > b/drivers/usb/misc/onboard_usb_hub.h index d3a5b6938582..7e743f4c8aaa
> > 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.h
> > +++ b/drivers/usb/misc/onboard_usb_hub.h
> > @@ -6,6 +6,11 @@
> > 
> >  #ifndef _USB_MISC_ONBOARD_USB_HUB_H
> >  #define _USB_MISC_ONBOARD_USB_HUB_H
> > 
> > +struct onboard_hub_devtype_data {
> > +	unsigned long power_stable_time;	/* power stabilization time in us 
*/
> > +	unsigned long reset_duration;		/* reset pulse width in 
us */
> 
> Let's encode the unit in the field name:
> 
> 	unsigned long power_stable_us;
> 	unsigned long reset_us;

Sure thing. Will be changed.

> > +};
> 
> Is it necessary to define this struct in onboard_usb_hub.h? It seems it
> could as well be defined in onboard_usb_hub.c.

As you pointed out in Patch 3 not possible, unfortunately.

Thanks and best regards,
Alexander

Re: Re: [PATCH v2 3/3] usb: misc: onboard_usb_hub: Add TI USB8041 hub support

[Alexander Stein]

Am Freitag, 15. Juli 2022, 21:50:41 CEST schrieb Matthias Kaehlcke:
> On Fri, Jul 15, 2022 at 09:33:00AM +0200, Alexander Stein wrote:
> > This is a 4-port 3.0 USB hub.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v2:
> > * Add devtype data containing waiting times
> > 
> >  drivers/usb/misc/onboard_usb_hub.c | 3 +++
> >  drivers/usb/misc/onboard_usb_hub.h | 7 +++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > b/drivers/usb/misc/onboard_usb_hub.c index 1dd7f4767def..319b4b1748fb
> > 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.c
> > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > @@ -339,6 +339,7 @@ static struct platform_driver onboard_hub_driver = {
> > 
> >  /************************** USB driver **************************/
> >  
> >  #define VENDOR_ID_REALTEK	0x0bda
> > 
> > +#define VENDOR_ID_TI		0x0451
> > 
> >  /*
> >  
> >   * Returns the onboard_hub platform device that is associated with the
> >   USB
> > 
> > @@ -416,6 +417,8 @@ static const struct usb_device_id
> > onboard_hub_id_table[] = {> 
> >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
> >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
> >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
> > 
> > +	{ USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
> > +	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
> > 
> >  	{}
> >  
> >  };
> >  MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.h
> > b/drivers/usb/misc/onboard_usb_hub.h index 7e743f4c8aaa..fcb6a9024bbd
> > 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.h
> > +++ b/drivers/usb/misc/onboard_usb_hub.h
> > @@ -11,7 +11,14 @@ struct onboard_hub_devtype_data {
> > 
> >  	unsigned long reset_duration;		/* reset pulse width in 
us */
> >  
> >  };
> > 
> > +static const struct onboard_hub_devtype_data ti_tusb8041_data = {
> > +	.power_stable_time = 3000,
> > +	.reset_duration = 3000,
> 
> Aren't these two values actually the same thing, i.e. the minimum
> duration of the reset?
> 
> From the data sheet:
> 
>   A minimum reset duration of 3 ms is required
> 
>   td2: VDD and VDD33 stable before de-assertion of GRSTz (>= 3ms)
> 
> My understanding is that td2 is just another expression of the first
> requirement.

They may have the same values, but IMHO they are applied differently.
'power_stable_time' (or power_stable_us now) applies after supplying power, 
while 'reset_duration' (or reset_us now) applies when you deassert the reset 
line. 
For that reason I would like to keep them separated.

Thanks and best regards,
Alexander

Re: [PATCH v2 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support

[Matthias Kaehlcke]

On Wed, Jul 20, 2022 at 09:15:12AM +0200, Alexander Stein wrote:
> Hi Matthias,
> 
> Am Freitag, 15. Juli 2022, 21:44:12 CEST schrieb Matthias Kaehlcke:
> > On Fri, Jul 15, 2022 at 09:32:59AM +0200, Alexander Stein wrote:
> > > Despite default reset upon probe, release reset line after powering up
> > > the hub and assert reset again before powering down.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > Changes in v2:
> > > * Use device specific sleep times, if present
> > > * Use fsleep instead of usleep_range
> > > 
> > >  drivers/usb/misc/onboard_usb_hub.c | 29 +++++++++++++++++++++++++++++
> > >  drivers/usb/misc/onboard_usb_hub.h |  5 +++++
> > >  2 files changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index 6b9b949d17d3..1dd7f4767def
> > > 100644
> > > --- a/drivers/usb/misc/onboard_usb_hub.c
> > > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > > @@ -7,6 +7,7 @@
> > > 
> > >  #include <linux/device.h>
> > >  #include <linux/export.h>
> > > 
> > > +#include <linux/gpio/consumer.h>
> > > 
> > >  #include <linux/init.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/list.h>
> > > 
> > > @@ -38,6 +39,8 @@ struct usbdev_node {
> > > 
> > >  struct onboard_hub {
> > >  
> > >  	struct regulator *vdd;
> > >  	struct device *dev;
> > > 
> > > +	const struct onboard_hub_devtype_data *devtype_data;
> > 
> > This kind of device specific data is often call platform data, let's
> > call the struct 'onboard_hub_pdata' and the field 'pdata'?
> 
> I took flexcan as a reference, but I do not mind using other names if that is 
> preferred.
> 
> > > +	struct gpio_desc *reset_gpio;
> > > 
> > >  	bool always_powered_in_suspend;
> > >  	bool is_powered_on;
> > >  	bool going_away;
> > > 
> > > @@ -56,6 +59,11 @@ static int onboard_hub_power_on(struct onboard_hub
> > > *hub)
> > > 
> > >  		return err;
> > >  	
> > >  	}
> > > 
> > > +	if (hub->devtype_data)
> > 
> > Instead of these checks let's make sure all hubs have pdata. Actually your
> > onboard_hub_probe() already esnures that the field is assigned.
> 
> For Realtek hubs (no platform data yet), of_id->data therefore hub-
> >devtype_data is NULL.
> But I agree that providing platformdata for all hubs seems reasonable to get 
> rid of these checks.
> 
> > > +		fsleep(hub->devtype_data->power_stable_time);
> > > +	if (hub->reset_gpio)
> > > +		gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > 
> > I would have expected:
> > 
> >   	if (hub->reset_gpio) {
> > 		fsleep(hub->devtype_data->power_stable_time);
> > 		gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > 	}
> > 
> > For the TUSB8041 the 'power_stable_time' (td2 in the datasheet) is "VDD and
> > VDD33 stable before de-assertion of GRSTz", so no delay without reset.
> 
> Your suggestion only works if you control the reset line (GRSTz) by GPIO. If 
> this line is controlled by hardware using RC circuits this waiting time still 
> has to be respected when power is supplied.
> I prefer that the USB hub is properly reset once this function exits. Without 
> waiting time the hardware controlled GRSTz might not yet be at a proper level.

Ok, in that case let's omit the check for 'hub->reset_gpio' and just do:

	fsleep(hub->devtype_data->power_stable_time);
	gpiod_set_value_cansleep(hub->reset_gpio, 0);

The point of adding the check was to skip the delay when no reset GPIO is
specified, but apparently that's not what we actually want.

Re: [PATCH v2 3/3] usb: misc: onboard_usb_hub: Add TI USB8041 hub support

[Matthias Kaehlcke]

On Wed, Jul 20, 2022 at 09:18:50AM +0200, Alexander Stein wrote:
> Am Freitag, 15. Juli 2022, 21:50:41 CEST schrieb Matthias Kaehlcke:
> > On Fri, Jul 15, 2022 at 09:33:00AM +0200, Alexander Stein wrote:
> > > This is a 4-port 3.0 USB hub.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > Changes in v2:
> > > * Add devtype data containing waiting times
> > > 
> > >  drivers/usb/misc/onboard_usb_hub.c | 3 +++
> > >  drivers/usb/misc/onboard_usb_hub.h | 7 +++++++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index 1dd7f4767def..319b4b1748fb
> > > 100644
> > > --- a/drivers/usb/misc/onboard_usb_hub.c
> > > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > > @@ -339,6 +339,7 @@ static struct platform_driver onboard_hub_driver = {
> > > 
> > >  /************************** USB driver **************************/
> > >  
> > >  #define VENDOR_ID_REALTEK	0x0bda
> > > 
> > > +#define VENDOR_ID_TI		0x0451
> > > 
> > >  /*
> > >  
> > >   * Returns the onboard_hub platform device that is associated with the
> > >   USB
> > > 
> > > @@ -416,6 +417,8 @@ static const struct usb_device_id
> > > onboard_hub_id_table[] = {> 
> > >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
> > >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
> > >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
> > > 
> > > +	{ USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
> > > +	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
> > > 
> > >  	{}
> > >  
> > >  };
> > >  MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.h
> > > b/drivers/usb/misc/onboard_usb_hub.h index 7e743f4c8aaa..fcb6a9024bbd
> > > 100644
> > > --- a/drivers/usb/misc/onboard_usb_hub.h
> > > +++ b/drivers/usb/misc/onboard_usb_hub.h
> > > @@ -11,7 +11,14 @@ struct onboard_hub_devtype_data {
> > > 
> > >  	unsigned long reset_duration;		/* reset pulse width in 
> us */
> > >  
> > >  };
> > > 
> > > +static const struct onboard_hub_devtype_data ti_tusb8041_data = {
> > > +	.power_stable_time = 3000,
> > > +	.reset_duration = 3000,
> > 
> > Aren't these two values actually the same thing, i.e. the minimum
> > duration of the reset?
> > 
> > From the data sheet:
> > 
> >   A minimum reset duration of 3 ms is required
> > 
> >   td2: VDD and VDD33 stable before de-assertion of GRSTz (>= 3ms)
> > 
> > My understanding is that td2 is just another expression of the first
> > requirement.
> 
> They may have the same values, but IMHO they are applied differently.
> 'power_stable_time' (or power_stable_us now) applies after supplying power, 
> while 'reset_duration' (or reset_us now) applies when you deassert the reset 
> line. 
> For that reason I would like to keep them separated.

In both cases the datasheet talks about the reset duration of 3 ms in
relation with the power supplies:

  7.6 Timing Requirements, Power-Up

  td2: VDD and VDD33 stable before de-assertion of GRSTz


  8.3.7 Power-Up and Reset

  A minimum reset duration of 3 ms is required. This is defined as the time when
  the power supplies are in the recommended operating range to the de-assertion
  of GRSTz.

At this point I see no good reason for having two different values. If it turns
out later that it is really needed for other hubs another fiels like
'power_stable_us' can still be added.