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

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

Hi Matthias,

Am Mittwoch, 13. Juli 2022, 18:59:44 CEST schrieb Matthias Kaehlcke:
> 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@ew.tq-group.com>
> > > > ---
> > > > 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'?

Sure, if you have a regulators you can do that. But even for the reset GPIO 
lines an RC circuit can stretch the ramp up. AFAIK there is no way to handle 
this despite inserting a waiting time in driver code itself.
For now this is good, but it might be necessary to accompany for that at some 
point.

Regards,
Alexander

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