[PATCH 0/3] Three changes for UFS WriteBooster

[PATCH 0/3] Three changes for UFS WriteBooster

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>


Bean Huo (3):
  scsi: ufs: Add "wb_on" sysfs node to control WB on/off
  scsi: ufs: Keep device power on only
    fWriteBoosterBufferFlushDuringHibernate == 1
  scsi: ufs: Changes comment in the function ufshcd_wb_probe()

 drivers/scsi/ufs/ufs-sysfs.c | 33 +++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs.h       |  2 ++
 drivers/scsi/ufs/ufshcd.c    | 21 ++++++++++++++-------
 drivers/scsi/ufs/ufshcd.h    |  2 ++
 4 files changed, 51 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Currently we let UFS WriteBooster driver use clock scaling
up/down to set WB on/off, for the platform which doesn't
support UFSHCD_CAP_CLK_SCALING, WB will be always on. Provide
a sysfs attribute to enable/disable WB during runtime.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs-sysfs.c | 33 +++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.c    |  3 +--
 drivers/scsi/ufs/ufshcd.h    |  2 ++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 08e72b7eef6a..e41d8eb779ec 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -189,6 +189,37 @@ static ssize_t auto_hibern8_store(struct device *dev,
 	return count;
 }
 
+static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
+}
+
+static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	unsigned int wb_enable;
+	ssize_t res;
+
+	if (!ufshcd_is_wb_allowed(hba))
+		return -EOPNOTSUPP;
+
+	if (kstrtouint(buf, 0, &wb_enable))
+		return -EINVAL;
+
+	if (wb_enable != 0 && wb_enable != 1)
+		return -EINVAL;
+
+	pm_runtime_get_sync(hba->dev);
+	res = ufshcd_wb_ctrl(hba, wb_enable);
+	pm_runtime_put_sync(hba->dev);
+
+	return res < 0 ? res : count;
+}
+
 static DEVICE_ATTR_RW(rpm_lvl);
 static DEVICE_ATTR_RO(rpm_target_dev_state);
 static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -196,6 +227,7 @@ static DEVICE_ATTR_RW(spm_lvl);
 static DEVICE_ATTR_RO(spm_target_dev_state);
 static DEVICE_ATTR_RO(spm_target_link_state);
 static DEVICE_ATTR_RW(auto_hibern8);
+static DEVICE_ATTR_RW(wb_on);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -205,6 +237,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_spm_target_dev_state.attr,
 	&dev_attr_spm_target_link_state.attr,
 	&dev_attr_auto_hibern8.attr,
+	&dev_attr_wb_on.attr,
 	NULL
 };
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d169db41ee16..639ba9d1ccbb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -247,7 +247,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
 static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
 static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
 static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
 static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
@@ -5299,7 +5298,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
 				__func__, err);
 }
 
-static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
 {
 	int ret;
 	u8 index;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index d0b68df07eef..c7bb61a4e484 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1067,6 +1067,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 			     u8 *desc_buff, int *buff_len,
 			     enum query_opcode desc_op);
 
+int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
+
 /* Wrapper functions for safely calling variant operations */
 static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
 {
-- 
2.17.1

Re: [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Asutosh Das (asd)]

On 11/30/2020 10:11 AM, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Currently we let UFS WriteBooster driver use clock scaling
> up/down to set WB on/off, for the platform which doesn't
> support UFSHCD_CAP_CLK_SCALING, WB will be always on. Provide
> a sysfs attribute to enable/disable WB during runtime.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>   drivers/scsi/ufs/ufs-sysfs.c | 33 +++++++++++++++++++++++++++++++++
>   drivers/scsi/ufs/ufshcd.c    |  3 +--
>   drivers/scsi/ufs/ufshcd.h    |  2 ++
>   3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 08e72b7eef6a..e41d8eb779ec 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -189,6 +189,37 @@ static ssize_t auto_hibern8_store(struct device *dev,
>   	return count;
>   }
>   
> +static ssize_t wb_on_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", hba->wb_enabled);
> +}
> +
> +static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	unsigned int wb_enable;
> +	ssize_t res;
> +
> +	if (!ufshcd_is_wb_allowed(hba))
> +		return -EOPNOTSUPP;
> +
> +	if (kstrtouint(buf, 0, &wb_enable))
> +		return -EINVAL;
> +
> +	if (wb_enable != 0 && wb_enable != 1)
> +		return -EINVAL;
> +
> +	pm_runtime_get_sync(hba->dev);
> +	res = ufshcd_wb_ctrl(hba, wb_enable);

Say, a platform supports clock-scaling and this bit is toggled.
The control goes into ufshcd_wb_ctrl for both this sysfs and 
clock-scaling contexts. The clock-scaling context passes all checks and 
blocks on waiting for this wb control to be disabled and then tries to 
enable wb when it's already disabled. Perhaps that's a race there?

> +	pm_runtime_put_sync(hba->dev);
> +
> +	return res < 0 ? res : count;
> +}
> +
>   static DEVICE_ATTR_RW(rpm_lvl);
>   static DEVICE_ATTR_RO(rpm_target_dev_state);
>   static DEVICE_ATTR_RO(rpm_target_link_state);
> @@ -196,6 +227,7 @@ static DEVICE_ATTR_RW(spm_lvl);
>   static DEVICE_ATTR_RO(spm_target_dev_state);
>   static DEVICE_ATTR_RO(spm_target_link_state);
>   static DEVICE_ATTR_RW(auto_hibern8);
> +static DEVICE_ATTR_RW(wb_on);
>   
>   static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>   	&dev_attr_rpm_lvl.attr,
> @@ -205,6 +237,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>   	&dev_attr_spm_target_dev_state.attr,
>   	&dev_attr_spm_target_link_state.attr,
>   	&dev_attr_auto_hibern8.attr,
> +	&dev_attr_wb_on.attr,
>   	NULL
>   };
>   
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d169db41ee16..639ba9d1ccbb 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -247,7 +247,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
>   static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
>   static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba);
>   static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba);
> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
>   static int ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set);
>   static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
>   static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
> @@ -5299,7 +5298,7 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
>   				__func__, err);
>   }
>   
> -static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable)
>   {
>   	int ret;
>   	u8 index;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index d0b68df07eef..c7bb61a4e484 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1067,6 +1067,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
>   			     u8 *desc_buff, int *buff_len,
>   			     enum query_opcode desc_op);
>   
> +int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable);
> +
>   /* Wrapper functions for safely calling variant operations */
>   static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
>   {
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

Re: [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Bean Huo]

On Mon, 2020-11-30 at 15:19 -0800, Asutosh Das (asd) wrote:
> > +             return -EINVAL;
> > +
> > +     pm_runtime_get_sync(hba->dev);
> > +     res = ufshcd_wb_ctrl(hba, wb_enable);
> 
> Say, a platform supports clock-scaling and this bit is toggled.
> The control goes into ufshcd_wb_ctrl for both this sysfs and 
> clock-scaling contexts. The clock-scaling context passes all checks
> and 
> blocks on waiting for this wb control to be disabled and then tries
> to 
> enable wb when it's already disabled. Perhaps that's a race there?

Hi Asutosh
Appreciate your review.
There is only inconsistent problem between clock-scaling and sysfs,
since hba->dev_cmd.lock can garantee there is only one can change
fWriteBoosterEn. But this is only happening on user willfully wants to
control WB through sysfs even they know the platform supports clock-
scaling.

Since this is for the platform which doesn't support clock-scaling, I
think based on your comments, it should be acceptable for you like
this: 


+static ssize_t wb_on_store(struct device *dev, struct device_attribute
*attr,
+                          const char *buf, size_t count)
+{
+       struct ufs_hba *hba = dev_get_drvdata(dev);
+       unsigned int wb_enable;
+       ssize_t res;
+
+       if (ufshcd_is_clkscaling_supported(hba)) {
+          dev_err(dev, "supports dynamic clk scaling, control WB
+                       through sysfs is not allowed!");
+          return -EOPNOTSUPP;
+       } 
+       if (!ufshcd_is_wb_allowed(hba))
+               return -EOPNOTSUPP;
+
+       if (kstrtouint(buf, 0, &wb_enable))
+               return -EINVAL;
+
+       if (wb_enable != 0 && wb_enable != 1)
+               return -EINVAL;
+
+       pm_runtime_get_sync(hba->dev);
+       res = ufshcd_wb_ctrl(hba, wb_enable);
+       pm_runtime_put_sync(hba->dev);
+
+       return res < 0 ? res : count;
+}

thanks,
Bean

Re: [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off

[Asutosh Das (asd)]

On 12/2/2020 8:20 AM, Bean Huo wrote:
> On Mon, 2020-11-30 at 15:19 -0800, Asutosh Das (asd) wrote:
>>> +             return -EINVAL;
>>> +
>>> +     pm_runtime_get_sync(hba->dev);
>>> +     res = ufshcd_wb_ctrl(hba, wb_enable);
>>
>> Say, a platform supports clock-scaling and this bit is toggled.
>> The control goes into ufshcd_wb_ctrl for both this sysfs and
>> clock-scaling contexts. The clock-scaling context passes all checks
>> and
>> blocks on waiting for this wb control to be disabled and then tries
>> to
>> enable wb when it's already disabled. Perhaps that's a race there?
> 
> Hi Asutosh
> Appreciate your review.
> There is only inconsistent problem between clock-scaling and sysfs,
> since hba->dev_cmd.lock can garantee there is only one can change
> fWriteBoosterEn. But this is only happening on user willfully wants to
> control WB through sysfs even they know the platform supports clock-
> scaling.
> 
> Since this is for the platform which doesn't support clock-scaling, I
> think based on your comments, it should be acceptable for you like
> this:

Or a synchronization primitive b/w the 2 contexts would work just as 
well. However, I don't have an issue if the user is denied toggling of 
wb anyway. LGTM.

> 
> 
> +static ssize_t wb_on_store(struct device *dev, struct device_attribute
> *attr,
> +                          const char *buf, size_t count)
> +{
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +       unsigned int wb_enable;
> +       ssize_t res;
> +
> +       if (ufshcd_is_clkscaling_supported(hba)) {
> +          dev_err(dev, "supports dynamic clk scaling, control WB
> +                       through sysfs is not allowed!");
> +          return -EOPNOTSUPP;
> +       }
> +       if (!ufshcd_is_wb_allowed(hba))
> +               return -EOPNOTSUPP;
> +
> +       if (kstrtouint(buf, 0, &wb_enable))
> +               return -EINVAL;
> +
> +       if (wb_enable != 0 && wb_enable != 1)
> +               return -EINVAL;
> +
> +       pm_runtime_get_sync(hba->dev);
> +       res = ufshcd_wb_ctrl(hba, wb_enable);
> +       pm_runtime_put_sync(hba->dev);
> +
> +       return res < 0 ? res : count;
> +}
> 
> thanks,
> Bean
> 
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

[PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Keep device power mode as active power mode and VCC supply only if
fWriteBoosterBufferFlushDuringHibernate setting 1 is successful.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufs.h    |  2 ++
 drivers/scsi/ufs/ufshcd.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index d593edb48767..311d5f7a024d 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -530,6 +530,8 @@ struct ufs_dev_info {
 	bool f_power_on_wp_en;
 	/* Keeps information if any of the LU is power on write protected */
 	bool is_lu_power_on_wp;
+	/* Indicates if flush WB buffer during hibern8 successfully enabled */
+	bool is_hibern8_wb_flush;
 	/* Maximum number of general LU supported by the UFS device */
 	u8 max_lu_supported;
 	u8 wb_dedicated_lu;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 639ba9d1ccbb..eb7a2534b072 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -285,10 +285,16 @@ static inline void ufshcd_wb_config(struct ufs_hba *hba)
 		dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
 	else
 		dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
+
 	ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
-	if (ret)
+	if (ret) {
 		dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
 			__func__, ret);
+		hba->dev_info.is_hibern8_wb_flush = false;
+	} else {
+		hba->dev_info.is_hibern8_wb_flush = true;
+	}
+
 	ufshcd_wb_toggle_flush(hba, true);
 }
 
@@ -5440,6 +5446,9 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
 
 	if (!ufshcd_is_wb_allowed(hba))
 		return false;
+
+	if (!hba->dev_info.is_hibern8_wb_flush)
+		return false;
 	/*
 	 * The ufs device needs the vcc to be ON to flush.
 	 * With user-space reduction enabled, it's enough to enable flush
-- 
2.17.1

RE: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Avri Altman]

> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Keep device power mode as active power mode and VCC supply only if
> fWriteBoosterBufferFlushDuringHibernate setting 1 is successful.
Why would it fail?
Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the device doesn't support wb,
The check ufshcd_is_wb_allowed should suffice, isn't it?

Thanks,
Avri

Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote:
> > 
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > Keep device power mode as active power mode and VCC supply only if
> > fWriteBoosterBufferFlushDuringHibernate setting 1 is successful.
> 
Hi Avri
Thanks so much taking time reiew.

> Why would it fail?

During the reliability testing in harsh environments, such as:
EMS testing, in the high/low-temperature environment. The system would
reboot itself, there will be programming failure very likely.
If we assume failure will never hit, why we capture its result
following with dev_err(). If you keep using your phone in a harsh
environment, you will see this print message.

Of course, in a normal environment, the chance of failure likes you to
win a lottery, but the possibility still exists.

  
> Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the
> device doesn't support wb,
> The check ufshcd_is_wb_allowed should suffice, isn't it?
> 
Tot at all, UFSHCD_CAP_WB_EN only tells us if the platform supports WB,
doesn't tell us fWriteBoosterBufferFlushDuringHibernate status.

Thanks,
Bean

Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote:
> > 
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > Keep device power mode as active power mode and VCC supply only if
> > fWriteBoosterBufferFlushDuringHibernate setting 1 is successful.

Hi Avri
Thanks so much taking time reiew.

> Why would it fail?

During the reliability testing in harsh environments, such as:
EMS testing, in the high/low-temperature environment. The system would
reboot itself, there will be programming failure very likely.
If we assume failure will never hit, why we capture its result
following with dev_err(). If you keep using your phone in a harsh
environment, you will see this print message.

Of course, in a normal environment, the chance of failure likes you to
win a lottery, but the possibility still exists.

  
> Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the
> device doesn't support wb,
> The check ufshcd_is_wb_allowed should suffice, isn't it?
> 

No, UFSHCD_CAP_WB_EN only tells us if the platform supports WB,
doesn't tell us fWriteBoosterBufferFlushDuringHibernate status.

Thanks,
Bean

RE: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Avri Altman]

> 
> On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote:
> > >
> > > From: Bean Huo <beanhuo@micron.com>
> > >
> > > Keep device power mode as active power mode and VCC supply only if
> > > fWriteBoosterBufferFlushDuringHibernate setting 1 is successful.
> 
> Hi Avri
> Thanks so much taking time reiew.
> 
> > Why would it fail?
> 
> During the reliability testing in harsh environments, such as:
> EMS testing, in the high/low-temperature environment. The system would
> reboot itself, there will be programming failure very likely.
> If we assume failure will never hit, why we capture its result
> following with dev_err(). If you keep using your phone in a harsh
> environment, you will see this print message.
> 
> Of course, in a normal environment, the chance of failure likes you to
> win a lottery, but the possibility still exists.
Exactly.
Hence we need-not any extra logic protecting device management command failures.

if reading the configuration pass correctly, and UFSHCD_CAP_WB_EN is set,
one should expect that any other functionality would work.

Otherwise, any non-standard behavior should be added with a quirk.

Thanks,
Avri
> 
> 
> > Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the
> > device doesn't support wb,
> > The check ufshcd_is_wb_allowed should suffice, isn't it?
> >
> 
> No, UFSHCD_CAP_WB_EN only tells us if the platform supports WB,
> doesn't tell us fWriteBoosterBufferFlushDuringHibernate status.
> 
> Thanks,
> Bean
> 

Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

On Thu, 2020-12-03 at 10:46 +0000, Avri Altman wrote:
> > > > From: Bean Huo <beanhuo@micron.com>
> > > > 
> > > > Keep device power mode as active power mode and VCC supply only
> > > > if
> > > > fWriteBoosterBufferFlushDuringHibernate setting 1 is
> > > > successful.
> > 
> > Hi Avri
> > Thanks so much taking time reiew.
> > 
> > > Why would it fail?
> > 
> > During the reliability testing in harsh environments, such as:
> > EMS testing, in the high/low-temperature environment. The system
> > would
> > reboot itself, there will be programming failure very likely.
> > If we assume failure will never hit, why we capture its result
> > following with dev_err(). If you keep using your phone in a harsh
> > environment, you will see this print message.
> > 
> > Of course, in a normal environment, the chance of failure likes you
> > to
> > win a lottery, but the possibility still exists.
> 
> Exactly.

so, you agree the possiblity of failure  exists.

> Hence we need-not any extra logic protecting device management
> command failures.

what extra logic? 

> 
> if reading the configuration pass correctly, and UFSHCD_CAP_WB_EN is
> set,


UFSHCD_CAP_WB_EN set is DRAM level. still in the cache.

> one should expect that any other functionality would work.
> 
No,  The programming will consume more power than reading, the
later setting will more possbile fail than reading.

> Otherwise, any non-standard behavior should be added with a quirk.
> 

NO, this is not what is standard or non-standard. This is independent
of UFS device/controller. It is a software design. IMO, we didn't deal
with programming status that is a potential bug. If having to impose to
a component, do you think should be controller or device? Instead of
addin a quirk, I prefer dropping this patch.




> Thanks,
> Avri
> > 
> > 
> > > Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the
> > > device doesn't support wb,
> > > The check ufshcd_is_wb_allowed should suffice, isn't it?
> > > 
> > 
> > No, UFSHCD_CAP_WB_EN only tells us if the platform supports WB,
> > doesn't tell us fWriteBoosterBufferFlushDuringHibernate status.
> > 
> > Thanks,
> > Bean

RE: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Avri Altman]

> On Thu, 2020-12-03 at 10:46 +0000, Avri Altman wrote:
> > > > > From: Bean Huo <beanhuo@micron.com>
> > > > >
> > > > > Keep device power mode as active power mode and VCC supply only
> > > > > if
> > > > > fWriteBoosterBufferFlushDuringHibernate setting 1 is
> > > > > successful.
> > >
> > > Hi Avri
> > > Thanks so much taking time reiew.
> > >
> > > > Why would it fail?
> > >
> > > During the reliability testing in harsh environments, such as:
> > > EMS testing, in the high/low-temperature environment. The system
> > > would
> > > reboot itself, there will be programming failure very likely.
> > > If we assume failure will never hit, why we capture its result
> > > following with dev_err(). If you keep using your phone in a harsh
> > > environment, you will see this print message.
> > >
> > > Of course, in a normal environment, the chance of failure likes you
> > > to
> > > win a lottery, but the possibility still exists.
> >
> > Exactly.
> 
> so, you agree the possiblity of failure  exists.
I was more relating to the lottery part.

> 
> > Hence we need-not any extra logic protecting device management
> > command failures.
> 
> what extra logic?
> 
> >
> > if reading the configuration pass correctly, and UFSHCD_CAP_WB_EN is
> > set,
> 
> 
> UFSHCD_CAP_WB_EN set is DRAM level. still in the cache.
> 
> > one should expect that any other functionality would work.
> >
> No,  The programming will consume more power than reading, the
> later setting will more possbile fail than reading.
> 
> > Otherwise, any non-standard behavior should be added with a quirk.
> >
> 
> NO, this is not what is standard or non-standard. This is independent
> of UFS device/controller. It is a software design. IMO, we didn't deal
> with programming status that is a potential bug. If having to impose to
> a component, do you think should be controller or device? Instead of
> addin a quirk, I prefer dropping this patch.
It seems you are adding some special treatment in case some device management command failed,
A vanishingly unlikely event but a one that has significant impact over power consumption.
If a device is not responding properly to some specific device management command,
It should be treated accordingly.

Thanks,
Avri

> 
> 
> 
> 
> > Thanks,
> > Avri
> > >
> > >
> > > > Since UFSHCD_CAP_WB_EN is toggled off on ufshcd_wb_probe If the
> > > > device doesn't support wb,
> > > > The check ufshcd_is_wb_allowed should suffice, isn't it?
> > > >
> > >
> > > No, UFSHCD_CAP_WB_EN only tells us if the platform supports WB,
> > > doesn't tell us fWriteBoosterBufferFlushDuringHibernate status.
> > >
> > > Thanks,
> > > Bean

Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

On Thu, 2020-12-03 at 12:15 +0000, Avri Altman wrote:
> > so, you agree the possiblity of failure  exists.
> 
> I was more relating to the lottery part.
It doesn't matter. even the possibility of winning a lottery is very
low, but still there is.
> > 
> > > Hence we need-not any extra logic protecting device management
> > > command failures.
> > 
> > what extra logic?
> > 
> > > 
> > > if reading the configuration pass correctly, and UFSHCD_CAP_WB_EN
> > > is
> > > set,
> > 
> > 
> > UFSHCD_CAP_WB_EN set is DRAM level. still in the cache.
> > 
> > > one should expect that any other functionality would work.
> > > 
> > 
> > No,  The programming will consume more power than reading, the
> > later setting will more possbile fail than reading.
> > 
> > > Otherwise, any non-standard behavior should be added with a
> > > quirk.
> > > 
> > 
> > NO, this is not what is standard or non-standard. This is
> > independent
> > of UFS device/controller. It is a software design. IMO, we didn't
> > deal
> > with programming status that is a potential bug. If having to
> > impose to
> > a component, do you think should be controller or device? Instead
> > of
> > addin a quirk, I prefer dropping this patch.
> 
> It seems you are adding some special treatment in case some device
> management command failed,
> A vanishingly unlikely event but a one that has significant impact
> over power consumption.

again, there is nothing with device. Obviously, you didn't do system
reliability testing in harsh environment. you don't believe this is WB
driver bug. I will send my next version patch with a fix-tag. even It
cannot merge. but I want to highlight it is a bug.

Thanks,
Bean

Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Can Guo]

On 2020-12-01 02:11, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Keep device power mode as active power mode and VCC supply only if
> fWriteBoosterBufferFlushDuringHibernate setting 1 is successful.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufs.h    |  2 ++
>  drivers/scsi/ufs/ufshcd.c | 11 ++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index d593edb48767..311d5f7a024d 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -530,6 +530,8 @@ struct ufs_dev_info {
>  	bool f_power_on_wp_en;
>  	/* Keeps information if any of the LU is power on write protected */
>  	bool is_lu_power_on_wp;
> +	/* Indicates if flush WB buffer during hibern8 successfully enabled 
> */
> +	bool is_hibern8_wb_flush;
>  	/* Maximum number of general LU supported by the UFS device */
>  	u8 max_lu_supported;
>  	u8 wb_dedicated_lu;
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 639ba9d1ccbb..eb7a2534b072 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -285,10 +285,16 @@ static inline void ufshcd_wb_config(struct 
> ufs_hba *hba)
>  		dev_err(hba->dev, "%s: Enable WB failed: %d\n", __func__, ret);
>  	else
>  		dev_info(hba->dev, "%s: Write Booster Configured\n", __func__);
> +
>  	ret = ufshcd_wb_toggle_flush_during_h8(hba, true);
> -	if (ret)
> +	if (ret) {
>  		dev_err(hba->dev, "%s: En WB flush during H8: failed: %d\n",
>  			__func__, ret);
> +		hba->dev_info.is_hibern8_wb_flush = false;
> +	} else {
> +		hba->dev_info.is_hibern8_wb_flush = true;
> +	}
> +
>  	ufshcd_wb_toggle_flush(hba, true);
>  }
> 
> @@ -5440,6 +5446,9 @@ static bool ufshcd_wb_need_flush(struct ufs_hba 
> *hba)
> 
>  	if (!ufshcd_is_wb_allowed(hba))
>  		return false;
> +
> +	if (!hba->dev_info.is_hibern8_wb_flush)
> +		return false;

The check is in the wrong place - even if say
fWriteBoosterBufferFlushDuringHibernate is failed to be enabled,
ufshcd_wb_need_flush() still needs to reflect the fact that whether
the wb buffer needs to be flushed or not - it should not be decided
by the flag.

Thanks,

Can Guo.

>  	/*
>  	 * The ufs device needs the vcc to be ON to flush.
>  	 * With user-space reduction enabled, it's enough to enable flush

Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1

[Bean Huo]

On Fri, 2020-12-04 at 11:26 +0800, Can Guo wrote:
> > 
> >        if (!ufshcd_is_wb_allowed(hba))
> >                return false;
> > +
> > +     if (!hba->dev_info.is_hibern8_wb_flush)
> > +             return false;
> 
> The check is in the wrong place - even if say
> fWriteBoosterBufferFlushDuringHibernate is failed to be enabled,
> ufshcd_wb_need_flush() still needs to reflect the fact that whether
> the wb buffer needs to be flushed or not - it should not be decided
> by the flag.
> 
Can,
you are right, let me take it out from this function, and see if
acceptable.

Thanks,
Bean

> Thanks,
> 
> Can Guo.

[PATCH 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

USFHCD supports WriteBooster "LU dedicated buffer” mode and
“shared buffer” mode both, so changes the comment in the
function ufshcd_wb_probe().

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index eb7a2534b072..2091775ed3e2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7166,10 +7166,9 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 		goto wb_disabled;
 
 	/*
-	 * WB may be supported but not configured while provisioning.
-	 * The spec says, in dedicated wb buffer mode,
-	 * a max of 1 lun would have wb buffer configured.
-	 * Now only shared buffer mode is supported.
+	 * WB may be supported but not configured while provisioning. The spec
+	 * says, in dedicated wb buffer mode, a max of 1 lun would have wb
+	 * buffer configured.
 	 */
 	dev_info->b_wb_buffer_type =
 		desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
-- 
2.17.1

Re: [PATCH 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe()

[Can Guo]

On 2020-12-01 02:11, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> USFHCD supports WriteBooster "LU dedicated buffer” mode and
> “shared buffer” mode both, so changes the comment in the
> function ufshcd_wb_probe().
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>

Reviewed-by: Can Guo <cang@codeaurora.org>

> ---
>  drivers/scsi/ufs/ufshcd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index eb7a2534b072..2091775ed3e2 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7166,10 +7166,9 @@ static void ufshcd_wb_probe(struct ufs_hba
> *hba, u8 *desc_buf)
>  		goto wb_disabled;
> 
>  	/*
> -	 * WB may be supported but not configured while provisioning.
> -	 * The spec says, in dedicated wb buffer mode,
> -	 * a max of 1 lun would have wb buffer configured.
> -	 * Now only shared buffer mode is supported.
> +	 * WB may be supported but not configured while provisioning. The 
> spec
> +	 * says, in dedicated wb buffer mode, a max of 1 lun would have wb
> +	 * buffer configured.
>  	 */
>  	dev_info->b_wb_buffer_type =
>  		desc_buf[DEVICE_DESC_PARAM_WB_TYPE];