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 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 sb...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/8] clk: Add floor and ceiling constraints to clock rates
On 11 October 2014 01:55, Stephen Boyd wrote:
> On 10/09, Tomeu Vizoso wrote:
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 4db918a..97cf1a3 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1629,18 +1609,27 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
>> /* prevent racing with updates to the clock topology */
>> clk_prepare_lock();
>>
>> + hlist_for_each_entry(clk_user, >per_user_clks, child_node) {
>> + rate = max(rate, clk_user->floor_constraint);
>> + }
>> +
>> + hlist_for_each_entry(clk_user, >per_user_clks, child_node) {
>> + if (clk_user->ceiling_constraint > 0)
>> + rate = min(rate, clk_user->ceiling_constraint);
>> + }
>> +
>> /* bail early if nothing to do */
>> - if (rate == clk_get_rate(clk))
>> + if (rate == clk_core_get_rate(clk))
>
> Can we use the nolock variant here?
It's true that we have the lock already, but I'm not sure about the
implications of not calling __clk_recalc_rates. Nothing that cannot be
solved with one more function though...
> As an aside, this is going to
> make my per-clock locking series complicated. I'm not even sure
> how the two series will work together. In the per-clock locking
> series I was hoping we could check if rate == current rate
> without holding any locks. Otherwise we get into a recursive lock
> situation. Need to think more about that one.
Ok.
>> goto out;
>>
>> - if ((clk->core->flags & CLK_SET_RATE_GATE) &&
>> - clk->core->prepare_count) {
>> + if ((clk->flags & CLK_SET_RATE_GATE) &&
>> + clk->prepare_count) {
>> ret = -EBUSY;
>> goto out;
>> }
>>
>> /* calculate new rates and get the topmost changed clock */
>> - top = clk_calc_new_rates(clk->core, rate);
>> + top = clk_calc_new_rates(clk, rate);
>> if (!top) {
>> ret = -EINVAL;
>> goto out;
>> @@ -1664,8 +1653,69 @@ out:
> [...]
>> +int clk_set_rate(struct clk *clk, unsigned long rate)
>> +{
>> + return clk_core_set_rate(clk->core, rate);
>> +}
>> EXPORT_SYMBOL_GPL(clk_set_rate);
>>
>
> What about clk_round_rate()? That needs to tell us what the rate
> will be if we call clk_set_rate() with whatever value is passed
> here.
My thinking was that clock consumers would be setting constraints and
calling set_rate regardless of any constraints that may be set at the
moment.
Though in my particular use case (scaling of the external memory bus)
it shouldn't really matter as the clock would be given a suitably low
frequency at startup and from that moment on only the constraints
would be updated.
> I would expect that the rate aggregation logic would be
> somewhere in that codepath but I don't see it.
I can certainly make round_rate aware of constraints, no problem.
Mike, what do you think?
>> +int clk_set_floor_rate(struct clk *clk, unsigned long rate)
>
> We need some documentation for these new functions. I see we have
> them in the header, maybe we can copy that here.
Ok.
>> +{
>> + 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.
>> + ret = clk_set_rate(clk, clk_get_rate(clk));
>> +
>> + clk_prepare_unlock();
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_floor_rate);
>> +
>> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate)
>> +{
>> + int ret;
>> +
>> + clk_prepare_lock();
>
> We don't need to grab the lock this early right? Presumably the
> caller is doing any required locking so they don't call different
> clk APIs for the same clk pointer at the same time. So we should
> be able to grab this lock after checking for this error
> condition.
Sounds good.
>> +
>> + 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.
>> +
>> + clk_prepare_unlock();
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
>> +
>> static struct clk_core *clk_core_get_parent(struct clk_core *clk)
>> {
>> struct clk_core *parent;
>>
Re: [PATCH v3 8/8] clk: Add floor and ceiling constraints to clock rates
On 11 October 2014 01:55, Stephen Boyd sb...@codeaurora.org wrote:
On 10/09, Tomeu Vizoso wrote:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4db918a..97cf1a3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1629,18 +1609,27 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
+ hlist_for_each_entry(clk_user, clk-per_user_clks, child_node) {
+ rate = max(rate, clk_user-floor_constraint);
+ }
+
+ hlist_for_each_entry(clk_user, clk-per_user_clks, child_node) {
+ if (clk_user-ceiling_constraint 0)
+ rate = min(rate, clk_user-ceiling_constraint);
+ }
+
/* bail early if nothing to do */
- if (rate == clk_get_rate(clk))
+ if (rate == clk_core_get_rate(clk))
Can we use the nolock variant here?
It's true that we have the lock already, but I'm not sure about the
implications of not calling __clk_recalc_rates. Nothing that cannot be
solved with one more function though...
As an aside, this is going to
make my per-clock locking series complicated. I'm not even sure
how the two series will work together. In the per-clock locking
series I was hoping we could check if rate == current rate
without holding any locks. Otherwise we get into a recursive lock
situation. Need to think more about that one.
Ok.
goto out;
- if ((clk-core-flags CLK_SET_RATE_GATE)
- clk-core-prepare_count) {
+ if ((clk-flags CLK_SET_RATE_GATE)
+ clk-prepare_count) {
ret = -EBUSY;
goto out;
}
/* calculate new rates and get the topmost changed clock */
- top = clk_calc_new_rates(clk-core, rate);
+ top = clk_calc_new_rates(clk, rate);
if (!top) {
ret = -EINVAL;
goto out;
@@ -1664,8 +1653,69 @@ out:
[...]
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ return clk_core_set_rate(clk-core, rate);
+}
EXPORT_SYMBOL_GPL(clk_set_rate);
What about clk_round_rate()? That needs to tell us what the rate
will be if we call clk_set_rate() with whatever value is passed
here.
My thinking was that clock consumers would be setting constraints and
calling set_rate regardless of any constraints that may be set at the
moment.
Though in my particular use case (scaling of the external memory bus)
it shouldn't really matter as the clock would be given a suitably low
frequency at startup and from that moment on only the constraints
would be updated.
I would expect that the rate aggregation logic would be
somewhere in that codepath but I don't see it.
I can certainly make round_rate aware of constraints, no problem.
Mike, what do you think?
+int clk_set_floor_rate(struct clk *clk, unsigned long rate)
We need some documentation for these new functions. I see we have
them in the header, maybe we can copy that here.
Ok.
+{
+ 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.
+ ret = clk_set_rate(clk, clk_get_rate(clk));
+
+ clk_prepare_unlock();
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_floor_rate);
+
+int clk_set_ceiling_rate(struct clk *clk, unsigned long rate)
+{
+ int ret;
+
+ clk_prepare_lock();
We don't need to grab the lock this early right? Presumably the
caller is doing any required locking so they don't call different
clk APIs for the same clk pointer at the same time. So we should
be able to grab this lock after checking for this error
condition.
Sounds good.
+
+ 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.
+
+ clk_prepare_unlock();
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
+
static struct clk_core *clk_core_get_parent(struct clk_core *clk)
{
struct clk_core *parent;
@@ -2143,6 +2195,12 @@ struct clk *__clk_register(struct device *dev, struct
clk_hw *hw)
}
EXPORT_SYMBOL_GPL(__clk_register);
+static void __clk_free_clk(struct clk *clk_user)
+{
+
Re: [PATCH v3 8/8] clk: Add floor and ceiling constraints to clock rates
On 10/09, Tomeu Vizoso wrote:
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 4db918a..97cf1a3 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1629,18 +1609,27 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
> /* prevent racing with updates to the clock topology */
> clk_prepare_lock();
>
> + hlist_for_each_entry(clk_user, >per_user_clks, child_node) {
> + rate = max(rate, clk_user->floor_constraint);
> + }
> +
> + hlist_for_each_entry(clk_user, >per_user_clks, child_node) {
> + if (clk_user->ceiling_constraint > 0)
> + rate = min(rate, clk_user->ceiling_constraint);
> + }
> +
> /* bail early if nothing to do */
> - if (rate == clk_get_rate(clk))
> + if (rate == clk_core_get_rate(clk))
Can we use the nolock variant here? As an aside, this is going to
make my per-clock locking series complicated. I'm not even sure
how the two series will work together. In the per-clock locking
series I was hoping we could check if rate == current rate
without holding any locks. Otherwise we get into a recursive lock
situation. Need to think more about that one.
> goto out;
>
> - if ((clk->core->flags & CLK_SET_RATE_GATE) &&
> - clk->core->prepare_count) {
> + if ((clk->flags & CLK_SET_RATE_GATE) &&
> + clk->prepare_count) {
> ret = -EBUSY;
> goto out;
> }
>
> /* calculate new rates and get the topmost changed clock */
> - top = clk_calc_new_rates(clk->core, rate);
> + top = clk_calc_new_rates(clk, rate);
> if (!top) {
> ret = -EINVAL;
> goto out;
> @@ -1664,8 +1653,69 @@ out:
[...]
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> + return clk_core_set_rate(clk->core, rate);
> +}
> EXPORT_SYMBOL_GPL(clk_set_rate);
>
What about clk_round_rate()? That needs to tell us what the rate
will be if we call clk_set_rate() with whatever value is passed
here. I would expect that the rate aggregation logic would be
somewhere in that codepath but I don't see it.
> +int clk_set_floor_rate(struct clk *clk, unsigned long rate)
We need some documentation for these new functions. I see we have
them in the header, maybe we can copy that here.
> +{
> + int ret;
> +
> + clk_prepare_lock();
> +
> + clk->floor_constraint = rate;
No check for ceiling being less than floor?
> + ret = clk_set_rate(clk, clk_get_rate(clk));
> +
> + clk_prepare_unlock();
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_floor_rate);
> +
> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate)
> +{
> + int ret;
> +
> + clk_prepare_lock();
We don't need to grab the lock this early right? Presumably the
caller is doing any required locking so they don't call different
clk APIs for the same clk pointer at the same time. So we should
be able to grab this lock after checking for this error
condition.
> +
> + 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.
> +
> + clk_prepare_unlock();
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
> +
> static struct clk_core *clk_core_get_parent(struct clk_core *clk)
> {
> struct clk_core *parent;
> @@ -2143,6 +2195,12 @@ struct clk *__clk_register(struct device *dev, struct
> clk_hw *hw)
> }
> EXPORT_SYMBOL_GPL(__clk_register);
>
> +static void __clk_free_clk(struct clk *clk_user)
> +{
> + hlist_del(_user->child_node);
> + kfree(clk_user);
We need to re-evaluate the rate when a user is removed, right?
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?
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.
> +}
> +
> /**
> * clk_register - allocate a new clock, register it and return an opaque
> cookie
> * @dev: device that
Re: [PATCH v3 8/8] clk: Add floor and ceiling constraints to clock rates
On 10/09, Tomeu Vizoso wrote:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4db918a..97cf1a3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1629,18 +1609,27 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
+ hlist_for_each_entry(clk_user, clk-per_user_clks, child_node) {
+ rate = max(rate, clk_user-floor_constraint);
+ }
+
+ hlist_for_each_entry(clk_user, clk-per_user_clks, child_node) {
+ if (clk_user-ceiling_constraint 0)
+ rate = min(rate, clk_user-ceiling_constraint);
+ }
+
/* bail early if nothing to do */
- if (rate == clk_get_rate(clk))
+ if (rate == clk_core_get_rate(clk))
Can we use the nolock variant here? As an aside, this is going to
make my per-clock locking series complicated. I'm not even sure
how the two series will work together. In the per-clock locking
series I was hoping we could check if rate == current rate
without holding any locks. Otherwise we get into a recursive lock
situation. Need to think more about that one.
goto out;
- if ((clk-core-flags CLK_SET_RATE_GATE)
- clk-core-prepare_count) {
+ if ((clk-flags CLK_SET_RATE_GATE)
+ clk-prepare_count) {
ret = -EBUSY;
goto out;
}
/* calculate new rates and get the topmost changed clock */
- top = clk_calc_new_rates(clk-core, rate);
+ top = clk_calc_new_rates(clk, rate);
if (!top) {
ret = -EINVAL;
goto out;
@@ -1664,8 +1653,69 @@ out:
[...]
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ return clk_core_set_rate(clk-core, rate);
+}
EXPORT_SYMBOL_GPL(clk_set_rate);
What about clk_round_rate()? That needs to tell us what the rate
will be if we call clk_set_rate() with whatever value is passed
here. I would expect that the rate aggregation logic would be
somewhere in that codepath but I don't see it.
+int clk_set_floor_rate(struct clk *clk, unsigned long rate)
We need some documentation for these new functions. I see we have
them in the header, maybe we can copy that here.
+{
+ int ret;
+
+ clk_prepare_lock();
+
+ clk-floor_constraint = rate;
No check for ceiling being less than floor?
+ ret = clk_set_rate(clk, clk_get_rate(clk));
+
+ clk_prepare_unlock();
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_floor_rate);
+
+int clk_set_ceiling_rate(struct clk *clk, unsigned long rate)
+{
+ int ret;
+
+ clk_prepare_lock();
We don't need to grab the lock this early right? Presumably the
caller is doing any required locking so they don't call different
clk APIs for the same clk pointer at the same time. So we should
be able to grab this lock after checking for this error
condition.
+
+ 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.
+
+ clk_prepare_unlock();
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
+
static struct clk_core *clk_core_get_parent(struct clk_core *clk)
{
struct clk_core *parent;
@@ -2143,6 +2195,12 @@ struct clk *__clk_register(struct device *dev, struct
clk_hw *hw)
}
EXPORT_SYMBOL_GPL(__clk_register);
+static void __clk_free_clk(struct clk *clk_user)
+{
+ hlist_del(clk_user-child_node);
+ kfree(clk_user);
We need to re-evaluate the rate when a user is removed, right?
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?
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.
+}
+
/**
* clk_register - allocate a new clock, register it and return an opaque
cookie
* @dev: device that is registering this clock
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index 1cdb727..025aca2
[PATCH v3 8/8] clk: Add floor and ceiling constraints to clock rates
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
---
drivers/clk/clk.c | 118 +---
include/linux/clk-private.h | 5 ++
include/linux/clk.h | 18 +++
3 files changed, 113 insertions(+), 28 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4db918a..97cf1a3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1597,30 +1597,10 @@ static void clk_change_rate(struct clk_core *clk)
clk_change_rate(clk->new_child);
}
-/**
- * clk_set_rate - specify a new rate for clk
- * @clk: the clk whose rate is being changed
- * @rate: the new rate for clk
- *
- * In the simplest case clk_set_rate will only adjust the rate of clk.
- *
- * Setting the CLK_SET_RATE_PARENT flag allows the rate change operation to
- * propagate up to clk's parent; whether or not this happens depends on the
- * outcome of clk's .round_rate implementation. If *parent_rate is unchanged
- * after calling .round_rate then upstream parent propagation is ignored. If
- * *parent_rate comes back with a new rate for clk's parent then we propagate
- * up to clk's parent and set its rate. Upward propagation will continue
- * until either a clk does not support the CLK_SET_RATE_PARENT flag or
- * .round_rate stops requesting changes to clk's parent_rate.
- *
- * Rate changes are accomplished via tree traversal that also recalculates the
- * rates for the clocks and fires off POST_RATE_CHANGE notifiers.
- *
- * Returns 0 on success, -EERROR otherwise.
- */
-int clk_set_rate(struct clk *clk, unsigned long rate)
+static int clk_core_set_rate(struct clk_core *clk, unsigned long rate)
{
struct clk_core *top, *fail_clk;
+ struct clk *clk_user;
int ret = 0;
if (!clk)
@@ -1629,18 +1609,27 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
+ hlist_for_each_entry(clk_user, >per_user_clks, child_node) {
+ rate = max(rate, clk_user->floor_constraint);
+ }
+
+ hlist_for_each_entry(clk_user, >per_user_clks, child_node) {
+ if (clk_user->ceiling_constraint > 0)
+ rate = min(rate, clk_user->ceiling_constraint);
+ }
+
/* bail early if nothing to do */
- if (rate == clk_get_rate(clk))
+ if (rate == clk_core_get_rate(clk))
goto out;
- if ((clk->core->flags & CLK_SET_RATE_GATE) &&
- clk->core->prepare_count) {
+ if ((clk->flags & CLK_SET_RATE_GATE) &&
+ clk->prepare_count) {
ret = -EBUSY;
goto out;
}
/* calculate new rates and get the topmost changed clock */
- top = clk_calc_new_rates(clk->core, rate);
+ top = clk_calc_new_rates(clk, rate);
if (!top) {
ret = -EINVAL;
goto out;
@@ -1664,8 +1653,69 @@ out:
return ret;
}
+
+/**
+ * clk_set_rate - specify a new rate for clk
+ * @clk: the clk whose rate is being changed
+ * @rate: the new rate for clk
+ *
+ * In the simplest case clk_set_rate will only adjust the rate of clk.
+ *
+ * Setting the CLK_SET_RATE_PARENT flag allows the rate change operation to
+ * propagate up to clk's parent; whether or not this happens depends on the
+ * outcome of clk's .round_rate implementation. If *parent_rate is unchanged
+ * after calling .round_rate then upstream parent propagation is ignored. If
+ * *parent_rate comes back with a new rate for clk's parent then we propagate
+ * up to clk's parent and set its rate. Upward propagation will continue
+ * until either a clk does not support the CLK_SET_RATE_PARENT flag or
+ * .round_rate stops requesting changes to clk's parent_rate.
+ *
+ * Rate changes are accomplished via tree traversal that also recalculates the
+ * rates for the clocks and fires off POST_RATE_CHANGE notifiers.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ return clk_core_set_rate(clk->core, rate);
+}
EXPORT_SYMBOL_GPL(clk_set_rate);
+int clk_set_floor_rate(struct clk *clk, unsigned long rate)
+{
+ int ret;
+
+ clk_prepare_lock();
+
+ clk->floor_constraint = rate;
+ ret = clk_set_rate(clk, clk_get_rate(clk));
+
+ clk_prepare_unlock();
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_floor_rate);
+
+int clk_set_ceiling_rate(struct clk *clk, unsigned long rate)
+{
+ int ret;
+
+ clk_prepare_lock();
+
+ 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,
+
[PATCH v3 8/8] clk: Add floor and ceiling constraints to clock rates
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.viz...@collabora.com
---
drivers/clk/clk.c | 118 +---
include/linux/clk-private.h | 5 ++
include/linux/clk.h | 18 +++
3 files changed, 113 insertions(+), 28 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4db918a..97cf1a3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1597,30 +1597,10 @@ static void clk_change_rate(struct clk_core *clk)
clk_change_rate(clk-new_child);
}
-/**
- * clk_set_rate - specify a new rate for clk
- * @clk: the clk whose rate is being changed
- * @rate: the new rate for clk
- *
- * In the simplest case clk_set_rate will only adjust the rate of clk.
- *
- * Setting the CLK_SET_RATE_PARENT flag allows the rate change operation to
- * propagate up to clk's parent; whether or not this happens depends on the
- * outcome of clk's .round_rate implementation. If *parent_rate is unchanged
- * after calling .round_rate then upstream parent propagation is ignored. If
- * *parent_rate comes back with a new rate for clk's parent then we propagate
- * up to clk's parent and set its rate. Upward propagation will continue
- * until either a clk does not support the CLK_SET_RATE_PARENT flag or
- * .round_rate stops requesting changes to clk's parent_rate.
- *
- * Rate changes are accomplished via tree traversal that also recalculates the
- * rates for the clocks and fires off POST_RATE_CHANGE notifiers.
- *
- * Returns 0 on success, -EERROR otherwise.
- */
-int clk_set_rate(struct clk *clk, unsigned long rate)
+static int clk_core_set_rate(struct clk_core *clk, unsigned long rate)
{
struct clk_core *top, *fail_clk;
+ struct clk *clk_user;
int ret = 0;
if (!clk)
@@ -1629,18 +1609,27 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
+ hlist_for_each_entry(clk_user, clk-per_user_clks, child_node) {
+ rate = max(rate, clk_user-floor_constraint);
+ }
+
+ hlist_for_each_entry(clk_user, clk-per_user_clks, child_node) {
+ if (clk_user-ceiling_constraint 0)
+ rate = min(rate, clk_user-ceiling_constraint);
+ }
+
/* bail early if nothing to do */
- if (rate == clk_get_rate(clk))
+ if (rate == clk_core_get_rate(clk))
goto out;
- if ((clk-core-flags CLK_SET_RATE_GATE)
- clk-core-prepare_count) {
+ if ((clk-flags CLK_SET_RATE_GATE)
+ clk-prepare_count) {
ret = -EBUSY;
goto out;
}
/* calculate new rates and get the topmost changed clock */
- top = clk_calc_new_rates(clk-core, rate);
+ top = clk_calc_new_rates(clk, rate);
if (!top) {
ret = -EINVAL;
goto out;
@@ -1664,8 +1653,69 @@ out:
return ret;
}
+
+/**
+ * clk_set_rate - specify a new rate for clk
+ * @clk: the clk whose rate is being changed
+ * @rate: the new rate for clk
+ *
+ * In the simplest case clk_set_rate will only adjust the rate of clk.
+ *
+ * Setting the CLK_SET_RATE_PARENT flag allows the rate change operation to
+ * propagate up to clk's parent; whether or not this happens depends on the
+ * outcome of clk's .round_rate implementation. If *parent_rate is unchanged
+ * after calling .round_rate then upstream parent propagation is ignored. If
+ * *parent_rate comes back with a new rate for clk's parent then we propagate
+ * up to clk's parent and set its rate. Upward propagation will continue
+ * until either a clk does not support the CLK_SET_RATE_PARENT flag or
+ * .round_rate stops requesting changes to clk's parent_rate.
+ *
+ * Rate changes are accomplished via tree traversal that also recalculates the
+ * rates for the clocks and fires off POST_RATE_CHANGE notifiers.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ return clk_core_set_rate(clk-core, rate);
+}
EXPORT_SYMBOL_GPL(clk_set_rate);
+int clk_set_floor_rate(struct clk *clk, unsigned long rate)
+{
+ int ret;
+
+ clk_prepare_lock();
+
+ clk-floor_constraint = rate;
+ ret = clk_set_rate(clk, clk_get_rate(clk));
+
+ clk_prepare_unlock();
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_set_floor_rate);
+
+int clk_set_ceiling_rate(struct clk *clk, unsigned long rate)
+{
+ int ret;
+
+ clk_prepare_lock();
+
+ 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,
+
