| Message ID | 20201125144847.3920-1-punitagrawal@gmail.com |
|---|---|
| Headers | show |
| Series | Add processor to the ignore PSD override list | expand |
On Wed, Nov 25, 2020 at 11:48:47PM +0900, Punit Agrawal wrote: > Replace the raw values for AMD processor family with recently > introduced identifier macros to improve code readability and > maintainability. > > Signed-off-by: Punit Agrawal <punitagrawal@gmail.com> > --- > drivers/cpufreq/acpi-cpufreq.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index 29f1cd93541e..d8b8300ae9e0 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -202,8 +202,8 @@ static int override_acpi_psd(unsigned int cpu_id) > * CPU's before Zen3 (except some Zen2) need the > * override. > */ > - return (c->x86 < 0x19) && > - !(c->x86 == 0x17 && c->x86_model == 0x60 && > + return (c->x86 < AMD_FAM_ZEN3) && > + !(c->x86 == AMD_FAM_ZEN && c->x86_model == 0x60 && This is what I mean - that's Zen2 as the comment above says so having c->x86 == AMD_FAM_ZEN is not enough. And you have a comment above it stating which CPUs are matched here so I'm not sure those family defines make it any better... -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov <bp@alien8.de> writes: > On Wed, Nov 25, 2020 at 11:48:47PM +0900, Punit Agrawal wrote: >> Replace the raw values for AMD processor family with recently >> introduced identifier macros to improve code readability and >> maintainability. >> >> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com> >> --- >> drivers/cpufreq/acpi-cpufreq.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c >> index 29f1cd93541e..d8b8300ae9e0 100644 >> --- a/drivers/cpufreq/acpi-cpufreq.c >> +++ b/drivers/cpufreq/acpi-cpufreq.c >> @@ -202,8 +202,8 @@ static int override_acpi_psd(unsigned int cpu_id) >> * CPU's before Zen3 (except some Zen2) need the >> * override. >> */ >> - return (c->x86 < 0x19) && >> - !(c->x86 == 0x17 && c->x86_model == 0x60 && >> + return (c->x86 < AMD_FAM_ZEN3) && >> + !(c->x86 == AMD_FAM_ZEN && c->x86_model == 0x60 && > > This is what I mean - that's Zen2 as the comment above says so having > > c->x86 == AMD_FAM_ZEN > > is not enough. And you have a comment above it stating which CPUs are > matched here so I'm not sure those family defines make it any better... Hmm.. for this series, it probably doesn't add much value - especially with the comment and macro mismatch. The last two patches were posted to check if there's wider interest in the changes. If the macro conversion is useful, I can split the patches from this series into a separate set with more sites being updated. I'll wait to see if there's any further feedback. Thanks, Punit
Hi Rafael, Punit Agrawal <punitagrawal@gmail.com> writes: > Hi, > > While looking into Giovanni's patches to enable frequency invariance > on AMD systems[0], I noticed an issue with initialising frequency > domain information on a recent AMD APU. > > Patch 1 refactors the test to ignore firmware provided frequency > domain into a separate function. > > Patch 2 adds said APU (Family: 0x17, Model: 0x60, Stepping: 0x01) to > the list of CPUs for which the PSD override is ignored. I am not quite > happy with having to special case a particular CPU but also couldn't > find any documentation to help identify the CPUs that don't need the > override. Are you be OK to pick the first two patches if there are no issues? Thanks, Punit > Patch 3 and 4 are somewhat independent and a first step towards > improving the situation with regards to the use of raw identifiers for > AMD processors throughout the kernel. > > All feedback welcome. > > Thanks, > Punit > > [0] https://lore.kernel.org/linux-acpi/20201112182614.10700-1-ggherdovich@suse.cz/ > > Punit Agrawal (4): > cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD > cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list > x86/cpu: amd: Define processor families > cpufreq: acpi-cpufreq: Use identifiers for AMD processor family > > arch/x86/include/asm/amd-family.h | 18 ++++++++++++++++++ > arch/x86/include/asm/cpu_device_id.h | 2 ++ > drivers/cpufreq/acpi-cpufreq.c | 24 +++++++++++++++++++++--- > 3 files changed, 41 insertions(+), 3 deletions(-) > create mode 100644 arch/x86/include/asm/amd-family.h
On Fri, Dec 4, 2020 at 11:45 PM Punit Agrawal <punitagrawal@gmail.com> wrote: > > Hi Rafael, > > Punit Agrawal <punitagrawal@gmail.com> writes: > > > Hi, > > > > While looking into Giovanni's patches to enable frequency invariance > > on AMD systems[0], I noticed an issue with initialising frequency > > domain information on a recent AMD APU. > > > > Patch 1 refactors the test to ignore firmware provided frequency > > domain into a separate function. > > > > Patch 2 adds said APU (Family: 0x17, Model: 0x60, Stepping: 0x01) to > > the list of CPUs for which the PSD override is ignored. I am not quite > > happy with having to special case a particular CPU but also couldn't > > find any documentation to help identify the CPUs that don't need the > > override. > > Are you be OK to pick the first two patches if there are no issues? Please send them as non-RFC and change the name of override_acpi_psd() to indicate that it is AMD-specific. Thanks!
On 11/25/20 8:48 AM, Punit Agrawal wrote: > Re-factor the code to override the firmware provided frequency domain > information (via PSD) to localise the checks in one function. > > No functional change intended. > > Signed-off-by: Punit Agrawal <punitagrawal@gmail.com> > Cc: Wei Huang <wei.huang2@amd.com> > --- > drivers/cpufreq/acpi-cpufreq.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index 1e4fbb002a31..b1e7df96d428 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -191,6 +191,20 @@ static int check_amd_hwpstate_cpu(unsigned int cpuid) > return cpu_has(cpu, X86_FEATURE_HW_PSTATE); > } > > +static int override_acpi_psd(unsigned int cpu_id) ^^^^^ int is fine, but it might be better to use bool. Otherwise I don't see any issues with this patch. > +{ > + struct cpuinfo_x86 *c = &boot_cpu_data; > + > + if (c->x86_vendor == X86_VENDOR_AMD) { > + if (!check_amd_hwpstate_cpu(cpu_id)) > + return false; > + > + return c->x86 < 0x19; > + } > + > + return false; > +} > + > static unsigned extract_io(struct cpufreq_policy *policy, u32 value) > { > struct acpi_cpufreq_data *data = policy->driver_data; > @@ -691,8 +705,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) > cpumask_copy(policy->cpus, topology_core_cpumask(cpu)); > } > > - if (check_amd_hwpstate_cpu(cpu) && boot_cpu_data.x86 < 0x19 && > - !acpi_pstate_strict) { > + if (override_acpi_psd(cpu) && !acpi_pstate_strict) { > cpumask_clear(policy->cpus); > cpumask_set_cpu(cpu, policy->cpus); > cpumask_copy(data->freqdomain_cpus, >
Hi Rafael, "Rafael J. Wysocki" <rafael@kernel.org> writes: > On Fri, Dec 4, 2020 at 11:45 PM Punit Agrawal <punitagrawal@gmail.com> wrote: >> >> Hi Rafael, >> >> Punit Agrawal <punitagrawal@gmail.com> writes: >> >> > Hi, >> > >> > While looking into Giovanni's patches to enable frequency invariance >> > on AMD systems[0], I noticed an issue with initialising frequency >> > domain information on a recent AMD APU. >> > >> > Patch 1 refactors the test to ignore firmware provided frequency >> > domain into a separate function. >> > >> > Patch 2 adds said APU (Family: 0x17, Model: 0x60, Stepping: 0x01) to >> > the list of CPUs for which the PSD override is ignored. I am not quite >> > happy with having to special case a particular CPU but also couldn't >> > find any documentation to help identify the CPUs that don't need the >> > override. >> >> Are you be OK to pick the first two patches if there are no issues? > > Please send them as non-RFC and change the name of override_acpi_psd() > to indicate that it is AMD-specific. Ack - I will incorporate your comments in the next version (once the ongoing discussion finishes). Thanks.
Hi Wei, Wei Huang <whuang2@amd.com> writes: > On 11/25/20 8:48 AM, Punit Agrawal wrote: >> Re-factor the code to override the firmware provided frequency domain >> information (via PSD) to localise the checks in one function. >> >> No functional change intended. >> >> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com> >> Cc: Wei Huang <wei.huang2@amd.com> >> --- >> drivers/cpufreq/acpi-cpufreq.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c >> index 1e4fbb002a31..b1e7df96d428 100644 >> --- a/drivers/cpufreq/acpi-cpufreq.c >> +++ b/drivers/cpufreq/acpi-cpufreq.c >> @@ -191,6 +191,20 @@ static int check_amd_hwpstate_cpu(unsigned int cpuid) >> return cpu_has(cpu, X86_FEATURE_HW_PSTATE); >> } >> >> +static int override_acpi_psd(unsigned int cpu_id) > ^^^^^ > int is fine, but it might be better to use bool. Otherwise I don't see > any issues with this patch. Makes sense - I will switch to a boolean in the next update. Thanks for taking a look. Punit > >> +{ >> + struct cpuinfo_x86 *c = &boot_cpu_data; >> + >> + if (c->x86_vendor == X86_VENDOR_AMD) { >> + if (!check_amd_hwpstate_cpu(cpu_id)) >> + return false; >> + >> + return c->x86 < 0x19; >> + } >> + >> + return false; >> +} >> + >> static unsigned extract_io(struct cpufreq_policy *policy, u32 value) >> { >> struct acpi_cpufreq_data *data = policy->driver_data; >> @@ -691,8 +705,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) >> cpumask_copy(policy->cpus, topology_core_cpumask(cpu)); >> } >> >> - if (check_amd_hwpstate_cpu(cpu) && boot_cpu_data.x86 < 0x19 && >> - !acpi_pstate_strict) { >> + if (override_acpi_psd(cpu) && !acpi_pstate_strict) { >> cpumask_clear(policy->cpus); >> cpumask_set_cpu(cpu, policy->cpus); >> cpumask_copy(data->freqdomain_cpus, >>