* [PATCH] clk: Re-evaluate clock rate on min/max update @ 2017-03-21 13:43 Peter De Schrijver 2017-04-07 11:44 ` Peter De Schrijver 2017-04-12 16:46 ` Stephen Boyd 0 siblings, 2 replies; 9+ messages in thread From: Peter De Schrijver @ 2017-03-21 13:43 UTC (permalink / raw) To: Peter De Schrijver, Michael Turquette, Stephen Boyd, linux-clk, linux-kernel 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 <pdeschrijver@nvidia.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] clk: Re-evaluate clock rate on min/max update 2017-03-21 13:43 [PATCH] clk: Re-evaluate clock rate on min/max update Peter De Schrijver @ 2017-04-07 11:44 ` Peter De Schrijver 2017-04-12 16:46 ` Stephen Boyd 1 sibling, 0 replies; 9+ messages in thread From: Peter De Schrijver @ 2017-04-07 11:44 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd, linux-clk, linux-kernel 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 <pdeschrijver@nvidia.com> > --- > 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clk: Re-evaluate clock rate on min/max update 2017-03-21 13:43 [PATCH] clk: Re-evaluate clock rate on min/max update Peter De Schrijver 2017-04-07 11:44 ` Peter De Schrijver @ 2017-04-12 16:46 ` Stephen Boyd 2017-04-13 7:48 ` Peter De Schrijver 1 sibling, 1 reply; 9+ messages in thread From: Stephen Boyd @ 2017-04-12 16:46 UTC (permalink / raw) To: Peter De Schrijver; +Cc: Michael Turquette, linux-clk, linux-kernel 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 <pdeschrijver@nvidia.com> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clk: Re-evaluate clock rate on min/max update 2017-04-12 16:46 ` Stephen Boyd @ 2017-04-13 7:48 ` Peter De Schrijver 2017-04-28 7:17 ` Peter De Schrijver 2017-06-01 9:12 ` Stephen Boyd 0 siblings, 2 replies; 9+ messages in thread From: Peter De Schrijver @ 2017-04-13 7:48 UTC (permalink / raw) To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, linux-kernel 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 <pdeschrijver@nvidia.com> > > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clk: Re-evaluate clock rate on min/max update 2017-04-13 7:48 ` Peter De Schrijver @ 2017-04-28 7:17 ` Peter De Schrijver 2017-05-16 7:38 ` Peter De Schrijver 2017-06-01 9:12 ` Stephen Boyd 1 sibling, 1 reply; 9+ messages in thread From: Peter De Schrijver @ 2017-04-28 7:17 UTC (permalink / raw) To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, linux-kernel 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 <pdeschrijver@nvidia.com> > > > > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clk: Re-evaluate clock rate on min/max update 2017-04-28 7:17 ` Peter De Schrijver @ 2017-05-16 7:38 ` Peter De Schrijver 0 siblings, 0 replies; 9+ messages in thread From: Peter De Schrijver @ 2017-05-16 7:38 UTC (permalink / raw) To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, linux-kernel On Fri, Apr 28, 2017 at 10:17:43AM +0300, Peter De Schrijver wrote: > 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 <pdeschrijver@nvidia.com> > > > > > > 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? > Ping! Peter. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clk: Re-evaluate clock rate on min/max update 2017-04-13 7:48 ` Peter De Schrijver 2017-04-28 7:17 ` Peter De Schrijver @ 2017-06-01 9:12 ` Stephen Boyd 2017-06-02 10:15 ` Peter De Schrijver 1 sibling, 1 reply; 9+ messages in thread From: Stephen Boyd @ 2017-06-01 9:12 UTC (permalink / raw) To: Peter De Schrijver; +Cc: Michael Turquette, linux-clk, linux-kernel 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 <pdeschrijver@nvidia.com> > > > > 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 [sboyd@codeaurora.org: 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clk: Re-evaluate clock rate on min/max update 2017-06-01 9:12 ` Stephen Boyd @ 2017-06-02 10:15 ` Peter De Schrijver 2017-06-02 22:45 ` Stephen Boyd 0 siblings, 1 reply; 9+ messages in thread From: Peter De Schrijver @ 2017-06-02 10:15 UTC (permalink / raw) To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, linux-kernel 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 <pdeschrijver@nvidia.com> > > > > > > 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 > [sboyd@codeaurora.org: 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] clk: Re-evaluate clock rate on min/max update 2017-06-02 10:15 ` Peter De Schrijver @ 2017-06-02 22:45 ` Stephen Boyd 0 siblings, 0 replies; 9+ messages in thread From: Stephen Boyd @ 2017-06-02 22:45 UTC (permalink / raw) To: Peter De Schrijver; +Cc: Michael Turquette, linux-clk, linux-kernel 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 > > [sboyd@codeaurora.org: 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-02 22:45 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-21 13:43 [PATCH] clk: Re-evaluate clock rate on min/max update Peter De Schrijver 2017-04-07 11:44 ` Peter De Schrijver 2017-04-12 16:46 ` Stephen Boyd 2017-04-13 7:48 ` Peter De Schrijver 2017-04-28 7:17 ` Peter De Schrijver 2017-05-16 7:38 ` Peter De Schrijver 2017-06-01 9:12 ` Stephen Boyd 2017-06-02 10:15 ` Peter De Schrijver 2017-06-02 22:45 ` Stephen Boyd
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).