[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <5449A333.3050504@codeaurora.org>
Date: Thu, 23 Oct 2014 17:54:11 -0700
From: Stephen Boyd <sboyd@...eaurora.org>
To: Tomeu Vizoso <tomeu.vizoso@...labora.com>
CC: Mike Turquette <mturquette@...aro.org>,
Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
Russell King <linux@....linux.org.uk>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [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 <sboyd@...eaurora.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists