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