Whenever a user change its min or max rate limit of a clock, we need to
re-evaluate the current clock rate and possibly change it if the new limits
require so. To do this clk_set_rate_range() already calls
clk_core_set_rate_nolock, however this won't have the intended effect
because the core clock rate hasn't changed. To fix this, move the test to
avoid setting the same core clock rate again, to clk_set_rate() so
clk_core_set_rate_nolock() can change the clock rate when min or max have
been updated, even when the core clock rate has not changed.
Signed-off-by: Peter De Schrijver <[email protected]>
---
drivers/clk/clk.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2fa2fb8..0b815d1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1569,10 +1569,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
if (!core)
return 0;
- /* bail early if nothing to do */
- if (rate == clk_core_get_rate_nolock(core))
- return 0;
-
if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
return -EBUSY;
@@ -1621,16 +1617,21 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
*/
int clk_set_rate(struct clk *clk, unsigned long rate)
{
- int ret;
+ int ret = 0;
if (!clk)
- return 0;
+ return ret;
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
+ /* bail early if nothing to do */
+ if (rate == clk_core_get_rate_nolock(clk->core))
+ goto out;
+
ret = clk_core_set_rate_nolock(clk->core, rate);
+out:
clk_prepare_unlock();
return ret;
--
1.9.1
On Tue, Mar 21, 2017 at 03:43:26PM +0200, Peter De Schrijver wrote:
> Whenever a user change its min or max rate limit of a clock, we need to
> re-evaluate the current clock rate and possibly change it if the new limits
> require so. To do this clk_set_rate_range() already calls
> clk_core_set_rate_nolock, however this won't have the intended effect
> because the core clock rate hasn't changed. To fix this, move the test to
> avoid setting the same core clock rate again, to clk_set_rate() so
> clk_core_set_rate_nolock() can change the clock rate when min or max have
> been updated, even when the core clock rate has not changed.
>
Ping! Any comments on this?
Peter.
> Signed-off-by: Peter De Schrijver <[email protected]>
> ---
> drivers/clk/clk.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 2fa2fb8..0b815d1 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1569,10 +1569,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
> if (!core)
> return 0;
>
> - /* bail early if nothing to do */
> - if (rate == clk_core_get_rate_nolock(core))
> - return 0;
> -
> if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> return -EBUSY;
>
> @@ -1621,16 +1617,21 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
> */
> int clk_set_rate(struct clk *clk, unsigned long rate)
> {
> - int ret;
> + int ret = 0;
>
> if (!clk)
> - return 0;
> + return ret;
>
> /* prevent racing with updates to the clock topology */
> clk_prepare_lock();
>
> + /* bail early if nothing to do */
> + if (rate == clk_core_get_rate_nolock(clk->core))
> + goto out;
> +
> ret = clk_core_set_rate_nolock(clk->core, rate);
>
> +out:
> clk_prepare_unlock();
>
> return ret;
> --
> 1.9.1
>
On 03/21, Peter De Schrijver wrote:
> Whenever a user change its min or max rate limit of a clock, we need to
> re-evaluate the current clock rate and possibly change it if the new limits
> require so. To do this clk_set_rate_range() already calls
> clk_core_set_rate_nolock, however this won't have the intended effect
> because the core clock rate hasn't changed. To fix this, move the test to
> avoid setting the same core clock rate again, to clk_set_rate() so
> clk_core_set_rate_nolock() can change the clock rate when min or max have
> been updated, even when the core clock rate has not changed.
I'd expect some sort of Fixes: tag here? Or it never worked!?
>
> Signed-off-by: Peter De Schrijver <[email protected]>
I seem to recall some problems here around rate aggregation that
we fixed after the patches merged. Sorry, but I have to go back
and look at those conversations to refresh my memory and make
sure this is all fine.
Are you relying on the rate setting op to be called with the new
min/max requirements if the aggregated rate is the same? I don't
understand why clk drivers care.
> ---
> drivers/clk/clk.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 2fa2fb8..0b815d1 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1569,10 +1569,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
> if (!core)
> return 0;
>
> - /* bail early if nothing to do */
> - if (rate == clk_core_get_rate_nolock(core))
> - return 0;
> -
> if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> return -EBUSY;
>
> @@ -1621,16 +1617,21 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
> */
> int clk_set_rate(struct clk *clk, unsigned long rate)
> {
> - int ret;
> + int ret = 0;
>
> if (!clk)
> - return 0;
> + return ret;
Why? Noise?
>
> /* prevent racing with updates to the clock topology */
> clk_prepare_lock();
>
> + /* bail early if nothing to do */
> + if (rate == clk_core_get_rate_nolock(clk->core))
> + goto out;
> +
> ret = clk_core_set_rate_nolock(clk->core, rate);
>
> +out:
> clk_prepare_unlock();
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed, Apr 12, 2017 at 09:46:05AM -0700, Stephen Boyd wrote:
> On 03/21, Peter De Schrijver wrote:
> > Whenever a user change its min or max rate limit of a clock, we need to
> > re-evaluate the current clock rate and possibly change it if the new limits
> > require so. To do this clk_set_rate_range() already calls
> > clk_core_set_rate_nolock, however this won't have the intended effect
> > because the core clock rate hasn't changed. To fix this, move the test to
> > avoid setting the same core clock rate again, to clk_set_rate() so
> > clk_core_set_rate_nolock() can change the clock rate when min or max have
> > been updated, even when the core clock rate has not changed.
>
> I'd expect some sort of Fixes: tag here? Or it never worked!?
I don't think this ever worked.
>
> >
> > Signed-off-by: Peter De Schrijver <[email protected]>
>
> I seem to recall some problems here around rate aggregation that
> we fixed after the patches merged. Sorry, but I have to go back
> and look at those conversations to refresh my memory and make
> sure this is all fine.
>
> Are you relying on the rate setting op to be called with the new
> min/max requirements if the aggregated rate is the same? I don't
> understand why clk drivers care.
>
No. But I do rely on the rate setting op to be called when a new min or max
rate would cause the rate to be changed even when there is no new rate request.
Eg:
min = 100MHz, max = 500MHz, current rate request is 400MHz, then max changes to
300MHz. Today the rate setting op will not be called, while I think it should
be called to lower the rate to 300MHz.
Peter.
> > ---
> > drivers/clk/clk.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 2fa2fb8..0b815d1 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1569,10 +1569,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
> > if (!core)
> > return 0;
> >
> > - /* bail early if nothing to do */
> > - if (rate == clk_core_get_rate_nolock(core))
> > - return 0;
> > -
> > if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> > return -EBUSY;
> >
> > @@ -1621,16 +1617,21 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
> > */
> > int clk_set_rate(struct clk *clk, unsigned long rate)
> > {
> > - int ret;
> > + int ret = 0;
> >
> > if (!clk)
> > - return 0;
> > + return ret;
>
> Why? Noise?
>
> >
> > /* prevent racing with updates to the clock topology */
> > clk_prepare_lock();
> >
> > + /* bail early if nothing to do */
> > + if (rate == clk_core_get_rate_nolock(clk->core))
> > + goto out;
> > +
> > ret = clk_core_set_rate_nolock(clk->core, rate);
> >
> > +out:
> > clk_prepare_unlock();
> >
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
On Thu, Apr 13, 2017 at 10:48:19AM +0300, Peter De Schrijver wrote:
> On Wed, Apr 12, 2017 at 09:46:05AM -0700, Stephen Boyd wrote:
> > On 03/21, Peter De Schrijver wrote:
> > > Whenever a user change its min or max rate limit of a clock, we need to
> > > re-evaluate the current clock rate and possibly change it if the new limits
> > > require so. To do this clk_set_rate_range() already calls
> > > clk_core_set_rate_nolock, however this won't have the intended effect
> > > because the core clock rate hasn't changed. To fix this, move the test to
> > > avoid setting the same core clock rate again, to clk_set_rate() so
> > > clk_core_set_rate_nolock() can change the clock rate when min or max have
> > > been updated, even when the core clock rate has not changed.
> >
> > I'd expect some sort of Fixes: tag here? Or it never worked!?
>
> I don't think this ever worked.
>
> >
> > >
> > > Signed-off-by: Peter De Schrijver <[email protected]>
> >
> > I seem to recall some problems here around rate aggregation that
> > we fixed after the patches merged. Sorry, but I have to go back
> > and look at those conversations to refresh my memory and make
> > sure this is all fine.
> >
> > Are you relying on the rate setting op to be called with the new
> > min/max requirements if the aggregated rate is the same? I don't
> > understand why clk drivers care.
> >
>
> No. But I do rely on the rate setting op to be called when a new min or max
> rate would cause the rate to be changed even when there is no new rate request.
>
> Eg:
>
> min = 100MHz, max = 500MHz, current rate request is 400MHz, then max changes to
> 300MHz. Today the rate setting op will not be called, while I think it should
> be called to lower the rate to 300MHz.
>
Any news on this? or do you think this is an unreasonable assumption?
Thanks,
Peter.
On 04/13, Peter De Schrijver wrote:
> On Wed, Apr 12, 2017 at 09:46:05AM -0700, Stephen Boyd wrote:
> > On 03/21, Peter De Schrijver wrote:
> > > Whenever a user change its min or max rate limit of a clock, we need to
> > > re-evaluate the current clock rate and possibly change it if the new limits
> > > require so. To do this clk_set_rate_range() already calls
> > > clk_core_set_rate_nolock, however this won't have the intended effect
> > > because the core clock rate hasn't changed. To fix this, move the test to
> > > avoid setting the same core clock rate again, to clk_set_rate() so
> > > clk_core_set_rate_nolock() can change the clock rate when min or max have
> > > been updated, even when the core clock rate has not changed.
> >
> > I'd expect some sort of Fixes: tag here? Or it never worked!?
>
> I don't think this ever worked.
>
> >
> > >
> > > Signed-off-by: Peter De Schrijver <[email protected]>
> >
> > I seem to recall some problems here around rate aggregation that
> > we fixed after the patches merged. Sorry, but I have to go back
> > and look at those conversations to refresh my memory and make
> > sure this is all fine.
> >
> > Are you relying on the rate setting op to be called with the new
> > min/max requirements if the aggregated rate is the same? I don't
> > understand why clk drivers care.
> >
>
> No. But I do rely on the rate setting op to be called when a new min or max
> rate would cause the rate to be changed even when there is no new rate request.
>
> Eg:
>
> min = 100MHz, max = 500MHz, current rate request is 400MHz, then max changes to
> 300MHz. Today the rate setting op will not be called, while I think it should
> be called to lower the rate to 300MHz.
Ok. Can you please describe the sequence in more detail? What is
core::req_rate when the clk is registered? What is the rate of
the clk when the first rate is set?
Because I have a maintainer tag on commit 1c8e600440c of
[[email protected]: set req_rate in __clk_init] which may be a
problem if the clk is orphaned when registered and thus req_rate
is totally bogus because we can't calculate the rate[1].
We will need to only set req_rate when a clk is actually parented
to something, urgh. But that definitely doesn't look to even be
the bug you're talking about. From what I can tell, the whole
design is borked, because nobody has really used or tested this
code! We should really be making sure that a clk range request
doesn't become disjoint from other consumer requests. If it does,
it will be unsatisfiable. Furthermore, we should remove the
min/max constraints on failure out of set_rate() because it
didn't work.
We have req_rate there to make sure we bring the clk rate back to
within some range when a constraint goes away, but we should
probably just evaluate the constraints before calling
clk_core_set_rate_nolock() and then clamp the req_rate to within
the min/max that we determine, leaning toward the lowest rate.
That's sort of what you're doing here, but we lost the check to
make sure we don't call the set_rate op with the same rate we
already have. I'd prefer we maintain that part of the code even
for rate constraints.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/321467.html
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, Jun 01, 2017 at 02:12:51AM -0700, Stephen Boyd wrote:
> On 04/13, Peter De Schrijver wrote:
> > On Wed, Apr 12, 2017 at 09:46:05AM -0700, Stephen Boyd wrote:
> > > On 03/21, Peter De Schrijver wrote:
> > > > Whenever a user change its min or max rate limit of a clock, we need to
> > > > re-evaluate the current clock rate and possibly change it if the new limits
> > > > require so. To do this clk_set_rate_range() already calls
> > > > clk_core_set_rate_nolock, however this won't have the intended effect
> > > > because the core clock rate hasn't changed. To fix this, move the test to
> > > > avoid setting the same core clock rate again, to clk_set_rate() so
> > > > clk_core_set_rate_nolock() can change the clock rate when min or max have
> > > > been updated, even when the core clock rate has not changed.
> > >
> > > I'd expect some sort of Fixes: tag here? Or it never worked!?
> >
> > I don't think this ever worked.
> >
> > >
> > > >
> > > > Signed-off-by: Peter De Schrijver <[email protected]>
> > >
> > > I seem to recall some problems here around rate aggregation that
> > > we fixed after the patches merged. Sorry, but I have to go back
> > > and look at those conversations to refresh my memory and make
> > > sure this is all fine.
> > >
> > > Are you relying on the rate setting op to be called with the new
> > > min/max requirements if the aggregated rate is the same? I don't
> > > understand why clk drivers care.
> > >
> >
> > No. But I do rely on the rate setting op to be called when a new min or max
> > rate would cause the rate to be changed even when there is no new rate request.
> >
> > Eg:
> >
> > min = 100MHz, max = 500MHz, current rate request is 400MHz, then max changes to
> > 300MHz. Today the rate setting op will not be called, while I think it should
> > be called to lower the rate to 300MHz.
>
> Ok. Can you please describe the sequence in more detail? What is
> core::req_rate when the clk is registered? What is the rate of
> the clk when the first rate is set?
>
1) req_rate at registration time is the current rate of the clk: 100MHz
2) clk_set_rate sets req_rate to 400MHz, set_rate clk op is called to change
the rate
3) clk_set_min_rate is called with 100MHz, req_rate is 400MHz, no clock
operations are called
4) clk_set_max_rate is called with 500MHz, req_rate is 400Mhz, no clock
operations are called
5) clk_set_max_rate is called with 300MHz, req_rate is 400Mhz, no clock
operations are called because req_rate didn't change. This however is
wrong IMO. the set_rate op should be called to lower the clock rate
to 300MHz.
> Because I have a maintainer tag on commit 1c8e600440c of
> [[email protected]: set req_rate in __clk_init] which may be a
> problem if the clk is orphaned when registered and thus req_rate
> is totally bogus because we can't calculate the rate[1].
>
> We will need to only set req_rate when a clk is actually parented
> to something, urgh. But that definitely doesn't look to even be
The same happens for core::rate, however core::rate is updated by
__clk_recalc_rates when the parent appears. We should update req_rate
as well then. However this can't be done easily it seems because
__clk_recalc_rates is also called in other cases (eg when reparenting).
In theory updating req_rate when 'reparenting' from orphan to the real
parent would cause an existing req_rate to be discarded. However I don't
think we should allow any calls by consumers to orphaned clocks, because
this clearly is an inconsistent state. In practice all clocks are properly
parented by the time the consumers are starting to make calls to CCF. So
this should not cause any problem.
> the bug you're talking about. From what I can tell, the whole
> design is borked, because nobody has really used or tested this
> code! We should really be making sure that a clk range request
I'm trying to use it now :)
> doesn't become disjoint from other consumer requests. If it does,
> it will be unsatisfiable. Furthermore, we should remove the
> min/max constraints on failure out of set_rate() because it
> didn't work.
>
> We have req_rate there to make sure we bring the clk rate back to
> within some range when a constraint goes away, but we should
> probably just evaluate the constraints before calling
> clk_core_set_rate_nolock() and then clamp the req_rate to within
> the min/max that we determine, leaning toward the lowest rate.
> That's sort of what you're doing here, but we lost the check to
> make sure we don't call the set_rate op with the same rate we
> already have. I'd prefer we maintain that part of the code even
> for rate constraints.
>
Ok. I will rework the patch to avoid calling set_rate with the current rate.
Peter.
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/321467.html
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
On 06/02, Peter De Schrijver wrote:
> On Thu, Jun 01, 2017 at 02:12:51AM -0700, Stephen Boyd wrote:
> > On 04/13, Peter De Schrijver wrote:
> > > On Wed, Apr 12, 2017 at 09:46:05AM -0700, Stephen Boyd wrote:
> > > > On 03/21, Peter De Schrijver wrote:
> > >
> > > No. But I do rely on the rate setting op to be called when a new min or max
> > > rate would cause the rate to be changed even when there is no new rate request.
> > >
> > > Eg:
> > >
> > > min = 100MHz, max = 500MHz, current rate request is 400MHz, then max changes to
> > > 300MHz. Today the rate setting op will not be called, while I think it should
> > > be called to lower the rate to 300MHz.
> >
> > Ok. Can you please describe the sequence in more detail? What is
> > core::req_rate when the clk is registered? What is the rate of
> > the clk when the first rate is set?
> >
>
> 1) req_rate at registration time is the current rate of the clk: 100MHz
> 2) clk_set_rate sets req_rate to 400MHz, set_rate clk op is called to change
> the rate
> 3) clk_set_min_rate is called with 100MHz, req_rate is 400MHz, no clock
> operations are called
> 4) clk_set_max_rate is called with 500MHz, req_rate is 400Mhz, no clock
> operations are called
> 5) clk_set_max_rate is called with 300MHz, req_rate is 400Mhz, no clock
> operations are called because req_rate didn't change. This however is
> wrong IMO. the set_rate op should be called to lower the clock rate
> to 300MHz.
Thanks. Makes sense!
>
> > Because I have a maintainer tag on commit 1c8e600440c of
> > [[email protected]: set req_rate in __clk_init] which may be a
> > problem if the clk is orphaned when registered and thus req_rate
> > is totally bogus because we can't calculate the rate[1].
> >
> > We will need to only set req_rate when a clk is actually parented
> > to something, urgh. But that definitely doesn't look to even be
>
> The same happens for core::rate, however core::rate is updated by
> __clk_recalc_rates when the parent appears. We should update req_rate
> as well then. However this can't be done easily it seems because
> __clk_recalc_rates is also called in other cases (eg when reparenting).
> In theory updating req_rate when 'reparenting' from orphan to the real
> parent would cause an existing req_rate to be discarded. However I don't
> think we should allow any calls by consumers to orphaned clocks, because
> this clearly is an inconsistent state. In practice all clocks are properly
> parented by the time the consumers are starting to make calls to CCF. So
> this should not cause any problem.
Right. Reminds me. I need to merge that probe defer orphans patch
now.
>
> > the bug you're talking about. From what I can tell, the whole
> > design is borked, because nobody has really used or tested this
> > code! We should really be making sure that a clk range request
>
> I'm trying to use it now :)
>
> > doesn't become disjoint from other consumer requests. If it does,
> > it will be unsatisfiable. Furthermore, we should remove the
> > min/max constraints on failure out of set_rate() because it
> > didn't work.
> >
> > We have req_rate there to make sure we bring the clk rate back to
> > within some range when a constraint goes away, but we should
> > probably just evaluate the constraints before calling
> > clk_core_set_rate_nolock() and then clamp the req_rate to within
> > the min/max that we determine, leaning toward the lowest rate.
> > That's sort of what you're doing here, but we lost the check to
> > make sure we don't call the set_rate op with the same rate we
> > already have. I'd prefer we maintain that part of the code even
> > for rate constraints.
> >
>
>
> Ok. I will rework the patch to avoid calling set_rate with the current rate.
>
Thanks.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project