| Message ID | 333bf5868dfbfb6de474f060dc216fa6d33848e2.1376619363.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
2013/8/16 Viresh Kumar <viresh.kumar@linaro.org> > 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. > Hi Viresh: > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/acpi-cpufreq.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/cpufreq/acpi-cpufreq.c > b/drivers/cpufreq/acpi-cpufreq.c > index 7536e7d..6b00cd8 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -428,14 +428,10 @@ static int acpi_cpufreq_target(struct cpufreq_policy > *policy, > { > struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu); > struct acpi_processor_performance *perf; > - struct cpufreq_freqs freqs; > struct drv_cmd cmd; > unsigned int next_perf_state = 0; /* Index into perf table */ > int result = 0; > > - pr_debug("acpi_cpufreq_target %d (%d)\n", > - data->freq_table[index].frequency, policy->cpu); > - > if (unlikely(data == NULL || > data->acpi_data == NULL || data->freq_table == NULL)) { > return -ENODEV; > @@ -483,23 +479,17 @@ static int acpi_cpufreq_target(struct cpufreq_policy > *policy, > else > cmd.mask = cpumask_of(policy->cpu); > > - freqs.old = perf->states[perf->state].core_frequency * 1000; > - freqs.new = data->freq_table[index].frequency; > - cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); > - > drv_write(&cmd); > > if (acpi_pstate_strict) { > - if (!check_freqs(cmd.mask, freqs.new, data)) { > + if (!check_freqs(cmd.mask, > data->freq_table[index].frequency, > + data)) { > pr_debug("acpi_cpufreq_target failed (%d)\n", > policy->cpu); > result = -EAGAIN; > - freqs.new = freqs.old; > } > } > > - cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); > - > if (!result) > perf->state = next_perf_state; > > -- > 1.7.12.rc2.18.g61b472e > > -- > To unsubscribe from this list: send the line "unsubscribe cpufreq" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
2013/8/16 Lan Tianyu <lantianyu1986@gmail.com> > > > > 2013/8/16 Viresh Kumar <viresh.kumar@linaro.org> > >> 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. >> > Sorry for misoperation. Hi Viresh: > > Hi Viresh: One concern. Target() callback may return before changing cpufreq actually due to some check failures. After this change, prechange event will be triggered when these check failures take place. I am not sure whether this should be took into account. > > >> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> drivers/cpufreq/acpi-cpufreq.c | 14 ++------------ >> 1 file changed, 2 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/cpufreq/acpi-cpufreq.c >> b/drivers/cpufreq/acpi-cpufreq.c >> index 7536e7d..6b00cd8 100644 >> --- a/drivers/cpufreq/acpi-cpufreq.c >> +++ b/drivers/cpufreq/acpi-cpufreq.c >> @@ -428,14 +428,10 @@ static int acpi_cpufreq_target(struct >> cpufreq_policy *policy, >> { >> struct acpi_cpufreq_data *data = per_cpu(acfreq_data, >> policy->cpu); >> struct acpi_processor_performance *perf; >> - struct cpufreq_freqs freqs; >> struct drv_cmd cmd; >> unsigned int next_perf_state = 0; /* Index into perf table */ >> int result = 0; >> >> - pr_debug("acpi_cpufreq_target %d (%d)\n", >> - data->freq_table[index].frequency, policy->cpu); >> - >> if (unlikely(data == NULL || >> data->acpi_data == NULL || data->freq_table == NULL)) { >> return -ENODEV; >> @@ -483,23 +479,17 @@ static int acpi_cpufreq_target(struct >> cpufreq_policy *policy, >> else >> cmd.mask = cpumask_of(policy->cpu); >> >> - freqs.old = perf->states[perf->state].core_frequency * 1000; >> - freqs.new = data->freq_table[index].frequency; >> - cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); >> - >> drv_write(&cmd); >> >> if (acpi_pstate_strict) { >> - if (!check_freqs(cmd.mask, freqs.new, data)) { >> + if (!check_freqs(cmd.mask, >> data->freq_table[index].frequency, >> + data)) { >> pr_debug("acpi_cpufreq_target failed (%d)\n", >> policy->cpu); >> result = -EAGAIN; >> - freqs.new = freqs.old; >> } >> } >> >> - cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); >> - >> if (!result) >> perf->state = next_perf_state; >> >> -- >> 1.7.12.rc2.18.g61b472e >> >> -- >> To unsubscribe from this list: send the line "unsubscribe cpufreq" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > > -- > Best regards > Tianyu Lan >
On 16 August 2013 13:24, Lan Tianyu <lantianyu1986@gmail.com> wrote: > Sorry for misoperation. No Problem... > One concern. Target() callback may return before changing > cpufreq actually due to some check failures. After this change, prechange > event will be triggered when these check failures take place. I am not sure > whether this should be took into account. Yes, if you see the first patch of this series, it takes this into account.. In case target() failed and returned an error, we simply notify the POST CHANGE notification with old frequencies instead of new ones. I believe that would be enough.. This is exactly what acpi-cpufreq and others are doing currently. Hope I answered your question well? -- viresh
2013/8/16 Viresh Kumar <viresh.kumar@linaro.org>: > On 16 August 2013 13:24, Lan Tianyu <lantianyu1986@gmail.com> wrote: >> Sorry for misoperation. > > No Problem... > >> One concern. Target() callback may return before changing >> cpufreq actually due to some check failures. After this change, prechange >> event will be triggered when these check failures take place. I am not sure >> whether this should be took into account. > > Yes, if you see the first patch of this series, it takes this into > account.. In case > target() failed and returned an error, we simply notify the POST CHANGE > notification with old frequencies instead of new ones. I believe that would be > enough.. Yes, I have seen it but I missed the following two patches because they are not in the linux-pm tree. You moved the cpufreq_frequency_table_target() to cpufreq core and before notifying PRE CHANGE notification. The major check has been done. Now I think it's ok. Thanks for explanation. http://www.spinics.net/lists/cpufreq/msg06970.html http://www.spinics.net/lists/cpufreq/msg06896.html Reviewed-by: Lan Tianyu <tianyu.lan@intel.com> > > This is exactly what acpi-cpufreq and others are doing currently. > > Hope I answered your question well? > > -- > viresh
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 7536e7d..6b00cd8 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -428,14 +428,10 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy, { struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu); struct acpi_processor_performance *perf; - struct cpufreq_freqs freqs; struct drv_cmd cmd; unsigned int next_perf_state = 0; /* Index into perf table */ int result = 0; - pr_debug("acpi_cpufreq_target %d (%d)\n", - data->freq_table[index].frequency, policy->cpu); - if (unlikely(data == NULL || data->acpi_data == NULL || data->freq_table == NULL)) { return -ENODEV; @@ -483,23 +479,17 @@ static int acpi_cpufreq_target(struct cpufreq_policy *policy, else cmd.mask = cpumask_of(policy->cpu); - freqs.old = perf->states[perf->state].core_frequency * 1000; - freqs.new = data->freq_table[index].frequency; - cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); - drv_write(&cmd); if (acpi_pstate_strict) { - if (!check_freqs(cmd.mask, freqs.new, data)) { + if (!check_freqs(cmd.mask, data->freq_table[index].frequency, + data)) { pr_debug("acpi_cpufreq_target failed (%d)\n", policy->cpu); result = -EAGAIN; - freqs.new = freqs.old; } } - cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); - if (!result) perf->state = next_perf_state;
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. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/acpi-cpufreq.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)