Re: [PATCH v5 7/7] clk: Add floor and ceiling constraints to clock rates
On 22 November 2014 at 03:35, Stephen Boyd wrote:
> On 11/18/2014 08:31 AM, Tomeu Vizoso wrote:
>> On 14 November 2014 08:50, Stephen Boyd wrote:
>>> It's
>>> possible that whatever is constrained at this user level goes
>>> down to the hardware driver and then is rounded up or down to a
>>> value that is outside of the constraints, in which case the
>>> constraints did nothing besides control the value that the
>>> hardware driver sees in the .round_rate() op. I doubt that was
>>> intended.
>> Indeed. Wonder what can be done about it with the least impact on
>> existing code. I see the situation as clk implementations being able
>> to apply arbitrary constraints in determine_rate() and round_rate(),
>> and they would need to take into account the per-user constraints so
>> they can all be applied consistently.
>
> I was thinking that we put the loop over .round_rate() in the clock
> framework, but you're right, we should provide the limits to the
> hardware drivers via the ops so that they can figure out the acceptable
> rate within whatever bounds the framework is maintaining. Given that
> we're changing the signature of determine_rate() in this series perhaps
> we should also add the floor/ceiling rates in there too. Then we can
> hope that we've finally gotten that new op right and set it in stone and
> migrate everyone over to .determine_rate() instead of .round_rate().
Sounds good, I'm implementing this in the CCF but leaving the
determine_rate implementations as they are for now.
>>> I also wonder what we should do about clocks that are in the
>>> middle of the tree (i.e. not a leaf) and have constraints set on
>>> them. It looks like if a child of that clock can propagate rates
>>> up to the parent that we've constrained with the per-user
>>> constraints, those constraints won't be respected at all, leading
>>> to a hole in the constraints.
>> True. Do we want to support per-user constraints on non-leaf clocks?
>
> I have an mmc clock rate where it would be helpful.
Ok, I have done some testing and the next revision should support this.
>>> I'm all for having a clk_set_rate_range() API (and floor/ceiling
>>> too), but it needs to be done much deeper in the core framework
>>> to actually work properly. Having a range API would make all the
>>> confusion about how a particular clock driver decides to round a
>>> rate go away and just leave an API that sanely says the rate will
>>> be within these bounds or an error will be returned stating it
>>> can't be satisfied.
>> This sounds like a good way forward, but TBH I don't understand what
>> you are proposing. Would you care to elaborate on how the API that you
>> propose would look like?
>>
>
> clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max);
>
> clk_set_floor(struct clk *clk, unsigned long floor)
> {
> return clk_set_rate_range(clk, floor, ULONG_MAX);
> }
I have actually gone with:
return clk_set_rate_range(clk, rate, clk->ceiling_constraint);
Because I think that setting a floor shouldn't remove the existing
ceiling as a side effect.
> clk_set_ceiling(struct clk *clk, unsigned long ceiling)
> {
> return clk_set_rate_range(clk, 0, ceiling);
> }
Ditto.
> Unfortunately we can't make clk_set_rate() a thin wrapper on top that
> says min/max is the same as the requested rate because that would
> horribly break current users of the API. I suppose we could call
> clk_round_rate() and then clk_set_rate_range() with the floor as the
> rounded rate and the ceiling as ULONG_MAX? Or maybe floor is 0 and
> ceiling is rounded rate, not sure it actually matters.
>
> clk_set_rate(struct clk *clk, unsigned long rate)
> {
> unsigned long rounded;
>
> rounded = clk_round_rate(clk, rate);
> return clk_set_rate_range(rounded, ULONG_MAX);
> }
I'm not fond of this because the rate is a quality of the clk_core
while constraints are per-user.
> Now we can get down to the real work. __clk_round_rate() will apply the
> constraints. It will also send down the constraints to .determine_rate()
> ops
Cool, this works quite well here.
> and throw errors if things don't work out (ugh we may need to return
> the rate by reference so we can return unsigned long rates here or use
> IS_ERR_VALUE() on the return value).
What errors were you thinking of? I was thinking that we are just
adding two more constraints to the existing set, so we aren't adding
any new error conditions.
> In the case that clk_round_rate()
> is calling this function it will need to know that we don't want any
> constraints applied, so it will need to take min, max, and rate and
> clk_round_rate() will just pass 0, ULONG_MAX, and rate respectfully.
Ok.
> Next clk_set_rate() will be renamed to clk_set_rate_range() (in clk.c)
> and then it will pass the constraints to clk_calc_new_rates(). We can
> also try to bail out early here by checking the constraints against the
> current rate to make sure it's within bounds.
As mentioned before, I haven't
Re: [PATCH v5 7/7] clk: Add floor and ceiling constraints to clock rates
On 22 November 2014 at 03:35, Stephen Boyd sb...@codeaurora.org wrote:
On 11/18/2014 08:31 AM, Tomeu Vizoso wrote:
On 14 November 2014 08:50, Stephen Boyd sb...@codeaurora.org wrote:
It's
possible that whatever is constrained at this user level goes
down to the hardware driver and then is rounded up or down to a
value that is outside of the constraints, in which case the
constraints did nothing besides control the value that the
hardware driver sees in the .round_rate() op. I doubt that was
intended.
Indeed. Wonder what can be done about it with the least impact on
existing code. I see the situation as clk implementations being able
to apply arbitrary constraints in determine_rate() and round_rate(),
and they would need to take into account the per-user constraints so
they can all be applied consistently.
I was thinking that we put the loop over .round_rate() in the clock
framework, but you're right, we should provide the limits to the
hardware drivers via the ops so that they can figure out the acceptable
rate within whatever bounds the framework is maintaining. Given that
we're changing the signature of determine_rate() in this series perhaps
we should also add the floor/ceiling rates in there too. Then we can
hope that we've finally gotten that new op right and set it in stone and
migrate everyone over to .determine_rate() instead of .round_rate().
Sounds good, I'm implementing this in the CCF but leaving the
determine_rate implementations as they are for now.
I also wonder what we should do about clocks that are in the
middle of the tree (i.e. not a leaf) and have constraints set on
them. It looks like if a child of that clock can propagate rates
up to the parent that we've constrained with the per-user
constraints, those constraints won't be respected at all, leading
to a hole in the constraints.
True. Do we want to support per-user constraints on non-leaf clocks?
I have an mmc clock rate where it would be helpful.
Ok, I have done some testing and the next revision should support this.
I'm all for having a clk_set_rate_range() API (and floor/ceiling
too), but it needs to be done much deeper in the core framework
to actually work properly. Having a range API would make all the
confusion about how a particular clock driver decides to round a
rate go away and just leave an API that sanely says the rate will
be within these bounds or an error will be returned stating it
can't be satisfied.
This sounds like a good way forward, but TBH I don't understand what
you are proposing. Would you care to elaborate on how the API that you
propose would look like?
clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max);
clk_set_floor(struct clk *clk, unsigned long floor)
{
return clk_set_rate_range(clk, floor, ULONG_MAX);
}
I have actually gone with:
return clk_set_rate_range(clk, rate, clk-ceiling_constraint);
Because I think that setting a floor shouldn't remove the existing
ceiling as a side effect.
clk_set_ceiling(struct clk *clk, unsigned long ceiling)
{
return clk_set_rate_range(clk, 0, ceiling);
}
Ditto.
Unfortunately we can't make clk_set_rate() a thin wrapper on top that
says min/max is the same as the requested rate because that would
horribly break current users of the API. I suppose we could call
clk_round_rate() and then clk_set_rate_range() with the floor as the
rounded rate and the ceiling as ULONG_MAX? Or maybe floor is 0 and
ceiling is rounded rate, not sure it actually matters.
clk_set_rate(struct clk *clk, unsigned long rate)
{
unsigned long rounded;
rounded = clk_round_rate(clk, rate);
return clk_set_rate_range(rounded, ULONG_MAX);
}
I'm not fond of this because the rate is a quality of the clk_core
while constraints are per-user.
Now we can get down to the real work. __clk_round_rate() will apply the
constraints. It will also send down the constraints to .determine_rate()
ops
Cool, this works quite well here.
and throw errors if things don't work out (ugh we may need to return
the rate by reference so we can return unsigned long rates here or use
IS_ERR_VALUE() on the return value).
What errors were you thinking of? I was thinking that we are just
adding two more constraints to the existing set, so we aren't adding
any new error conditions.
In the case that clk_round_rate()
is calling this function it will need to know that we don't want any
constraints applied, so it will need to take min, max, and rate and
clk_round_rate() will just pass 0, ULONG_MAX, and rate respectfully.
Ok.
Next clk_set_rate() will be renamed to clk_set_rate_range() (in clk.c)
and then it will pass the constraints to clk_calc_new_rates(). We can
also try to bail out early here by checking the constraints against the
current rate to make sure it's within bounds.
As mentioned before, I haven't gone with this. A use case I have in
mind is a thermal driver throttling the external memory
Re: [PATCH v5 7/7] clk: Add floor and ceiling constraints to clock rates
On 11/18/2014 08:31 AM, Tomeu Vizoso wrote:
> On 14 November 2014 08:50, Stephen Boyd wrote:
>> It's
>> possible that whatever is constrained at this user level goes
>> down to the hardware driver and then is rounded up or down to a
>> value that is outside of the constraints, in which case the
>> constraints did nothing besides control the value that the
>> hardware driver sees in the .round_rate() op. I doubt that was
>> intended.
> Indeed. Wonder what can be done about it with the least impact on
> existing code. I see the situation as clk implementations being able
> to apply arbitrary constraints in determine_rate() and round_rate(),
> and they would need to take into account the per-user constraints so
> they can all be applied consistently.
I was thinking that we put the loop over .round_rate() in the clock
framework, but you're right, we should provide the limits to the
hardware drivers via the ops so that they can figure out the acceptable
rate within whatever bounds the framework is maintaining. Given that
we're changing the signature of determine_rate() in this series perhaps
we should also add the floor/ceiling rates in there too. Then we can
hope that we've finally gotten that new op right and set it in stone and
migrate everyone over to .determine_rate() instead of .round_rate().
>
>> I also wonder what we should do about clocks that are in the
>> middle of the tree (i.e. not a leaf) and have constraints set on
>> them. It looks like if a child of that clock can propagate rates
>> up to the parent that we've constrained with the per-user
>> constraints, those constraints won't be respected at all, leading
>> to a hole in the constraints.
> True. Do we want to support per-user constraints on non-leaf clocks?
I have an mmc clock rate where it would be helpful.
>> I'm all for having a clk_set_rate_range() API (and floor/ceiling
>> too), but it needs to be done much deeper in the core framework
>> to actually work properly. Having a range API would make all the
>> confusion about how a particular clock driver decides to round a
>> rate go away and just leave an API that sanely says the rate will
>> be within these bounds or an error will be returned stating it
>> can't be satisfied.
> This sounds like a good way forward, but TBH I don't understand what
> you are proposing. Would you care to elaborate on how the API that you
> propose would look like?
>
clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max);
clk_set_floor(struct clk *clk, unsigned long floor)
{
return clk_set_rate_range(clk, floor, ULONG_MAX);
}
clk_set_ceiling(struct clk *clk, unsigned long ceiling)
{
return clk_set_rate_range(clk, 0, ceiling);
}
Unfortunately we can't make clk_set_rate() a thin wrapper on top that
says min/max is the same as the requested rate because that would
horribly break current users of the API. I suppose we could call
clk_round_rate() and then clk_set_rate_range() with the floor as the
rounded rate and the ceiling as ULONG_MAX? Or maybe floor is 0 and
ceiling is rounded rate, not sure it actually matters.
clk_set_rate(struct clk *clk, unsigned long rate)
{
unsigned long rounded;
rounded = clk_round_rate(clk, rate);
return clk_set_rate_range(rounded, ULONG_MAX);
}
Now we can get down to the real work. __clk_round_rate() will apply the
constraints. It will also send down the constraints to .determine_rate()
ops and throw errors if things don't work out (ugh we may need to return
the rate by reference so we can return unsigned long rates here or use
IS_ERR_VALUE() on the return value). In the case that clk_round_rate()
is calling this function it will need to know that we don't want any
constraints applied, so it will need to take min, max, and rate and
clk_round_rate() will just pass 0, ULONG_MAX, and rate respectfully.
Next clk_set_rate() will be renamed to clk_set_rate_range() (in clk.c)
and then it will pass the constraints to clk_calc_new_rates(). We can
also try to bail out early here by checking the constraints against the
current rate to make sure it's within bounds. We can probably redo
clk_calc_new_rates() to be similar to __clk_round_rate() and apply any
constraints that are passed into the function along with any per-user
constraints that already exist.
Did I miss anything?
--
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 v5 7/7] clk: Add floor and ceiling constraints to clock rates
On 11/18/2014 08:31 AM, Tomeu Vizoso wrote:
On 14 November 2014 08:50, Stephen Boyd sb...@codeaurora.org wrote:
It's
possible that whatever is constrained at this user level goes
down to the hardware driver and then is rounded up or down to a
value that is outside of the constraints, in which case the
constraints did nothing besides control the value that the
hardware driver sees in the .round_rate() op. I doubt that was
intended.
Indeed. Wonder what can be done about it with the least impact on
existing code. I see the situation as clk implementations being able
to apply arbitrary constraints in determine_rate() and round_rate(),
and they would need to take into account the per-user constraints so
they can all be applied consistently.
I was thinking that we put the loop over .round_rate() in the clock
framework, but you're right, we should provide the limits to the
hardware drivers via the ops so that they can figure out the acceptable
rate within whatever bounds the framework is maintaining. Given that
we're changing the signature of determine_rate() in this series perhaps
we should also add the floor/ceiling rates in there too. Then we can
hope that we've finally gotten that new op right and set it in stone and
migrate everyone over to .determine_rate() instead of .round_rate().
I also wonder what we should do about clocks that are in the
middle of the tree (i.e. not a leaf) and have constraints set on
them. It looks like if a child of that clock can propagate rates
up to the parent that we've constrained with the per-user
constraints, those constraints won't be respected at all, leading
to a hole in the constraints.
True. Do we want to support per-user constraints on non-leaf clocks?
I have an mmc clock rate where it would be helpful.
I'm all for having a clk_set_rate_range() API (and floor/ceiling
too), but it needs to be done much deeper in the core framework
to actually work properly. Having a range API would make all the
confusion about how a particular clock driver decides to round a
rate go away and just leave an API that sanely says the rate will
be within these bounds or an error will be returned stating it
can't be satisfied.
This sounds like a good way forward, but TBH I don't understand what
you are proposing. Would you care to elaborate on how the API that you
propose would look like?
clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max);
clk_set_floor(struct clk *clk, unsigned long floor)
{
return clk_set_rate_range(clk, floor, ULONG_MAX);
}
clk_set_ceiling(struct clk *clk, unsigned long ceiling)
{
return clk_set_rate_range(clk, 0, ceiling);
}
Unfortunately we can't make clk_set_rate() a thin wrapper on top that
says min/max is the same as the requested rate because that would
horribly break current users of the API. I suppose we could call
clk_round_rate() and then clk_set_rate_range() with the floor as the
rounded rate and the ceiling as ULONG_MAX? Or maybe floor is 0 and
ceiling is rounded rate, not sure it actually matters.
clk_set_rate(struct clk *clk, unsigned long rate)
{
unsigned long rounded;
rounded = clk_round_rate(clk, rate);
return clk_set_rate_range(rounded, ULONG_MAX);
}
Now we can get down to the real work. __clk_round_rate() will apply the
constraints. It will also send down the constraints to .determine_rate()
ops and throw errors if things don't work out (ugh we may need to return
the rate by reference so we can return unsigned long rates here or use
IS_ERR_VALUE() on the return value). In the case that clk_round_rate()
is calling this function it will need to know that we don't want any
constraints applied, so it will need to take min, max, and rate and
clk_round_rate() will just pass 0, ULONG_MAX, and rate respectfully.
Next clk_set_rate() will be renamed to clk_set_rate_range() (in clk.c)
and then it will pass the constraints to clk_calc_new_rates(). We can
also try to bail out early here by checking the constraints against the
current rate to make sure it's within bounds. We can probably redo
clk_calc_new_rates() to be similar to __clk_round_rate() and apply any
constraints that are passed into the function along with any per-user
constraints that already exist.
Did I miss anything?
--
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 v5 7/7] clk: Add floor and ceiling constraints to clock rates
On 14 November 2014 08:50, Stephen Boyd wrote:
> On 10/30, Tomeu Vizoso wrote:
>> @@ -2145,6 +2218,16 @@ struct clk *__clk_register(struct device *dev, struct
>> clk_hw *hw)
>> }
>> EXPORT_SYMBOL_GPL(__clk_register);
>>
>> +static void __clk_free_clk(struct clk *clk)
>> +{
>> + struct clk_core *core = clk->core;
>> +
>> + hlist_del(>child_node);
>
> Does this race with aggregation in clk_core_set_rate()? I would think
> we need to hold the prepare lock here so we don't traverse the list
> while it's being modified?
Yes, thanks.
>> + kfree(clk);
>> +
>> + clk_core_set_rate(core, core->req_rate);
>> +}
>> +
>> /**
>> * 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.h b/include/linux/clk.h
>> index c7f258a..0d55570 100644
>> --- a/include/linux/clk.h
>> +++ b/include/linux/clk.h
>> @@ -302,6 +302,24 @@ long clk_round_rate(struct clk *clk, unsigned long
>> rate);
>> int clk_set_rate(struct clk *clk, unsigned long rate);
>>
>> /**
>> + * clk_set_floor_rate - set a minimum clock rate for a clock source
>> + * @clk: clock source
>> + * @rate: desired minimum clock rate in Hz
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_floor_rate(struct clk *clk, unsigned long rate);
>> +
>> +/**
>> + * clk_set_ceiling_rate - set a maximum clock rate for a clock source
>> + * @clk: clock source
>> + * @rate: desired maximum clock rate in Hz
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate);
>> +
>
> I still don't see anything to do with clk_round_rate()?
Yeah sorry, i don't really have any good ideas, and was kind of hoping
that Mike would comment.
> It's
> possible that whatever is constrained at this user level goes
> down to the hardware driver and then is rounded up or down to a
> value that is outside of the constraints, in which case the
> constraints did nothing besides control the value that the
> hardware driver sees in the .round_rate() op. I doubt that was
> intended.
Indeed. Wonder what can be done about it with the least impact on
existing code. I see the situation as clk implementations being able
to apply arbitrary constraints in determine_rate() and round_rate(),
and they would need to take into account the per-user constraints so
they can all be applied consistently.
> I also wonder what we should do about clocks that are in the
> middle of the tree (i.e. not a leaf) and have constraints set on
> them. It looks like if a child of that clock can propagate rates
> up to the parent that we've constrained with the per-user
> constraints, those constraints won't be respected at all, leading
> to a hole in the constraints.
True. Do we want to support per-user constraints on non-leaf clocks?
> I imagine both of these points don't matter to the emc clock
> scaling patch
That's right.
> (BTW is there some pointer to that and the usage of
> these APIs?)
There's the proposed ACTMON driver, that currently is the solely user
of the EMC clock, so it's using clk_set_rate for now:
http://thread.gmane.org/gmane.linux.kernel/1816846/focus=1826037
In the future, it would set a floor constraint, and drivers such as
thermal and battery (when we have them) would set ceiling rates.
This is the last version of the EMC clock for Tegra124:
https://lkml.org/lkml/2014/11/18/272
> because that is only dealing with a leaf clock that
> doesn't care about clk_set_rate() being used along with
> constraints and the rounding behavior doesn't violate a floor?
>
> I'm all for having a clk_set_rate_range() API (and floor/ceiling
> too), but it needs to be done much deeper in the core framework
> to actually work properly. Having a range API would make all the
> confusion about how a particular clock driver decides to round a
> rate go away and just leave an API that sanely says the rate will
> be within these bounds or an error will be returned stating it
> can't be satisfied.
This sounds like a good way forward, but TBH I don't understand what
you are proposing. Would you care to elaborate on how the API that you
propose would look like?
Thanks,
Tomeu
> This would be useful so we don't have a bunch
> of drivers littered with code that loops on clk_round_rate() to
> figure out what their hardware actually supports or having some
> hard-coded frequency table per driver because the hardware can't
> generate some frequency that's part of a spec (but still lies
> within some acceptable tolerance!). It would also make
> clk_round_rate() mostly obsolete because we know the rate is
> within whatever acceptable bounds we've chosen. Eventually
> clk_set_rate() could be become a small wrapper on top of
> clk_set_rate_range(), constraining the rate to be exactly
> whatever the clock driver returns as the rounded rate.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora
Re: [PATCH v5 7/7] clk: Add floor and ceiling constraints to clock rates
On 14 November 2014 08:50, Stephen Boyd sb...@codeaurora.org wrote:
On 10/30, Tomeu Vizoso wrote:
@@ -2145,6 +2218,16 @@ struct clk *__clk_register(struct device *dev, struct
clk_hw *hw)
}
EXPORT_SYMBOL_GPL(__clk_register);
+static void __clk_free_clk(struct clk *clk)
+{
+ struct clk_core *core = clk-core;
+
+ hlist_del(clk-child_node);
Does this race with aggregation in clk_core_set_rate()? I would think
we need to hold the prepare lock here so we don't traverse the list
while it's being modified?
Yes, thanks.
+ kfree(clk);
+
+ clk_core_set_rate(core, core-req_rate);
+}
+
/**
* 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.h b/include/linux/clk.h
index c7f258a..0d55570 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -302,6 +302,24 @@ long clk_round_rate(struct clk *clk, unsigned long
rate);
int clk_set_rate(struct clk *clk, unsigned long rate);
/**
+ * clk_set_floor_rate - set a minimum clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired minimum clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_floor_rate(struct clk *clk, unsigned long rate);
+
+/**
+ * clk_set_ceiling_rate - set a maximum clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired maximum clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_ceiling_rate(struct clk *clk, unsigned long rate);
+
I still don't see anything to do with clk_round_rate()?
Yeah sorry, i don't really have any good ideas, and was kind of hoping
that Mike would comment.
It's
possible that whatever is constrained at this user level goes
down to the hardware driver and then is rounded up or down to a
value that is outside of the constraints, in which case the
constraints did nothing besides control the value that the
hardware driver sees in the .round_rate() op. I doubt that was
intended.
Indeed. Wonder what can be done about it with the least impact on
existing code. I see the situation as clk implementations being able
to apply arbitrary constraints in determine_rate() and round_rate(),
and they would need to take into account the per-user constraints so
they can all be applied consistently.
I also wonder what we should do about clocks that are in the
middle of the tree (i.e. not a leaf) and have constraints set on
them. It looks like if a child of that clock can propagate rates
up to the parent that we've constrained with the per-user
constraints, those constraints won't be respected at all, leading
to a hole in the constraints.
True. Do we want to support per-user constraints on non-leaf clocks?
I imagine both of these points don't matter to the emc clock
scaling patch
That's right.
(BTW is there some pointer to that and the usage of
these APIs?)
There's the proposed ACTMON driver, that currently is the solely user
of the EMC clock, so it's using clk_set_rate for now:
http://thread.gmane.org/gmane.linux.kernel/1816846/focus=1826037
In the future, it would set a floor constraint, and drivers such as
thermal and battery (when we have them) would set ceiling rates.
This is the last version of the EMC clock for Tegra124:
https://lkml.org/lkml/2014/11/18/272
because that is only dealing with a leaf clock that
doesn't care about clk_set_rate() being used along with
constraints and the rounding behavior doesn't violate a floor?
I'm all for having a clk_set_rate_range() API (and floor/ceiling
too), but it needs to be done much deeper in the core framework
to actually work properly. Having a range API would make all the
confusion about how a particular clock driver decides to round a
rate go away and just leave an API that sanely says the rate will
be within these bounds or an error will be returned stating it
can't be satisfied.
This sounds like a good way forward, but TBH I don't understand what
you are proposing. Would you care to elaborate on how the API that you
propose would look like?
Thanks,
Tomeu
This would be useful so we don't have a bunch
of drivers littered with code that loops on clk_round_rate() to
figure out what their hardware actually supports or having some
hard-coded frequency table per driver because the hardware can't
generate some frequency that's part of a spec (but still lies
within some acceptable tolerance!). It would also make
clk_round_rate() mostly obsolete because we know the rate is
within whatever acceptable bounds we've chosen. Eventually
clk_set_rate() could be become a small wrapper on top of
clk_set_rate_range(), constraining the rate to be exactly
whatever the clock driver returns as the rounded rate.
--
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
Re: [PATCH v5 7/7] clk: Add floor and ceiling constraints to clock rates
On 10/30, Tomeu Vizoso wrote:
> @@ -2145,6 +2218,16 @@ struct clk *__clk_register(struct device *dev, struct
> clk_hw *hw)
> }
> EXPORT_SYMBOL_GPL(__clk_register);
>
> +static void __clk_free_clk(struct clk *clk)
> +{
> + struct clk_core *core = clk->core;
> +
> + hlist_del(>child_node);
Does this race with aggregation in clk_core_set_rate()? I would think
we need to hold the prepare lock here so we don't traverse the list
while it's being modified?
> + kfree(clk);
> +
> + clk_core_set_rate(core, core->req_rate);
> +}
> +
> /**
> * 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.h b/include/linux/clk.h
> index c7f258a..0d55570 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -302,6 +302,24 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
> int clk_set_rate(struct clk *clk, unsigned long rate);
>
> /**
> + * clk_set_floor_rate - set a minimum clock rate for a clock source
> + * @clk: clock source
> + * @rate: desired minimum clock rate in Hz
> + *
> + * Returns success (0) or negative errno.
> + */
> +int clk_set_floor_rate(struct clk *clk, unsigned long rate);
> +
> +/**
> + * clk_set_ceiling_rate - set a maximum clock rate for a clock source
> + * @clk: clock source
> + * @rate: desired maximum clock rate in Hz
> + *
> + * Returns success (0) or negative errno.
> + */
> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate);
> +
I still don't see anything to do with clk_round_rate()? It's
possible that whatever is constrained at this user level goes
down to the hardware driver and then is rounded up or down to a
value that is outside of the constraints, in which case the
constraints did nothing besides control the value that the
hardware driver sees in the .round_rate() op. I doubt that was
intended.
I also wonder what we should do about clocks that are in the
middle of the tree (i.e. not a leaf) and have constraints set on
them. It looks like if a child of that clock can propagate rates
up to the parent that we've constrained with the per-user
constraints, those constraints won't be respected at all, leading
to a hole in the constraints.
I imagine both of these points don't matter to the emc clock
scaling patch (BTW is there some pointer to that and the usage of
these APIs?) because that is only dealing with a leaf clock that
doesn't care about clk_set_rate() being used along with
constraints and the rounding behavior doesn't violate a floor?
I'm all for having a clk_set_rate_range() API (and floor/ceiling
too), but it needs to be done much deeper in the core framework
to actually work properly. Having a range API would make all the
confusion about how a particular clock driver decides to round a
rate go away and just leave an API that sanely says the rate will
be within these bounds or an error will be returned stating it
can't be satisfied. This would be useful so we don't have a bunch
of drivers littered with code that loops on clk_round_rate() to
figure out what their hardware actually supports or having some
hard-coded frequency table per driver because the hardware can't
generate some frequency that's part of a spec (but still lies
within some acceptable tolerance!). It would also make
clk_round_rate() mostly obsolete because we know the rate is
within whatever acceptable bounds we've chosen. Eventually
clk_set_rate() could be become a small wrapper on top of
clk_set_rate_range(), constraining the rate to be exactly
whatever the clock driver returns as the rounded rate.
--
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 v5 7/7] clk: Add floor and ceiling constraints to clock rates
On 10/30, Tomeu Vizoso wrote:
@@ -2145,6 +2218,16 @@ struct clk *__clk_register(struct device *dev, struct
clk_hw *hw)
}
EXPORT_SYMBOL_GPL(__clk_register);
+static void __clk_free_clk(struct clk *clk)
+{
+ struct clk_core *core = clk-core;
+
+ hlist_del(clk-child_node);
Does this race with aggregation in clk_core_set_rate()? I would think
we need to hold the prepare lock here so we don't traverse the list
while it's being modified?
+ kfree(clk);
+
+ clk_core_set_rate(core, core-req_rate);
+}
+
/**
* 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.h b/include/linux/clk.h
index c7f258a..0d55570 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -302,6 +302,24 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
int clk_set_rate(struct clk *clk, unsigned long rate);
/**
+ * clk_set_floor_rate - set a minimum clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired minimum clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_floor_rate(struct clk *clk, unsigned long rate);
+
+/**
+ * clk_set_ceiling_rate - set a maximum clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired maximum clock rate in Hz
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_ceiling_rate(struct clk *clk, unsigned long rate);
+
I still don't see anything to do with clk_round_rate()? It's
possible that whatever is constrained at this user level goes
down to the hardware driver and then is rounded up or down to a
value that is outside of the constraints, in which case the
constraints did nothing besides control the value that the
hardware driver sees in the .round_rate() op. I doubt that was
intended.
I also wonder what we should do about clocks that are in the
middle of the tree (i.e. not a leaf) and have constraints set on
them. It looks like if a child of that clock can propagate rates
up to the parent that we've constrained with the per-user
constraints, those constraints won't be respected at all, leading
to a hole in the constraints.
I imagine both of these points don't matter to the emc clock
scaling patch (BTW is there some pointer to that and the usage of
these APIs?) because that is only dealing with a leaf clock that
doesn't care about clk_set_rate() being used along with
constraints and the rounding behavior doesn't violate a floor?
I'm all for having a clk_set_rate_range() API (and floor/ceiling
too), but it needs to be done much deeper in the core framework
to actually work properly. Having a range API would make all the
confusion about how a particular clock driver decides to round a
rate go away and just leave an API that sanely says the rate will
be within these bounds or an error will be returned stating it
can't be satisfied. This would be useful so we don't have a bunch
of drivers littered with code that loops on clk_round_rate() to
figure out what their hardware actually supports or having some
hard-coded frequency table per driver because the hardware can't
generate some frequency that's part of a spec (but still lies
within some acceptable tolerance!). It would also make
clk_round_rate() mostly obsolete because we know the rate is
within whatever acceptable bounds we've chosen. Eventually
clk_set_rate() could be become a small wrapper on top of
clk_set_rate_range(), constraining the rate to be exactly
whatever the clock driver returns as the rounded rate.
--
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/
[PATCH v5 7/7] 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
---
v5: * Initialize clk.ceiling_constraint to ULONG_MAX
* Warn about inconsistent constraints
v4: * Copy function docs from header
* Move WARN out of critical section
* Refresh rate after removing a per-user clk
* Rename clk_core.per_user_clks to clk_core.clks
* Store requested rate and re-apply it when constraints are updated
---
drivers/clk/clk.c | 144 +++-
include/linux/clk-private.h | 6 ++
include/linux/clk.h | 18 ++
3 files changed, 140 insertions(+), 28 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9fae072..72685c2 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1599,30 +1599,11 @@ 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 req_rate)
{
struct clk_core *top, *fail_clk;
+ struct clk *clk_user;
+ unsigned long rate = req_rate;
int ret = 0;
if (!clk)
@@ -1631,18 +1612,26 @@ 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, >clks, child_node) {
+ rate = max(rate, clk_user->floor_constraint);
+ }
+
+ hlist_for_each_entry(clk_user, >clks, child_node) {
+ 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_nolock(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;
@@ -1661,13 +1650,95 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
/* change the rates */
clk_change_rate(top);
+ clk->req_rate = req_rate;
+
out:
clk_prepare_unlock();
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);
+/**
+ * clk_set_floor_rate - set a minimum clock rate for a clock source
+ * @clk: clock source
+ * @rate: desired
[PATCH v5 7/7] 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
---
v5: * Initialize clk.ceiling_constraint to ULONG_MAX
* Warn about inconsistent constraints
v4: * Copy function docs from header
* Move WARN out of critical section
* Refresh rate after removing a per-user clk
* Rename clk_core.per_user_clks to clk_core.clks
* Store requested rate and re-apply it when constraints are updated
---
drivers/clk/clk.c | 144 +++-
include/linux/clk-private.h | 6 ++
include/linux/clk.h | 18 ++
3 files changed, 140 insertions(+), 28 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9fae072..72685c2 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1599,30 +1599,11 @@ 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 req_rate)
{
struct clk_core *top, *fail_clk;
+ struct clk *clk_user;
+ unsigned long rate = req_rate;
int ret = 0;
if (!clk)
@@ -1631,18 +1612,26 @@ 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-clks, child_node) {
+ rate = max(rate, clk_user-floor_constraint);
+ }
+
+ hlist_for_each_entry(clk_user, clk-clks, child_node) {
+ 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_nolock(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;
@@ -1661,13 +1650,95 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
/* change the rates */
clk_change_rate(top);
+ clk-req_rate = req_rate;
+
out:
clk_prepare_unlock();
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);
+/**
+ * clk_set_floor_rate - set a minimum clock rate for a clock source
+ * @clk: clock source
+ * @rate:
