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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Matthias,

Am Dienstag, 12. Juli 2022, 20:18:05 CEST schrieb Matthias Kaehlcke:
> On Tue, Jul 12, 2022 at 05:06:26PM +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@xxxxxxxxxxxxxxx>
> > ---
> > My current DT node on my TQMa8MPxL looks like this
> > ```
> > &usb_dwc3_1 {
> > 
> > 	dr_mode = "host";
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 	pinctrl-names = "default";
> > 	pinctrl-0 = <&pinctrl_usbhub>;
> > 	status = "okay";
> > 	
> > 	hub_2_0: hub@1 {
> > 	
> > 		compatible = "usb451,8142";
> > 		reg = <1>;
> > 		peer-hub = <&hub_3_0>;
> > 		reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> > 	
> > 	};
> > 	
> > 	hub_3_0: hub@2 {
> > 	
> > 		compatible = "usb451,8140";
> > 		reg = <2>;
> > 		peer-hub = <&hub_2_0>;
> > 		reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> > 	
> > 	};
> > 
> > };
> > ```
> > which I don't like much for 2 reasons:
> > * the pinctrl has to be put in a common top-node of USB hub node. The
> > pinctrl> 
> >   can not be requested twice.
> 
> Agreed, that's not great. The pinctrl doesn't have to be necessarily in the
> USB controller node, it could also be in the static section of the board,
> but that isn't really much of an improvement :( Not sure there is much to
> do given that the USB devices also process the pinctrl info (besides the
> onboard_hub platform device doing the same).

I tend to keep the pinctrl property next to the ones actually using them. But 
in this case it's not possible unfortunately.

> > * Apparently there is no conflict on the reset-gpio only because just one
> > device> 
> >   gets probed here:
> > > $ ls /sys/bus/platform/drivers/onboard-usb-hub/
> > > 38200000.usb:hub@1  bind  uevent  unbind
> 
> Right, the driver creates a single platform device for each physical hub.

Thanks for confirmation.

> > But this seems better than to use a common fixed-regulator referenced by
> > both hub nodes, which just is controlled by GPIO and does not supply any
> > voltages.
> Agreed, if the GPIO controls a reset line it should be implemented as such.
> 
> > Note: It might also be necessary to add bindings to specify ramp up times
> > and/or reset timeouts.
> 
> The times are hub specific, not board specific, right? If that's the case
> then a binding shouldn't be needed, the timing can be derived from the
> compatible string.

Well, yes they are hub specific, but board design might influence them as 
well, as in increased ramp up delay.

> >  drivers/usb/misc/onboard_usb_hub.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > b/drivers/usb/misc/onboard_usb_hub.c index 6b9b949d17d3..348fb5270266
> > 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,7 @@ struct usbdev_node {
> > 
> >  struct onboard_hub {
> >  
> >  	struct regulator *vdd;
> >  	struct device *dev;
> > 
> > +	struct gpio_desc *reset_gpio;
> > 
> >  	bool always_powered_in_suspend;
> >  	bool is_powered_on;
> >  	bool going_away;
> > 
> > @@ -56,6 +58,10 @@ static int onboard_hub_power_on(struct onboard_hub
> > *hub)
> > 
> >  		return err;
> >  	
> >  	}
> > 
> > +	/* Deassert reset */
> 
> The comment isn't really needed, it's clear from the context.

Ok, removed.

> > +	usleep_range(3000, 3100);
> 
> These shouldn't be hard coded. Instead you could add a model specific struct
> 'hub_data' (or similar) and associate it with the compatible string through
> onboard_hub_match.data

Will do.

> You could use fsleep() instead of usleep_range(). It does the _range part
> automatically (with a value of 2x).

Nice idea.

> > +	gpiod_set_value_cansleep(hub->reset_gpio, 0);
> 
> Since this includes delays maybe put the reset inside an 'if
> (hub->reset_gpio)' block. Not super important for these short delays, but
> they might be longer for some hubs.

gpiod_set_value_cansleep includes delays? Without gpio_desc it returns early 
on. Or do you mean the usleep_range before?
Actually in this case the 3ms is the minimum time from VDD stable to de-
assertion of GRST. So even in case the GPIO is manged by hardware itself, 
software has to wait here before proceeding, IMHO.

> > +
> > 
> >  	hub->is_powered_on = true;
> >  	
> >  	return 0;
> > 
> > @@ -65,6 +71,10 @@ static int onboard_hub_power_off(struct onboard_hub
> > *hub)> 
> >  {
> >  
> >  	int err;
> > 
> > +	/* Assert reset */
> 
> drop comment

Will do.

> > +	gpiod_set_value_cansleep(hub->reset_gpio, 1);
> 
> Put inside 'if (hub->reset_gpio)' to avoid unnecessary delays when no reset
> is configured.
> 
> > +	usleep_range(4000, 5000);
> 
> Use per-model values.

Will do.

> > +
> > 
> >  	err = regulator_disable(hub->vdd);
> >  	if (err) {
> >  	
> >  		dev_err(hub->dev, "failed to disable regulator: %d\n", 
err);
> > 
> > @@ -231,6 +241,14 @@ static int onboard_hub_probe(struct platform_device
> > *pdev)> 
> >  	if (IS_ERR(hub->vdd))
> >  	
> >  		return PTR_ERR(hub->vdd);
> > 
> > +	/* Put the hub into reset, pull reset line low, and assure 4ms 
reset low
> > timing. */
> drop comment, it's mostly evident from the code. Maybe not the
> usleep_range() part, but that should become clearer when per model values
> are used.

Will do.

> > +	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"); +
> > +	usleep_range(4000, 5000);
> > +
> > 
> >  	hub->dev = dev;
> >  	mutex_init(&hub->lock);
> >  	INIT_LIST_HEAD(&hub->udev_list);







[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux