| Message ID | 20220701074654epcms2p5fcc0a8abe766fa00851b00dff98ad3c7@epcms2p5 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: ufs: wb: Add sysfs and cleanup | expand |
> The Function names were changed clearly, and the location of the comments > was modified and added properly. > > In addition, the conditional test of the toggle functions was different, so it > was modified. > > Unnecessary logs were removed and modified appropriately. > > Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com> > --- > drivers/ufs/core/ufs-sysfs.c | 2 +- > drivers/ufs/core/ufshcd.c | 69 +++++++++++++++++------------------- > include/ufs/ufshcd.h | 7 ++++ > 3 files changed, 40 insertions(+), 38 deletions(-) > > diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index > 0a088b47d557..6253606b93b4 100644 > --- a/drivers/ufs/core/ufs-sysfs.c > +++ b/drivers/ufs/core/ufs-sysfs.c > @@ -230,7 +230,7 @@ static ssize_t wb_on_store(struct device *dev, struct > device_attribute *attr, > * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB > * on/off will be done while clock scaling up/down. > */ > - dev_warn(dev, "To control WB through wb_on is not allowed!\n"); > + dev_warn(dev, "It is not allowed to control WB!\n"); > return -EOPNOTSUPP; > } > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 1d3214e6b364..f98d023e44ae 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -268,8 +268,7 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, > bool on); static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba, > struct ufs_vreg *vreg); static int > ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag); -static void > 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_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba, > +bool set); > static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba); static void > ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba); > > @@ -289,16 +288,16 @@ static inline void ufshcd_disable_irq(struct ufs_hba > *hba) > } > } > > -static inline void ufshcd_wb_config(struct ufs_hba *hba) > +static void ufshcd_wb_set_default_flags(struct ufs_hba *hba) > { > if (!ufshcd_is_wb_allowed(hba)) > return; > > ufshcd_wb_toggle(hba, true); > + ufshcd_wb_toggle_buf_flush_during_h8(hba, true); > > - ufshcd_wb_toggle_flush_during_h8(hba, true); > - if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) > - ufshcd_wb_toggle_flush(hba, true); > + if (ufshcd_is_wb_buf_flush_allowed(hba)) > + ufshcd_wb_toggle_buf_flush(hba, true); > } > > static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba) @@ -1289,9 > +1288,10 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool > scale_up) > } > } > > - /* Enable Write Booster if we have scaled up else disable it */ > downgrade_write(&hba->clk_scaling_lock); > is_writelock = false; > + > + /* Enable Write Booster if we have scaled up else disable it */ > ufshcd_wb_toggle(hba, scale_up); > > out_unprepare: > @@ -5715,6 +5715,9 @@ static int __ufshcd_wb_toggle(struct ufs_hba *hba, > bool set, enum flag_idn idn) > enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG : > UPIU_QUERY_OPCODE_CLEAR_FLAG; > > + if (!ufshcd_is_wb_allowed(hba)) > + return -EPERM; > + > index = ufshcd_wb_get_query_index(hba); > return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL); } @@ - > 5723,60 +5726,50 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool > enable) { Nobody is checking the return value of ufshcd_wb_toggle(), maybe make it void instead? Other than that - looks good to me. Reviewed-by: Avri Altman <avri.altman@wdc.com> > int ret; > > - if (!ufshcd_is_wb_allowed(hba)) > - return 0; > - > - if (!(enable ^ hba->dev_info.wb_enabled)) > + if (hba->dev_info.wb_enabled == enable) > return 0; > > ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_EN); > if (ret) { > - dev_err(hba->dev, "%s Write Booster %s failed %d\n", > + dev_err(hba->dev, "%s: failed to %s WB %d\n", > __func__, enable ? "enable" : "disable", ret); > return ret; > } > > hba->dev_info.wb_enabled = enable; > - dev_info(hba->dev, "%s Write Booster %s\n", > - __func__, enable ? "enabled" : "disabled"); > > return ret; > } > > -static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool > set) > +static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba, > + bool enable) > { > int ret; > > - ret = __ufshcd_wb_toggle(hba, set, > - QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8); > - if (ret) { > - dev_err(hba->dev, "%s: WB-Buf Flush during H8 %s failed: %d\n", > - __func__, set ? "enable" : "disable", ret); > - return; > - } > - dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n", > - __func__, set ? "enabled" : "disabled"); > + ret = __ufshcd_wb_toggle(hba, enable, > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8); > + if (ret) > + dev_err(hba->dev, "%s: failed to %s WB buf flush during H8 %d\n", > + __func__, enable ? "enable" : "disable", ret); > } > > -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable) > +int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable) > { > int ret; > > - if (!ufshcd_is_wb_allowed(hba) || > - hba->dev_info.wb_buf_flush_enabled == enable) > - return; > + if (hba->dev_info.wb_buf_flush_enabled == enable) > + return 0; > > ret = __ufshcd_wb_toggle(hba, enable, > QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN); > if (ret) { > - dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__, > - enable ? "enable" : "disable", ret); > - return; > + dev_err(hba->dev, "%s: failed to %s WB buf flush %d\n", > + __func__, enable ? "enable" : "disable", ret); > + return ret; > } > > hba->dev_info.wb_buf_flush_enabled = enable; > > - dev_dbg(hba->dev, "%s WB-Buf Flush %s\n", > - __func__, enable ? "enabled" : "disabled"); > + return ret; > } > > static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba, > @@ -5807,10 +5800,10 @@ static bool > ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba, > > static void ufshcd_wb_force_disable(struct ufs_hba *hba) { > - if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) > - ufshcd_wb_toggle_flush(hba, false); > + if (ufshcd_is_wb_buf_flush_allowed(hba)) > + ufshcd_wb_toggle_buf_flush(hba, false); > > - ufshcd_wb_toggle_flush_during_h8(hba, false); > + ufshcd_wb_toggle_buf_flush_during_h8(hba, false); > ufshcd_wb_toggle(hba, false); > hba->caps &= ~UFSHCD_CAP_WB_EN; > > @@ -8197,7 +8190,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, > bool init_dev_params) > */ > ufshcd_set_active_icc_lvl(hba); > > - ufshcd_wb_config(hba); > + /* Enable UFS Write Booster if supported */ > + ufshcd_wb_set_default_flags(hba); > + > if (hba->ee_usr_mask) > ufshcd_write_ee_control(hba); > /* Enable Auto-Hibernate if configured */ diff --git > a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index > 7fe1a926cd99..78adc556444a 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -1017,6 +1017,12 @@ static inline bool ufshcd_is_wb_allowed(struct > ufs_hba *hba) > return hba->caps & UFSHCD_CAP_WB_EN; } > > +static inline bool ufshcd_is_wb_buf_flush_allowed(struct ufs_hba *hba) > +{ > + return ufshcd_is_wb_allowed(hba) && > + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL); > +} > + > #define ufshcd_writel(hba, val, reg) \ > writel((val), (hba)->mmio_base + (reg)) #define ufshcd_readl(hba, reg) > \ @@ -1211,6 +1217,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba > *hba, > enum query_opcode desc_op); > > int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable); > +int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable); > int ufshcd_suspend_prepare(struct device *dev); int > __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm); > void ufshcd_resume_complete(struct device *dev); > -- > 2.25.1
>> @@ -5715,6 +5715,9 @@ static int __ufshcd_wb_toggle(struct ufs_hba *hba, >> bool set, enum flag_idn idn) >> enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG : >> UPIU_QUERY_OPCODE_CLEAR_FLAG; >> >> + if (!ufshcd_is_wb_allowed(hba)) >> + return -EPERM; >> + >> index = ufshcd_wb_get_query_index(hba); >> return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL); } @@ - >> 5723,60 +5726,50 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool >> enable) { > Nobody is checking the return value of ufshcd_wb_toggle(), maybe make it void instead? > > Other than that - looks good to me. > > Reviewed-by: Avri Altman <avri.altman@wdc.com> It is used in "wb_on_store()" that turns WB on/off. Thank you for your review. :) Jinyoung.
On 7/1/22 00:46, Jinyoung CHOI wrote: > The Function names were changed clearly, and the location of the > comments was modified and added properly. > > In addition, the conditional test of the toggle functions was > different, so it was modified. > > Unnecessary logs were removed and modified appropriately. There are too many changes in this patch. Please split this patch, e.g. one patch that introduces ufshcd_is_wb_buf_flush_allowed(), one patch that renames ufshcd_wb_toggle_flush_during_h8() and its arguments and another patch that renames ufshcd_wb_config(). > diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c > index 0a088b47d557..6253606b93b4 100644 > --- a/drivers/ufs/core/ufs-sysfs.c > +++ b/drivers/ufs/core/ufs-sysfs.c > @@ -230,7 +230,7 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr, > * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB > * on/off will be done while clock scaling up/down. > */ > - dev_warn(dev, "To control WB through wb_on is not allowed!\n"); > + dev_warn(dev, "It is not allowed to control WB!\n"); I suggest to change "control" into "configure". > static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba) > @@ -1289,9 +1288,10 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up) > } > } > > - /* Enable Write Booster if we have scaled up else disable it */ > downgrade_write(&hba->clk_scaling_lock); > is_writelock = false; > + > + /* Enable Write Booster if we have scaled up else disable it */ > ufshcd_wb_toggle(hba, scale_up); The above change could be yet another patch. > out_unprepare: > @@ -5715,6 +5715,9 @@ static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn) > enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG : > UPIU_QUERY_OPCODE_CLEAR_FLAG; > > + if (!ufshcd_is_wb_allowed(hba)) > + return -EPERM; > + Moving the ufshcd_is_wb_allowed() call from ufshcd_wb_toggle() into __ufshcd_wb_toggle() should be yet another patch. > - if (!(enable ^ hba->dev_info.wb_enabled)) > + if (hba->dev_info.wb_enabled == enable) > return 0; This change is independent of the rest of this patch and hence could be yet another patch. > - dev_info(hba->dev, "%s Write Booster %s\n", > - __func__, enable ? "enabled" : "disabled"); Why has this code been left out? > - ret = __ufshcd_wb_toggle(hba, set, > - QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8); > - if (ret) { > - dev_err(hba->dev, "%s: WB-Buf Flush during H8 %s failed: %d\n", > - __func__, set ? "enable" : "disable", ret); > - return; > - } > - dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n", > - __func__, set ? "enabled" : "disabled"); > + ret = __ufshcd_wb_toggle(hba, enable, > + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8); > + if (ret) > + dev_err(hba->dev, "%s: failed to %s WB buf flush during H8 %d\n", > + __func__, enable ? "enable" : "disable", ret); The above error message is worse than the original error message. Please either keep the original message or change it into something better than the original, e.g. "Failed to %s WB buffer flushing during H8". Thanks, Bart.
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index 0a088b47d557..6253606b93b4 100644 --- a/drivers/ufs/core/ufs-sysfs.c +++ b/drivers/ufs/core/ufs-sysfs.c @@ -230,7 +230,7 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr, * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB * on/off will be done while clock scaling up/down. */ - dev_warn(dev, "To control WB through wb_on is not allowed!\n"); + dev_warn(dev, "It is not allowed to control WB!\n"); return -EOPNOTSUPP; } diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 1d3214e6b364..f98d023e44ae 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -268,8 +268,7 @@ static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on); static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba, struct ufs_vreg *vreg); static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag); -static void 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_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba, bool set); static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba); static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba); @@ -289,16 +288,16 @@ static inline void ufshcd_disable_irq(struct ufs_hba *hba) } } -static inline void ufshcd_wb_config(struct ufs_hba *hba) +static void ufshcd_wb_set_default_flags(struct ufs_hba *hba) { if (!ufshcd_is_wb_allowed(hba)) return; ufshcd_wb_toggle(hba, true); + ufshcd_wb_toggle_buf_flush_during_h8(hba, true); - ufshcd_wb_toggle_flush_during_h8(hba, true); - if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) - ufshcd_wb_toggle_flush(hba, true); + if (ufshcd_is_wb_buf_flush_allowed(hba)) + ufshcd_wb_toggle_buf_flush(hba, true); } static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba) @@ -1289,9 +1288,10 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up) } } - /* Enable Write Booster if we have scaled up else disable it */ downgrade_write(&hba->clk_scaling_lock); is_writelock = false; + + /* Enable Write Booster if we have scaled up else disable it */ ufshcd_wb_toggle(hba, scale_up); out_unprepare: @@ -5715,6 +5715,9 @@ static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn) enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG : UPIU_QUERY_OPCODE_CLEAR_FLAG; + if (!ufshcd_is_wb_allowed(hba)) + return -EPERM; + index = ufshcd_wb_get_query_index(hba); return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL); } @@ -5723,60 +5726,50 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable) { int ret; - if (!ufshcd_is_wb_allowed(hba)) - return 0; - - if (!(enable ^ hba->dev_info.wb_enabled)) + if (hba->dev_info.wb_enabled == enable) return 0; ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_EN); if (ret) { - dev_err(hba->dev, "%s Write Booster %s failed %d\n", + dev_err(hba->dev, "%s: failed to %s WB %d\n", __func__, enable ? "enable" : "disable", ret); return ret; } hba->dev_info.wb_enabled = enable; - dev_info(hba->dev, "%s Write Booster %s\n", - __func__, enable ? "enabled" : "disabled"); return ret; } -static void ufshcd_wb_toggle_flush_during_h8(struct ufs_hba *hba, bool set) +static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba, + bool enable) { int ret; - ret = __ufshcd_wb_toggle(hba, set, - QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8); - if (ret) { - dev_err(hba->dev, "%s: WB-Buf Flush during H8 %s failed: %d\n", - __func__, set ? "enable" : "disable", ret); - return; - } - dev_dbg(hba->dev, "%s WB-Buf Flush during H8 %s\n", - __func__, set ? "enabled" : "disabled"); + ret = __ufshcd_wb_toggle(hba, enable, + QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8); + if (ret) + dev_err(hba->dev, "%s: failed to %s WB buf flush during H8 %d\n", + __func__, enable ? "enable" : "disable", ret); } -static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable) +int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable) { int ret; - if (!ufshcd_is_wb_allowed(hba) || - hba->dev_info.wb_buf_flush_enabled == enable) - return; + if (hba->dev_info.wb_buf_flush_enabled == enable) + return 0; ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN); if (ret) { - dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__, - enable ? "enable" : "disable", ret); - return; + dev_err(hba->dev, "%s: failed to %s WB buf flush %d\n", + __func__, enable ? "enable" : "disable", ret); + return ret; } hba->dev_info.wb_buf_flush_enabled = enable; - dev_dbg(hba->dev, "%s WB-Buf Flush %s\n", - __func__, enable ? "enabled" : "disabled"); + return ret; } static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba, @@ -5807,10 +5800,10 @@ static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba, static void ufshcd_wb_force_disable(struct ufs_hba *hba) { - if (!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) - ufshcd_wb_toggle_flush(hba, false); + if (ufshcd_is_wb_buf_flush_allowed(hba)) + ufshcd_wb_toggle_buf_flush(hba, false); - ufshcd_wb_toggle_flush_during_h8(hba, false); + ufshcd_wb_toggle_buf_flush_during_h8(hba, false); ufshcd_wb_toggle(hba, false); hba->caps &= ~UFSHCD_CAP_WB_EN; @@ -8197,7 +8190,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params) */ ufshcd_set_active_icc_lvl(hba); - ufshcd_wb_config(hba); + /* Enable UFS Write Booster if supported */ + ufshcd_wb_set_default_flags(hba); + if (hba->ee_usr_mask) ufshcd_write_ee_control(hba); /* Enable Auto-Hibernate if configured */ diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 7fe1a926cd99..78adc556444a 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -1017,6 +1017,12 @@ static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba) return hba->caps & UFSHCD_CAP_WB_EN; } +static inline bool ufshcd_is_wb_buf_flush_allowed(struct ufs_hba *hba) +{ + return ufshcd_is_wb_allowed(hba) && + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL); +} + #define ufshcd_writel(hba, val, reg) \ writel((val), (hba)->mmio_base + (reg)) #define ufshcd_readl(hba, reg) \ @@ -1211,6 +1217,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, enum query_opcode desc_op); int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable); +int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable); int ufshcd_suspend_prepare(struct device *dev); int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm); void ufshcd_resume_complete(struct device *dev);
The Function names were changed clearly, and the location of the comments was modified and added properly. In addition, the conditional test of the toggle functions was different, so it was modified. Unnecessary logs were removed and modified appropriately. Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com> --- drivers/ufs/core/ufs-sysfs.c | 2 +- drivers/ufs/core/ufshcd.c | 69 +++++++++++++++++------------------- include/ufs/ufshcd.h | 7 ++++ 3 files changed, 40 insertions(+), 38 deletions(-)