| Message ID | CAKohpokUmvkkGwCDDa1d8OCz8YcsE2J0GQkA-dJ=EE+6REm7Mg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Viresh Thanks for your review. On 2013?06?25? 11:56, Viresh Kumar wrote: > On 25 June 2013 07:36, Lan Tianyu <tianyu.lan@intel.com> wrote: >> diff --git a/Documentation/cpu-freq/user-guide.txt b/Documentation/cpu-freq/user-guide.txt >> index ff2f283..0cc72f7 100644 >> --- a/Documentation/cpu-freq/user-guide.txt >> +++ b/Documentation/cpu-freq/user-guide.txt >> @@ -196,6 +196,10 @@ affected_cpus : List of Online CPUs that require software >> related_cpus : List of Online + Offline CPUs that need software >> coordination of frequency. >> >> +freqdomain_cpus : List of Online + Offline CPUs in same CPU dependency >> + domain. (This is only available for acpi-cpufreq >> + driver) >> + > > This is generic file, don't add this information here. Add this in > acpi-cpufreq file. I don't find acpi-cpufreq.txt under Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it? > >> scaling_driver : Hardware driver for cpufreq. >> >> scaling_cur_freq : Current frequency of the CPU as determined by >> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c >> index 17e3496..b859997 100644 >> --- a/drivers/cpufreq/acpi-cpufreq.c >> +++ b/drivers/cpufreq/acpi-cpufreq.c >> @@ -176,6 +176,16 @@ static struct global_attr global_boost = __ATTR(boost, 0644, >> show_global_boost, >> store_global_boost); >> >> +static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf) >> +{ >> + struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu); >> + struct acpi_processor_performance *perf = data->acpi_data; >> + >> + return cpufreq_show_cpus(perf->shared_cpu_map, buf); >> +} > > I am not sure if this is enough. Check this commit: > > aa77a52764a92216b61a6c8079b5c01937c046cd > > It had these changes: Please see the acpi_processor_preregister_performance() in the drivers/acpi/processor_perlib.c. All cpus in the same dependency domain are stored in the perf->shared_cpu_map(including the reference cpu the perf belongs to). Original code will copy shared_cpu_map to policy->related_cpus, do cpumask_or between policy->related_cpus and policy->cpus in the cpufreq_add_dev() and store the result into policy->related_cpus. Expose the data via related_cpus. For acpi-cpufreq driver, the shared_cpu_map is the biggest subset since it regardless the coordination type. After the commit aa77a5, only the cpus in the software or software&hardware coordination type dependency domain will copy to policy->cpus and finally store into policy->related_cpus in the cpufreq_add_dev() by cpumask_or. And then we miss the cpus in the hardware coordination type domain. But these info is still stored in the policy->shared_cpu_map. Does this make sense? > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index 937bc28..57a8774 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -730,7 +730,6 @@ static int acpi_cpufreq_cpu_init(struct > cpufreq_policy *policy) > policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) { > cpumask_copy(policy->cpus, perf->shared_cpu_map); > } > - cpumask_copy(policy->related_cpus, perf->shared_cpu_map); > > #ifdef CONFIG_SMP > dmi_check_system(sw_any_bug_dmi_table); > @@ -742,7 +741,6 @@ static int acpi_cpufreq_cpu_init(struct > cpufreq_policy *policy) > if (check_amd_hwpstate_cpu(cpu) && !acpi_pstate_strict) { > cpumask_clear(policy->cpus); > cpumask_set_cpu(cpu, policy->cpus); > - cpumask_copy(policy->related_cpus, cpu_sibling_mask(cpu)); > policy->shared_type = CPUFREQ_SHARED_TYPE_HW; > pr_info_once(PFX "overriding BIOS provided _PSD data\n"); >
On 25 June 2013 12:24, Lan Tianyu <tianyu.lan@intel.com> wrote: > On 2013?06?25? 11:56, Viresh Kumar wrote: >> This is generic file, don't add this information here. Add this in >> acpi-cpufreq file. > I don't find acpi-cpufreq.txt under > Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it? I meant acpi-cpufreq.c file > Please see the acpi_processor_preregister_performance() in the > drivers/acpi/processor_perlib.c. All cpus in the same dependency domain > are stored in the perf->shared_cpu_map(including the reference cpu the > perf belongs to). Original code will copy shared_cpu_map to > policy->related_cpus, do cpumask_or between policy->related_cpus and > policy->cpus in the cpufreq_add_dev() and store the result into > policy->related_cpus. Expose the data via related_cpus. > > For acpi-cpufreq driver, the shared_cpu_map is the biggest subset since > it regardless the coordination type. > > After the commit aa77a5, only the cpus in the software or > software&hardware coordination type dependency domain will copy to > policy->cpus and finally store into policy->related_cpus in the > cpufreq_add_dev() by cpumask_or. And then we miss the cpus in the > hardware coordination type domain. But these info is still stored in the > policy->shared_cpu_map. Does this make sense? I was concerned about this code that was present earlier. That changes related_cpus based on some conditions. if (check_amd_hwpstate_cpu(cpu) && !acpi_pstate_strict) { cpumask_clear(policy->cpus); cpumask_set_cpu(cpu, policy->cpus); cpumask_copy(policy->related_cpus, cpu_sibling_mask(cpu)); policy->shared_type = CPUFREQ_SHARED_TYPE_HW; pr_info_once(PFX "overriding BIOS provided _PSD data\n"); } -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013?06?25? 15:48, Viresh Kumar wrote: > On 25 June 2013 12:24, Lan Tianyu <tianyu.lan@intel.com> wrote: >> On 2013?06?25? 11:56, Viresh Kumar wrote: > >>> This is generic file, don't add this information here. Add this in >>> acpi-cpufreq file. >> I don't find acpi-cpufreq.txt under >> Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it? > > I meant acpi-cpufreq.c file Ok. From my opinion, the new attribute is an ABI and it's better to add descriptor under Document directory. The user can be easy to find how to use it. > >> Please see the acpi_processor_preregister_performance() in the >> drivers/acpi/processor_perlib.c. All cpus in the same dependency domain >> are stored in the perf->shared_cpu_map(including the reference cpu the >> perf belongs to). Original code will copy shared_cpu_map to >> policy->related_cpus, do cpumask_or between policy->related_cpus and >> policy->cpus in the cpufreq_add_dev() and store the result into >> policy->related_cpus. Expose the data via related_cpus. >> >> For acpi-cpufreq driver, the shared_cpu_map is the biggest subset since >> it regardless the coordination type. >> >> After the commit aa77a5, only the cpus in the software or >> software&hardware coordination type dependency domain will copy to >> policy->cpus and finally store into policy->related_cpus in the >> cpufreq_add_dev() by cpumask_or. And then we miss the cpus in the >> hardware coordination type domain. But these info is still stored in the >> policy->shared_cpu_map. Does this make sense? > > I was concerned about this code that was present earlier. That changes > related_cpus based on some conditions. Please see the commit which add the code. Maybe, we should overwrite shared_cpu_map by sibling_cpus for this case? commit acd316248205d553594296f1895ba5196b89ffcc Author: Andre Przywara <andre.przywara@amd.com> Date: Tue Sep 4 08:28:03 2012 +0000 acpi-cpufreq: Add quirk to disable _PSD usage on all AMD CPUs To workaround some Windows specific behavior, the ACPI _PSD table on AMD desktop boards advertises all cores as dependent, meaning that they all can only use the same P-state. acpi-cpufreq strictly obeys this description, instantiating one CPU only and symlinking the others. But the hardware can have distinct frequencies for each core and powernow-k8 did it that way. So, in order to use the hardware to its full potential and keep the original powernow-k8 behavior, lets override the _PSD table setting on AMD hardware. We use the siblings table, as it matches the current hardware behavior. Signed-off-by: Andre Przywara <andre.przywara@amd.com> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > > if (check_amd_hwpstate_cpu(cpu) && !acpi_pstate_strict) { > cpumask_clear(policy->cpus); > cpumask_set_cpu(cpu, policy->cpus); > cpumask_copy(policy->related_cpus, cpu_sibling_mask(cpu)); > policy->shared_type = CPUFREQ_SHARED_TYPE_HW; > pr_info_once(PFX "overriding BIOS provided _PSD data\n"); > } >
On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote: > Ok. From my opinion, the new attribute is an ABI and it's better to add > descriptor under Document directory. The user can be easy to find how to > use it. Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then it can have more details about the driver in future. > Please see the commit which add the code. Maybe, we should overwrite > shared_cpu_map by sibling_cpus for this case? Not sure if changing shared_cpu_map has any other implications or not. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, June 25, 2013 04:19:14 PM Lan Tianyu wrote: > On 2013?06?25? 15:48, Viresh Kumar wrote: > > On 25 June 2013 12:24, Lan Tianyu <tianyu.lan@intel.com> wrote: > >> On 2013?06?25? 11:56, Viresh Kumar wrote: > > > >>> This is generic file, don't add this information here. Add this in > >>> acpi-cpufreq file. > >> I don't find acpi-cpufreq.txt under > >> Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it? > > > > I meant acpi-cpufreq.c file > Ok. From my opinion, the new attribute is an ABI and it's better to add > descriptor under Document directory. The user can be easy to find how to > use it. Yes, this should be documented under Documentation/ABI/testing/. Thanks, Rafael
On Tuesday, June 25, 2013 09:01:03 PM Viresh Kumar wrote: > On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote: > > Ok. From my opinion, the new attribute is an ABI and it's better to add > > descriptor under Document directory. The user can be easy to find how to > > use it. > > Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then > it can have more details about the driver in future. > > > Please see the commit which add the code. Maybe, we should overwrite > > shared_cpu_map by sibling_cpus for this case? > > Not sure if changing shared_cpu_map has any other implications or not. Well, I wouldn't change it, then. Thanks, Rafael
On 2013?06?26? 07:02, Rafael J. Wysocki wrote: > On Tuesday, June 25, 2013 04:19:14 PM Lan Tianyu wrote: >> On 2013?06?25? 15:48, Viresh Kumar wrote: >>> On 25 June 2013 12:24, Lan Tianyu <tianyu.lan@intel.com> wrote: >>>> On 2013?06?25? 11:56, Viresh Kumar wrote: >>> >>>>> This is generic file, don't add this information here. Add this in >>>>> acpi-cpufreq file. >>>> I don't find acpi-cpufreq.txt under >>>> Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it? >>> >>> I meant acpi-cpufreq.c file >> Ok. From my opinion, the new attribute is an ABI and it's better to add >> descriptor under Document directory. The user can be easy to find how to >> use it. > > Yes, this should be documented under Documentation/ABI/testing/. I find the following descriptor in the Documentation/ABI/testing/sysfs-devices-system-cpu. So I originally put the new attribute descriptor in the user-guide.txt. Now, creating new acpi-cpufreq file under Documentation/cpu-freq/ and adding the descriptor in the new file maybe a good choice? What: /sys/devices/system/cpu/cpu#/cpufreq/* Date: pre-git history Contact: cpufreq@vger.kernel.org Description: Discover and change clock speed of CPUs Clock scaling allows you to change the clock speed of the CPUs on the fly. This is a nice method to save battery power, because the lower the clock speed, the less power the CPU consumes. There are many knobs to tweak in this directory. See files in Documentation/cpu-freq/ for more information. In particular, read Documentation/cpu-freq/user-guide.txt to learn how to control the knobs. > > Thanks, > Rafael > >
On 2013?06?26? 07:03, Rafael J. Wysocki wrote: > On Tuesday, June 25, 2013 09:01:03 PM Viresh Kumar wrote: >> On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote: >>> Ok. From my opinion, the new attribute is an ABI and it's better to add >>> descriptor under Document directory. The user can be easy to find how to >>> use it. >> >> Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then >> it can have more details about the driver in future. >> >>> Please see the commit which add the code. Maybe, we should overwrite >>> shared_cpu_map by sibling_cpus for this case? >> >> Not sure if changing shared_cpu_map has any other implications or not. > > Well, I wouldn't change it, then. Ok. How about add new field "cpufreqdomain_cpus" in the struct acpi_cpufreq_data and expose its value for new attribute. For normal case, copy the shared_cpu_map to it for normal case. For AMD case, copy sibling_cpus to it. Another choice, just keeping what my patch has done. Wait for the new request since current patch can satisfy reporter and it's not clear whether the patch is enough for AMD platform.(At last from my view). > > Thanks, > Rafael > >
On 26 June 2013 08:11, Lan Tianyu <tianyu.lan@intel.com> wrote: > On 2013?06?26? 07:03, Rafael J. Wysocki wrote: >> On Tuesday, June 25, 2013 09:01:03 PM Viresh Kumar wrote: >>> On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote: >>>> Ok. From my opinion, the new attribute is an ABI and it's better to add >>>> descriptor under Document directory. The user can be easy to find how to >>>> use it. >>> >>> Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then >>> it can have more details about the driver in future. >>> >>>> Please see the commit which add the code. Maybe, we should overwrite >>>> shared_cpu_map by sibling_cpus for this case? >>> >>> Not sure if changing shared_cpu_map has any other implications or not. >> >> Well, I wouldn't change it, then. Please add a blank line before and after your reply. It makes it more readable. > Ok. How about add new field "cpufreqdomain_cpus" in the struct > acpi_cpufreq_data and expose its value for new attribute. For normal > case, copy the shared_cpu_map to it for normal case. For AMD case, copy > sibling_cpus to it. This should work I guess. > Another choice, just keeping what my patch has done. Wait for the new > request since current patch can satisfy reporter and it's not clear > whether the patch is enough for AMD platform.(At last from my view). NAK. We must keep it as it was before my patch. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013?06?26? 14:54, Viresh Kumar wrote: > On 26 June 2013 08:11, Lan Tianyu <tianyu.lan@intel.com> wrote: >> On 2013?06?26? 07:03, Rafael J. Wysocki wrote: >>> On Tuesday, June 25, 2013 09:01:03 PM Viresh Kumar wrote: >>>> On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote: >>>>> Ok. From my opinion, the new attribute is an ABI and it's better to add >>>>> descriptor under Document directory. The user can be easy to find how to >>>>> use it. >>>> >>>> Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then >>>> it can have more details about the driver in future. >>>> >>>>> Please see the commit which add the code. Maybe, we should overwrite >>>>> shared_cpu_map by sibling_cpus for this case? >>>> >>>> Not sure if changing shared_cpu_map has any other implications or not. >>> >>> Well, I wouldn't change it, then. > > Please add a blank line before and after your reply. It makes it more > readable. Thanks for reminder. I will notice this. > >> Ok. How about add new field "cpufreqdomain_cpus" in the struct >> acpi_cpufreq_data and expose its value for new attribute. For normal >> case, copy the shared_cpu_map to it for normal case. For AMD case, copy >> sibling_cpus to it. > > This should work I guess. Ok. I will follow this one. > >> Another choice, just keeping what my patch has done. Wait for the new >> request since current patch can satisfy reporter and it's not clear >> whether the patch is enough for AMD platform.(At last from my view). > > NAK. We must keep it as it was before my patch. > OK. Please ignore this one.
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 937bc28..57a8774 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -730,7 +730,6 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) { cpumask_copy(policy->cpus, perf->shared_cpu_map); } - cpumask_copy(policy->related_cpus, perf->shared_cpu_map); #ifdef CONFIG_SMP