lkml.org 
[lkml]   [2014]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC 5/5] clk: Add floor and ceiling constraints to clock rates
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


\
 
 \ /
  Last update: 2014-07-03 16:41    [W:0.278 / U:0.792 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site