Re: [PATCH v3 8/8] clk: Add floor and ceiling constraints to clock rates

[Stephen Boyd <sboyd@codeaurora.org>]

On 10/14/2014 06:27 AM, Tomeu Vizoso wrote:
> On 11 October 2014 01:55, Stephen Boyd <sboyd@codeaurora.org> 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).

Ok.

>> 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