* [PATCH 0/3] Three changes for UFS WriteBooster @ 2020-11-30 18:11 Bean Huo 2020-11-30 18:11 ` [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off Bean Huo ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Bean Huo @ 2020-11-30 18:11 UTC (permalink / raw) To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off 2020-11-30 18:11 [PATCH 0/3] Three changes for UFS WriteBooster Bean Huo @ 2020-11-30 18:11 ` Bean Huo 2020-11-30 23:19 ` Asutosh Das (asd) 2020-11-30 18:11 ` [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 Bean Huo 2020-11-30 18:11 ` [PATCH 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe() Bean Huo 2 siblings, 1 reply; 17+ messages in thread From: Bean Huo @ 2020-11-30 18:11 UTC (permalink / raw) To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off 2020-11-30 18:11 ` [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off Bean Huo @ 2020-11-30 23:19 ` Asutosh Das (asd) 2020-12-02 16:20 ` Bean Huo 0 siblings, 1 reply; 17+ messages in thread From: Asutosh Das (asd) @ 2020-11-30 23:19 UTC (permalink / raw) To: Bean Huo, alim.akhtar, avri.altman, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off 2020-11-30 23:19 ` Asutosh Das (asd) @ 2020-12-02 16:20 ` Bean Huo 2020-12-02 17:58 ` Asutosh Das (asd) 0 siblings, 1 reply; 17+ messages in thread From: Bean Huo @ 2020-12-02 16:20 UTC (permalink / raw) To: Asutosh Das (asd), alim.akhtar, avri.altman, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off 2020-12-02 16:20 ` Bean Huo @ 2020-12-02 17:58 ` Asutosh Das (asd) 0 siblings, 0 replies; 17+ messages in thread From: Asutosh Das (asd) @ 2020-12-02 17:58 UTC (permalink / raw) To: Bean Huo, alim.akhtar, avri.altman, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-11-30 18:11 [PATCH 0/3] Three changes for UFS WriteBooster Bean Huo 2020-11-30 18:11 ` [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off Bean Huo @ 2020-11-30 18:11 ` Bean Huo [not found] ` <BY5PR04MB6599826730BD3FB0E547E60587F30@BY5PR04MB6599.namprd04.prod.outlook.com> 2020-12-04 3:26 ` Can Guo 2020-11-30 18:11 ` [PATCH 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe() Bean Huo 2 siblings, 2 replies; 17+ messages in thread From: Bean Huo @ 2020-11-30 18:11 UTC (permalink / raw) To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
[parent not found: <BY5PR04MB6599826730BD3FB0E547E60587F30@BY5PR04MB6599.namprd04.prod.outlook.com>]
* RE: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 [not found] ` <BY5PR04MB6599826730BD3FB0E547E60587F30@BY5PR04MB6599.namprd04.prod.outlook.com> @ 2020-12-03 7:27 ` Avri Altman 2020-12-03 9:36 ` Bean Huo 2020-12-03 9:40 ` Bean Huo 0 siblings, 2 replies; 17+ messages in thread From: Avri Altman @ 2020-12-03 7:27 UTC (permalink / raw) To: alim.akhtar, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-12-03 7:27 ` Avri Altman @ 2020-12-03 9:36 ` Bean Huo 2020-12-03 9:40 ` Bean Huo 1 sibling, 0 replies; 17+ messages in thread From: Bean Huo @ 2020-12-03 9:36 UTC (permalink / raw) To: Avri Altman, alim.akhtar, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-12-03 7:27 ` Avri Altman 2020-12-03 9:36 ` Bean Huo @ 2020-12-03 9:40 ` Bean Huo 2020-12-03 10:46 ` Avri Altman 1 sibling, 1 reply; 17+ messages in thread From: Bean Huo @ 2020-12-03 9:40 UTC (permalink / raw) To: Avri Altman, alim.akhtar, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-12-03 9:40 ` Bean Huo @ 2020-12-03 10:46 ` Avri Altman 2020-12-03 11:45 ` Bean Huo 0 siblings, 1 reply; 17+ messages in thread From: Avri Altman @ 2020-12-03 10:46 UTC (permalink / raw) To: Bean Huo, alim.akhtar, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel > > 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-12-03 10:46 ` Avri Altman @ 2020-12-03 11:45 ` Bean Huo 2020-12-03 12:15 ` Avri Altman 0 siblings, 1 reply; 17+ messages in thread From: Bean Huo @ 2020-12-03 11:45 UTC (permalink / raw) To: Avri Altman, alim.akhtar, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-12-03 11:45 ` Bean Huo @ 2020-12-03 12:15 ` Avri Altman 2020-12-03 12:31 ` Bean Huo 0 siblings, 1 reply; 17+ messages in thread From: Avri Altman @ 2020-12-03 12:15 UTC (permalink / raw) To: Bean Huo, alim.akhtar, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-12-03 12:15 ` Avri Altman @ 2020-12-03 12:31 ` Bean Huo 0 siblings, 0 replies; 17+ messages in thread From: Bean Huo @ 2020-12-03 12:31 UTC (permalink / raw) To: Avri Altman, alim.akhtar, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-11-30 18:11 ` [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 Bean Huo [not found] ` <BY5PR04MB6599826730BD3FB0E547E60587F30@BY5PR04MB6599.namprd04.prod.outlook.com> @ 2020-12-04 3:26 ` Can Guo 2020-12-04 8:28 ` Bean Huo 1 sibling, 1 reply; 17+ messages in thread From: Can Guo @ 2020-12-04 3:26 UTC (permalink / raw) To: Bean Huo Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, linux-scsi, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 2020-12-04 3:26 ` Can Guo @ 2020-12-04 8:28 ` Bean Huo 0 siblings, 0 replies; 17+ messages in thread From: Bean Huo @ 2020-12-04 8:28 UTC (permalink / raw) To: Can Guo Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, linux-scsi, linux-kernel 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe() 2020-11-30 18:11 [PATCH 0/3] Three changes for UFS WriteBooster Bean Huo 2020-11-30 18:11 ` [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off Bean Huo 2020-11-30 18:11 ` [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 Bean Huo @ 2020-11-30 18:11 ` Bean Huo 2020-12-04 3:27 ` Can Guo 2 siblings, 1 reply; 17+ messages in thread From: Bean Huo @ 2020-11-30 18:11 UTC (permalink / raw) To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe() 2020-11-30 18:11 ` [PATCH 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe() Bean Huo @ 2020-12-04 3:27 ` Can Guo 0 siblings, 0 replies; 17+ messages in thread From: Can Guo @ 2020-12-04 3:27 UTC (permalink / raw) To: Bean Huo Cc: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, linux-scsi, linux-kernel 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]; ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-12-05 11:07 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-30 18:11 [PATCH 0/3] Three changes for UFS WriteBooster Bean Huo 2020-11-30 18:11 ` [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off Bean Huo 2020-11-30 23:19 ` Asutosh Das (asd) 2020-12-02 16:20 ` Bean Huo 2020-12-02 17:58 ` Asutosh Das (asd) 2020-11-30 18:11 ` [PATCH 2/3] scsi: ufs: Keep device power on only fWriteBoosterBufferFlushDuringHibernate == 1 Bean Huo [not found] ` <BY5PR04MB6599826730BD3FB0E547E60587F30@BY5PR04MB6599.namprd04.prod.outlook.com> 2020-12-03 7:27 ` Avri Altman 2020-12-03 9:36 ` Bean Huo 2020-12-03 9:40 ` Bean Huo 2020-12-03 10:46 ` Avri Altman 2020-12-03 11:45 ` Bean Huo 2020-12-03 12:15 ` Avri Altman 2020-12-03 12:31 ` Bean Huo 2020-12-04 3:26 ` Can Guo 2020-12-04 8:28 ` Bean Huo 2020-11-30 18:11 ` [PATCH 3/3] scsi: ufs: Changes comment in the function ufshcd_wb_probe() Bean Huo 2020-12-04 3:27 ` Can Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).