Re: [PATCH 2/3] scsi: ufs: Allow resetting the UFS device

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 04 Jun 00:53 PDT 2019, Marc Gonzalez wrote:

> [ Shuffling the recipients list ]
> 
> On 04/06/2019 09:20, Bjorn Andersson wrote:
> 
> > Acquire the device-reset GPIO and toggle this to reset the UFS device
> > during initialization and host reset.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 44 +++++++++++++++++++++++++++++++++++++++
> >  drivers/scsi/ufs/ufshcd.h |  4 ++++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 8c1c551f2b42..951a0efee536 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -42,6 +42,7 @@
> >  #include <linux/nls.h>
> >  #include <linux/of.h>
> >  #include <linux/bitfield.h>
> > +#include <linux/gpio/consumer.h>
> >  #include "ufshcd.h"
> >  #include "ufs_quirks.h"
> >  #include "unipro.h"
> > @@ -6104,6 +6105,25 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> >  	return err;
> >  }
> >  
> > +/**
> > + ufshcd_device_reset() - toggle the (optional) device reset line
> > + * @hba: per-adapter instance
> > + *
> > + * Toggles the (optional) reset line to reset the attached device.
> > + */
> > +static void ufshcd_device_reset(struct ufs_hba *hba)
> > +{
> > +	/*
> > +	 * The USB device shall detect reset pulses of 1us, sleep for 10us to
> > +	 * be on the safe side.
> > +	 */
> > +	gpiod_set_value_cansleep(hba->device_reset, 1);
> > +	usleep_range(10, 15);
> > +
> > +	gpiod_set_value_cansleep(hba->device_reset, 0);
> > +	usleep_range(10, 15);
> > +}
> > +
> >  /**
> >   * ufshcd_host_reset_and_restore - reset and restore host controller
> >   * @hba: per-adapter instance
> > @@ -6159,6 +6179,9 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
> >  	int retries = MAX_HOST_RESET_RETRIES;
> >  
> >  	do {
> > +		/* Reset the attached device */
> > +		ufshcd_device_reset(hba);
> > +
> >  		err = ufshcd_host_reset_and_restore(hba);
> >  	} while (err && --retries);
> >  
> > @@ -7355,6 +7378,18 @@ static void ufshcd_variant_hba_exit(struct ufs_hba *hba)
> >  	ufshcd_vops_exit(hba);
> >  }
> >  
> > +static int ufshcd_init_device_reset(struct ufs_hba *hba)
> > +{
> > +	hba->device_reset = devm_gpiod_get_optional(hba->dev, "device-reset",
> > +						    GPIOD_OUT_HIGH);
> > +	if (IS_ERR(hba->device_reset)) {
> > +		dev_err(hba->dev, "failed to acquire reset gpio: %ld\n",
> > +			PTR_ERR(hba->device_reset));
> > +	}
> > +
> > +	return PTR_ERR_OR_ZERO(hba->device_reset);
> > +}
> > +
> >  static int ufshcd_hba_init(struct ufs_hba *hba)
> >  {
> >  	int err;
> > @@ -7394,9 +7429,15 @@ static int ufshcd_hba_init(struct ufs_hba *hba)
> >  	if (err)
> >  		goto out_disable_vreg;
> >  
> > +	err = ufshcd_init_device_reset(hba);
> > +	if (err)
> > +		goto out_disable_variant;
> > +
> >  	hba->is_powered = true;
> >  	goto out;
> >  
> > +out_disable_variant:
> > +	ufshcd_vops_setup_regulators(hba, false);
> >  out_disable_vreg:
> >  	ufshcd_setup_vreg(hba, false);
> >  out_disable_clks:
> > @@ -8290,6 +8331,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> >  		goto exit_gating;
> >  	}
> >  
> > +	/* Reset the attached device */
> > +	ufshcd_device_reset(hba);
> > +
> >  	/* Host controller enable */
> >  	err = ufshcd_hba_enable(hba);
> >  	if (err) {
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index ecfa898b9ccc..d8be67742168 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -72,6 +72,8 @@
> >  #define UFSHCD "ufshcd"
> >  #define UFSHCD_DRIVER_VERSION "0.2"
> >  
> > +struct gpio_desc;
> > +
> >  struct ufs_hba;
> >  
> >  enum dev_cmd_type {
> > @@ -706,6 +708,8 @@ struct ufs_hba {
> >  
> >  	struct device		bsg_dev;
> >  	struct request_queue	*bsg_queue;
> > +
> > +	struct gpio_desc *device_reset;
> >  };
> >  
> >  /* Returns true if clocks can be gated. Otherwise false */
> > 
> 
> Why is this needed on 845 and not on 8998?
> 

It's needed with certain UFS chips and TLMM in both msm8996 and msm8998
seems to provide an identical mechanism - so patch 1 would have to be
duplicated for those.

> On 8998 we already have:
> 
> 			resets = <&gcc GCC_UFS_BCR>;
> 			reset-names = "rst";
> 

This is the SoC-internal reset signal for the UFS host controller block.

> The above reset line gets wiggled/frobbed when appropriate.
> 
> (What's the difference between gpio and pinctrl? vs a reset "clock" as above)
> 

The TLMM block is the piece responsible for this and it implements GPIO,
pinmux and pinconf functionality. So patch 1 extends the SDM845 pinctrl
driver to expose the UFS reset pin as a GPIO.

> ufshcd_device_reset_ctrl() vs ufshcd_init_device_reset()
> 
> Sounds like the nomenclature could be unified or clarified.
> 

Agreed, some consistency there would look better.

Thanks,
Bjorn