[lkml]   [2014]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v3 8/8] clk: Add floor and ceiling constraints to clock rates
On 10/14/2014 06:27 AM, Tomeu Vizoso wrote:
> On 11 October 2014 01:55, Stephen Boyd <> wrote:
>> On 10/09, Tomeu Vizoso wrote:
>>> +{
>>> + int ret;
>>> +
>>> + clk_prepare_lock();
>>> +
>>> + clk->floor_constraint = rate;
>> No check for ceiling being less than floor?
> No, otherwise the calling code would have to be very careful to set
> both constraints in the correct order based on the current and next
> values. In practice I expect a given consumer to either set a floor or
> a constraint, but not both.

I totally missed this. Why can't we set the ceiling to ULONG_MAX when
the clock is created? That way we can drop the if check in the
aggregation logic for a 0 valued ceiling and then we can add the ceiling
being less than floor check here too?

>>> +
>>> + WARN(rate > 0 && rate < clk->floor_constraint,
>>> + "clk %s dev %s con %s: new ceiling %lu lower than existing floor %lu\n",
>>> + clk->core->name, clk->dev_id, clk->con_id, rate,
>>> + clk->floor_constraint);
>>> +
>>> + clk->ceiling_constraint = rate;
>>> + ret = clk_set_rate(clk, clk_get_rate(clk));
>> Why not just pass 0 as the second argument? The same comment
>> applies in the floor function.
> Both behaviours seem equally correct to me, but wonder if it wouldn't
> be better to store the rate that was last set explicitly with set_rate
> and try to reapply that one after every update to the constraints,
> instead of the current rate, or 0/INT_MAX.
>> This leads to another question though. What does it means for a
>> per-user clock to be disabled and change it's floor or ceiling?
>> Should that count in the aggregation logic?
> Per-user clocks don't get disabled, the whole clock does (but we can
> use per-user clk information to provide better debug information about
> unbalanced calls to prepare and enable).


>> So far we haven't required drivers to explicitly call
>> clk_set_rate() with 0 so they can "drop" their rate request
>> because there isn't any other user and disabling the clock is
>> pretty much the same. With multiple users it looks like we're
>> going to require them to set the floor or ceiling to 0 or INT_MAX
>> if they want to remove their request. Alternatively we could
>> track the prepare/unprepare state of the per-user clock and drop
>> the constraint when that instance is unprepared (or reapply it
>> when prepared). It's pretty much a semantic difference, but one
>> benefit would be that we don't have to make two (or three?) calls
>> to the clock framework if we want to drop the rate constraints
>> and disable the clock.
> In my mind this is not such an issue because I view clock constraints
> as attributes of the per-user clks, while the enable and prepare
> states and the actual rate are attributes of the global clock
> instance.

Alright, I just want to make sure we thought about it. I'll try to think
of any reason for this behavior and if I don't think of anything I'm
happy with the way things are.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

 \ /
  Last update: 2014-10-24 03:21    [W:0.055 / U:10.784 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site