diff mbox series

[v7,6/6] cpufreq:amd-pstate: initialize capabilities in amd_pstate_init_perf

Message ID e450342257c4c80d42245723b273a80a521c00fb.1710323410.git.perry.yuan@amd.com (mailing list archive)
State Changes Requested, archived
Headers show
Series AMD Pstate Fixes And Enhancements | expand

Commit Message

Yuan, Perry March 13, 2024, 9:59 a.m. UTC 
Moved the initialization of some perf and frequency values related
to cpudata to the amd_pstate_init_perf and cppc_init_perf functions.
It can avoid duplicate calls to cppc_get_perf_caps function.

Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 43 ++++++++++++++----------------------
 include/linux/amd-pstate.h   |  1 +
 2 files changed, 18 insertions(+), 26 deletions(-)

Comments

Gautham R.Shenoy March 14, 2024, 6:38 a.m. UTC | #1 
Hello Perry,

On Wed, Mar 13, 2024 at 05:59:18PM +0800, Perry Yuan wrote:
> Moved the initialization of some perf and frequency values related
> to cpudata to the amd_pstate_init_perf and cppc_init_perf functions.
> It can avoid duplicate calls to cppc_get_perf_caps function.

Does it make sense to fold this into Patch 2 where you are caching the
nominal frequency for later use ?

Otherwise, this patch looks good to me.

--
Thanks and Regards
gautham.

> 
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 43 ++++++++++++++----------------------
>  include/linux/amd-pstate.h   |  1 +
>  2 files changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 59bcdf829c93..3877d4ecb5d4 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -330,12 +330,18 @@ static int pstate_init_perf(struct amd_cpudata *cpudata)
>  {
>  	u64 cap1;
>  	u32 highest_perf;
> +	struct cppc_perf_caps cppc_perf;
> +	int ret;
>  
> -	int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
> +	ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
>  				     &cap1);
>  	if (ret)
>  		return ret;
>  
> +	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> +	if (ret)
> +		return ret;
> +
>  	/* For platforms that do not support the preferred core feature, the
>  	 * highest_pef may be configured with 166 or 255, to avoid max frequency
>  	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
> @@ -353,6 +359,9 @@ static int pstate_init_perf(struct amd_cpudata *cpudata)
>  	WRITE_ONCE(cpudata->lowest_perf, AMD_CPPC_LOWEST_PERF(cap1));
>  	WRITE_ONCE(cpudata->prefcore_ranking, AMD_CPPC_HIGHEST_PERF(cap1));
>  	WRITE_ONCE(cpudata->min_limit_perf, AMD_CPPC_LOWEST_PERF(cap1));
> +	WRITE_ONCE(cpudata->lowest_freq, cppc_perf.lowest_freq);
> +	WRITE_ONCE(cpudata->nominal_freq, cppc_perf.nominal_freq);
> +
>  	return 0;
>  }
>  
> @@ -360,8 +369,9 @@ static int cppc_init_perf(struct amd_cpudata *cpudata)
>  {
>  	struct cppc_perf_caps cppc_perf;
>  	u32 highest_perf;
> +	int ret;
>  
> -	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> +	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
>  	if (ret)
>  		return ret;
>  
> @@ -378,6 +388,8 @@ static int cppc_init_perf(struct amd_cpudata *cpudata)
>  	WRITE_ONCE(cpudata->lowest_perf, cppc_perf.lowest_perf);
>  	WRITE_ONCE(cpudata->prefcore_ranking, cppc_perf.highest_perf);
>  	WRITE_ONCE(cpudata->min_limit_perf, cppc_perf.lowest_perf);
> +	WRITE_ONCE(cpudata->lowest_freq, cppc_perf.lowest_freq);
> +	WRITE_ONCE(cpudata->nominal_freq, cppc_perf.nominal_freq);
>  
>  	if (cppc_state == AMD_PSTATE_ACTIVE)
>  		return 0;
> @@ -642,17 +654,12 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>  
>  static int amd_get_min_freq(struct amd_cpudata *cpudata)
>  {
> -	struct cppc_perf_caps cppc_perf;
>  	u32 lowest_freq;
>  
> -	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> -	if (ret)
> -		return ret;
> -
>  	if (quirks && quirks->lowest_freq)
>  		lowest_freq = quirks->lowest_freq;
>  	else
> -		lowest_freq = cppc_perf.lowest_freq;
> +		lowest_freq = READ_ONCE(cpudata->lowest_freq);
>  
>  	/* Switch to khz */
>  	return lowest_freq * 1000;
> @@ -660,14 +667,9 @@ static int amd_get_min_freq(struct amd_cpudata *cpudata)
>  
>  static int amd_get_max_freq(struct amd_cpudata *cpudata)
>  {
> -	struct cppc_perf_caps cppc_perf;
>  	u32 max_perf, max_freq, nominal_freq, nominal_perf;
>  	u64 boost_ratio;
>  
> -	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> -	if (ret)
> -		return ret;
> -
>  	nominal_freq = READ_ONCE(cpudata->nominal_freq);
>  	nominal_perf = READ_ONCE(cpudata->nominal_perf);
>  	max_perf = READ_ONCE(cpudata->highest_perf);
> @@ -683,36 +685,25 @@ static int amd_get_max_freq(struct amd_cpudata *cpudata)
>  
>  static int amd_get_nominal_freq(struct amd_cpudata *cpudata)
>  {
> -	struct cppc_perf_caps cppc_perf;
>  	u32 nominal_freq;
>  
> -	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> -	if (ret)
> -		return ret;
> -
>  	if (quirks && quirks->nominal_freq)
>  		nominal_freq = quirks->nominal_freq;
>  	else
> -		nominal_freq = cppc_perf.nominal_freq;
> +		nominal_freq = READ_ONCE(cpudata->nominal_freq);
>  
>  	return nominal_freq;
>  }
>  
>  static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
>  {
> -	struct cppc_perf_caps cppc_perf;
>  	u32 lowest_nonlinear_freq, lowest_nonlinear_perf,
>  	    nominal_freq, nominal_perf;
>  	u64 lowest_nonlinear_ratio;
>  
> -	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> -	if (ret)
> -		return ret;
> -
>  	nominal_freq = READ_ONCE(cpudata->nominal_freq);
>  	nominal_perf = READ_ONCE(cpudata->nominal_perf);
> -
> -	lowest_nonlinear_perf = cppc_perf.lowest_nonlinear_perf;
> +	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
>  
>  	lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
>  					 nominal_perf);
> diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> index 7b2cbb892fd9..1fbbe75c3dcc 100644
> --- a/include/linux/amd-pstate.h
> +++ b/include/linux/amd-pstate.h
> @@ -88,6 +88,7 @@ struct amd_cpudata {
>  	u32	min_freq;
>  	u32	nominal_freq;
>  	u32	lowest_nonlinear_freq;
> +	u32	lowest_freq;
>  
>  	struct amd_aperf_mperf cur;
>  	struct amd_aperf_mperf prev;
> -- 
> 2.34.1
>
Yuan, Perry March 14, 2024, 8:24 a.m. UTC | #2 
[AMD Official Use Only - General]

Hi Gautham,

> -----Original Message-----
> From: Shenoy, Gautham Ranjal <gautham.shenoy@amd.com>
> Sent: Thursday, March 14, 2024 2:39 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; Limonciello, Mario
> <Mario.Limonciello@amd.com>; viresh.kumar@linaro.org; Huang, Ray
> <Ray.Huang@amd.com>; Petkov, Borislav <Borislav.Petkov@amd.com>;
> Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v7 6/6] cpufreq:amd-pstate: initialize capabilities in
> amd_pstate_init_perf
>
> Hello Perry,
>
> On Wed, Mar 13, 2024 at 05:59:18PM +0800, Perry Yuan wrote:
> > Moved the initialization of some perf and frequency values related to
> > cpudata to the amd_pstate_init_perf and cppc_init_perf functions.
> > It can avoid duplicate calls to cppc_get_perf_caps function.
>
> Does it make sense to fold this into Patch 2 where you are caching the nominal
> frequency for later use ?
>
> Otherwise, this patch looks good to me.

That nominal perf change is reviewed by Mario,
This patch can be reviewed separately and the whole changes can be applied after that without function impact.
It will be simpler to look what we changed in this one.
diff mbox series

Patch

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 59bcdf829c93..3877d4ecb5d4 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -330,12 +330,18 @@  static int pstate_init_perf(struct amd_cpudata *cpudata)
 {
 	u64 cap1;
 	u32 highest_perf;
+	struct cppc_perf_caps cppc_perf;
+	int ret;
 
-	int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
+	ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
 				     &cap1);
 	if (ret)
 		return ret;
 
+	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
+	if (ret)
+		return ret;
+
 	/* For platforms that do not support the preferred core feature, the
 	 * highest_pef may be configured with 166 or 255, to avoid max frequency
 	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
@@ -353,6 +359,9 @@  static int pstate_init_perf(struct amd_cpudata *cpudata)
 	WRITE_ONCE(cpudata->lowest_perf, AMD_CPPC_LOWEST_PERF(cap1));
 	WRITE_ONCE(cpudata->prefcore_ranking, AMD_CPPC_HIGHEST_PERF(cap1));
 	WRITE_ONCE(cpudata->min_limit_perf, AMD_CPPC_LOWEST_PERF(cap1));
+	WRITE_ONCE(cpudata->lowest_freq, cppc_perf.lowest_freq);
+	WRITE_ONCE(cpudata->nominal_freq, cppc_perf.nominal_freq);
+
 	return 0;
 }
 
@@ -360,8 +369,9 @@  static int cppc_init_perf(struct amd_cpudata *cpudata)
 {
 	struct cppc_perf_caps cppc_perf;
 	u32 highest_perf;
+	int ret;
 
-	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
+	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
 	if (ret)
 		return ret;
 
@@ -378,6 +388,8 @@  static int cppc_init_perf(struct amd_cpudata *cpudata)
 	WRITE_ONCE(cpudata->lowest_perf, cppc_perf.lowest_perf);
 	WRITE_ONCE(cpudata->prefcore_ranking, cppc_perf.highest_perf);
 	WRITE_ONCE(cpudata->min_limit_perf, cppc_perf.lowest_perf);
+	WRITE_ONCE(cpudata->lowest_freq, cppc_perf.lowest_freq);
+	WRITE_ONCE(cpudata->nominal_freq, cppc_perf.nominal_freq);
 
 	if (cppc_state == AMD_PSTATE_ACTIVE)
 		return 0;
@@ -642,17 +654,12 @@  static void amd_pstate_adjust_perf(unsigned int cpu,
 
 static int amd_get_min_freq(struct amd_cpudata *cpudata)
 {
-	struct cppc_perf_caps cppc_perf;
 	u32 lowest_freq;
 
-	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
-	if (ret)
-		return ret;
-
 	if (quirks && quirks->lowest_freq)
 		lowest_freq = quirks->lowest_freq;
 	else
-		lowest_freq = cppc_perf.lowest_freq;
+		lowest_freq = READ_ONCE(cpudata->lowest_freq);
 
 	/* Switch to khz */
 	return lowest_freq * 1000;
@@ -660,14 +667,9 @@  static int amd_get_min_freq(struct amd_cpudata *cpudata)
 
 static int amd_get_max_freq(struct amd_cpudata *cpudata)
 {
-	struct cppc_perf_caps cppc_perf;
 	u32 max_perf, max_freq, nominal_freq, nominal_perf;
 	u64 boost_ratio;
 
-	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
-	if (ret)
-		return ret;
-
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
 	nominal_perf = READ_ONCE(cpudata->nominal_perf);
 	max_perf = READ_ONCE(cpudata->highest_perf);
@@ -683,36 +685,25 @@  static int amd_get_max_freq(struct amd_cpudata *cpudata)
 
 static int amd_get_nominal_freq(struct amd_cpudata *cpudata)
 {
-	struct cppc_perf_caps cppc_perf;
 	u32 nominal_freq;
 
-	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
-	if (ret)
-		return ret;
-
 	if (quirks && quirks->nominal_freq)
 		nominal_freq = quirks->nominal_freq;
 	else
-		nominal_freq = cppc_perf.nominal_freq;
+		nominal_freq = READ_ONCE(cpudata->nominal_freq);
 
 	return nominal_freq;
 }
 
 static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
 {
-	struct cppc_perf_caps cppc_perf;
 	u32 lowest_nonlinear_freq, lowest_nonlinear_perf,
 	    nominal_freq, nominal_perf;
 	u64 lowest_nonlinear_ratio;
 
-	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
-	if (ret)
-		return ret;
-
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
 	nominal_perf = READ_ONCE(cpudata->nominal_perf);
-
-	lowest_nonlinear_perf = cppc_perf.lowest_nonlinear_perf;
+	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
 
 	lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
 					 nominal_perf);
diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
index 7b2cbb892fd9..1fbbe75c3dcc 100644
--- a/include/linux/amd-pstate.h
+++ b/include/linux/amd-pstate.h
@@ -88,6 +88,7 @@  struct amd_cpudata {
 	u32	min_freq;
 	u32	nominal_freq;
 	u32	lowest_nonlinear_freq;
+	u32	lowest_freq;
 
 	struct amd_aperf_mperf cur;
 	struct amd_aperf_mperf prev;