[1/2] clk: Add clk_set_rate_safe() API support

Commit Message

Shawn Lin April 28, 2018, 9:16 a.m. UTC

From: Elaine Zhang <[e-mail]>

Compared with clk_set_rate(), this API would maintain the clock rate
of its children to avoid exception if the new rate is higher than
the old rate.

Signed-off-by: Elaine Zhang <[e-mail]>
Signed-off-by: Shawn Lin <[e-mail]>
---

 drivers/clk/clk.c   | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h | 15 +++++++++++++++
 2 files changed, 66 insertions(+)

Comments

Stephen Boyd June 2, 2018, 6:43 a.m. UTC | #1

Quoting Shawn Lin (2018-04-28 02:16:57)
> From: Elaine Zhang <[e-mail]>
> 
> Compared with clk_set_rate(), this API would maintain the clock rate
> of its children to avoid exception if the new rate is higher than
> the old rate.
> 
> Signed-off-by: Elaine Zhang <[e-mail]>
> Signed-off-by: Shawn Lin <[e-mail]>
> ---

There's no cover letter, and the length and quality of the commit
message leaves me with more questions than answers. Please resend with a
proper argument for this patch set.

My first question is why can't we fix clk_set_rate() or
clk_set_rate_range() APIs to do what you want?

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shawn Lin June 7, 2018, 2:11 a.m. UTC | #2

Hi Stephen,

On 2018/6/2 14:43, Stephen Boyd wrote:
> Quoting Shawn Lin (2018-04-28 02:16:57)
>> From: Elaine Zhang <[e-mail]>
>>
>> Compared with clk_set_rate(), this API would maintain the clock rate
>> of its children to avoid exception if the new rate is higher than
>> the old rate.
>>
>> Signed-off-by: Elaine Zhang <[e-mail]>
>> Signed-off-by: Shawn Lin <[e-mail]>
>> ---
> 
> There's no cover letter, and the length and quality of the commit
> message leaves me with more questions than answers. Please resend with a
> proper argument for this patch set.

Sure.

> 
> My first question is why can't we fix clk_set_rate() or
> clk_set_rate_range() APIs to do what you want?

Yep, I was thinking if there are any drivers/instances implicitly
set child's rate depending on seting a higher rate for its
parent?

But now I think if that happens, we need fixes for the
drivers/instances, as clk_set_rate*() didn't guarantee this. So we could
do the right constraint for clk_set_rate() or clk_set_rate_range().

Will update V2.

> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9ae92aa..7dec6a8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2013,6 +2013,57 @@  int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 EXPORT_SYMBOL_GPL(clk_set_rate_exclusive);
 
 /**
+ * clk_set_rate_safe - specify a new rate for clk
+ * @clk: the clk whose rate is being changed
+ * @new_rate: the new rate for clk
+ *
+ * Compared with clk_set_rate, this API would also make sure children
+ * of @clk are in the safe clock rate range by maintaining the original
+ * clock rate.
+ *
+ * When changing the parent's clock rate in some cases, we should make
+ * sure the children's clock rate are still in the safe range, namely
+ * [clk->min_rate, clk->max_rate], which is especially important
+ * if its child is a system bus. In general, the lower clock rate the
+ * safer, so simply walk through the parent's clock chain to maintain
+ * the children's clock rate if the new clock rate is higher than the
+ * older one.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_set_rate_safe(struct clk *clk, unsigned long new_rate)
+{
+	unsigned long old_rate, child_rate;
+	struct hlist_node *hn;
+	struct clk_core *core;
+	int div, ret;
+
+	if (!clk)
+		return 0;
+
+	old_rate = clk_get_rate(clk);
+
+	if (new_rate <= old_rate)
+		goto set_rate;
+
+	hlist_for_each_entry_safe(core, hn, &clk->core->children,
+				  child_node) {
+		if (strcmp(core->parent->name, clk->core->name))
+			continue;
+
+		child_rate = core->rate;
+		div = DIV_ROUND_UP_ULL((u64)new_rate, old_rate);
+		core->rate = child_rate / div;
+		clk_change_rate(core);
+	}
+
+set_rate:
+	ret = clk_set_rate(clk, new_rate);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate_safe);
+
+/**
  * clk_set_rate_range - set a rate range for a clock source
  * @clk: clock source
  * @min: desired minimum clock rate in Hz, inclusive
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 0dbd088..244d030 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -505,6 +505,16 @@  int __must_check clk_bulk_enable(int num_clks,
 int clk_set_rate(struct clk *clk, unsigned long rate);
 
 /**
+ * clk_set_rate_safe - set the clock rate for a clock source and make sure
+ *		       its children still maintain the original rate
+ * @clk: clock source
+ * @rate: desired clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_rate_safe(struct clk *clk, unsigned long rate);
+
+/**
  * clk_set_rate_exclusive- set the clock rate and claim exclusivity over
  *                         clock source
  * @clk: clock source
@@ -671,6 +681,11 @@  static inline int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 	return 0;
 }
 
+static inline int clk_set_rate_safe(struct clk *clk, unsigned long rate)
+{
+	return 0;
+}
+
 static inline long clk_round_rate(struct clk *clk, unsigned long rate)
 {
 	return 0;