[PATCH v4 1/2] usb: misc: onboard_usb_hub: Add reset-gpio support

[PATCH v4 1/2] 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>
---
* Patch 1 dropped as it already applied

Changes in v4:
* Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs snprintf to sysfs_emit")
* Added platform data for usb424
  Reset pulse length taken from [2], Table 3-2 Symbol RESET_N
  Completely untested

[1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch usb-testing
[2] http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf

 drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++
 drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++-----
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index de3627af3c84..0c81417dd9a7 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_pdata *pdata;
+	struct gpio_desc *reset_gpio;
 	bool always_powered_in_suspend;
 	bool is_powered_on;
 	bool going_away;
@@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
 		return err;
 	}
 
+	fsleep(hub->pdata->reset_us);
+	gpiod_set_value_cansleep(hub->reset_gpio, 0);
+
 	hub->is_powered_on = true;
 
 	return 0;
@@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
 {
 	int err;
 
+	if (hub->reset_gpio) {
+		gpiod_set_value_cansleep(hub->reset_gpio, 1);
+		fsleep(hub->pdata->reset_us);
+	}
+
 	err = regulator_disable(hub->vdd);
 	if (err) {
 		dev_err(hub->dev, "failed to disable regulator: %d\n", err);
@@ -219,6 +230,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 +239,26 @@ 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)
+		return -ENODEV;
+
+	hub->pdata = of_id->data;
+	if (!hub->pdata)
+		return -EINVAL;
+
 	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->reset_gpio)
+		fsleep(hub->pdata->reset_us);
+
 	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 3820669eb98c..562fa48fcf10 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -6,12 +6,24 @@
 #ifndef _USB_MISC_ONBOARD_USB_HUB_H
 #define _USB_MISC_ONBOARD_USB_HUB_H
 
+struct onboard_hub_pdata {
+	unsigned long reset_us;		/* reset pulse width in us */
+};
+
+static const struct onboard_hub_pdata microchip_usb424_data = {
+	.reset_us = 1,
+};
+
+static const struct onboard_hub_pdata realtek_rts5411_data = {
+	.reset_us = 0,
+};
+
 static const struct of_device_id onboard_hub_match[] = {
-	{ .compatible = "usb424,2514" },
-	{ .compatible = "usbbda,411" },
-	{ .compatible = "usbbda,5411" },
-	{ .compatible = "usbbda,414" },
-	{ .compatible = "usbbda,5414" },
+	{ .compatible = "usb424,2514", .data = &microchip_usb424_data, },
+	{ .compatible = "usbbda,411", .data = &realtek_rts5411_data, },
+	{ .compatible = "usbbda,5411", .data = &realtek_rts5411_data, },
+	{ .compatible = "usbbda,414", .data = &realtek_rts5411_data, },
+	{ .compatible = "usbbda,5414", .data = &realtek_rts5411_data, },
 	{}
 };
 
-- 
2.25.1

[PATCH v4 2/2] 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 v4:
* Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs snprintf to sysfs_emit")

[1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch usb-testing

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

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 0c81417dd9a7..eb8aef25a22d 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 = {
 
 #define VENDOR_ID_MICROCHIP	0x0424
 #define VENDOR_ID_REALTEK	0x0bda
+#define VENDOR_ID_TI		0x0451
 
 /*
  * Returns the onboard_hub platform device that is associated with the USB
@@ -417,6 +418,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 562fa48fcf10..34beab8bce3d 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -18,8 +18,14 @@ static const struct onboard_hub_pdata realtek_rts5411_data = {
 	.reset_us = 0,
 };
 
+static const struct onboard_hub_pdata ti_tusb8041_data = {
+	.reset_us = 3000,
+};
+
 static const struct of_device_id onboard_hub_match[] = {
 	{ .compatible = "usb424,2514", .data = &microchip_usb424_data, },
+	{ .compatible = "usb451,8140", .data = &ti_tusb8041_data, },
+	{ .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
 	{ .compatible = "usbbda,411", .data = &realtek_rts5411_data, },
 	{ .compatible = "usbbda,5411", .data = &realtek_rts5411_data, },
 	{ .compatible = "usbbda,414", .data = &realtek_rts5411_data, },
-- 
2.25.1

Re: [PATCH v4 2/2] usb: misc: onboard_usb_hub: Add TI USB8041 hub support

[Matthias Kaehlcke]

On Wed, Jul 27, 2022 at 04:11:17PM +0200, Alexander Stein wrote:
> This is a 4-port 3.0 USB hub.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Acked-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v4 1/2] usb: misc: onboard_usb_hub: Add reset-gpio support

[Matthias Kaehlcke]

Hi Alexander,

(copying my comments from v3 to keep the discussion on the latest version)

On Wed, Jul 27, 2022 at 04:11:16PM +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>
> ---
> * Patch 1 dropped as it already applied
> 
> Changes in v4:
> * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs snprintf to sysfs_emit")
> * Added platform data for usb424
>   Reset pulse length taken from [2], Table 3-2 Symbol RESET_N
>   Completely untested
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch usb-testing
> [2] http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf
> 
>  drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++
>  drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++-----
>  2 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index de3627af3c84..0c81417dd9a7 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_pdata *pdata;
> +	struct gpio_desc *reset_gpio;
>  	bool always_powered_in_suspend;
>  	bool is_powered_on;
>  	bool going_away;
> @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
>  		return err;
>  	}
>  
> +	fsleep(hub->pdata->reset_us);
> +	gpiod_set_value_cansleep(hub->reset_gpio, 0);
> +
>  	hub->is_powered_on = true;
>  
>  	return 0;
> @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
>  {
>  	int err;
>  
> +	if (hub->reset_gpio) {
> +		gpiod_set_value_cansleep(hub->reset_gpio, 1);
> +		fsleep(hub->pdata->reset_us);

Is this delay here actually needed? There is a delay in onboard_hub_power_on(),
before de-asserting the reset, isn't that enough?

> +	}
> +
>  	err = regulator_disable(hub->vdd);
>  	if (err) {
>  		dev_err(hub->dev, "failed to disable regulator: %d\n", err);
> @@ -219,6 +230,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 +239,26 @@ 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)
> +		return -ENODEV;
> +
> +	hub->pdata = of_id->data;
> +	if (!hub->pdata)
> +		return -EINVAL;
> +
>  	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->reset_gpio)
> +		fsleep(hub->pdata->reset_us);

Same question here: onboard_hub_power_on() is called a few lines below and
has a delay before de-asserting the reset. Is the delay here really needed?

Re: [PATCH v4 1/2] usb: misc: onboard_usb_hub: Add reset-gpio support

[Alexander Stein]

Hello Matthias,

Am Mittwoch, 27. Juli 2022, 19:14:57 CEST schrieb Matthias Kaehlcke:
> Hi Alexander,
> 
> (copying my comments from v3 to keep the discussion on the latest version)
> 
> On Wed, Jul 27, 2022 at 04:11:16PM +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>
> > ---
> > * Patch 1 dropped as it already applied
> > 
> > Changes in v4:
> > * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs
> > snprintf to sysfs_emit") * Added platform data for usb424
> > 
> >   Reset pulse length taken from [2], Table 3-2 Symbol RESET_N
> >   Completely untested
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch
> > usb-testing [2]
> > http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf
> > 
> >  drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++
> >  drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++-----
> >  2 files changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7
> > 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_pdata *pdata;
> > +	struct gpio_desc *reset_gpio;
> > 
> >  	bool always_powered_in_suspend;
> >  	bool is_powered_on;
> >  	bool going_away;
> > 
> > @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
> > 
> >  		return err;
> >  	
> >  	}
> > 
> > +	fsleep(hub->pdata->reset_us);
> > +	gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > +
> > 
> >  	hub->is_powered_on = true;
> >  	
> >  	return 0;
> > 
> > @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub
> > *hub)> 
> >  {
> >  
> >  	int err;
> > 
> > +	if (hub->reset_gpio) {
> > +		gpiod_set_value_cansleep(hub->reset_gpio, 1);
> > +		fsleep(hub->pdata->reset_us);
> 
> Is this delay here actually needed? There is a delay in
> onboard_hub_power_on(), before de-asserting the reset, isn't that enough?

If you see both delays together you are right, but I tend to think in that way 
it is to ensure whenever we apply a reset it is long enough.
As said before the powering on delay is to ensure the pulse length delay even 
if there is no reset GPIO but it is controlled by hardware.

> > +	}
> > +
> > 
> >  	err = regulator_disable(hub->vdd);
> >  	if (err) {
> >  	
> >  		dev_err(hub->dev, "failed to disable regulator: %d\n", 
err);
> > 
> > @@ -219,6 +230,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 +239,26 @@ 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)
> > +		return -ENODEV;
> > +
> > +	hub->pdata = of_id->data;
> > +	if (!hub->pdata)
> > +		return -EINVAL;
> > +
> > 
> >  	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->reset_gpio)
> > +		fsleep(hub->pdata->reset_us);
> 
> Same question here: onboard_hub_power_on() is called a few lines below and
> has a delay before de-asserting the reset. Is the delay here really needed?

This actually looks like the delay is duplicated here. I agree with removing 
this.
How shall we proceed now that the whole series (incl. the bindings patch 1/3 
from v3) has landed in usb-testing? I can create a patch on top of this if 
this is the way to go.

Best regards,
Alexander

Re: [PATCH v4 1/2] usb: misc: onboard_usb_hub: Add reset-gpio support

[Greg Kroah-Hartman]

On Thu, Jul 28, 2022 at 08:20:13AM +0200, Alexander Stein wrote:
> Hello Matthias,
> 
> Am Mittwoch, 27. Juli 2022, 19:14:57 CEST schrieb Matthias Kaehlcke:
> > Hi Alexander,
> > 
> > (copying my comments from v3 to keep the discussion on the latest version)
> > 
> > On Wed, Jul 27, 2022 at 04:11:16PM +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>
> > > ---
> > > * Patch 1 dropped as it already applied
> > > 
> > > Changes in v4:
> > > * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs
> > > snprintf to sysfs_emit") * Added platform data for usb424
> > > 
> > >   Reset pulse length taken from [2], Table 3-2 Symbol RESET_N
> > >   Completely untested
> > > 
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch
> > > usb-testing [2]
> > > http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf
> > > 
> > >  drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++
> > >  drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++-----
> > >  2 files changed, 45 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7
> > > 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_pdata *pdata;
> > > +	struct gpio_desc *reset_gpio;
> > > 
> > >  	bool always_powered_in_suspend;
> > >  	bool is_powered_on;
> > >  	bool going_away;
> > > 
> > > @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
> > > 
> > >  		return err;
> > >  	
> > >  	}
> > > 
> > > +	fsleep(hub->pdata->reset_us);
> > > +	gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > > +
> > > 
> > >  	hub->is_powered_on = true;
> > >  	
> > >  	return 0;
> > > 
> > > @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub
> > > *hub)> 
> > >  {
> > >  
> > >  	int err;
> > > 
> > > +	if (hub->reset_gpio) {
> > > +		gpiod_set_value_cansleep(hub->reset_gpio, 1);
> > > +		fsleep(hub->pdata->reset_us);
> > 
> > Is this delay here actually needed? There is a delay in
> > onboard_hub_power_on(), before de-asserting the reset, isn't that enough?
> 
> If you see both delays together you are right, but I tend to think in that way 
> it is to ensure whenever we apply a reset it is long enough.
> As said before the powering on delay is to ensure the pulse length delay even 
> if there is no reset GPIO but it is controlled by hardware.
> 
> > > +	}
> > > +
> > > 
> > >  	err = regulator_disable(hub->vdd);
> > >  	if (err) {
> > >  	
> > >  		dev_err(hub->dev, "failed to disable regulator: %d\n", 
> err);
> > > 
> > > @@ -219,6 +230,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 +239,26 @@ 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)
> > > +		return -ENODEV;
> > > +
> > > +	hub->pdata = of_id->data;
> > > +	if (!hub->pdata)
> > > +		return -EINVAL;
> > > +
> > > 
> > >  	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->reset_gpio)
> > > +		fsleep(hub->pdata->reset_us);
> > 
> > Same question here: onboard_hub_power_on() is called a few lines below and
> > has a delay before de-asserting the reset. Is the delay here really needed?
> 
> This actually looks like the delay is duplicated here. I agree with removing 
> this.
> How shall we proceed now that the whole series (incl. the bindings patch 1/3 
> from v3) has landed in usb-testing? I can create a patch on top of this if 
> this is the way to go.

Please do.

Re: [PATCH v4 1/2] usb: misc: onboard_usb_hub: Add reset-gpio support

[Matthias Kaehlcke]

On Thu, Jul 28, 2022 at 08:20:13AM +0200, Alexander Stein wrote:
> Hello Matthias,
> 
> Am Mittwoch, 27. Juli 2022, 19:14:57 CEST schrieb Matthias Kaehlcke:
> > Hi Alexander,
> > 
> > (copying my comments from v3 to keep the discussion on the latest version)
> > 
> > On Wed, Jul 27, 2022 at 04:11:16PM +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>
> > > ---
> > > * Patch 1 dropped as it already applied
> > > 
> > > Changes in v4:
> > > * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs
> > > snprintf to sysfs_emit") * Added platform data for usb424
> > > 
> > >   Reset pulse length taken from [2], Table 3-2 Symbol RESET_N
> > >   Completely untested
> > > 
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch
> > > usb-testing [2]
> > > http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf
> > > 
> > >  drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++
> > >  drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++-----
> > >  2 files changed, 45 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7
> > > 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_pdata *pdata;
> > > +	struct gpio_desc *reset_gpio;
> > > 
> > >  	bool always_powered_in_suspend;
> > >  	bool is_powered_on;
> > >  	bool going_away;
> > > 
> > > @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
> > > 
> > >  		return err;
> > >  	
> > >  	}
> > > 
> > > +	fsleep(hub->pdata->reset_us);
> > > +	gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > > +
> > > 
> > >  	hub->is_powered_on = true;
> > >  	
> > >  	return 0;
> > > 
> > > @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub
> > > *hub)> 
> > >  {
> > >  
> > >  	int err;
> > > 
> > > +	if (hub->reset_gpio) {
> > > +		gpiod_set_value_cansleep(hub->reset_gpio, 1);
> > > +		fsleep(hub->pdata->reset_us);
> > 
> > Is this delay here actually needed? There is a delay in
> > onboard_hub_power_on(), before de-asserting the reset, isn't that enough?
> 
> If you see both delays together you are right, but I tend to think in that way 
> it is to ensure whenever we apply a reset it is long enough.
> As said before the powering on delay is to ensure the pulse length delay even
> if there is no reset GPIO but it is controlled by hardware.

In _power_off() the delay is currently only applied when the reset line is
*not* controlled by the hardware. In that case the delay in _power_on()
already ensures that the reset is applied long enough. Am I missing
something?

As per my reply on v2 [1] the datasheet does not suggest that there are two
different delays:

> 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.

If there is evidence that a reset delay and a separate power stable delay are
needed I'm very open to add a second delay, but as of now my interpretation of
the datasheet is that the reset should be applied for at least 3ms after the
regulators are turned on (and power is stable, which should be ensured by
the regulator config).

[1] https://lore.kernel.org/all/Ytm5RrXYaKtwEg23@google.com/

> > > +	}
> > > +
> > > 
> > >  	err = regulator_disable(hub->vdd);
> > >  	if (err) {
> > >  	
> > >  		dev_err(hub->dev, "failed to disable regulator: %d\n", 
> err);
> > > 
> > > @@ -219,6 +230,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 +239,26 @@ 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)
> > > +		return -ENODEV;
> > > +
> > > +	hub->pdata = of_id->data;
> > > +	if (!hub->pdata)
> > > +		return -EINVAL;
> > > +
> > > 
> > >  	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->reset_gpio)
> > > +		fsleep(hub->pdata->reset_us);
> > 
> > Same question here: onboard_hub_power_on() is called a few lines below and
> > has a delay before de-asserting the reset. Is the delay here really needed?
> 
> This actually looks like the delay is duplicated here. I agree with removing 
> this.
> How shall we proceed now that the whole series (incl. the bindings patch 1/3 
> from v3) has landed in usb-testing? I can create a patch on top of this if 
> this is the way to go.
> 
> Best regards,
> Alexander
> 
> 
>