Re: [PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2020-03-18 16:19, Avri Altman wrote:
Hi,


From: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>

This change introduces a func ufshcd_set_clk_freq() to explicitly
set clock frequency so that it can be used in reset_and_resotre path and in ufshcd_scale_clks(). Meanwhile, this change cleans up the clock scaling
error out path.

Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2a2a63b..63aaa88f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -855,28 +855,29 @@ static bool
ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
                return false;
 }

-static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
+/**
+ * ufshcd_set_clk_freq - set UFS controller clock frequencies
+ * @hba: per adapter instance
+ * @scale_up: If True, set max possible frequency othewise set low
frequency
+ *
+ * Returns 0 if successful
+ * Returns < 0 for any other errors
+ */
+static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
Personally I prefer using the convention of "__scale_clks" to describe
the privet core of scale_clks,
But I know that many people are against it.

 {
        int ret = 0;
        struct ufs_clk_info *clki;
        struct list_head *head = &hba->clk_list_head;
-       ktime_t start = ktime_get();
-       bool clk_state_changed = false;

        if (list_empty(head))
                goto out;

-       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
-       if (ret)
-               return ret;
-
        list_for_each_entry(clki, head, list) {
                if (!IS_ERR_OR_NULL(clki->clk)) {
                        if (scale_up && clki->max_freq) {
                                if (clki->curr_freq == clki->max_freq)
                                        continue;

-                               clk_state_changed = true;
ret = clk_set_rate(clki->clk, clki->max_freq);
                                if (ret) {
dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed,
%d\n",
@@ -895,7 +896,6 @@ static int ufshcd_scale_clks(struct ufs_hba *hba,
bool scale_up)
                                if (clki->curr_freq == clki->min_freq)
                                        continue;

-                               clk_state_changed = true;
ret = clk_set_rate(clki->clk, clki->min_freq);
                                if (ret) {
dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed,
%d\n",
@@ -914,13 +914,36 @@ static int ufshcd_scale_clks(struct ufs_hba *hba,
bool scale_up)
                                clki->name, clk_get_rate(clki->clk));
        }

+out:
+       return ret;
+}
+
+/**
+ * ufshcd_scale_clks - scale up or scale down UFS controller clocks
+ * @hba: per adapter instance
+ * @scale_up: True if scaling up and false if scaling down
+ *
+ * Returns 0 if successful
+ * Returns < 0 for any other errors
+ */
+static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
+{
+       int ret = 0;
+
+       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
+       if (ret)
+               return ret;
+
+       ret = ufshcd_set_clk_freq(hba, scale_up);
+       if (ret)
+               return ret;
+
ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
+       if (ret) {
+               ufshcd_set_clk_freq(hba, !scale_up);
+               return ret;
+       }

-out:
-       if (clk_state_changed)
-               trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
-                       (scale_up ? "up" : "down"),
- ktime_to_us(ktime_sub(ktime_get(), start)), ret);

Why remove the ufshcd_profile_clk_scaling trace?



Shall add it back, this is just a collection of Subhash's changes.

        return ret;
 }

@@ -1106,35 +1129,36 @@ static int ufshcd_devfreq_scale(struct ufs_hba
*hba, bool scale_up)

        ret = ufshcd_clock_scaling_prepare(hba);
        if (ret)
-               return ret;
+               goto out;
Are you fixing here a hold without release?
Should make note of that.


Yes, true


        /* scale down the gear before scaling down clocks */
        if (!scale_up) {
                ret = ufshcd_scale_gear(hba, false);
                if (ret)
-                       goto out;
+                       goto clk_scaling_unprepare;
        }

        ret = ufshcd_scale_clks(hba, scale_up);
-       if (ret) {
-               if (!scale_up)
-                       ufshcd_scale_gear(hba, true);
-               goto out;
-       }
+       if (ret)
+               goto scale_up_gear;

        /* scale up the gear after scaling up clocks */
        if (scale_up) {
                ret = ufshcd_scale_gear(hba, true);
                if (ret) {
                        ufshcd_scale_clks(hba, false);
-                       goto out;
+                       goto clk_scaling_unprepare;
                }
        }

- ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
+       goto clk_scaling_unprepare;
I think you should find a way to make this function more readable.
Adding all those "spaghetti" gotos making it even harder to read.

Thanks,
Avri


This is just a collection of Subhash's changes. I think the 2
clk_scaling_unprepare and out gotos make sense, but the
scale_up_gear goto is a bit vague. I will remove the
scale_up_gear goto.

Thanks,

Can Guo.


-out:
+scale_up_gear:
+       if (!scale_up)
+               ufshcd_scale_gear(hba, true);
+clk_scaling_unprepare:
        ufshcd_clock_scaling_unprepare(hba);
+out:
        ufshcd_release(hba);
        return ret;
 }
@@ -6251,7 +6275,7 @@ static int ufshcd_host_reset_and_restore(struct
ufs_hba *hba)
        spin_unlock_irqrestore(hba->host->host_lock, flags);

/* scale up clocks to max frequency before full reinitialization */
-       ufshcd_scale_clks(hba, true);
+       ufshcd_set_clk_freq(hba, true);

        err = ufshcd_hba_enable(hba);
        if (err)
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project.



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux