lkml.org 
[lkml]   [2022]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRE: [PATCH v3 1/2] scsi: ufs: wb: renaming & cleanups functions
    Date
    > 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
    \
     
     \ /
      Last update: 2022-07-03 15:02    [W:5.068 / U:0.216 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site