| Message ID | 6e2ca86ce948c0bc35f7709baf22672771fcf1e8.1379063063.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Sep 13, 2013 at 06:32:32PM +0530, Viresh Kumar wrote: > Most of the drivers do following in their ->target_index() routines: > > struct cpufreq_freqs freqs; > freqs.old = old freq... > freqs.new = new freq... > > cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); > > /* Change rate here */ > > cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); > > This is replicated over all cpufreq drivers today and there doesn't exists a > good enough reason why this shouldn't be moved to cpufreq core instead. > > Earlier patches have added support in cpufreq core to do cpufreq notification on > frequency change, this one removes it from this driver. > > Some related minor cleanups are also done along with it. > > Cc: Russell King <linux@arm.linux.org.uk> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Shouldn't this patch set CPUFREQ_ASYNC_NOTIFICATION somewhere?
On 13 September 2013 21:24, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Sep 13, 2013 at 06:32:32PM +0530, Viresh Kumar wrote: >> Most of the drivers do following in their ->target_index() routines: >> >> struct cpufreq_freqs freqs; >> freqs.old = old freq... >> freqs.new = new freq... >> >> cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); >> >> /* Change rate here */ >> >> cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); >> >> This is replicated over all cpufreq drivers today and there doesn't exists a >> good enough reason why this shouldn't be moved to cpufreq core instead. >> >> Earlier patches have added support in cpufreq core to do cpufreq notification on >> frequency change, this one removes it from this driver. >> >> Some related minor cleanups are also done along with it. >> >> Cc: Russell King <linux@arm.linux.org.uk> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > Shouldn't this patch set CPUFREQ_ASYNC_NOTIFICATION somewhere? As far as I can see, sa11x0 completes frequency transition from within target() and so it does it synchronously.. And so it doesn't need to set CPUFREQ_ASYNC_NOTIFICATION... Am I missing something?
On Fri, Sep 13, 2013 at 09:42:23PM +0530, Viresh Kumar wrote: > On 13 September 2013 21:24, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Fri, Sep 13, 2013 at 06:32:32PM +0530, Viresh Kumar wrote: > >> Most of the drivers do following in their ->target_index() routines: > >> > >> struct cpufreq_freqs freqs; > >> freqs.old = old freq... > >> freqs.new = new freq... > >> > >> cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); > >> > >> /* Change rate here */ > >> > >> cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); > >> > >> This is replicated over all cpufreq drivers today and there doesn't exists a > >> good enough reason why this shouldn't be moved to cpufreq core instead. > >> > >> Earlier patches have added support in cpufreq core to do cpufreq notification on > >> frequency change, this one removes it from this driver. > >> > >> Some related minor cleanups are also done along with it. > >> > >> Cc: Russell King <linux@arm.linux.org.uk> > >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > Shouldn't this patch set CPUFREQ_ASYNC_NOTIFICATION somewhere? > > As far as I can see, sa11x0 completes frequency transition from within > target() and so it does it synchronously.. And so it doesn't need to set > CPUFREQ_ASYNC_NOTIFICATION... > > Am I missing something? The patch to which I'm replying removes the above calls. These calls are necessary to shutdown various bits of CPU-clock dependent hardware before changing the CPU clock, and restore them - reconfiguring them for the new clock rate after the transition has happened. So, if you're removing these calls, what replaces them? I don't see anything which does without the above set.
On 13 September 2013 21:45, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > The patch to which I'm replying removes the above calls. These calls are > necessary to shutdown various bits of CPU-clock dependent hardware > before changing the CPU clock, and restore them - reconfiguring them > for the new clock rate after the transition has happened. > > So, if you're removing these calls, what replaces them? I don't see > anything which does without the above set. The other patch on which you commented about unnecessary read locks being taken: [PATCH 181/228] cpufreq: move freq change notifications to cpufreq That calls these notifiers, for all platforms except the ones that have set CPUFREQ_ASYNC_NOTIFICATION, before and after calling ->target_index().. And so functionally the code is supposed to be the same.. Unless I have done some stupid mistake..
On Fri, Sep 13, 2013 at 09:52:31PM +0530, Viresh Kumar wrote: > On 13 September 2013 21:45, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > The patch to which I'm replying removes the above calls. These calls are > > necessary to shutdown various bits of CPU-clock dependent hardware > > before changing the CPU clock, and restore them - reconfiguring them > > for the new clock rate after the transition has happened. > > > > So, if you're removing these calls, what replaces them? I don't see > > anything which does without the above set. > > The other patch on which you commented about unnecessary read > locks being taken: > > [PATCH 181/228] cpufreq: move freq change notifications to cpufreq > > That calls these notifiers, for all platforms except the ones that have > set CPUFREQ_ASYNC_NOTIFICATION, before and after calling > ->target_index().. > > And so functionally the code is supposed to be the same.. Unless I > have done some stupid mistake.. Ah, sorry, read that that test the other way around. In that case, this patch is fine and can have my ack as per the other acks I've already given. Thanks.
On 13 September 2013 21:56, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Ah, sorry, read that that test the other way around. In that case, > this patch is fine and can have my ack as per the other acks I've > already given. Thanks :)
diff --git a/drivers/cpufreq/sa1100-cpufreq.c b/drivers/cpufreq/sa1100-cpufreq.c index b0da1fe..623da74 100644 --- a/drivers/cpufreq/sa1100-cpufreq.c +++ b/drivers/cpufreq/sa1100-cpufreq.c @@ -180,22 +180,17 @@ static void sa1100_update_dram_timings(int current_speed, int new_speed) static int sa1100_target(struct cpufreq_policy *policy, unsigned int ppcr) { unsigned int cur = sa11x0_getspeed(0); - struct cpufreq_freqs freqs; + unsigned int new_freq; - freqs.old = cur; - freqs.new = sa11x0_freq_table[ppcr].frequency; + new_freq = sa11x0_freq_table[ppcr].frequency; - cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); - - if (freqs.new > cur) - sa1100_update_dram_timings(cur, freqs.new); + if (new_freq > cur) + sa1100_update_dram_timings(cur, new_freq); PPCR = ppcr; - if (freqs.new < cur) - sa1100_update_dram_timings(cur, freqs.new); - - cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); + if (new_freq < cur) + sa1100_update_dram_timings(cur, new_freq); return 0; } diff --git a/drivers/cpufreq/sa1110-cpufreq.c b/drivers/cpufreq/sa1110-cpufreq.c index 55b1818c..2c2b2e6 100644 --- a/drivers/cpufreq/sa1110-cpufreq.c +++ b/drivers/cpufreq/sa1110-cpufreq.c @@ -232,15 +232,11 @@ sdram_update_refresh(u_int cpu_khz, struct sdram_params *sdram) static int sa1110_target(struct cpufreq_policy *policy, unsigned int ppcr) { struct sdram_params *sdram = &sdram_params; - struct cpufreq_freqs freqs; struct sdram_info sd; unsigned long flags; unsigned int unused; - freqs.old = sa11x0_getspeed(0); - freqs.new = sa11x0_freq_table[ppcr].frequency; - - sdram_calculate_timing(&sd, freqs.new, sdram); + sdram_calculate_timing(&sd, sa11x0_freq_table[ppcr].frequency, sdram); #if 0 /* @@ -259,8 +255,6 @@ static int sa1110_target(struct cpufreq_policy *policy, unsigned int ppcr) sd.mdcas[2] = 0xaaaaaaaa; #endif - cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); - /* * The clock could be going away for some time. Set the SDRAMs * to refresh rapidly (every 64 memory clock cycles). To get @@ -305,9 +299,7 @@ static int sa1110_target(struct cpufreq_policy *policy, unsigned int ppcr) /* * Now, return the SDRAM refresh back to normal. */ - sdram_update_refresh(freqs.new, sdram); - - cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); + sdram_update_refresh(sa11x0_freq_table[ppcr].frequency, sdram); return 0; }
Most of the drivers do following in their ->target_index() routines: struct cpufreq_freqs freqs; freqs.old = old freq... freqs.new = new freq... cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); /* Change rate here */ cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); This is replicated over all cpufreq drivers today and there doesn't exists a good enough reason why this shouldn't be moved to cpufreq core instead. Earlier patches have added support in cpufreq core to do cpufreq notification on frequency change, this one removes it from this driver. Some related minor cleanups are also done along with it. Cc: Russell King <linux@arm.linux.org.uk> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/sa1100-cpufreq.c | 17 ++++++----------- drivers/cpufreq/sa1110-cpufreq.c | 12 ++---------- 2 files changed, 8 insertions(+), 21 deletions(-)