* [PATCH 1/1] scsi: ufs: add clock-scalable property for clk scaling
@ 2022-04-16 8:56 ` Po-Wen Kao
0 siblings, 0 replies; 6+ messages in thread
From: Po-Wen Kao @ 2022-04-16 8:56 UTC (permalink / raw)
To: Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Steven Rostedt, Ingo Molnar,
Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, powen.kao, alice.chao,
chun-hung.wu, cc.chou, chaotian.jing, jiajie.hao, linux-scsi,
linux-kernel, linux-arm-kernel, linux-mediatek
In clk scaling, clk_set_rate is invoked to set dedicated PLL clk rate
on scale up and down. On some MTK platform, scaling is only possible
by switching parent of a mux, therefore we introduce a new
"clock-scalable" property to gain fine control over which clock rate can
be scaled by calling clk_set_rate. If a clock is defined as non-scalable,
clk_set_rate won't be invoked while clki->current_rate is still updated
if min/max freq is defined. Customized clk operation may be embedded in
pre/post change of hba->vops->clk_scale_notify.
All clocks in dts without "clock-scalable" property defined are assumed
scalable and works as usual. There is no neeed to make change on existing
dts.
To specify scalable property, one may define dts as follow
&ufshci {
clocks =
<clk1>,
<clk2>,
...
<clkn>;
clock-names =
"clk1",
"clk2",
...
"clkn";
freq-table-hz =
<1000 2000>,
<0 0>,
...
<0 0>;
clock-scalable =
<0
0
...
0
>;
};
Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
---
drivers/scsi/ufs/ufshcd-pltfrm.c | 99 ++++++++++++++++++++++++--------
drivers/scsi/ufs/ufshcd.c | 52 +++++++----------
drivers/scsi/ufs/ufshcd.h | 2 +
include/trace/events/ufs.h | 16 +++---
4 files changed, 105 insertions(+), 64 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 87975d1a21c8..757ba2ef2a4c 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -18,6 +18,59 @@
#define UFSHCD_DEFAULT_LANES_PER_DIRECTION 2
+/**
+ * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
+ * shape check.
+ * @hba: per-adapter instance
+ * @propname: name of property
+ * @cols: column count
+ * @rows: calculated row count with given cols
+ * @array: u32 array pointer of property data with propname
+ */
+static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
+ size_t cols, size_t *rows, u32 **array)
+{
+ struct device *dev = hba->dev;
+ struct device_node *np = dev->of_node;
+ int len = 0, ret = 0;
+ int _rows = 0;
+
+ if (!of_get_property(np, propname, &len)) {
+ ret = -EINVAL;
+ dev_warn(dev, "%s property not specified.\n", propname);
+ goto out;
+ }
+
+ len = len / sizeof(**array);
+ _rows = len / cols;
+ if (len % cols != 0 || !cols || !_rows) {
+ dev_warn(dev, "%s property define error.\n", propname);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ *array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
+ if (!*array) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = of_property_read_u32_array(np, propname, *array, len);
+
+ if (ret) {
+ dev_err(dev, "%s: error reading array %d\n",
+ propname, ret);
+ goto out;
+ }
+
+ *rows = _rows;
+
+ return 0;
+out:
+ devm_kfree(dev, *array);
+ return ret;
+}
+
static int ufshcd_parse_clock_info(struct ufs_hba *hba)
{
int ret = 0;
@@ -27,13 +80,14 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
struct device_node *np = dev->of_node;
char *name;
u32 *clkfreq = NULL;
+ u32 *scalable = NULL;
struct ufs_clk_info *clki;
- int len = 0;
size_t sz = 0;
if (!np)
goto out;
+ /* clock-names */
cnt = of_property_count_strings(np, "clock-names");
if (!cnt || (cnt == -EINVAL)) {
dev_info(dev, "%s: Unable to find clocks, assuming enabled\n",
@@ -47,37 +101,29 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
if (cnt <= 0)
goto out;
- if (!of_get_property(np, "freq-table-hz", &len)) {
- dev_info(dev, "freq-table-hz property not specified\n");
+ /* clock-scalable (optional) */
+ ret = ufshcd_get_clk_u32_array(hba, "clock-scalable", 1, &sz, &scalable);
+ if (ret) {
+ dev_warn(dev, "Optional property %s load failed. Assume all clocks scalable.\n",
+ "clock-scalable");
+ } else if (sz != cnt) {
+ dev_err(dev, "%s len mismatch\n", "clock-scalable");
+ ret = -EINVAL;
goto out;
}
- if (len <= 0)
+ /* freq-table-hz */
+ ret = ufshcd_get_clk_u32_array(hba, "freq-table-hz", 2, &sz, &clkfreq);
+ if (ret) {
+ dev_err(dev, "Property %s load failed.", "freq-table-hz");
goto out;
-
- sz = len / sizeof(*clkfreq);
- if (sz != 2 * cnt) {
+ } else if (sz != cnt) {
dev_err(dev, "%s len mismatch\n", "freq-table-hz");
ret = -EINVAL;
goto out;
}
- clkfreq = devm_kcalloc(dev, sz, sizeof(*clkfreq),
- GFP_KERNEL);
- if (!clkfreq) {
- ret = -ENOMEM;
- goto out;
- }
-
- ret = of_property_read_u32_array(np, "freq-table-hz",
- clkfreq, sz);
- if (ret && (ret != -EINVAL)) {
- dev_err(dev, "%s: error reading array %d\n",
- "freq-table-hz", ret);
- return ret;
- }
-
- for (i = 0; i < sz; i += 2) {
+ for (i = 0; i < cnt * 2; i += 2) {
ret = of_property_read_string_index(np,
"clock-names", i/2, (const char **)&name);
if (ret)
@@ -92,6 +138,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
clki->min_freq = clkfreq[i];
clki->max_freq = clkfreq[i+1];
clki->name = devm_kstrdup(dev, name, GFP_KERNEL);
+ clki->scalable = scalable ? !!scalable[i / 2] : true;
if (!clki->name) {
ret = -ENOMEM;
goto out;
@@ -99,11 +146,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
if (!strcmp(name, "ref_clk"))
clki->keep_link_active = true;
- dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
- clki->min_freq, clki->max_freq, clki->name);
+ dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
+ clki->name, clki->scalable, clki->min_freq, clki->max_freq);
list_add_tail(&clki->list, &hba->clk_list_head);
}
out:
+ devm_kfree(dev, scalable);
+ devm_kfree(dev, clkfreq);
return ret;
}
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index eea5af5fa166..295f8a314f37 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -950,6 +950,7 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
{
int ret = 0;
+ u32 target_rate = 0;
struct ufs_clk_info *clki;
struct list_head *head = &hba->clk_list_head;
@@ -958,41 +959,27 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
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;
-
- ret = clk_set_rate(clki->clk, clki->max_freq);
- if (ret) {
- dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
- __func__, clki->name,
- clki->max_freq, ret);
- break;
- }
- trace_ufshcd_clk_scaling(dev_name(hba->dev),
- "scaled up", clki->name,
- clki->curr_freq,
- clki->max_freq);
+ target_rate = scale_up ? clki->max_freq : clki->min_freq;
- clki->curr_freq = clki->max_freq;
+ if (!target_rate || clki->curr_freq == target_rate)
+ continue;
- } else if (!scale_up && clki->min_freq) {
- if (clki->curr_freq == clki->min_freq)
- continue;
+ if (!clki->scalable)
+ goto skip;
- ret = clk_set_rate(clki->clk, clki->min_freq);
- if (ret) {
- dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
- __func__, clki->name,
- clki->min_freq, ret);
- break;
- }
- trace_ufshcd_clk_scaling(dev_name(hba->dev),
- "scaled down", clki->name,
- clki->curr_freq,
- clki->min_freq);
- clki->curr_freq = clki->min_freq;
+ ret = clk_set_rate(clki->clk, target_rate);
+ if (ret) {
+ dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
+ __func__, clki->name,
+ target_rate, ret);
+ break;
}
+skip:
+ trace_ufshcd_clk_scaling(dev_name(hba->dev),
+ clki->name, scale_up,
+ clki->curr_freq,
+ target_rate);
+ clki->curr_freq = target_rate;
}
dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
clki->name, clk_get_rate(clki->clk));
@@ -8364,6 +8351,8 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
ufshcd_parse_dev_ref_clk_freq(hba, clki->clk);
if (clki->max_freq) {
+ if (!clki->scalable)
+ goto skip;
ret = clk_set_rate(clki->clk, clki->max_freq);
if (ret) {
dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
@@ -8371,6 +8360,7 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
clki->max_freq, ret);
goto out;
}
+skip:
clki->curr_freq = clki->max_freq;
}
dev_dbg(dev, "%s: clk: %s, rate: %lu\n", __func__,
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 1637f5cc1420..eaa3d6c5b9ab 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -260,6 +260,7 @@ struct ufs_dev_cmd {
* @keep_link_active: indicates that the clk should not be disabled if
link is active
* @enabled: variable to check against multiple enable/disable
+ * @scalable: indicates if clk is scalable in clk scaling
*/
struct ufs_clk_info {
struct list_head list;
@@ -270,6 +271,7 @@ struct ufs_clk_info {
u32 curr_freq;
bool keep_link_active;
bool enabled;
+ bool scalable;
};
enum ufs_notify_change_status {
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 599739ee7b20..780c964e52c2 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -103,30 +103,30 @@ TRACE_EVENT(ufshcd_clk_gating,
TRACE_EVENT(ufshcd_clk_scaling,
- TP_PROTO(const char *dev_name, const char *state, const char *clk,
- u32 prev_state, u32 curr_state),
+ TP_PROTO(const char *dev_name, const char *clk,
+ bool up, u32 prev_state, u32 curr_state),
- TP_ARGS(dev_name, state, clk, prev_state, curr_state),
+ TP_ARGS(dev_name, clk, up, prev_state, curr_state),
TP_STRUCT__entry(
__string(dev_name, dev_name)
- __string(state, state)
__string(clk, clk)
+ __field(bool, up)
__field(u32, prev_state)
__field(u32, curr_state)
),
TP_fast_assign(
__assign_str(dev_name, dev_name);
- __assign_str(state, state);
__assign_str(clk, clk);
+ __entry->up = up;
__entry->prev_state = prev_state;
__entry->curr_state = curr_state;
),
- TP_printk("%s: %s %s from %u to %u Hz",
- __get_str(dev_name), __get_str(state), __get_str(clk),
- __entry->prev_state, __entry->curr_state)
+ TP_printk("%s: scaled %s %s from %u to %u Hz",
+ __get_str(dev_name), __entry->up ? "up" : "down", __get_str(clk),
+ __entry->prev_state, __entry->curr_state)
);
TRACE_EVENT(ufshcd_auto_bkops_state,
--
2.18.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/1] scsi: ufs: add clock-scalable property for clk scaling
@ 2022-04-16 8:56 ` Po-Wen Kao
0 siblings, 0 replies; 6+ messages in thread
From: Po-Wen Kao @ 2022-04-16 8:56 UTC (permalink / raw)
To: Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Steven Rostedt, Ingo Molnar,
Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, powen.kao, alice.chao,
chun-hung.wu, cc.chou, chaotian.jing, jiajie.hao, linux-scsi,
linux-kernel, linux-arm-kernel, linux-mediatek
In clk scaling, clk_set_rate is invoked to set dedicated PLL clk rate
on scale up and down. On some MTK platform, scaling is only possible
by switching parent of a mux, therefore we introduce a new
"clock-scalable" property to gain fine control over which clock rate can
be scaled by calling clk_set_rate. If a clock is defined as non-scalable,
clk_set_rate won't be invoked while clki->current_rate is still updated
if min/max freq is defined. Customized clk operation may be embedded in
pre/post change of hba->vops->clk_scale_notify.
All clocks in dts without "clock-scalable" property defined are assumed
scalable and works as usual. There is no neeed to make change on existing
dts.
To specify scalable property, one may define dts as follow
&ufshci {
clocks =
<clk1>,
<clk2>,
...
<clkn>;
clock-names =
"clk1",
"clk2",
...
"clkn";
freq-table-hz =
<1000 2000>,
<0 0>,
...
<0 0>;
clock-scalable =
<0
0
...
0
>;
};
Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
---
drivers/scsi/ufs/ufshcd-pltfrm.c | 99 ++++++++++++++++++++++++--------
drivers/scsi/ufs/ufshcd.c | 52 +++++++----------
drivers/scsi/ufs/ufshcd.h | 2 +
include/trace/events/ufs.h | 16 +++---
4 files changed, 105 insertions(+), 64 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 87975d1a21c8..757ba2ef2a4c 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -18,6 +18,59 @@
#define UFSHCD_DEFAULT_LANES_PER_DIRECTION 2
+/**
+ * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
+ * shape check.
+ * @hba: per-adapter instance
+ * @propname: name of property
+ * @cols: column count
+ * @rows: calculated row count with given cols
+ * @array: u32 array pointer of property data with propname
+ */
+static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
+ size_t cols, size_t *rows, u32 **array)
+{
+ struct device *dev = hba->dev;
+ struct device_node *np = dev->of_node;
+ int len = 0, ret = 0;
+ int _rows = 0;
+
+ if (!of_get_property(np, propname, &len)) {
+ ret = -EINVAL;
+ dev_warn(dev, "%s property not specified.\n", propname);
+ goto out;
+ }
+
+ len = len / sizeof(**array);
+ _rows = len / cols;
+ if (len % cols != 0 || !cols || !_rows) {
+ dev_warn(dev, "%s property define error.\n", propname);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ *array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
+ if (!*array) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = of_property_read_u32_array(np, propname, *array, len);
+
+ if (ret) {
+ dev_err(dev, "%s: error reading array %d\n",
+ propname, ret);
+ goto out;
+ }
+
+ *rows = _rows;
+
+ return 0;
+out:
+ devm_kfree(dev, *array);
+ return ret;
+}
+
static int ufshcd_parse_clock_info(struct ufs_hba *hba)
{
int ret = 0;
@@ -27,13 +80,14 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
struct device_node *np = dev->of_node;
char *name;
u32 *clkfreq = NULL;
+ u32 *scalable = NULL;
struct ufs_clk_info *clki;
- int len = 0;
size_t sz = 0;
if (!np)
goto out;
+ /* clock-names */
cnt = of_property_count_strings(np, "clock-names");
if (!cnt || (cnt == -EINVAL)) {
dev_info(dev, "%s: Unable to find clocks, assuming enabled\n",
@@ -47,37 +101,29 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
if (cnt <= 0)
goto out;
- if (!of_get_property(np, "freq-table-hz", &len)) {
- dev_info(dev, "freq-table-hz property not specified\n");
+ /* clock-scalable (optional) */
+ ret = ufshcd_get_clk_u32_array(hba, "clock-scalable", 1, &sz, &scalable);
+ if (ret) {
+ dev_warn(dev, "Optional property %s load failed. Assume all clocks scalable.\n",
+ "clock-scalable");
+ } else if (sz != cnt) {
+ dev_err(dev, "%s len mismatch\n", "clock-scalable");
+ ret = -EINVAL;
goto out;
}
- if (len <= 0)
+ /* freq-table-hz */
+ ret = ufshcd_get_clk_u32_array(hba, "freq-table-hz", 2, &sz, &clkfreq);
+ if (ret) {
+ dev_err(dev, "Property %s load failed.", "freq-table-hz");
goto out;
-
- sz = len / sizeof(*clkfreq);
- if (sz != 2 * cnt) {
+ } else if (sz != cnt) {
dev_err(dev, "%s len mismatch\n", "freq-table-hz");
ret = -EINVAL;
goto out;
}
- clkfreq = devm_kcalloc(dev, sz, sizeof(*clkfreq),
- GFP_KERNEL);
- if (!clkfreq) {
- ret = -ENOMEM;
- goto out;
- }
-
- ret = of_property_read_u32_array(np, "freq-table-hz",
- clkfreq, sz);
- if (ret && (ret != -EINVAL)) {
- dev_err(dev, "%s: error reading array %d\n",
- "freq-table-hz", ret);
- return ret;
- }
-
- for (i = 0; i < sz; i += 2) {
+ for (i = 0; i < cnt * 2; i += 2) {
ret = of_property_read_string_index(np,
"clock-names", i/2, (const char **)&name);
if (ret)
@@ -92,6 +138,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
clki->min_freq = clkfreq[i];
clki->max_freq = clkfreq[i+1];
clki->name = devm_kstrdup(dev, name, GFP_KERNEL);
+ clki->scalable = scalable ? !!scalable[i / 2] : true;
if (!clki->name) {
ret = -ENOMEM;
goto out;
@@ -99,11 +146,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
if (!strcmp(name, "ref_clk"))
clki->keep_link_active = true;
- dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
- clki->min_freq, clki->max_freq, clki->name);
+ dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
+ clki->name, clki->scalable, clki->min_freq, clki->max_freq);
list_add_tail(&clki->list, &hba->clk_list_head);
}
out:
+ devm_kfree(dev, scalable);
+ devm_kfree(dev, clkfreq);
return ret;
}
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index eea5af5fa166..295f8a314f37 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -950,6 +950,7 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
{
int ret = 0;
+ u32 target_rate = 0;
struct ufs_clk_info *clki;
struct list_head *head = &hba->clk_list_head;
@@ -958,41 +959,27 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
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;
-
- ret = clk_set_rate(clki->clk, clki->max_freq);
- if (ret) {
- dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
- __func__, clki->name,
- clki->max_freq, ret);
- break;
- }
- trace_ufshcd_clk_scaling(dev_name(hba->dev),
- "scaled up", clki->name,
- clki->curr_freq,
- clki->max_freq);
+ target_rate = scale_up ? clki->max_freq : clki->min_freq;
- clki->curr_freq = clki->max_freq;
+ if (!target_rate || clki->curr_freq == target_rate)
+ continue;
- } else if (!scale_up && clki->min_freq) {
- if (clki->curr_freq == clki->min_freq)
- continue;
+ if (!clki->scalable)
+ goto skip;
- ret = clk_set_rate(clki->clk, clki->min_freq);
- if (ret) {
- dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
- __func__, clki->name,
- clki->min_freq, ret);
- break;
- }
- trace_ufshcd_clk_scaling(dev_name(hba->dev),
- "scaled down", clki->name,
- clki->curr_freq,
- clki->min_freq);
- clki->curr_freq = clki->min_freq;
+ ret = clk_set_rate(clki->clk, target_rate);
+ if (ret) {
+ dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
+ __func__, clki->name,
+ target_rate, ret);
+ break;
}
+skip:
+ trace_ufshcd_clk_scaling(dev_name(hba->dev),
+ clki->name, scale_up,
+ clki->curr_freq,
+ target_rate);
+ clki->curr_freq = target_rate;
}
dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
clki->name, clk_get_rate(clki->clk));
@@ -8364,6 +8351,8 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
ufshcd_parse_dev_ref_clk_freq(hba, clki->clk);
if (clki->max_freq) {
+ if (!clki->scalable)
+ goto skip;
ret = clk_set_rate(clki->clk, clki->max_freq);
if (ret) {
dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
@@ -8371,6 +8360,7 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
clki->max_freq, ret);
goto out;
}
+skip:
clki->curr_freq = clki->max_freq;
}
dev_dbg(dev, "%s: clk: %s, rate: %lu\n", __func__,
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 1637f5cc1420..eaa3d6c5b9ab 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -260,6 +260,7 @@ struct ufs_dev_cmd {
* @keep_link_active: indicates that the clk should not be disabled if
link is active
* @enabled: variable to check against multiple enable/disable
+ * @scalable: indicates if clk is scalable in clk scaling
*/
struct ufs_clk_info {
struct list_head list;
@@ -270,6 +271,7 @@ struct ufs_clk_info {
u32 curr_freq;
bool keep_link_active;
bool enabled;
+ bool scalable;
};
enum ufs_notify_change_status {
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 599739ee7b20..780c964e52c2 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -103,30 +103,30 @@ TRACE_EVENT(ufshcd_clk_gating,
TRACE_EVENT(ufshcd_clk_scaling,
- TP_PROTO(const char *dev_name, const char *state, const char *clk,
- u32 prev_state, u32 curr_state),
+ TP_PROTO(const char *dev_name, const char *clk,
+ bool up, u32 prev_state, u32 curr_state),
- TP_ARGS(dev_name, state, clk, prev_state, curr_state),
+ TP_ARGS(dev_name, clk, up, prev_state, curr_state),
TP_STRUCT__entry(
__string(dev_name, dev_name)
- __string(state, state)
__string(clk, clk)
+ __field(bool, up)
__field(u32, prev_state)
__field(u32, curr_state)
),
TP_fast_assign(
__assign_str(dev_name, dev_name);
- __assign_str(state, state);
__assign_str(clk, clk);
+ __entry->up = up;
__entry->prev_state = prev_state;
__entry->curr_state = curr_state;
),
- TP_printk("%s: %s %s from %u to %u Hz",
- __get_str(dev_name), __get_str(state), __get_str(clk),
- __entry->prev_state, __entry->curr_state)
+ TP_printk("%s: scaled %s %s from %u to %u Hz",
+ __get_str(dev_name), __entry->up ? "up" : "down", __get_str(clk),
+ __entry->prev_state, __entry->curr_state)
);
TRACE_EVENT(ufshcd_auto_bkops_state,
--
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/1] scsi: ufs: add clock-scalable property for clk scaling
@ 2022-04-16 8:56 ` Po-Wen Kao
0 siblings, 0 replies; 6+ messages in thread
From: Po-Wen Kao @ 2022-04-16 8:56 UTC (permalink / raw)
To: Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Steven Rostedt, Ingo Molnar,
Matthias Brugger
Cc: wsd_upstream, peter.wang, stanley.chu, powen.kao, alice.chao,
chun-hung.wu, cc.chou, chaotian.jing, jiajie.hao, linux-scsi,
linux-kernel, linux-arm-kernel, linux-mediatek
In clk scaling, clk_set_rate is invoked to set dedicated PLL clk rate
on scale up and down. On some MTK platform, scaling is only possible
by switching parent of a mux, therefore we introduce a new
"clock-scalable" property to gain fine control over which clock rate can
be scaled by calling clk_set_rate. If a clock is defined as non-scalable,
clk_set_rate won't be invoked while clki->current_rate is still updated
if min/max freq is defined. Customized clk operation may be embedded in
pre/post change of hba->vops->clk_scale_notify.
All clocks in dts without "clock-scalable" property defined are assumed
scalable and works as usual. There is no neeed to make change on existing
dts.
To specify scalable property, one may define dts as follow
&ufshci {
clocks =
<clk1>,
<clk2>,
...
<clkn>;
clock-names =
"clk1",
"clk2",
...
"clkn";
freq-table-hz =
<1000 2000>,
<0 0>,
...
<0 0>;
clock-scalable =
<0
0
...
0
>;
};
Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
---
drivers/scsi/ufs/ufshcd-pltfrm.c | 99 ++++++++++++++++++++++++--------
drivers/scsi/ufs/ufshcd.c | 52 +++++++----------
drivers/scsi/ufs/ufshcd.h | 2 +
include/trace/events/ufs.h | 16 +++---
4 files changed, 105 insertions(+), 64 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 87975d1a21c8..757ba2ef2a4c 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -18,6 +18,59 @@
#define UFSHCD_DEFAULT_LANES_PER_DIRECTION 2
+/**
+ * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
+ * shape check.
+ * @hba: per-adapter instance
+ * @propname: name of property
+ * @cols: column count
+ * @rows: calculated row count with given cols
+ * @array: u32 array pointer of property data with propname
+ */
+static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
+ size_t cols, size_t *rows, u32 **array)
+{
+ struct device *dev = hba->dev;
+ struct device_node *np = dev->of_node;
+ int len = 0, ret = 0;
+ int _rows = 0;
+
+ if (!of_get_property(np, propname, &len)) {
+ ret = -EINVAL;
+ dev_warn(dev, "%s property not specified.\n", propname);
+ goto out;
+ }
+
+ len = len / sizeof(**array);
+ _rows = len / cols;
+ if (len % cols != 0 || !cols || !_rows) {
+ dev_warn(dev, "%s property define error.\n", propname);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ *array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
+ if (!*array) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = of_property_read_u32_array(np, propname, *array, len);
+
+ if (ret) {
+ dev_err(dev, "%s: error reading array %d\n",
+ propname, ret);
+ goto out;
+ }
+
+ *rows = _rows;
+
+ return 0;
+out:
+ devm_kfree(dev, *array);
+ return ret;
+}
+
static int ufshcd_parse_clock_info(struct ufs_hba *hba)
{
int ret = 0;
@@ -27,13 +80,14 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
struct device_node *np = dev->of_node;
char *name;
u32 *clkfreq = NULL;
+ u32 *scalable = NULL;
struct ufs_clk_info *clki;
- int len = 0;
size_t sz = 0;
if (!np)
goto out;
+ /* clock-names */
cnt = of_property_count_strings(np, "clock-names");
if (!cnt || (cnt == -EINVAL)) {
dev_info(dev, "%s: Unable to find clocks, assuming enabled\n",
@@ -47,37 +101,29 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
if (cnt <= 0)
goto out;
- if (!of_get_property(np, "freq-table-hz", &len)) {
- dev_info(dev, "freq-table-hz property not specified\n");
+ /* clock-scalable (optional) */
+ ret = ufshcd_get_clk_u32_array(hba, "clock-scalable", 1, &sz, &scalable);
+ if (ret) {
+ dev_warn(dev, "Optional property %s load failed. Assume all clocks scalable.\n",
+ "clock-scalable");
+ } else if (sz != cnt) {
+ dev_err(dev, "%s len mismatch\n", "clock-scalable");
+ ret = -EINVAL;
goto out;
}
- if (len <= 0)
+ /* freq-table-hz */
+ ret = ufshcd_get_clk_u32_array(hba, "freq-table-hz", 2, &sz, &clkfreq);
+ if (ret) {
+ dev_err(dev, "Property %s load failed.", "freq-table-hz");
goto out;
-
- sz = len / sizeof(*clkfreq);
- if (sz != 2 * cnt) {
+ } else if (sz != cnt) {
dev_err(dev, "%s len mismatch\n", "freq-table-hz");
ret = -EINVAL;
goto out;
}
- clkfreq = devm_kcalloc(dev, sz, sizeof(*clkfreq),
- GFP_KERNEL);
- if (!clkfreq) {
- ret = -ENOMEM;
- goto out;
- }
-
- ret = of_property_read_u32_array(np, "freq-table-hz",
- clkfreq, sz);
- if (ret && (ret != -EINVAL)) {
- dev_err(dev, "%s: error reading array %d\n",
- "freq-table-hz", ret);
- return ret;
- }
-
- for (i = 0; i < sz; i += 2) {
+ for (i = 0; i < cnt * 2; i += 2) {
ret = of_property_read_string_index(np,
"clock-names", i/2, (const char **)&name);
if (ret)
@@ -92,6 +138,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
clki->min_freq = clkfreq[i];
clki->max_freq = clkfreq[i+1];
clki->name = devm_kstrdup(dev, name, GFP_KERNEL);
+ clki->scalable = scalable ? !!scalable[i / 2] : true;
if (!clki->name) {
ret = -ENOMEM;
goto out;
@@ -99,11 +146,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
if (!strcmp(name, "ref_clk"))
clki->keep_link_active = true;
- dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
- clki->min_freq, clki->max_freq, clki->name);
+ dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
+ clki->name, clki->scalable, clki->min_freq, clki->max_freq);
list_add_tail(&clki->list, &hba->clk_list_head);
}
out:
+ devm_kfree(dev, scalable);
+ devm_kfree(dev, clkfreq);
return ret;
}
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index eea5af5fa166..295f8a314f37 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -950,6 +950,7 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
{
int ret = 0;
+ u32 target_rate = 0;
struct ufs_clk_info *clki;
struct list_head *head = &hba->clk_list_head;
@@ -958,41 +959,27 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
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;
-
- ret = clk_set_rate(clki->clk, clki->max_freq);
- if (ret) {
- dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
- __func__, clki->name,
- clki->max_freq, ret);
- break;
- }
- trace_ufshcd_clk_scaling(dev_name(hba->dev),
- "scaled up", clki->name,
- clki->curr_freq,
- clki->max_freq);
+ target_rate = scale_up ? clki->max_freq : clki->min_freq;
- clki->curr_freq = clki->max_freq;
+ if (!target_rate || clki->curr_freq == target_rate)
+ continue;
- } else if (!scale_up && clki->min_freq) {
- if (clki->curr_freq == clki->min_freq)
- continue;
+ if (!clki->scalable)
+ goto skip;
- ret = clk_set_rate(clki->clk, clki->min_freq);
- if (ret) {
- dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
- __func__, clki->name,
- clki->min_freq, ret);
- break;
- }
- trace_ufshcd_clk_scaling(dev_name(hba->dev),
- "scaled down", clki->name,
- clki->curr_freq,
- clki->min_freq);
- clki->curr_freq = clki->min_freq;
+ ret = clk_set_rate(clki->clk, target_rate);
+ if (ret) {
+ dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
+ __func__, clki->name,
+ target_rate, ret);
+ break;
}
+skip:
+ trace_ufshcd_clk_scaling(dev_name(hba->dev),
+ clki->name, scale_up,
+ clki->curr_freq,
+ target_rate);
+ clki->curr_freq = target_rate;
}
dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
clki->name, clk_get_rate(clki->clk));
@@ -8364,6 +8351,8 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
ufshcd_parse_dev_ref_clk_freq(hba, clki->clk);
if (clki->max_freq) {
+ if (!clki->scalable)
+ goto skip;
ret = clk_set_rate(clki->clk, clki->max_freq);
if (ret) {
dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
@@ -8371,6 +8360,7 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
clki->max_freq, ret);
goto out;
}
+skip:
clki->curr_freq = clki->max_freq;
}
dev_dbg(dev, "%s: clk: %s, rate: %lu\n", __func__,
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 1637f5cc1420..eaa3d6c5b9ab 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -260,6 +260,7 @@ struct ufs_dev_cmd {
* @keep_link_active: indicates that the clk should not be disabled if
link is active
* @enabled: variable to check against multiple enable/disable
+ * @scalable: indicates if clk is scalable in clk scaling
*/
struct ufs_clk_info {
struct list_head list;
@@ -270,6 +271,7 @@ struct ufs_clk_info {
u32 curr_freq;
bool keep_link_active;
bool enabled;
+ bool scalable;
};
enum ufs_notify_change_status {
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 599739ee7b20..780c964e52c2 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -103,30 +103,30 @@ TRACE_EVENT(ufshcd_clk_gating,
TRACE_EVENT(ufshcd_clk_scaling,
- TP_PROTO(const char *dev_name, const char *state, const char *clk,
- u32 prev_state, u32 curr_state),
+ TP_PROTO(const char *dev_name, const char *clk,
+ bool up, u32 prev_state, u32 curr_state),
- TP_ARGS(dev_name, state, clk, prev_state, curr_state),
+ TP_ARGS(dev_name, clk, up, prev_state, curr_state),
TP_STRUCT__entry(
__string(dev_name, dev_name)
- __string(state, state)
__string(clk, clk)
+ __field(bool, up)
__field(u32, prev_state)
__field(u32, curr_state)
),
TP_fast_assign(
__assign_str(dev_name, dev_name);
- __assign_str(state, state);
__assign_str(clk, clk);
+ __entry->up = up;
__entry->prev_state = prev_state;
__entry->curr_state = curr_state;
),
- TP_printk("%s: %s %s from %u to %u Hz",
- __get_str(dev_name), __get_str(state), __get_str(clk),
- __entry->prev_state, __entry->curr_state)
+ TP_printk("%s: scaled %s %s from %u to %u Hz",
+ __get_str(dev_name), __entry->up ? "up" : "down", __get_str(clk),
+ __entry->prev_state, __entry->curr_state)
);
TRACE_EVENT(ufshcd_auto_bkops_state,
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH 1/1] scsi: ufs: add clock-scalable property for clk scaling
[not found] ` <CGME20220416085718epcas2p4b2afa580404f6d367bf1d34b7a9dbdb2@epcms2p5>
2022-04-27 8:39 ` Jinyoung CHOI
@ 2022-04-27 8:39 ` Jinyoung CHOI
0 siblings, 0 replies; 6+ messages in thread
From: Jinyoung CHOI @ 2022-04-27 8:39 UTC (permalink / raw)
To: Po-Wen Kao, ALIM AKHTAR, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Steven Rostedt, Ingo Molnar,
Matthias Brugger, cpgsproxy2, cpgsproxy3
Cc: wsd_upstream, peter.wang, stanley.chu, alice.chao, chun-hung.wu,
cc.chou, chaotian.jing, jiajie.hao, linux-scsi, linux-kernel,
linux-arm-kernel, linux-mediatek
> +/**
> + * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
> + * shape check.
> + * @hba: per-adapter instance
> + * @propname: name of property
> + * @cols: column count
> + * @rows: calculated row count with given cols
> + * @array: u32 array pointer of property data with propname
> + */
> +static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
> + size_t cols, size_t *rows, u32 **array)
> +{
> + struct device *dev = hba->dev;
> + struct device_node *np = dev->of_node;
> + int len = 0, ret = 0;
> + int _rows = 0;
> +
> + if (!of_get_property(np, propname, &len)) {
> + ret = -EINVAL;
> + dev_warn(dev, "%s property not specified.\n", propname);
> + goto out;
devm_kcalloc() has not been called yet.
There is no problem with calling devm_kfree(),
but wouldn't it be nice to return the error right away?
Isn't it "%s: ~~" ?
> + }
> +
> + len = len / sizeof(**array);
> + _rows = len / cols;
> + if (len % cols != 0 || !cols || !_rows) {
Wouldn't it be nice to have a condition test first?
If cols is 0, divide it by 0 and check the condition.
> + dev_warn(dev, "%s property define error.\n", propname);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + *array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
> + if (!*array) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = of_property_read_u32_array(np, propname, *array, len);
> +
> + if (ret) {
Wouldn't it be better to check the return value without a space?
> @@ -99,11 +146,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>
> if (!strcmp(name, "ref_clk"))
> clki->keep_link_active = true;
> - dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
> - clki->min_freq, clki->max_freq, clki->name);
> + dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
> + clki->name, clki->scalable, clki->min_freq, clki->max_freq);
> list_add_tail(&clki->list, &hba->clk_list_head);
> }
> out:
> + devm_kfree(dev, scalable);
> + devm_kfree(dev, clkfreq);
I have a question.
scalable and clkfreq are used only in this function. (I think... not managed resource but temporary)
Do you have the advantage of using devm_kcalloc instead of kcalloc?
> @@ -958,41 +959,27 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
>
> 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;
> -
> - ret = clk_set_rate(clki->clk, clki->max_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->max_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled up", clki->name,
> - clki->curr_freq,
> - clki->max_freq);
> + target_rate = scale_up ? clki->max_freq : clki->min_freq;
>
> - clki->curr_freq = clki->max_freq;
> + if (!target_rate || clki->curr_freq == target_rate)
> + continue;
>
> - } else if (!scale_up && clki->min_freq) {
> - if (clki->curr_freq == clki->min_freq)
> - continue;
> + if (!clki->scalable)
> + goto skip;
>
> - ret = clk_set_rate(clki->clk, clki->min_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->min_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled down", clki->name,
> - clki->curr_freq,
> - clki->min_freq);
> - clki->curr_freq = clki->min_freq;
> + ret = clk_set_rate(clki->clk, target_rate);
> + if (ret) {
> + dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> + __func__, clki->name,
> + target_rate, ret);
> + break;
> }
> +skip:
> + trace_ufshcd_clk_scaling(dev_name(hba->dev),
> + clki->name, scale_up,
> + clki->curr_freq,
> + target_rate);
> + clki->curr_freq = target_rate;
> }
> dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
> clki->name, clk_get_rate(clki->clk));
clki->scalable should always be confirmed before calling clk_set_rate().
Making this as a function,
I think you can get rid of goto state
and prevent the case to be called clk_set_rate() without checking the clki->scalable. :)
Thanks,
Jinyoung.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/1] scsi: ufs: add clock-scalable property for clk scaling
@ 2022-04-27 8:39 ` Jinyoung CHOI
0 siblings, 0 replies; 6+ messages in thread
From: Jinyoung CHOI @ 2022-04-27 8:39 UTC (permalink / raw)
To: Po-Wen Kao, ALIM AKHTAR, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Steven Rostedt, Ingo Molnar,
Matthias Brugger, cpgsproxy2, cpgsproxy3
Cc: wsd_upstream, peter.wang, stanley.chu, alice.chao, chun-hung.wu,
cc.chou, chaotian.jing, jiajie.hao, linux-scsi, linux-kernel,
linux-arm-kernel, linux-mediatek
> +/**
> + * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
> + * shape check.
> + * @hba: per-adapter instance
> + * @propname: name of property
> + * @cols: column count
> + * @rows: calculated row count with given cols
> + * @array: u32 array pointer of property data with propname
> + */
> +static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
> + size_t cols, size_t *rows, u32 **array)
> +{
> + struct device *dev = hba->dev;
> + struct device_node *np = dev->of_node;
> + int len = 0, ret = 0;
> + int _rows = 0;
> +
> + if (!of_get_property(np, propname, &len)) {
> + ret = -EINVAL;
> + dev_warn(dev, "%s property not specified.\n", propname);
> + goto out;
devm_kcalloc() has not been called yet.
There is no problem with calling devm_kfree(),
but wouldn't it be nice to return the error right away?
Isn't it "%s: ~~" ?
> + }
> +
> + len = len / sizeof(**array);
> + _rows = len / cols;
> + if (len % cols != 0 || !cols || !_rows) {
Wouldn't it be nice to have a condition test first?
If cols is 0, divide it by 0 and check the condition.
> + dev_warn(dev, "%s property define error.\n", propname);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + *array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
> + if (!*array) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = of_property_read_u32_array(np, propname, *array, len);
> +
> + if (ret) {
Wouldn't it be better to check the return value without a space?
> @@ -99,11 +146,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>
> if (!strcmp(name, "ref_clk"))
> clki->keep_link_active = true;
> - dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
> - clki->min_freq, clki->max_freq, clki->name);
> + dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
> + clki->name, clki->scalable, clki->min_freq, clki->max_freq);
> list_add_tail(&clki->list, &hba->clk_list_head);
> }
> out:
> + devm_kfree(dev, scalable);
> + devm_kfree(dev, clkfreq);
I have a question.
scalable and clkfreq are used only in this function. (I think... not managed resource but temporary)
Do you have the advantage of using devm_kcalloc instead of kcalloc?
> @@ -958,41 +959,27 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
>
> 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;
> -
> - ret = clk_set_rate(clki->clk, clki->max_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->max_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled up", clki->name,
> - clki->curr_freq,
> - clki->max_freq);
> + target_rate = scale_up ? clki->max_freq : clki->min_freq;
>
> - clki->curr_freq = clki->max_freq;
> + if (!target_rate || clki->curr_freq == target_rate)
> + continue;
>
> - } else if (!scale_up && clki->min_freq) {
> - if (clki->curr_freq == clki->min_freq)
> - continue;
> + if (!clki->scalable)
> + goto skip;
>
> - ret = clk_set_rate(clki->clk, clki->min_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->min_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled down", clki->name,
> - clki->curr_freq,
> - clki->min_freq);
> - clki->curr_freq = clki->min_freq;
> + ret = clk_set_rate(clki->clk, target_rate);
> + if (ret) {
> + dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> + __func__, clki->name,
> + target_rate, ret);
> + break;
> }
> +skip:
> + trace_ufshcd_clk_scaling(dev_name(hba->dev),
> + clki->name, scale_up,
> + clki->curr_freq,
> + target_rate);
> + clki->curr_freq = target_rate;
> }
> dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
> clki->name, clk_get_rate(clki->clk));
clki->scalable should always be confirmed before calling clk_set_rate().
Making this as a function,
I think you can get rid of goto state
and prevent the case to be called clk_set_rate() without checking the clki->scalable. :)
Thanks,
Jinyoung.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/1] scsi: ufs: add clock-scalable property for clk scaling
@ 2022-04-27 8:39 ` Jinyoung CHOI
0 siblings, 0 replies; 6+ messages in thread
From: Jinyoung CHOI @ 2022-04-27 8:39 UTC (permalink / raw)
To: Po-Wen Kao, ALIM AKHTAR, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Steven Rostedt, Ingo Molnar,
Matthias Brugger, cpgsproxy2, cpgsproxy3
Cc: wsd_upstream, peter.wang, stanley.chu, alice.chao, chun-hung.wu,
cc.chou, chaotian.jing, jiajie.hao, linux-scsi, linux-kernel,
linux-arm-kernel, linux-mediatek
> +/**
> + * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
> + * shape check.
> + * @hba: per-adapter instance
> + * @propname: name of property
> + * @cols: column count
> + * @rows: calculated row count with given cols
> + * @array: u32 array pointer of property data with propname
> + */
> +static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
> + size_t cols, size_t *rows, u32 **array)
> +{
> + struct device *dev = hba->dev;
> + struct device_node *np = dev->of_node;
> + int len = 0, ret = 0;
> + int _rows = 0;
> +
> + if (!of_get_property(np, propname, &len)) {
> + ret = -EINVAL;
> + dev_warn(dev, "%s property not specified.\n", propname);
> + goto out;
devm_kcalloc() has not been called yet.
There is no problem with calling devm_kfree(),
but wouldn't it be nice to return the error right away?
Isn't it "%s: ~~" ?
> + }
> +
> + len = len / sizeof(**array);
> + _rows = len / cols;
> + if (len % cols != 0 || !cols || !_rows) {
Wouldn't it be nice to have a condition test first?
If cols is 0, divide it by 0 and check the condition.
> + dev_warn(dev, "%s property define error.\n", propname);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + *array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
> + if (!*array) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = of_property_read_u32_array(np, propname, *array, len);
> +
> + if (ret) {
Wouldn't it be better to check the return value without a space?
> @@ -99,11 +146,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>
> if (!strcmp(name, "ref_clk"))
> clki->keep_link_active = true;
> - dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
> - clki->min_freq, clki->max_freq, clki->name);
> + dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
> + clki->name, clki->scalable, clki->min_freq, clki->max_freq);
> list_add_tail(&clki->list, &hba->clk_list_head);
> }
> out:
> + devm_kfree(dev, scalable);
> + devm_kfree(dev, clkfreq);
I have a question.
scalable and clkfreq are used only in this function. (I think... not managed resource but temporary)
Do you have the advantage of using devm_kcalloc instead of kcalloc?
> @@ -958,41 +959,27 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
>
> 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;
> -
> - ret = clk_set_rate(clki->clk, clki->max_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->max_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled up", clki->name,
> - clki->curr_freq,
> - clki->max_freq);
> + target_rate = scale_up ? clki->max_freq : clki->min_freq;
>
> - clki->curr_freq = clki->max_freq;
> + if (!target_rate || clki->curr_freq == target_rate)
> + continue;
>
> - } else if (!scale_up && clki->min_freq) {
> - if (clki->curr_freq == clki->min_freq)
> - continue;
> + if (!clki->scalable)
> + goto skip;
>
> - ret = clk_set_rate(clki->clk, clki->min_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->min_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled down", clki->name,
> - clki->curr_freq,
> - clki->min_freq);
> - clki->curr_freq = clki->min_freq;
> + ret = clk_set_rate(clki->clk, target_rate);
> + if (ret) {
> + dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> + __func__, clki->name,
> + target_rate, ret);
> + break;
> }
> +skip:
> + trace_ufshcd_clk_scaling(dev_name(hba->dev),
> + clki->name, scale_up,
> + clki->curr_freq,
> + target_rate);
> + clki->curr_freq = target_rate;
> }
> dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
> clki->name, clk_get_rate(clki->clk));
clki->scalable should always be confirmed before calling clk_set_rate().
Making this as a function,
I think you can get rid of goto state
and prevent the case to be called clk_set_rate() without checking the clki->scalable. :)
Thanks,
Jinyoung.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-27 8:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-16 8:56 [PATCH 1/1] scsi: ufs: add clock-scalable property for clk scaling Po-Wen Kao
2022-04-16 8:56 ` Po-Wen Kao
2022-04-16 8:56 ` Po-Wen Kao
[not found] ` <CGME20220416085718epcas2p4b2afa580404f6d367bf1d34b7a9dbdb2@epcms2p5>
2022-04-27 8:39 ` Jinyoung CHOI
2022-04-27 8:39 ` Jinyoung CHOI
2022-04-27 8:39 ` Jinyoung CHOI
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.