diff mbox

[v8,6/7] clk: Add floor and ceiling constraints to clock rates

Message ID 1409585675-26894-3-git-send-email-tomeu.vizoso@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomeu Vizoso Sept. 1, 2014, 3:34 p.m. UTC 
Adds a way for clock consumers to set maximum and minimum rates. This can be
used for thermal drivers to set ceiling rates, or by misc. drivers to set
floor rates to assure a minimum performance level.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>

---

v5: * Move the storage of constraints to the per-user clk struct, as suggested
      by Stephen Warren.
---
 drivers/clk/clk.c           | 36 ++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.h           |  1 +
 drivers/clk/clkdev.c        |  2 +-
 include/linux/clk-private.h |  5 +++++
 include/linux/clk.h         | 18 ++++++++++++++++++
 5 files changed, 61 insertions(+), 1 deletion(-)

Comments

Mike Turquette Sept. 3, 2014, 12:13 a.m. UTC | #1 
Quoting Tomeu Vizoso (2014-09-01 08:34:34)
> @@ -1633,6 +1636,13 @@ int clk_provider_set_rate(struct clk_core *clk, unsigned long rate)
>         /* prevent racing with updates to the clock topology */
>         clk_prepare_lock();
>  
> +       hlist_for_each_entry(clk_user, &clk->per_user_clks, child_node) {
> +               rate = max(rate, clk_user->floor_constraint);
> +
> +               if (clk_user->ceiling_constraint > 0)
> +                       rate = min(rate, clk_user->ceiling_constraint);

A ceiling_constraint from consumer_A could be less than a
floor_constraint from consumer_B. What should we do in this case?

In the code above the ceiling_constraint will always win. Is that by
design? We should document that behavior in Documentation/clk.txt.

This is the right place to check for the aforementioned corner case,
since we not only care about a single consumer having sane constraints
(e.g. min < max) but also mixing constraints across consumers.

However ...

> +       }
> +
>         /* bail early if nothing to do */
>         if (rate == clk_provider_get_rate(clk))
>                 goto out;
> @@ -1699,6 +1709,24 @@ int clk_set_rate(struct clk *clk_user, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_rate);
>  
> +int clk_set_floor_rate(struct clk *clk_user, unsigned long rate)
> +{
> +       struct clk_core *clk = clk_to_clk_core(clk_user);
> +
> +       clk_user->floor_constraint = rate;
> +       return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
> +}
> +EXPORT_SYMBOL_GPL(clk_set_floor_rate);
> +
> +int clk_set_ceiling_rate(struct clk *clk_user, unsigned long rate)
> +{
> +       struct clk_core *clk = clk_to_clk_core(clk_user);
> +
> +       clk_user->ceiling_constraint = rate;
> +       return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
> +}
> +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);

... we should probably sanity-check constraints here to make sure that
ceiling_rates for a given consumer are higher than floor_constraints for
that same consumer. It's a bit extra overhead but a WARN would probably
be helpful in this case.

Rest of the patch looks good.

Regards,
Mike
Tomeu Vizoso Sept. 3, 2014, 2:14 p.m. UTC | #2 
On 09/03/2014 02:13 AM, Mike Turquette wrote:
> Quoting Tomeu Vizoso (2014-09-01 08:34:34)
>> @@ -1633,6 +1636,13 @@ int clk_provider_set_rate(struct clk_core *clk, unsigned long rate)
>>          /* prevent racing with updates to the clock topology */
>>          clk_prepare_lock();
>>
>> +       hlist_for_each_entry(clk_user, &clk->per_user_clks, child_node) {
>> +               rate = max(rate, clk_user->floor_constraint);
>> +
>> +               if (clk_user->ceiling_constraint > 0)
>> +                       rate = min(rate, clk_user->ceiling_constraint);
>
> A ceiling_constraint from consumer_A could be less than a
> floor_constraint from consumer_B. What should we do in this case?
>
> In the code above the ceiling_constraint will always win. Is that by
> design? We should document that behavior in Documentation/clk.txt.
>
> This is the right place to check for the aforementioned corner case,
> since we not only care about a single consumer having sane constraints
> (e.g. min < max) but also mixing constraints across consumers.

Yeah. I think I lean towards first applying all floors, then applying 
all ceilings. Because hardware damage could happen if a ceiling from 
thermal isn't applied because of a bug in some other driver.

This also has the advantage of being deterministic, when with the 
current approach the result depends on the order in which the per-user 
clocks are iterated.

> However ...
>
>> +       }
>> +
>>          /* bail early if nothing to do */
>>          if (rate == clk_provider_get_rate(clk))
>>                  goto out;
>> @@ -1699,6 +1709,24 @@ int clk_set_rate(struct clk *clk_user, unsigned long rate)
>>   }
>>   EXPORT_SYMBOL_GPL(clk_set_rate);
>>
>> +int clk_set_floor_rate(struct clk *clk_user, unsigned long rate)
>> +{
>> +       struct clk_core *clk = clk_to_clk_core(clk_user);
>> +
>> +       clk_user->floor_constraint = rate;
>> +       return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_floor_rate);
>> +
>> +int clk_set_ceiling_rate(struct clk *clk_user, unsigned long rate)
>> +{
>> +       struct clk_core *clk = clk_to_clk_core(clk_user);
>> +
>> +       clk_user->ceiling_constraint = rate;
>> +       return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
>
> ... we should probably sanity-check constraints here to make sure that
> ceiling_rates for a given consumer are higher than floor_constraints for
> that same consumer. It's a bit extra overhead but a WARN would probably
> be helpful in this case.

Sounds like a good idea to me, will do.

Thanks,

Tomeu

> Rest of the patch looks good.
>
> Regards,
> Mike
>
diff mbox

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 61a3492..13ee3dd 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -560,6 +560,8 @@  struct clk *__clk_create_clk(struct clk_core *clk_core, const char *dev,
 	clk->dev_id = dev;
 	clk->con_id = con;
 
+	hlist_add_head(&clk->child_node, &clk_core->per_user_clks);
+
 	return clk;
 }
 
@@ -1625,6 +1627,7 @@  static void clk_change_rate(struct clk_core *clk)
 int clk_provider_set_rate(struct clk_core *clk, unsigned long rate)
 {
 	struct clk_core *top, *fail_clk;
+	struct clk *clk_user;
 	int ret = 0;
 
 	if (!clk)
@@ -1633,6 +1636,13 @@  int clk_provider_set_rate(struct clk_core *clk, unsigned long rate)
 	/* prevent racing with updates to the clock topology */
 	clk_prepare_lock();
 
+	hlist_for_each_entry(clk_user, &clk->per_user_clks, child_node) {
+		rate = max(rate, clk_user->floor_constraint);
+
+		if (clk_user->ceiling_constraint > 0)
+			rate = min(rate, clk_user->ceiling_constraint);
+	}
+
 	/* bail early if nothing to do */
 	if (rate == clk_provider_get_rate(clk))
 		goto out;
@@ -1699,6 +1709,24 @@  int clk_set_rate(struct clk *clk_user, unsigned long rate)
 }
 EXPORT_SYMBOL_GPL(clk_set_rate);
 
+int clk_set_floor_rate(struct clk *clk_user, unsigned long rate)
+{
+	struct clk_core *clk = clk_to_clk_core(clk_user);
+
+	clk_user->floor_constraint = rate;
+	return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
+}
+EXPORT_SYMBOL_GPL(clk_set_floor_rate);
+
+int clk_set_ceiling_rate(struct clk *clk_user, unsigned long rate)
+{
+	struct clk_core *clk = clk_to_clk_core(clk_user);
+
+	clk_user->ceiling_constraint = rate;
+	return clk_provider_set_rate(clk, clk_provider_get_rate(clk));
+}
+EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
+
 struct clk_core *clk_provider_get_parent(struct clk_core *clk)
 {
 	struct clk_core *parent;
@@ -2043,6 +2071,8 @@  int __clk_init(struct device *dev, struct clk_core *clk)
 			}
 	 }
 
+	INIT_HLIST_HEAD(&clk->per_user_clks);
+
 	/*
 	 * optional platform-specific magic
 	 *
@@ -2493,6 +2523,12 @@  int clk_notifier_unregister(struct clk *clk_user, struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(clk_notifier_unregister);
 
+void __clk_free_clk(struct clk *clk_user)
+{
+	hlist_del(&clk_user->child_node);
+	kfree(clk_user);
+}
+
 #ifdef CONFIG_OF
 /**
  * struct of_clk_provider - Clock provider registration structure
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 49eff38..005deb3 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -21,4 +21,5 @@  void of_clk_unlock(void);
 #if defined(CONFIG_COMMON_CLK)
 struct clk *__clk_create_clk(struct clk_core *clk_core, const char *dev,
 			     const char *con);
+void __clk_free_clk(struct clk *clk_user);
 #endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 0752cc9..0db829c 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -270,7 +270,7 @@  void clk_put(struct clk *clk)
 #if defined(CONFIG_COMMON_CLK)
 	clk_core_t *core = clk_to_clk_core(clk);
 
-	kfree(clk);
+	__clk_free_clk(clk);
 	__clk_put(core);
 #else
 	__clk_put(clk);
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index ce6a528..8126046 100644
--- a/include/linux/clk-private.h
+++ b/include/linux/clk-private.h
@@ -48,6 +48,7 @@  struct clk_core {
 	unsigned long		accuracy;
 	struct hlist_head	children;
 	struct hlist_node	child_node;
+	struct hlist_head	per_user_clks;
 	unsigned int		notifier_count;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry		*dentry;
@@ -62,6 +63,10 @@  struct clk {
 
 	unsigned int	enable_count;
 	void		*last_disable;
+
+	unsigned long	floor_constraint;
+	unsigned long	ceiling_constraint;
+	struct hlist_node child_node;
 };
 
 /*
diff --git a/include/linux/clk.h b/include/linux/clk.h
index f46a2eb..066b100 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -275,6 +275,24 @@  long clk_round_rate(struct clk *clk, unsigned long rate);
 int clk_set_rate(struct clk *clk, unsigned long rate);
 
 /**
+ * clk_set_floor_rate - set a minimum clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired minimum clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_floor_rate(struct clk *clk, unsigned long rate);
+
+/**
+ * clk_set_ceiling_rate - set a maximum clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired maximum clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_ceiling_rate(struct clk *clk, unsigned long rate);
+
+/**
  * clk_set_parent - set the parent clock source for this clock
  * @clk: clock source
  * @parent: parent clock source