[PATCH 0/3] (Qualcomm) UFS device reset support

[PATCH 0/3] (Qualcomm) UFS device reset support

[Bjorn Andersson]

This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
acquire and toggle this and then adds this to SDM845 MTP.

Bjorn Andersson (3):
  pinctrl: qcom: sdm845: Expose ufs_reset as gpio
  scsi: ufs: Allow resetting the UFS device
  arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  |  2 +-
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts       |  2 +
 drivers/pinctrl/qcom/pinctrl-sdm845.c         | 12 ++---
 drivers/scsi/ufs/ufshcd.c                     | 44 +++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h                     |  4 ++
 5 files changed, 57 insertions(+), 7 deletions(-)

-- 
2.18.0

[PATCH 0/3] (Qualcomm) UFS device reset support

[Bjorn Andersson]

This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
acquire and toggle this and then adds this to SDM845 MTP.

Bjorn Andersson (3):
  pinctrl: qcom: sdm845: Expose ufs_reset as gpio
  scsi: ufs: Allow resetting the UFS device
  arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  |  2 +-
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts       |  2 +
 drivers/pinctrl/qcom/pinctrl-sdm845.c         | 12 ++---
 drivers/scsi/ufs/ufshcd.c                     | 44 +++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h                     |  4 ++
 5 files changed, 57 insertions(+), 7 deletions(-)

-- 
2.18.0

[PATCH 1/3] pinctrl: qcom: sdm845: Expose ufs_reset as gpio

[Bjorn Andersson]

The ufs_reset pin is expected to be wired to the reset pin of the
primary UFS memory but is pretty much just a general purpose output pinr

Reorder the pins and expose it as gpio 150, so that the UFS driver can
toggle it.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt         |  2 +-
 drivers/pinctrl/qcom/pinctrl-sdm845.c                | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
index 665aadb5ea28..33ae08aa4b89 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
@@ -79,7 +79,7 @@ to specify in a pin configuration subnode:
 		      gpio0-gpio149
 		        Supports mux, bias and drive-strength
 
-		      sdc2_clk, sdc2_cmd, sdc2_data
+		      sdc2_clk, sdc2_cmd, sdc2_data, ufs_reset
 		        Supports bias and drive-strength
 
 - function:
diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index c97f20fca5fd..e4e5acade086 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -420,10 +420,10 @@ DECLARE_MSM_GPIO_PINS(147);
 DECLARE_MSM_GPIO_PINS(148);
 DECLARE_MSM_GPIO_PINS(149);
 
-static const unsigned int sdc2_clk_pins[] = { 150 };
-static const unsigned int sdc2_cmd_pins[] = { 151 };
-static const unsigned int sdc2_data_pins[] = { 152 };
-static const unsigned int ufs_reset_pins[] = { 153 };
+static const unsigned int ufs_reset_pins[] = { 150 };
+static const unsigned int sdc2_clk_pins[] = { 151 };
+static const unsigned int sdc2_cmd_pins[] = { 152 };
+static const unsigned int sdc2_data_pins[] = { 153 };
 
 enum sdm845_functions {
 	msm_mux_gpio,
@@ -1271,10 +1271,10 @@ static const struct msm_pingroup sdm845_groups[] = {
 	PINGROUP(147, NORTH, _, _, _, _, _, _, _, _, _, _),
 	PINGROUP(148, NORTH, _, _, _, _, _, _, _, _, _, _),
 	PINGROUP(149, NORTH, _, _, _, _, _, _, _, _, _, _),
+	UFS_RESET(ufs_reset, 0x99f000),
 	SDC_QDSD_PINGROUP(sdc2_clk, 0x99a000, 14, 6),
 	SDC_QDSD_PINGROUP(sdc2_cmd, 0x99a000, 11, 3),
 	SDC_QDSD_PINGROUP(sdc2_data, 0x99a000, 9, 0),
-	UFS_RESET(ufs_reset, 0x99f000),
 };
 
 static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
@@ -1284,7 +1284,7 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
 	.nfunctions = ARRAY_SIZE(sdm845_functions),
 	.groups = sdm845_groups,
 	.ngroups = ARRAY_SIZE(sdm845_groups),
-	.ngpios = 150,
+	.ngpios = 151,
 };
 
 static int sdm845_pinctrl_probe(struct platform_device *pdev)
-- 
2.18.0

Re: [PATCH 1/3] pinctrl: qcom: sdm845: Expose ufs_reset as gpio

[Linus Walleij]

On Tue, Jun 4, 2019 at 9:20 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> The ufs_reset pin is expected to be wired to the reset pin of the
> primary UFS memory but is pretty much just a general purpose output pinr
>
> Reorder the pins and expose it as gpio 150, so that the UFS driver can
> toggle it.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 1/3] pinctrl: qcom: sdm845: Expose ufs_reset as gpio

[Linus Walleij]

On Tue, Jun 4, 2019 at 9:20 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> The ufs_reset pin is expected to be wired to the reset pin of the
> primary UFS memory but is pretty much just a general purpose output pinr
>
> Reorder the pins and expose it as gpio 150, so that the UFS driver can
> toggle it.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Patch applied.

Yours,
Linus Walleij

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

[Bjorn Andersson]

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 */
-- 
2.18.0

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

[Marc Gonzalez]

[ 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?

On 8998 we already have:

			resets = <&gcc GCC_UFS_BCR>;
			reset-names = "rst";

The above reset line gets wiggled/frobbed when appropriate.

(What's the difference between gpio and pinctrl? vs a reset "clock" as above)

ufshcd_device_reset_ctrl() vs ufshcd_init_device_reset()

Sounds like the nomenclature could be unified or clarified.

Regards.

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

[Bjorn Andersson]

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

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

[Alim Akhtar]

Hi Marc
Thanks for coping me.

On 6/4/19 1:23 PM, 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?
> 
Not sure about MSM, but this is high implementation dependent, different 
SoC vendors implement device reset in different way, like one mentioned 
above in this patch, and in case of Samsung/exynos, HCI register control 
device reset. AFA ufs spec is concerns, it just mandate about connecting 
a active low signal to RST_n pin of the ufs device.
> On 8998 we already have:
> 
> 			resets = <&gcc GCC_UFS_BCR>;
> 			reset-names = "rst";
> 
> The above reset line gets wiggled/frobbed when appropriate.
> 
> (What's the difference between gpio and pinctrl? vs a reset "clock" as above)
> 
> ufshcd_device_reset_ctrl() vs ufshcd_init_device_reset()
> 
> Sounds like the nomenclature could be unified or clarified.
> 
> Regards.
> 
> 

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

[Bjorn Andersson]

On Fri 07 Jun 03:41 PDT 2019, Alim Akhtar wrote:

> Hi Marc
> Thanks for coping me.
> 
> On 6/4/19 1:23 PM, 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?
> > 
> Not sure about MSM, but this is high implementation dependent, different 
> SoC vendors implement device reset in different way, like one mentioned 
> above in this patch, and in case of Samsung/exynos, HCI register control 
> device reset. AFA ufs spec is concerns, it just mandate about connecting 
> a active low signal to RST_n pin of the ufs device.

I didn't consider the case where the host controller is the GPIO
controller, in which case it makes sense to move this to the platform
driver so that we don't have to implement a gpio-controller in the
exynos platform driver, to be consumed by the ufshcd.

I'll rework this.

Thanks,
Bjorn

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

[Bean Huo (beanhuo)]

Hi, Bjorn

>Acquire the device-reset GPIO and toggle this to reset the UFS device during
>initialization and host reset.
>
>+/**
>+ 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);
>+

what's problem you met, and you should reset UFS device here? could you give more info?

It is true that we don't reset UFS device in case of device fatal error. According to UFS host spec,
Host should be device reset except that in addition to resetting UIC. But as so far,
We didn't experience any problems result from this missing reset.

We have three UFS device reset ways.  Comparing to this hardware reset, 
I prefer to use DME_ENDPOINTRESET.req software reset.


> 		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 */
>--
>2.18.0

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

[John Stultz]

On Tue, Jun 4, 2019 at 1:14 AM Bean Huo (beanhuo) <beanhuo@micron.com> wrote:
>
> Hi, Bjorn
>
> >Acquire the device-reset GPIO and toggle this to reset the UFS device during
> >initialization and host reset.
> >
> >+/**
> >+ 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);
> >+
>
> what's problem you met, and you should reset UFS device here? could you give more info?

On the pixel3, devices with micron UFS chips won't boot upstream
kernels without this patch, which is a rewrite of an earlier patch:
  https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/p3&id=99f3dd8519a848b752679584451c45f42c326a17

Which was pulled from the downstream tree from here:
  https://android.googlesource.com/kernel/msm.git/+/9c8077087e818017%5E%21/

CCing Subhash as he may have additional context.

thanks
-john

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

[Bjorn Andersson]

On Tue 04 Jun 01:13 PDT 2019, Bean Huo (beanhuo) wrote:
> >@@ -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);
> >+
> 
> what's problem you met, and you should reset UFS device here? could you give more info?
> 
> It is true that we don't reset UFS device in case of device fatal error. According to UFS host spec,
> Host should be device reset except that in addition to resetting UIC. But as so far,
> We didn't experience any problems result from this missing reset.
> 
> We have three UFS device reset ways.  Comparing to this hardware reset, 
> I prefer to use DME_ENDPOINTRESET.req software reset.
> 

Hi Bean,

Thanks for your questions. With some memories we see issues establishing
the link during bootup, so that's the purpose of issuing this reset.

Unfortunately the downstream Qualcomm patch [1] (which I should have
remembered to attribute), does not mention why the reset during host
controller reset is needed - but I'm fairly certain that this scenario
would be similar to the handover from bootloader to kernel that we do
see an issue with.


[1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/commit/?h=msm-4.4&id=0c82737188e2d63a08196e078e411032dbbc3b89

Regards,
Bjorn

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

[Bean Huo (beanhuo)]

Hi, Bjorn
I think I should give up my preview suggestion DME_ENDPOINTRESET.req software reset,  go back to your HW reset.
Although SW reset can cover most of the requirement, and compatible with different vendor UFS device, for some device
fatal error cases, UFS internal stuck and would not accept SW Reset. An HW reset is expected.  
Please go on your reset way.

Thanks,
//Bean

>Andy Gross <agross@kernel.org>; Linus Walleij <linus.walleij@linaro.org>;
>Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
>linux-arm-msm@vger.kernel.org; linux-gpio@vger.kernel.org;
>devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>scsi@vger.kernel.org
>Subject: Re: [EXT] [PATCH 2/3] scsi: ufs: Allow resetting the UFS device
>
>On Tue 04 Jun 01:13 PDT 2019, Bean Huo (beanhuo) wrote:
>> >@@ -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);
>> >+
>>
>> what's problem you met, and you should reset UFS device here? could you
>give more info?
>>
>> It is true that we don't reset UFS device in case of device fatal
>> error. According to UFS host spec, Host should be device reset except
>> that in addition to resetting UIC. But as so far, We didn't experience any
>problems result from this missing reset.
>>
>> We have three UFS device reset ways.  Comparing to this hardware
>> reset, I prefer to use DME_ENDPOINTRESET.req software reset.
>>
>
>Hi Bean,
>
>Thanks for your questions. With some memories we see issues establishing
>the link during bootup, so that's the purpose of issuing this reset.
>
>Unfortunately the downstream Qualcomm patch [1] (which I should have
>remembered to attribute), does not mention why the reset during host
>controller reset is needed - but I'm fairly certain that this scenario would be
>similar to the handover from bootloader to kernel that we do see an issue
>with.
>
>
>[1]
>https://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource
>.codeaurora.org%2Fquic%2Fla%2Fkernel%2Fmsm-
>4.4%2Fcommit%2F%3Fh%3Dmsm-
>4.4%26id%3D0c82737188e2d63a08196e078e411032dbbc3b89&amp;data=02%
>7C01%7Cbeanhuo%40micron.com%7Cf72404eb906440e1f9c408d6e93c401d%7
>Cf38a5ecd28134862b11bac1d563c806f%7C0%7C0%7C636952842324926509&a
>mp;sdata=I%2FH7km6b34jyoUa1RVEPApfYt5uSFtHqL3%2BvV1bvryM%3D&amp
>;reserved=0
>
>Regards,
>Bjorn

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

[Stephen Boyd]

Quoting Bjorn Andersson (2019-06-04 00:20:00)
> @@ -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

This isn't usb though. Can we have a gpio reset driver and then
implement this in the reset framework instead? Or did that not work out
for some reason?

> +        * 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

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

[Bjorn Andersson]

On Tue 04 Jun 08:25 PDT 2019, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2019-06-04 00:20:00)
> > @@ -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
> 
> This isn't usb though.

No, it is not.

> Can we have a gpio reset driver and then
> implement this in the reset framework instead? Or did that not work out
> for some reason?
> 

The reset DT binding document clearly describes that resets are for
chip-internal resets, and this is a general purpose (output-only) pad on
the SoC that's connected to the reset pin on the UFS memory.

I actually see nothing preventing you to connect said reset pin to any
other GPIO.

Regards,
Bjorn

> > +        * 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

[PATCH 3/3] arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

[Bjorn Andersson]

Specify the UFS device-reset gpio, so that the controller will issue a
reset of the UFS device.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
index 2e78638eb73b..d116a0956a9c 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -388,6 +388,8 @@
 &ufs_mem_hc {
 	status = "okay";
 
+	device-reset-gpios = <&tlmm 150 GPIO_ACTIVE_LOW>;
+
 	vcc-supply = <&vreg_l20a_2p95>;
 	vcc-max-microamp = <600000>;
 };
-- 
2.18.0

Re: [PATCH 3/3] arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

[Stephen Boyd]

Quoting Bjorn Andersson (2019-06-04 00:20:01)
> Specify the UFS device-reset gpio, so that the controller will issue a
> reset of the UFS device.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 2e78638eb73b..d116a0956a9c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -388,6 +388,8 @@
>  &ufs_mem_hc {
>         status = "okay";
>  
> +       device-reset-gpios = <&tlmm 150 GPIO_ACTIVE_LOW>;
> +

We had to do something similar on one particular brand of UFS that we had. I
think it was an SK Hynix part that had trouble and wouldn't provision properly.
Either way, we did this with a pinctrl toggle in the DTS where the "init" state
has the UFS_RESET pin asserted and then "default" state has the pin deasserted.
That was good enough to make this work.

	&ufs_mem_hc {
		pinctrl-names = "init", "default";
		pinctrl-0 = <&ufs_dev_reset_assert>;
		pinctrl-1 = <&ufs_dev_reset_deassert>;
	};

        ufs_dev_reset_assert: ufs_dev_reset_assert {
                config {
                        pins = "ufs_reset";
                        bias-pull-down;         /* default: pull down */
                        drive-strength = <8>;   /* default: 3.1 mA */
                        output-low; /* active low reset */
                };
        };

        ufs_dev_reset_deassert: ufs_dev_reset_deassert {
                config {
                        pins = "ufs_reset";
                        bias-pull-down;         /* default: pull down */
                        drive-strength = <8>;
                        output-high; /* active low reset */
                };
        };

Re: [PATCH 3/3] arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

[Bjorn Andersson]

On Tue 04 Jun 09:22 PDT 2019, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2019-06-04 00:20:01)
> > Specify the UFS device-reset gpio, so that the controller will issue a
> > reset of the UFS device.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > index 2e78638eb73b..d116a0956a9c 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> > @@ -388,6 +388,8 @@
> >  &ufs_mem_hc {
> >         status = "okay";
> >  
> > +       device-reset-gpios = <&tlmm 150 GPIO_ACTIVE_LOW>;
> > +
> 
> We had to do something similar on one particular brand of UFS that we had. I
> think it was an SK Hynix part that had trouble and wouldn't provision properly.
> Either way, we did this with a pinctrl toggle in the DTS where the "init" state
> has the UFS_RESET pin asserted and then "default" state has the pin deasserted.
> That was good enough to make this work.
> 

Thanks for pointing this out, I forgot to attribute these downstream
changes. I can see how this works, but I must say I find it quite
hackish.

The downstream solution seems to have evolved this into naming these
states and jumping between them (with the appropriate sleeps) during a
host reset as well.


But thanks for the confirmation that there's more than John's memory
that needs this.

Regards,
Bjorn

> 	&ufs_mem_hc {
> 		pinctrl-names = "init", "default";
> 		pinctrl-0 = <&ufs_dev_reset_assert>;
> 		pinctrl-1 = <&ufs_dev_reset_deassert>;
> 	};
> 
>         ufs_dev_reset_assert: ufs_dev_reset_assert {
>                 config {
>                         pins = "ufs_reset";
>                         bias-pull-down;         /* default: pull down */
>                         drive-strength = <8>;   /* default: 3.1 mA */
>                         output-low; /* active low reset */
>                 };
>         };
> 
>         ufs_dev_reset_deassert: ufs_dev_reset_deassert {
>                 config {
>                         pins = "ufs_reset";
>                         bias-pull-down;         /* default: pull down */
>                         drive-strength = <8>;
>                         output-high; /* active low reset */
>                 };
>         };

Re: [PATCH 3/3] arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

[Linus Walleij]

On Tue, Jun 4, 2019 at 9:20 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> Specify the UFS device-reset gpio, so that the controller will issue a
> reset of the UFS device.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/3] arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

[Linus Walleij]

On Tue, Jun 4, 2019 at 9:20 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> Specify the UFS device-reset gpio, so that the controller will issue a
> reset of the UFS device.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 0/3] (Qualcomm) UFS device reset support

[John Stultz]

On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> acquire and toggle this and then adds this to SDM845 MTP.
>
> Bjorn Andersson (3):
>   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
>   scsi: ufs: Allow resetting the UFS device
>   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

Adding similar change as in sdm845-mtp to the not yet upstream
blueline dts, I validated this allows my micron UFS pixel3 to boot.

Tested-by: John Stultz <john.stultz@linaro.org>

thanks
-john

RE: [PATCH 0/3] (Qualcomm) UFS device reset support

[Avri Altman]

Hi,

> 
> On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> > acquire and toggle this and then adds this to SDM845 MTP.
> >
> > Bjorn Andersson (3):
> >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> >   scsi: ufs: Allow resetting the UFS device
> >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> 
> Adding similar change as in sdm845-mtp to the not yet upstream
> blueline dts, I validated this allows my micron UFS pixel3 to boot.
> 
> Tested-by: John Stultz <john.stultz@linaro.org>
Maybe ufs_hba_variant_ops would be the proper place to add this?

Thanks,
Avri



> 
> thanks
> -john

Re: [PATCH 0/3] (Qualcomm) UFS device reset support

[Bjorn Andersson]

On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:

> Hi,
> 
> > 
> > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> > > acquire and toggle this and then adds this to SDM845 MTP.
> > >
> > > Bjorn Andersson (3):
> > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > >   scsi: ufs: Allow resetting the UFS device
> > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > 
> > Adding similar change as in sdm845-mtp to the not yet upstream
> > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > 
> > Tested-by: John Stultz <john.stultz@linaro.org>
> Maybe ufs_hba_variant_ops would be the proper place to add this?
> 

Are you saying that these memories only need a reset when they are
paired with the Qualcomm host controller?

The way it's implemented it here is that the device-reset GPIO is
optional and only if you specify it we'll toggle the reset. So if your
board design has a UFS memory that requires a reset pulse during
initialization you specify this, regardless of which vendor your SoC
comes from.

Regards,
Bjorn

RE: [PATCH 0/3] (Qualcomm) UFS device reset support

[Avri Altman]

> 
> On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:
> 
> > Hi,
> >
> > >
> > > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > >
> > > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> > > > acquire and toggle this and then adds this to SDM845 MTP.
> > > >
> > > > Bjorn Andersson (3):
> > > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > > >   scsi: ufs: Allow resetting the UFS device
> > > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > >
> > > Adding similar change as in sdm845-mtp to the not yet upstream
> > > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > >
> > > Tested-by: John Stultz <john.stultz@linaro.org>
> > Maybe ufs_hba_variant_ops would be the proper place to add this?
> >
> 
> Are you saying that these memories only need a reset when they are
> paired with the Qualcomm host controller?
ufs_hba_variant_ops is for vendors to implement their own vops,
and as you can see, many of them do.
Adding hw_reset to that template seems like the proper way
to do what you are doing.

Thanks,
Avri
> 
> The way it's implemented it here is that the device-reset GPIO is
> optional and only if you specify it we'll toggle the reset. So if your
> board design has a UFS memory that requires a reset pulse during
> initialization you specify this, regardless of which vendor your SoC
> comes from.
> 
> Regards,
> Bjorn

Re: [PATCH 0/3] (Qualcomm) UFS device reset support

[Bjorn Andersson]

On Wed 05 Jun 02:32 PDT 2019, Avri Altman wrote:

> > 
> > On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:
> > 
> > > Hi,
> > >
> > > >
> > > > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > > > <bjorn.andersson@linaro.org> wrote:
> > > > >
> > > > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> > > > > acquire and toggle this and then adds this to SDM845 MTP.
> > > > >
> > > > > Bjorn Andersson (3):
> > > > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > > > >   scsi: ufs: Allow resetting the UFS device
> > > > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > > >
> > > > Adding similar change as in sdm845-mtp to the not yet upstream
> > > > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > > >
> > > > Tested-by: John Stultz <john.stultz@linaro.org>
> > > Maybe ufs_hba_variant_ops would be the proper place to add this?
> > >
> > 
> > Are you saying that these memories only need a reset when they are
> > paired with the Qualcomm host controller?
> ufs_hba_variant_ops is for vendors to implement their own vops,
> and as you can see, many of them do.
> Adding hw_reset to that template seems like the proper way
> to do what you are doing.
> 

Right, but the vops is operations related to the UFS controller, this
property relates to the memory connected.

E.g I have a Hynix memory and John have a Micron memory that needs this
reset and my assumption is that these memories will need their RESET pin
toggled regardless of which controller they are connected to.

Regards,
Bjorn

RE: [PATCH 0/3] (Qualcomm) UFS device reset support

[Avri Altman]

> 
> On Wed 05 Jun 02:32 PDT 2019, Avri Altman wrote:
> 
> > >
> > > On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:
> > >
> > > > Hi,
> > > >
> > > > >
> > > > > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > > > > <bjorn.andersson@linaro.org> wrote:
> > > > > >
> > > > > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd
> to
> > > > > > acquire and toggle this and then adds this to SDM845 MTP.
> > > > > >
> > > > > > Bjorn Andersson (3):
> > > > > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > > > > >   scsi: ufs: Allow resetting the UFS device
> > > > > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > > > >
> > > > > Adding similar change as in sdm845-mtp to the not yet upstream
> > > > > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > > > >
> > > > > Tested-by: John Stultz <john.stultz@linaro.org>
> > > > Maybe ufs_hba_variant_ops would be the proper place to add this?
> > > >
> > >
> > > Are you saying that these memories only need a reset when they are
> > > paired with the Qualcomm host controller?
> > ufs_hba_variant_ops is for vendors to implement their own vops,
> > and as you can see, many of them do.
> > Adding hw_reset to that template seems like the proper way
> > to do what you are doing.
> >
> 
> Right, but the vops is operations related to the UFS controller, this
> property relates to the memory connected.
This is not entirely accurate. Those are vendor/board specific,
As the original commit log indicates:
" vendor/board specific and hence determined with
 the help of compatible property in device tree."

I would rather have this new vop:
void    (*device_reset)(struct ufs_hba *), Or whatever, 
actively set in ufs_hba_variant_ops, rather than ufshcd_init_device_reset
failing as part of the default init flow.

Thanks,
Avri

> 
> E.g I have a Hynix memory and John have a Micron memory that needs this
> reset and my assumption is that these memories will need their RESET pin
> toggled regardless of which controller they are connected to.
> 
> Regards,
> Bjorn

Re: [PATCH 0/3] (Qualcomm) UFS device reset support

[Bjorn Andersson]

On Wed 05 Jun 23:32 PDT 2019, Avri Altman wrote:

> > 
> > On Wed 05 Jun 02:32 PDT 2019, Avri Altman wrote:
> > 
> > > >
> > > > On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > >
> > > > > > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > > > > > <bjorn.andersson@linaro.org> wrote:
> > > > > > >
> > > > > > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd
> > to
> > > > > > > acquire and toggle this and then adds this to SDM845 MTP.
> > > > > > >
> > > > > > > Bjorn Andersson (3):
> > > > > > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > > > > > >   scsi: ufs: Allow resetting the UFS device
> > > > > > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > > > > >
> > > > > > Adding similar change as in sdm845-mtp to the not yet upstream
> > > > > > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > > > > >
> > > > > > Tested-by: John Stultz <john.stultz@linaro.org>
> > > > > Maybe ufs_hba_variant_ops would be the proper place to add this?
> > > > >
> > > >
> > > > Are you saying that these memories only need a reset when they are
> > > > paired with the Qualcomm host controller?
> > > ufs_hba_variant_ops is for vendors to implement their own vops,
> > > and as you can see, many of them do.
> > > Adding hw_reset to that template seems like the proper way
> > > to do what you are doing.
> > >
> > 
> > Right, but the vops is operations related to the UFS controller, this
> > property relates to the memory connected.
> This is not entirely accurate. Those are vendor/board specific,
> As the original commit log indicates:
> " vendor/board specific and hence determined with
>  the help of compatible property in device tree."
> 
> I would rather have this new vop:
> void    (*device_reset)(struct ufs_hba *), Or whatever, 
> actively set in ufs_hba_variant_ops, rather than ufshcd_init_device_reset
> failing as part of the default init flow.
> 

But such an vops would allow me to provide a Qualcomm-specific way of
toggling the GPIO that is connected to the UFS_RESET pin on the
Hynix/Micron memory.

But acquiring and toggling GPIOs is not a Qualcomm thing, it's a
completely generic thing, and as it's not a chip-internal line it is a
GPIO and not a reset - regardless of SoC vendor.
Further more, it's optional so boards that does not have this pin
connected will just omit the property in their hardware description
(DeviceTree).


So I think the halting part here is that we don't have a representation
of the memory device's resources, because this is really a matter of
toggling the reset pin on the memory device.

Regards,
Bjorn