| Message ID | 20241205222847.7889-9-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | amd-pstate 6.14 cleanups and improvements | expand |
On Thu, Dec 05, 2024 at 04:28:40PM -0600, Mario Limonciello wrote: > Storing values in the cpudata structure in different units leads > to confusion and hardcoded conversions elsewhere. After ratios are > calculated store everything in khz for any future use. Adjust all > relevant consumers for this change as well. > > Suggested-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Thanks for cleaning this up. This also makes the code in-line with the comment for cpudata->nominal_freq which says * @nominal_freq: the frequency (in khz) that mapped to nominal_perf Couple of minor nits. See below. > --- > drivers/cpufreq/amd-pstate-ut.c | 12 +++++------- > drivers/cpufreq/amd-pstate.c | 30 +++++++++++++++--------------- > 2 files changed, 20 insertions(+), 22 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c > index a261d7300951e..3a0a380c3590c 100644 > --- a/drivers/cpufreq/amd-pstate-ut.c > +++ b/drivers/cpufreq/amd-pstate-ut.c > @@ -207,7 +207,6 @@ static void amd_pstate_ut_check_freq(u32 index) > int cpu = 0; > struct cpufreq_policy *policy = NULL; > struct amd_cpudata *cpudata = NULL; > - u32 nominal_freq_khz; > > for_each_possible_cpu(cpu) { > policy = cpufreq_cpu_get(cpu); > @@ -215,14 +214,13 @@ static void amd_pstate_ut_check_freq(u32 index) > break; > cpudata = policy->driver_data; > > - nominal_freq_khz = cpudata->nominal_freq*1000; > - if (!((cpudata->max_freq >= nominal_freq_khz) && > - (nominal_freq_khz > cpudata->lowest_nonlinear_freq) && > + if (!((cpudata->max_freq >= cpudata->nominal_freq) && > + (cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) && > (cpudata->lowest_nonlinear_freq > cpudata->min_freq) && > (cpudata->min_freq > 0))) { > amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL; > pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n", > - __func__, cpu, cpudata->max_freq, nominal_freq_khz, > + __func__, cpu, cpudata->max_freq, cpudata->nominal_freq, > cpudata->lowest_nonlinear_freq, cpudata->min_freq); > goto skip_test; > } > @@ -236,13 +234,13 @@ static void amd_pstate_ut_check_freq(u32 index) > > if (cpudata->boost_supported) { > if ((policy->max == cpudata->max_freq) || > - (policy->max == nominal_freq_khz)) > + (policy->max == cpudata->nominal_freq)) > amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS; > else { > amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL; > pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n", > __func__, cpu, policy->max, cpudata->max_freq, > - nominal_freq_khz); > + cpudata->nominal_freq); > goto skip_test; > } > } else { > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index ce70d1bfa55d0..464db6745d84e 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -739,8 +739,8 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on) > > if (on) > policy->cpuinfo.max_freq = max_freq; > - else if (policy->cpuinfo.max_freq > nominal_freq * 1000) > - policy->cpuinfo.max_freq = nominal_freq * 1000; > + else if (policy->cpuinfo.max_freq > nominal_freq) > + policy->cpuinfo.max_freq = nominal_freq; > > policy->max = policy->cpuinfo.max_freq; > > @@ -940,29 +940,29 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata) > return ret; > > if (quirks && quirks->lowest_freq) > - min_freq = quirks->lowest_freq * 1000; > + min_freq = quirks->lowest_freq; > else > - min_freq = cppc_perf.lowest_freq * 1000; > + min_freq = cppc_perf.lowest_freq; > > if (quirks && quirks->nominal_freq) > - nominal_freq = quirks->nominal_freq ; > + nominal_freq = quirks->nominal_freq; > else > nominal_freq = cppc_perf.nominal_freq; > > nominal_perf = READ_ONCE(cpudata->nominal_perf); > > boost_ratio = div_u64(cpudata->highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf); > - max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000; > + max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT); > > lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf); > lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT, > nominal_perf); > - lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT) * 1000; > + lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT); > > - WRITE_ONCE(cpudata->min_freq, min_freq); > - WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq); > - WRITE_ONCE(cpudata->nominal_freq, nominal_freq); > - WRITE_ONCE(cpudata->max_freq, max_freq); > + WRITE_ONCE(cpudata->min_freq, min_freq * 1000); > + WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq * 1000); > + WRITE_ONCE(cpudata->nominal_freq, nominal_freq * 1000); > + WRITE_ONCE(cpudata->max_freq, max_freq * 1000); > > /** > * Below values need to be initialized correctly, otherwise driver will fail to load > @@ -970,15 +970,15 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata) > * lowest_nonlinear_freq is a value between [min_freq, nominal_freq] > * Check _CPC in ACPI table objects if any values are incorrect > */ > - if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) { > + if (min_freq <= 0 || max_freq <= 0 || cpudata->nominal_freq <= 0 || min_freq > max_freq) { > pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n", > - min_freq, max_freq, nominal_freq * 1000); > + min_freq, max_freq, cpudata->nominal_freq); > return -EINVAL; We don't need this change. This looks odd since we are using min_freq and max_freq from local variables, but nominal_freq from cpudata. We may just as well use the local variables in all the cases since "cpudata->nominal_freq = nominal_freq * 1000", and thus won't make any difference to the checks above. > } > > - if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) { ^^^^^^^^^^^^^^^^^^^^ Simply replace this with nominal_freq ? Because both lowest_nonlinear_freq and nominal_freq are in MHz at this point. > + if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > cpudata->nominal_freq) { > pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n", > - lowest_nonlinear_freq, min_freq, nominal_freq * 1000); > + lowest_nonlinear_freq, min_freq, cpudata->nominal_freq); ^^^^^^^^^^^^^^^^^^^^^^ Likewise, this can just be nominal_freq > return -EINVAL; > } > > -- > 2.43.0 > Other than that, Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com> -- Thanks and Regards gautham.
diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c index a261d7300951e..3a0a380c3590c 100644 --- a/drivers/cpufreq/amd-pstate-ut.c +++ b/drivers/cpufreq/amd-pstate-ut.c @@ -207,7 +207,6 @@ static void amd_pstate_ut_check_freq(u32 index) int cpu = 0; struct cpufreq_policy *policy = NULL; struct amd_cpudata *cpudata = NULL; - u32 nominal_freq_khz; for_each_possible_cpu(cpu) { policy = cpufreq_cpu_get(cpu); @@ -215,14 +214,13 @@ static void amd_pstate_ut_check_freq(u32 index) break; cpudata = policy->driver_data; - nominal_freq_khz = cpudata->nominal_freq*1000; - if (!((cpudata->max_freq >= nominal_freq_khz) && - (nominal_freq_khz > cpudata->lowest_nonlinear_freq) && + if (!((cpudata->max_freq >= cpudata->nominal_freq) && + (cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) && (cpudata->lowest_nonlinear_freq > cpudata->min_freq) && (cpudata->min_freq > 0))) { amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL; pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n", - __func__, cpu, cpudata->max_freq, nominal_freq_khz, + __func__, cpu, cpudata->max_freq, cpudata->nominal_freq, cpudata->lowest_nonlinear_freq, cpudata->min_freq); goto skip_test; } @@ -236,13 +234,13 @@ static void amd_pstate_ut_check_freq(u32 index) if (cpudata->boost_supported) { if ((policy->max == cpudata->max_freq) || - (policy->max == nominal_freq_khz)) + (policy->max == cpudata->nominal_freq)) amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS; else { amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL; pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n", __func__, cpu, policy->max, cpudata->max_freq, - nominal_freq_khz); + cpudata->nominal_freq); goto skip_test; } } else { diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index ce70d1bfa55d0..464db6745d84e 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -739,8 +739,8 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on) if (on) policy->cpuinfo.max_freq = max_freq; - else if (policy->cpuinfo.max_freq > nominal_freq * 1000) - policy->cpuinfo.max_freq = nominal_freq * 1000; + else if (policy->cpuinfo.max_freq > nominal_freq) + policy->cpuinfo.max_freq = nominal_freq; policy->max = policy->cpuinfo.max_freq; @@ -940,29 +940,29 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata) return ret; if (quirks && quirks->lowest_freq) - min_freq = quirks->lowest_freq * 1000; + min_freq = quirks->lowest_freq; else - min_freq = cppc_perf.lowest_freq * 1000; + min_freq = cppc_perf.lowest_freq; if (quirks && quirks->nominal_freq) - nominal_freq = quirks->nominal_freq ; + nominal_freq = quirks->nominal_freq; else nominal_freq = cppc_perf.nominal_freq; nominal_perf = READ_ONCE(cpudata->nominal_perf); boost_ratio = div_u64(cpudata->highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf); - max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000; + max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT); lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf); lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT, nominal_perf); - lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT) * 1000; + lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT); - WRITE_ONCE(cpudata->min_freq, min_freq); - WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq); - WRITE_ONCE(cpudata->nominal_freq, nominal_freq); - WRITE_ONCE(cpudata->max_freq, max_freq); + WRITE_ONCE(cpudata->min_freq, min_freq * 1000); + WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq * 1000); + WRITE_ONCE(cpudata->nominal_freq, nominal_freq * 1000); + WRITE_ONCE(cpudata->max_freq, max_freq * 1000); /** * Below values need to be initialized correctly, otherwise driver will fail to load @@ -970,15 +970,15 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata) * lowest_nonlinear_freq is a value between [min_freq, nominal_freq] * Check _CPC in ACPI table objects if any values are incorrect */ - if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) { + if (min_freq <= 0 || max_freq <= 0 || cpudata->nominal_freq <= 0 || min_freq > max_freq) { pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n", - min_freq, max_freq, nominal_freq * 1000); + min_freq, max_freq, cpudata->nominal_freq); return -EINVAL; } - if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) { + if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > cpudata->nominal_freq) { pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n", - lowest_nonlinear_freq, min_freq, nominal_freq * 1000); + lowest_nonlinear_freq, min_freq, cpudata->nominal_freq); return -EINVAL; }
Storing values in the cpudata structure in different units leads to confusion and hardcoded conversions elsewhere. After ratios are calculated store everything in khz for any future use. Adjust all relevant consumers for this change as well. Suggested-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/cpufreq/amd-pstate-ut.c | 12 +++++------- drivers/cpufreq/amd-pstate.c | 30 +++++++++++++++--------------- 2 files changed, 20 insertions(+), 22 deletions(-)