| Message ID | 1405606399-26608-7-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/17/2014 08:13 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 > +struct rate_constraint { > + struct hlist_node node; > + const char *dev_id; > + const char *con_id; > + enum constraint_type type; > + unsigned long rate; > +}; I would personally still prefer either: a) The struct rate_constraints be stored in struct clk rather than struct clk_core, so they're stored in the container that "set" the constraints. This would mean iterating over a struct clk_core's associated struct clks, then iterating over the struct rate_constraints within each struct clk. or: b) struct rate_constraint to contain a pointer to the struct clk that created the constraint, rather than copying the dev_id/con_id from that struct clk into the struct rate_constraint. With either of those changes, the constraints are directly associated with the clock client object that created the constraint much better than right now, where the matching is only because the struct clk and struct rate_constraint "happen to" have the same dev_id/con_id values. Still, this is a pretty minor issue, and overall this series looks reasonable to me at a quick look.
On 07/22/2014 07:50 PM, Stephen Warren wrote: > On 07/17/2014 08:13 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 > >> +struct rate_constraint { >> + struct hlist_node node; >> + const char *dev_id; >> + const char *con_id; >> + enum constraint_type type; >> + unsigned long rate; >> +}; > > I would personally still prefer either: > > a) The struct rate_constraints be stored in struct clk rather than > struct clk_core, so they're stored in the container that "set" the > constraints. This would mean iterating over a struct clk_core's > associated struct clks, then iterating over the struct rate_constraints > within each struct clk. > > or: > > b) struct rate_constraint to contain a pointer to the struct clk that > created the constraint, rather than copying the dev_id/con_id from that > struct clk into the struct rate_constraint. > > With either of those changes, the constraints are directly associated > with the clock client object that created the constraint much better > than right now, where the matching is only because the struct clk and > struct rate_constraint "happen to" have the same dev_id/con_id values. > > Still, this is a pretty minor issue, and overall this series looks > reasonable to me at a quick look. Yeah, I agree and personally prefer a), but given the little feedback that I have gotten so far on the refactoring, I'm starting to consider forgetting about the per-user clk struct and go instead with a request-based API similar to that of pm_qos, for setting floor and ceiling frequencies. Regards, Tomeu
On 07/31/2014 05:47 AM, Tomeu Vizoso wrote: > On 07/22/2014 07:50 PM, Stephen Warren wrote: >> On 07/17/2014 08:13 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 >> >>> +struct rate_constraint { >>> + struct hlist_node node; >>> + const char *dev_id; >>> + const char *con_id; >>> + enum constraint_type type; >>> + unsigned long rate; >>> +}; >> >> I would personally still prefer either: >> >> a) The struct rate_constraints be stored in struct clk rather than >> struct clk_core, so they're stored in the container that "set" the >> constraints. This would mean iterating over a struct clk_core's >> associated struct clks, then iterating over the struct rate_constraints >> within each struct clk. >> >> or: >> >> b) struct rate_constraint to contain a pointer to the struct clk that >> created the constraint, rather than copying the dev_id/con_id from that >> struct clk into the struct rate_constraint. >> >> With either of those changes, the constraints are directly associated >> with the clock client object that created the constraint much better >> than right now, where the matching is only because the struct clk and >> struct rate_constraint "happen to" have the same dev_id/con_id values. >> >> Still, this is a pretty minor issue, and overall this series looks >> reasonable to me at a quick look. > > Yeah, I agree and personally prefer a), but given the little feedback > that I have gotten so far on the refactoring, I'm starting to consider > forgetting about the per-user clk struct and go instead with a > request-based API similar to that of pm_qos, for setting floor and > ceiling frequencies. I would obviously encourage you to continue pushing for this feature, although I understand it can be difficult to be motivated when the patches don't get much feedback.
Quoting Stephen Warren (2014-07-31 10:51:20) > On 07/31/2014 05:47 AM, Tomeu Vizoso wrote: > > On 07/22/2014 07:50 PM, Stephen Warren wrote: > >> On 07/17/2014 08:13 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 > >> > >>> +struct rate_constraint { > >>> + struct hlist_node node; > >>> + const char *dev_id; > >>> + const char *con_id; > >>> + enum constraint_type type; > >>> + unsigned long rate; > >>> +}; > >> > >> I would personally still prefer either: > >> > >> a) The struct rate_constraints be stored in struct clk rather than > >> struct clk_core, so they're stored in the container that "set" the > >> constraints. This would mean iterating over a struct clk_core's > >> associated struct clks, then iterating over the struct rate_constraints > >> within each struct clk. > >> > >> or: > >> > >> b) struct rate_constraint to contain a pointer to the struct clk that > >> created the constraint, rather than copying the dev_id/con_id from that > >> struct clk into the struct rate_constraint. > >> > >> With either of those changes, the constraints are directly associated > >> with the clock client object that created the constraint much better > >> than right now, where the matching is only because the struct clk and > >> struct rate_constraint "happen to" have the same dev_id/con_id values. > >> > >> Still, this is a pretty minor issue, and overall this series looks > >> reasonable to me at a quick look. > > > > Yeah, I agree and personally prefer a), but given the little feedback > > that I have gotten so far on the refactoring, I'm starting to consider > > forgetting about the per-user clk struct and go instead with a > > request-based API similar to that of pm_qos, for setting floor and > > ceiling frequencies. > > I would obviously encourage you to continue pushing for this feature, > although I understand it can be difficult to be motivated when the > patches don't get much feedback. Sorry for not giving this more of a look. My secret plan was to review this in more detail once we're closer to the merge window and, if appropriate, apply it as one of the first things since this is bound to cause some breakage early on which needs to be caught and fixed up quickly. We are very close to the merge window so I will try to get to this later this week. I like the per-user struct clk myself and I hope we can get this in for 3.18. Regards, Mike
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index f1cdc01..90db677 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); @@ -1653,6 +1666,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 rate_constraint *constraint; int ret = 0; if (!clk) @@ -1661,6 +1675,17 @@ 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(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_provider_get_rate(clk)) goto out; @@ -1727,6 +1752,65 @@ 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) + 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_provider_set_rate(clk, clk_provider_get_rate(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_provider_get_parent(struct clk_core *clk) { struct clk_core *parent; @@ -2072,6 +2156,8 @@ int __clk_init(struct device *dev, struct clk_core *clk) } } + INIT_HLIST_HEAD(&clk->rate_constraints); + /* * optional platform-specific magic * @@ -2508,6 +2594,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_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 517a25b..8a6a398 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -269,7 +269,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 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 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
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 | 103 ++++++++++++++++++++++++++++++++++++++++++++ drivers/clk/clk.h | 1 + drivers/clk/clkdev.c | 2 +- include/linux/clk-private.h | 1 + include/linux/clk.h | 18 ++++++++ 5 files changed, 124 insertions(+), 1 deletion(-)