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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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