diff mbox series

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

Message ID 20220715073300.868087-2-alexander.stein@ew.tq-group.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller | expand

Commit Message

Alexander Stein July 15, 2022, 7:32 a.m. UTC
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(+)

Comments

Matthias Kaehlcke July 15, 2022, 7:44 p.m. UTC | #1
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.
Alexander Stein July 20, 2022, 7:15 a.m. UTC | #2
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
Matthias Kaehlcke July 21, 2022, 8:25 p.m. UTC | #3
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.
diff mbox series

Patch

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" },