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

[Matthias Kaehlcke <mka@chromium.org>]

On Thu, Jul 28, 2022 at 08:20:13AM +0200, Alexander Stein wrote:
> Hello Matthias,
> 
> Am Mittwoch, 27. Juli 2022, 19:14:57 CEST schrieb Matthias Kaehlcke:
> > Hi Alexander,
> > 
> > (copying my comments from v3 to keep the discussion on the latest version)
> > 
> > On Wed, Jul 27, 2022 at 04:11:16PM +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>
> > > ---
> > > * Patch 1 dropped as it already applied
> > > 
> > > Changes in v4:
> > > * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs
> > > snprintf to sysfs_emit") * Added platform data for usb424
> > > 
> > >   Reset pulse length taken from [2], Table 3-2 Symbol RESET_N
> > >   Completely untested
> > > 
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch
> > > usb-testing [2]
> > > http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf
> > > 
> > >  drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++
> > >  drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++-----
> > >  2 files changed, 45 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7
> > > 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_pdata *pdata;
> > > +	struct gpio_desc *reset_gpio;
> > > 
> > >  	bool always_powered_in_suspend;
> > >  	bool is_powered_on;
> > >  	bool going_away;
> > > 
> > > @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
> > > 
> > >  		return err;
> > >  	
> > >  	}
> > > 
> > > +	fsleep(hub->pdata->reset_us);
> > > +	gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > > +
> > > 
> > >  	hub->is_powered_on = true;
> > >  	
> > >  	return 0;
> > > 
> > > @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub
> > > *hub)> 
> > >  {
> > >  
> > >  	int err;
> > > 
> > > +	if (hub->reset_gpio) {
> > > +		gpiod_set_value_cansleep(hub->reset_gpio, 1);
> > > +		fsleep(hub->pdata->reset_us);
> > 
> > Is this delay here actually needed? There is a delay in
> > onboard_hub_power_on(), before de-asserting the reset, isn't that enough?
> 
> If you see both delays together you are right, but I tend to think in that way 
> it is to ensure whenever we apply a reset it is long enough.
> As said before the powering on delay is to ensure the pulse length delay even
> if there is no reset GPIO but it is controlled by hardware.

In _power_off() the delay is currently only applied when the reset line is
*not* controlled by the hardware. In that case the delay in _power_on()
already ensures that the reset is applied long enough. Am I missing
something?

As per my reply on v2 [1] the datasheet does not suggest that there are two
different delays:

> In both cases the datasheet talks about the reset duration of 3 ms in
> relation with the power supplies:
>
>   7.6 Timing Requirements, Power-Up
>
>   td2: VDD and VDD33 stable before de-assertion of GRSTz
>
>
>   8.3.7 Power-Up and Reset
>
>   A minimum reset duration of 3 ms is required. This is defined as the time when
>   the power supplies are in the recommended operating range to the de-assertion
>   of GRSTz.

If there is evidence that a reset delay and a separate power stable delay are
needed I'm very open to add a second delay, but as of now my interpretation of
the datasheet is that the reset should be applied for at least 3ms after the
regulators are turned on (and power is stable, which should be ensured by
the regulator config).

[1] https://lore.kernel.org/all/Ytm5RrXYaKtwEg23@google.com/

> > > +	}
> > > +
> > > 
> > >  	err = regulator_disable(hub->vdd);
> > >  	if (err) {
> > >  	
> > >  		dev_err(hub->dev, "failed to disable regulator: %d\n", 
> err);
> > > 
> > > @@ -219,6 +230,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 +239,26 @@ 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)
> > > +		return -ENODEV;
> > > +
> > > +	hub->pdata = of_id->data;
> > > +	if (!hub->pdata)
> > > +		return -EINVAL;
> > > +
> > > 
> > >  	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->reset_gpio)
> > > +		fsleep(hub->pdata->reset_us);
> > 
> > Same question here: onboard_hub_power_on() is called a few lines below and
> > has a delay before de-asserting the reset. Is the delay here really needed?
> 
> This actually looks like the delay is duplicated here. I agree with removing 
> this.
> How shall we proceed now that the whole series (incl. the bindings patch 1/3 
> from v3) has landed in usb-testing? I can create a patch on top of this if 
> this is the way to go.
> 
> Best regards,
> Alexander
> 
> 
>