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 Alexander,

On Wed, Jul 13, 2022 at 08:46:56AM +0200, Alexander Stein wrote:
> 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.

Isn't the ramp up delay something that should be configured on the regulator
side with 'regulator-ramp-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?

Yes, I was referring to 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.

Agreed.



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux