From: Bean Huo <[email protected]>
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
From: Bean Huo <[email protected]>
Keep device power mode as active power mode and VCC supply only if
fWriteBoosterBufferFlushDuringHibernate setting 1 is successful.
Signed-off-by: Bean Huo <[email protected]>
---
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
From: Bean Huo <[email protected]>
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 <[email protected]>
---
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
>
> From: Bean Huo <[email protected]>
>
> 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
On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote:
> >
> > From: Bean Huo <[email protected]>
> >
> > 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
>
> On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote:
> > >
> > > From: Bean Huo <[email protected]>
> > >
> > > 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
>
On Thu, 2020-12-03 at 10:46 +0000, Avri Altman wrote:
> > > > From: Bean Huo <[email protected]>
> > > >
> > > > 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
> On Thu, 2020-12-03 at 10:46 +0000, Avri Altman wrote:
> > > > > From: Bean Huo <[email protected]>
> > > > >
> > > > > 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
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
On 2020-12-01 02:11, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Can Guo <[email protected]>
> ---
> 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];
On 2020-12-01 02:11, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> Keep device power mode as active power mode and VCC supply only if
> fWriteBoosterBufferFlushDuringHibernate setting 1 is successful.
>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> 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
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.
On Thu, 2020-12-03 at 07:27 +0000, Avri Altman wrote:
> >
> > From: Bean Huo <[email protected]>
> >
> > 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