| Message ID | 20250414074056.3696888-8-Penny.Zheng@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | amd-cppc CPU Performance Scaling Driver | expand |
On 14.04.2025 09:40, Penny Zheng wrote: > AMD Family 1Ah CPU needs a different COF(Core Operating Frequency) formula, > due to a change in the PStateDef MSR layout in AMD Family 1Ah. > In AMD Family 1Ah, Core current operating frequency in MHz is calculated as > follows: > CoreCOF = Core::X86::Msr::PStateDef[CpuFid[11:0]] * 5MHz > > We introduce a helper amd_parse_freq() to parse cpu min/nominal/max core > frequency from PstateDef register, to replace the original macro FREQ(v). > amd_parse_freq() is declared as const, as it mainly consists of mathematical > conputation. > > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com> As to the title: I don't think "fix" is appropriate here. Or else I'd expect a Fixes: tag to be there, which I think would be hard for you to fish out (as the earlier changes here weren't broken; information on Fam1A simply wasn't available at the time). > --- a/xen/arch/x86/cpu/amd.c > +++ b/xen/arch/x86/cpu/amd.c > @@ -56,6 +56,9 @@ bool __initdata amd_virt_spec_ctrl; > > static bool __read_mostly fam17_c6_disabled; > > +static uint64_t attr_const amd_parse_freq(unsigned char c, uint64_t value); Why is this declaration needed? > +#define INVAL_FREQ_MHZ ~(uint64_t)0 This may want to move down, ahead of the function. And the expansion wants to be wrapped in parentheses. > @@ -570,12 +573,35 @@ static void amd_get_topology(struct cpuinfo_x86 *c) > : c->cpu_core_id); > } > > +static uint64_t amd_parse_freq(unsigned char c, uint64_t value) Considering how it's used, does "value" need to be any wider than unsigned int? What about the return type? I also think the first argument would better be unsigned int, and would better be named e.g. "family". > +{ > + uint64_t freq = INVAL_FREQ_MHZ; > + > + switch (c) { > + case 0x10 ... 0x16: > + freq = (((value & 0x3f) + 0x10) * 100) >> ((value >> 6) & 7); > + break; > + case 0x17 ... 0x19: > + freq = ((value & 0xff) * 25 * 8) / ((value >> 8) & 0x3f); > + break; > + case 0x1A: > + freq = (value & 0xfff) * 5; > + break; > + default: Nit: Blank lines please between non-fall-through case blocks. > + printk(XENLOG_ERR > + "Unsupported cpu familly %c on cpufreq parsing", c); Nit: family Jan
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index ce4e1df710..f93dda927e 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -56,6 +56,9 @@ bool __initdata amd_virt_spec_ctrl; static bool __read_mostly fam17_c6_disabled; +static uint64_t attr_const amd_parse_freq(unsigned char c, uint64_t value); +#define INVAL_FREQ_MHZ ~(uint64_t)0 + static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo, unsigned int *hi) { @@ -570,12 +573,35 @@ static void amd_get_topology(struct cpuinfo_x86 *c) : c->cpu_core_id); } +static uint64_t amd_parse_freq(unsigned char c, uint64_t value) +{ + uint64_t freq = INVAL_FREQ_MHZ; + + switch (c) { + case 0x10 ... 0x16: + freq = (((value & 0x3f) + 0x10) * 100) >> ((value >> 6) & 7); + break; + case 0x17 ... 0x19: + freq = ((value & 0xff) * 25 * 8) / ((value >> 8) & 0x3f); + break; + case 0x1A: + freq = (value & 0xfff) * 5; + break; + default: + printk(XENLOG_ERR + "Unsupported cpu familly %c on cpufreq parsing", c); + break; + } + + return freq; +} + void amd_log_freq(const struct cpuinfo_x86 *c) { unsigned int idx = 0, h; uint64_t hi, lo, val; - if (c->x86 < 0x10 || c->x86 > 0x19 || + if (c->x86 < 0x10 || c->x86 > 0x1A || (c != &boot_cpu_data && (!opt_cpu_info || (c->apicid & (c->x86_num_siblings - 1))))) return; @@ -656,19 +682,22 @@ void amd_log_freq(const struct cpuinfo_x86 *c) if (!(lo >> 63)) return; -#define FREQ(v) (c->x86 < 0x17 ? ((((v) & 0x3f) + 0x10) * 100) >> (((v) >> 6) & 7) \ - : (((v) & 0xff) * 25 * 8) / (((v) >> 8) & 0x3f)) if (idx && idx < h && !rdmsr_safe(0xC0010064 + idx, val) && (val >> 63) && !rdmsr_safe(0xC0010064, hi) && (hi >> 63)) printk("CPU%u: %lu (%lu ... %lu) MHz\n", - smp_processor_id(), FREQ(val), FREQ(lo), FREQ(hi)); + smp_processor_id(), + amd_parse_freq(c->x86, val), + amd_parse_freq(c->x86, lo), + amd_parse_freq(c->x86, hi)); else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63)) printk("CPU%u: %lu ... %lu MHz\n", - smp_processor_id(), FREQ(lo), FREQ(hi)); + smp_processor_id(), + amd_parse_freq(c->x86, lo), + amd_parse_freq(c->x86, hi)); else - printk("CPU%u: %lu MHz\n", smp_processor_id(), FREQ(lo)); -#undef FREQ + printk("CPU%u: %lu MHz\n", smp_processor_id(), + amd_parse_freq(c->x86, lo)); } void cf_check early_init_amd(struct cpuinfo_x86 *c)
AMD Family 1Ah CPU needs a different COF(Core Operating Frequency) formula, due to a change in the PStateDef MSR layout in AMD Family 1Ah. In AMD Family 1Ah, Core current operating frequency in MHz is calculated as follows: CoreCOF = Core::X86::Msr::PStateDef[CpuFid[11:0]] * 5MHz We introduce a helper amd_parse_freq() to parse cpu min/nominal/max core frequency from PstateDef register, to replace the original macro FREQ(v). amd_parse_freq() is declared as const, as it mainly consists of mathematical conputation. Signed-off-by: Penny Zheng <Penny.Zheng@amd.com> --- v2 -> v3: - new commit --- v3 -> v4: - introduce amd_parse_freq() and declare it as const - express if-else-arry() as switch() --- xen/arch/x86/cpu/amd.c | 43 +++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)