| Message ID | 20190604072001.9288-3-bjorn.andersson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | (Qualcomm) UFS device reset support | expand |
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
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
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
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
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
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&data=02% >7C01%7Cbeanhuo%40micron.com%7Cf72404eb906440e1f9c408d6e93c401d%7 >Cf38a5ecd28134862b11bac1d563c806f%7C0%7C0%7C636952842324926509&a >mp;sdata=I%2FH7km6b34jyoUa1RVEPApfYt5uSFtHqL3%2BvV1bvryM%3D& >;reserved=0 > >Regards, >Bjorn
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 */
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(+)