| Message ID | 1403855872-14749-6-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/27/2014 01:57 AM, Tomeu Vizoso wrote: > 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. > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > +static struct rate_constraint *__ensure_constraint(struct clk *clk_user, > + enum constraint_type type) > + if (!found) { > + constraint = kzalloc(sizeof(*constraint), GFP_KERNEL); > + if (!constraint) { > + pr_err("%s: could not allocate constraint\n", __func__); Doesn't kzalloc print an error itself if the allocation fails? I've certainly seen quite a few patches ripping out custom "allocation failed" errors in code. > +void __clk_free_clk(struct clk *clk_user) > +{ > + struct clk_core *clk = clk_to_clk_core(clk_user); > + struct rate_constraint *constraint; > + struct hlist_node *tmp; > + > + hlist_for_each_entry_safe(constraint, tmp, &clk->rate_constraints, node) { > + if (constraint->dev_id == clk_user->dev_id && > + constraint->con_id == clk_user->con_id) { > + hlist_del(&constraint->node); > + kfree(constraint); Perhaps the list of constraints should be indexed by the client clk structure, so that test should be: if (constraint->clk_user == clk_user) It might be a bit more work, but perhaps the constraints should simply be stored directly in the struct clk rater than the struct clk_core. That would require a nested loop to apply constraints though; first over each struct clk associated with a struct clk_core, then over each constraints in that struct clk. It would slightly simplify adding/removing constraints though, and store the constraints at their "source". > diff --git a/include/linux/clk.h b/include/linux/clk.h > +int clk_set_floor_rate(struct clk *clk, unsigned long rate); > +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate); Additions functions to explicitly remove any previously requested floor/ceiling rate might be useful. The same effect could be achieved by a floor of 0 or a very high ceiling, but it feels cleaner to remove them. Overall, this series seems to implement the right kind of concept to me. It'll certainly stop us (NVIDIA at least) wanting to create all kinds of "virtual" clock objects (and associated clock IDs and device tree clock IDs) to achieve a similar effect.
On Fri, Jun 27, 2014 at 04:57:42PM -0600, Stephen Warren wrote: > On 06/27/2014 01:57 AM, Tomeu Vizoso wrote: > > 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. > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > +static struct rate_constraint *__ensure_constraint(struct clk *clk_user, > > + enum constraint_type type) > > > + if (!found) { > > + constraint = kzalloc(sizeof(*constraint), GFP_KERNEL); > > + if (!constraint) { > > + pr_err("%s: could not allocate constraint\n", __func__); > > Doesn't kzalloc print an error itself if the allocation fails? I've > certainly seen quite a few patches ripping out custom "allocation > failed" errors in code. Yes, these are unnecessary. There's even a checkpatch warning for this construct nowadays: f9a5a624f414 checkpatch: attempt to find unnecessary 'out of memory' messages Thierry
On 06/28/2014 12:57 AM, Stephen Warren wrote: > On 06/27/2014 01:57 AM, Tomeu Vizoso wrote: >> 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. > >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > >> +static struct rate_constraint *__ensure_constraint(struct clk *clk_user, >> + enum constraint_type type) > >> + if (!found) { >> + constraint = kzalloc(sizeof(*constraint), GFP_KERNEL); >> + if (!constraint) { >> + pr_err("%s: could not allocate constraint\n", __func__); > > Doesn't kzalloc print an error itself if the allocation fails? I've > certainly seen quite a few patches ripping out custom "allocation > failed" errors in code. Thanks for pointing this out, have fixed it in a new version that will be sent soon. >> +void __clk_free_clk(struct clk *clk_user) >> +{ >> + struct clk_core *clk = clk_to_clk_core(clk_user); >> + struct rate_constraint *constraint; >> + struct hlist_node *tmp; >> + >> + hlist_for_each_entry_safe(constraint, tmp, &clk->rate_constraints, node) { >> + if (constraint->dev_id == clk_user->dev_id && >> + constraint->con_id == clk_user->con_id) { >> + hlist_del(&constraint->node); >> + kfree(constraint); > > Perhaps the list of constraints should be indexed by the client clk > structure, so that test should be: > > if (constraint->clk_user == clk_user) > > It might be a bit more work, but perhaps the constraints should simply > be stored directly in the struct clk rater than the struct clk_core. > That would require a nested loop to apply constraints though; first over > each struct clk associated with a struct clk_core, then over each > constraints in that struct clk. It would slightly simplify > adding/removing constraints though, and store the constraints at their > "source". Yeah, I like this alternative from a code organization point of view, but I found the increased code complexity hard to justify. Wonder if anybody else has an opinion on this. >> diff --git a/include/linux/clk.h b/include/linux/clk.h > >> +int clk_set_floor_rate(struct clk *clk, unsigned long rate); > >> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate); > > Additions functions to explicitly remove any previously requested > floor/ceiling rate might be useful. The same effect could be achieved by > a floor of 0 or a very high ceiling, but it feels cleaner to remove them. I'm also on the fence on whether the improved readability justifies the additional API, because I find convenient that one just has to grep for a single function call to find all usages (it was certainly useful when I looked at how downstream does the equivalent). I'll be glad to add the functions in a future version if there's consensus on this. > Overall, this series seems to implement the right kind of concept to me. > It'll certainly stop us (NVIDIA at least) wanting to create all kinds of > "virtual" clock objects (and associated clock IDs and device tree clock > IDs) to achieve a similar effect. Thanks for the great feedback, Tomeu
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 7fc1937..4d93066 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -23,6 +23,19 @@ #include "clk.h" +enum constraint_type { + CONSTRAINT_FLOOR, + CONSTRAINT_CEILING, +}; + +struct rate_constraint { + struct hlist_node node; + const char *dev_id; + const char *con_id; + enum constraint_type type; + unsigned long rate; +}; + static DEFINE_SPINLOCK(enable_lock); static DEFINE_MUTEX(prepare_lock); @@ -1624,6 +1637,7 @@ static void clk_change_rate(struct clk_core *clk) int __clk_set_rate_internal(struct clk_core *clk, unsigned long rate) { struct clk_core *top, *fail_clk; + struct rate_constraint *constraint; int ret = 0; if (!clk) @@ -1632,6 +1646,17 @@ int __clk_set_rate_internal(struct clk_core *clk, unsigned long rate) /* prevent racing with updates to the clock topology */ clk_prepare_lock(); + hlist_for_each_entry(constraint, &clk->rate_constraints, node) { + switch(constraint->type) { + case CONSTRAINT_FLOOR: + rate = max(rate, constraint->rate); + break; + case CONSTRAINT_CEILING: + rate = min(rate, constraint->rate); + break; + } + } + /* bail early if nothing to do */ if (rate == __clk_get_rate_internal(clk)) goto out; @@ -1695,6 +1720,67 @@ int clk_set_rate(struct clk *clk_user, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_set_rate); +static struct rate_constraint *__ensure_constraint(struct clk *clk_user, + enum constraint_type type) +{ + struct clk_core *clk = clk_to_clk_core(clk_user); + struct rate_constraint *constraint = NULL; + bool found = false; + + hlist_for_each_entry(constraint, &clk->rate_constraints, node) { + if (constraint->dev_id == clk_user->dev_id && + constraint->con_id == clk_user->con_id && + constraint->type == type) + found = true; + } + + if (!found) { + constraint = kzalloc(sizeof(*constraint), GFP_KERNEL); + if (!constraint) { + pr_err("%s: could not allocate constraint\n", __func__); + return NULL; + } + hlist_add_head(&constraint->node, &clk->rate_constraints); + } + + return constraint; +} + +static int __clk_set_constraint(struct clk *clk_user, unsigned long rate, + enum constraint_type type) +{ + struct clk_core *clk = clk_to_clk_core(clk_user); + struct rate_constraint *constraint; + int ret; + + clk_prepare_lock(); + + constraint = __ensure_constraint(clk_user, type); + if (!constraint) + return -ENOMEM; + + constraint->rate = rate; + + /* Update the rate so the new constraint is taken into account */ + ret = __clk_set_rate_internal(clk, __clk_get_rate_internal(clk)); + + clk_prepare_unlock(); + + return ret; +} + +int clk_set_floor_rate(struct clk *clk_user, unsigned long rate) +{ + return __clk_set_constraint(clk_user, rate, CONSTRAINT_FLOOR); +} +EXPORT_SYMBOL_GPL(clk_set_floor_rate); + +int clk_set_ceiling_rate(struct clk *clk_user, unsigned long rate) +{ + return __clk_set_constraint(clk_user, rate, CONSTRAINT_CEILING); +} +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate); + struct clk_core *__clk_get_parent_internal(struct clk_core *clk) { struct clk_core *parent; @@ -2028,6 +2114,8 @@ int __clk_init(struct device *dev, struct clk_core *clk) } } + INIT_HLIST_HEAD(&clk->rate_constraints); + /* * optional platform-specific magic * @@ -2458,6 +2546,23 @@ 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) +{ + struct clk_core *clk = clk_to_clk_core(clk_user); + struct rate_constraint *constraint; + struct hlist_node *tmp; + + hlist_for_each_entry_safe(constraint, tmp, &clk->rate_constraints, node) { + if (constraint->dev_id == clk_user->dev_id && + constraint->con_id == clk_user->con_id) { + hlist_del(&constraint->node); + kfree(constraint); + } + } + + kfree(clk); +} + #ifdef CONFIG_OF /** * struct of_clk_provider - Clock provider registration structure diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h index 2f682bf..ea84797 100644 --- a/drivers/clk/clk.h +++ b/drivers/clk/clk.h @@ -18,4 +18,5 @@ void of_clk_unlock(void); #if defined(CONFIG_COMMON_CLK) struct clk_core *clk_to_clk_core(struct clk *clk); +void __clk_free_clk(struct clk *clk_user); #endif diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index bf1a2d7..88448b7 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -230,16 +230,11 @@ struct clk *clk_get(struct device *dev, const char *con_id) } EXPORT_SYMBOL(clk_get); -static void clk_free_clk(struct clk *clk) -{ - kfree(clk); -} - void clk_put(struct clk *clk) { clk_core_t *core = clk_to_clk_core(clk); - clk_free_clk(clk); + __clk_free_clk(clk); __clk_put(core); } EXPORT_SYMBOL(clk_put); diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h index 9657fc8..a8279e1 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 rate_constraints; unsigned int notifier_count; #ifdef CONFIG_DEBUG_FS struct dentry *dentry; diff --git a/include/linux/clk.h b/include/linux/clk.h index b8938e4..d6234bd 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -274,6 +274,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
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> --- drivers/clk/clk.c | 105 ++++++++++++++++++++++++++++++++++++++++++++ drivers/clk/clk.h | 1 + drivers/clk/clkdev.c | 7 +-- include/linux/clk-private.h | 1 + include/linux/clk.h | 18 ++++++++ 5 files changed, 126 insertions(+), 6 deletions(-)