[PATCH 0/8] cpupower: Updates and cleanup to support AMD Family 0x19

[PATCH 0/8] cpupower: Updates and cleanup to support AMD Family 0x19

[Nathan Fontenot]

Updates to the cpupower command to add support for AMD family 0x19
and cleanup the code to remove many of the family checks to hopefully
make any future family updates easier.

The first couple of patches are simple updates to rename the structs
in the msr_pstate union to better reflect current support and correcting
the name of the CPUPOWER_CAP_AMD_CPB cpuid cap flag.

Patches 3, 5, and 8 update the family checks to either replace
them with a new cpuid cap flag based off of cpuid checks or check for
family >= 0x17 where removing the direct family check isn't possible.

The reamianing patches are cleanups to remove unneeded extra enabled bit
checking, remove passing no longer used variables, and remove unused
variables in decode_pstates().

---

Nathan Fontenot (7):
      cpupower: Update msr_pstate union struct naming
      cpupower: Add CPUPOWER_CAP_AMD_HW_PSTATE cpuid caps flag
      cpupower: Remove unused pscur variable.
      cpupower: Update family checks when decoding HW pstates
      cpupower: Condense pstate enabled bit checks in decode_pstates()
      cpupower: Remove family arg to decode_pstates()
      cpupower: Add cpuid cap flag for MSR_AMD_HWCR support

Robert Richter (1):
      cpupower: Correct macro name for CPB caps flag


 tools/power/cpupower/utils/cpufreq-info.c    |    3 -
 tools/power/cpupower/utils/helpers/amd.c     |   62 +++++++++++---------------
 tools/power/cpupower/utils/helpers/cpuid.c   |   20 +++++++-
 tools/power/cpupower/utils/helpers/helpers.h |   14 +++---
 tools/power/cpupower/utils/helpers/misc.c    |    9 +---
 5 files changed, 54 insertions(+), 54 deletions(-)

--
Nathan Fontenot

[PATCH 1/8] cpupower: Update msr_pstate union struct naming

[Nathan Fontenot]

The msr_pstate union struct named fam17h_bits is misleading since
this is the struct to use for all families >= 0x17, not just
for family 0x17. Rename the bits structs to be 'pstate' (for pre
family 17h CPUs) and 'pstatedef' (for CPUs since fam 17h) to align
closer with PPR/BDKG naming.

There are no functional changes as part of this update.

Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
---
 tools/power/cpupower/utils/helpers/amd.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
index 7c4f83a8c973..34368436bbd6 100644
--- a/tools/power/cpupower/utils/helpers/amd.c
+++ b/tools/power/cpupower/utils/helpers/amd.c
@@ -13,7 +13,8 @@
 #define MSR_AMD_PSTATE		0xc0010064
 #define MSR_AMD_PSTATE_LIMIT	0xc0010061
 
-union msr_pstate {
+union core_pstate {
+	/* pre fam 17h: */
 	struct {
 		unsigned fid:6;
 		unsigned did:3;
@@ -26,7 +27,8 @@ union msr_pstate {
 		unsigned idddiv:2;
 		unsigned res3:21;
 		unsigned en:1;
-	} bits;
+	} pstate;
+	/* since fam 17h: */
 	struct {
 		unsigned fid:8;
 		unsigned did:6;
@@ -35,36 +37,36 @@ union msr_pstate {
 		unsigned idddiv:2;
 		unsigned res1:31;
 		unsigned en:1;
-	} fam17h_bits;
+	} pstatedef;
 	unsigned long long val;
 };
 
-static int get_did(int family, union msr_pstate pstate)
+static int get_did(int family, union core_pstate pstate)
 {
 	int t;
 
 	if (family == 0x12)
 		t = pstate.val & 0xf;
 	else if (family == 0x17 || family == 0x18)
-		t = pstate.fam17h_bits.did;
+		t = pstate.pstatedef.did;
 	else
-		t = pstate.bits.did;
+		t = pstate.pstate.did;
 
 	return t;
 }
 
-static int get_cof(int family, union msr_pstate pstate)
+static int get_cof(int family, union core_pstate pstate)
 {
 	int t;
 	int fid, did, cof;
 
 	did = get_did(family, pstate);
 	if (family == 0x17 || family == 0x18) {
-		fid = pstate.fam17h_bits.fid;
+		fid = pstate.pstatedef.fid;
 		cof = 200 * fid / did;
 	} else {
 		t = 0x10;
-		fid = pstate.bits.fid;
+		fid = pstate.pstate.fid;
 		if (family == 0x11)
 			t = 0x8;
 		cof = (100 * (fid + t)) >> did;
@@ -89,7 +91,7 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
 		   int boost_states, unsigned long *pstates, int *no)
 {
 	int i, psmax, pscur;
-	union msr_pstate pstate;
+	union core_pstate pstate;
 	unsigned long long val;
 
 	/* Only read out frequencies from HW when CPU might be boostable
@@ -119,9 +121,9 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
 		}
 		if (read_msr(cpu, MSR_AMD_PSTATE + i, &pstate.val))
 			return -1;
-		if ((cpu_family == 0x17) && (!pstate.fam17h_bits.en))
+		if ((cpu_family == 0x17) && (!pstate.pstatedef.en))
 			continue;
-		else if (!pstate.bits.en)
+		else if (!pstate.pstate.en)
 			continue;
 
 		pstates[i] = get_cof(cpu_family, pstate);

Re: [PATCH 1/8] cpupower: Update msr_pstate union struct naming

[Shuah Khan]

On 1/22/21 10:38 AM, Nathan Fontenot wrote:
> The msr_pstate union struct named fam17h_bits is misleading since
> this is the struct to use for all families >= 0x17, not just
> for family 0x17. Rename the bits structs to be 'pstate' (for pre
> family 17h CPUs) and 'pstatedef' (for CPUs since fam 17h) to align
> closer with PPR/BDKG naming.

What is PPR/PDKG - would be helpful to know what it is and provide
link to the doc if applicable.
> 
> There are no functional changes as part of this update.
> 
> Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
> ---
>   tools/power/cpupower/utils/helpers/amd.c |   26 ++++++++++++++------------
>   1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
> index 7c4f83a8c973..34368436bbd6 100644
> --- a/tools/power/cpupower/utils/helpers/amd.c
> +++ b/tools/power/cpupower/utils/helpers/amd.c
> @@ -13,7 +13,8 @@
>   #define MSR_AMD_PSTATE		0xc0010064
>   #define MSR_AMD_PSTATE_LIMIT	0xc0010061
>   
> -union msr_pstate {
> +union core_pstate {
> +	/* pre fam 17h: */
>   	struct {
>   		unsigned fid:6;
>   		unsigned did:3;
> @@ -26,7 +27,8 @@ union msr_pstate {
>   		unsigned idddiv:2;
>   		unsigned res3:21;
>   		unsigned en:1;
> -	} bits;
> +	} pstate;
> +	/* since fam 17h: */
>   	struct {
>   		unsigned fid:8;
>   		unsigned did:6;
> @@ -35,36 +37,36 @@ union msr_pstate {
>   		unsigned idddiv:2;
>   		unsigned res1:31;
>   		unsigned en:1;
> -	} fam17h_bits;
> +	} pstatedef;

Does pstatedef indicate this is pstate default?

>   	unsigned long long val;
>   };
>   
> -static int get_did(int family, union msr_pstate pstate)
> +static int get_did(int family, union core_pstate pstate)
>   {
>   	int t;
>   
>   	if (family == 0x12)
>   		t = pstate.val & 0xf;
>   	else if (family == 0x17 || family == 0x18)
> -		t = pstate.fam17h_bits.did;
> +		t = pstate.pstatedef.did;
>   	else
> -		t = pstate.bits.did;
> +		t = pstate.pstate.did;
>   
>   	return t;
>   }
>   
> -static int get_cof(int family, union msr_pstate pstate)
> +static int get_cof(int family, union core_pstate pstate)
>   {
>   	int t;
>   	int fid, did, cof;
>   
>   	did = get_did(family, pstate);
>   	if (family == 0x17 || family == 0x18) {
> -		fid = pstate.fam17h_bits.fid;
> +		fid = pstate.pstatedef.fid;
>   		cof = 200 * fid / did;
>   	} else {
>   		t = 0x10;
> -		fid = pstate.bits.fid;
> +		fid = pstate.pstate.fid;
>   		if (family == 0x11)
>   			t = 0x8;
>   		cof = (100 * (fid + t)) >> did;
> @@ -89,7 +91,7 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
>   		   int boost_states, unsigned long *pstates, int *no)
>   {
>   	int i, psmax, pscur;
> -	union msr_pstate pstate;
> +	union core_pstate pstate;
>   	unsigned long long val;
>   
>   	/* Only read out frequencies from HW when CPU might be boostable
> @@ -119,9 +121,9 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
>   		}
>   		if (read_msr(cpu, MSR_AMD_PSTATE + i, &pstate.val))
>   			return -1;
> -		if ((cpu_family == 0x17) && (!pstate.fam17h_bits.en))
> +		if ((cpu_family == 0x17) && (!pstate.pstatedef.en))
>   			continue;
> -		else if (!pstate.bits.en)
> +		else if (!pstate.pstate.en)
>   			continue;
>   
>   		pstates[i] = get_cof(cpu_family, pstate);
> 
> 

thanks,
-- Shuah

[PATCH 2/8] cpupower: Correct macro name for CPB caps flag

[Nathan Fontenot]

From: Robert Richter <rrichter@amd.com>

The name is Core Performance Boost (CPB) for the cpuid flag. Correct
cpuid caps flag to use this name (instead of CBP).

Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
---
 tools/power/cpupower/utils/helpers/cpuid.c   |    2 +-
 tools/power/cpupower/utils/helpers/helpers.h |    2 +-
 tools/power/cpupower/utils/helpers/misc.c    |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/cpuid.c b/tools/power/cpupower/utils/helpers/cpuid.c
index 73bfafc60e9b..f9a66a430b72 100644
--- a/tools/power/cpupower/utils/helpers/cpuid.c
+++ b/tools/power/cpupower/utils/helpers/cpuid.c
@@ -130,7 +130,7 @@ int get_cpu_info(struct cpupower_cpu_info *cpu_info)
 	    cpu_info->vendor == X86_VENDOR_HYGON) {
 		if (ext_cpuid_level >= 0x80000007 &&
 		    (cpuid_edx(0x80000007) & (1 << 9)))
-			cpu_info->caps |= CPUPOWER_CAP_AMD_CBP;
+			cpu_info->caps |= CPUPOWER_CAP_AMD_CPB;
 
 		if (ext_cpuid_level >= 0x80000008 &&
 		    cpuid_ebx(0x80000008) & (1 << 4))
diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index 0642e60a6ce1..a84f85a9dbd2 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -64,7 +64,7 @@ enum cpupower_cpu_vendor {X86_VENDOR_UNKNOWN = 0, X86_VENDOR_INTEL,
 
 #define CPUPOWER_CAP_INV_TSC		0x00000001
 #define CPUPOWER_CAP_APERF		0x00000002
-#define CPUPOWER_CAP_AMD_CBP		0x00000004
+#define CPUPOWER_CAP_AMD_CPB		0x00000004
 #define CPUPOWER_CAP_PERF_BIAS		0x00000008
 #define CPUPOWER_CAP_HAS_TURBO_RATIO	0x00000010
 #define CPUPOWER_CAP_IS_SNB		0x00000020
diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
index 650b9a9a6584..f9bcce9c72d5 100644
--- a/tools/power/cpupower/utils/helpers/misc.c
+++ b/tools/power/cpupower/utils/helpers/misc.c
@@ -26,7 +26,7 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
 	if (ret)
 		return ret;
 
-	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CBP) {
+	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB) {
 		*support = 1;
 
 		/* AMD Family 0x17 does not utilize PCI D18F4 like prior

[PATCH 3/8] cpupower: Add CPUPOWER_CAP_AMD_HW_PSTATE cpuid caps flag

[Nathan Fontenot]

Add a check in get_cpu_info() for the ability to read frequencies
from hardware and set the CPUPOWER_CAP_AMD_HW_PSTATE cpuid flag.
The cpuid flag is set when CPUID_80000007_EDX[7] is set,
which is all families >= 10h. The check excludes family 14h
because HW pstate reporting was not implemented on family 14h.

This is intended to reduce family checks in the main code paths.

Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
---
 tools/power/cpupower/utils/helpers/amd.c     |    6 +-----
 tools/power/cpupower/utils/helpers/cpuid.c   |   12 +++++++++---
 tools/power/cpupower/utils/helpers/helpers.h |    1 +
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
index 34368436bbd6..496844a20fe2 100644
--- a/tools/power/cpupower/utils/helpers/amd.c
+++ b/tools/power/cpupower/utils/helpers/amd.c
@@ -94,11 +94,7 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
 	union core_pstate pstate;
 	unsigned long long val;
 
-	/* Only read out frequencies from HW when CPU might be boostable
-	   to keep the code as short and clean as possible.
-	   Otherwise frequencies are exported via ACPI tables.
-	*/
-	if (cpu_family < 0x10 || cpu_family == 0x14)
+	if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_HW_PSTATE))
 		return -1;
 
 	if (read_msr(cpu, MSR_AMD_PSTATE_LIMIT, &val))
diff --git a/tools/power/cpupower/utils/helpers/cpuid.c b/tools/power/cpupower/utils/helpers/cpuid.c
index f9a66a430b72..d577220a193b 100644
--- a/tools/power/cpupower/utils/helpers/cpuid.c
+++ b/tools/power/cpupower/utils/helpers/cpuid.c
@@ -128,9 +128,15 @@ int get_cpu_info(struct cpupower_cpu_info *cpu_info)
 	/* AMD or Hygon Boost state enable/disable register */
 	if (cpu_info->vendor == X86_VENDOR_AMD ||
 	    cpu_info->vendor == X86_VENDOR_HYGON) {
-		if (ext_cpuid_level >= 0x80000007 &&
-		    (cpuid_edx(0x80000007) & (1 << 9)))
-			cpu_info->caps |= CPUPOWER_CAP_AMD_CPB;
+		if (ext_cpuid_level >= 0x80000007) {
+			if (cpuid_edx(0x80000007) & (1 << 9))
+				cpu_info->caps |= CPUPOWER_CAP_AMD_CPB;
+
+			if ((cpuid_edx(0x80000007) & (1 << 7)) &&
+			    cpu_info->family != 0x14)
+				/* HW pstate was not implemented in family 0x14 */
+				cpu_info->caps |= CPUPOWER_CAP_AMD_HW_PSTATE;
+		}
 
 		if (ext_cpuid_level >= 0x80000008 &&
 		    cpuid_ebx(0x80000008) & (1 << 4))
diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index a84f85a9dbd2..5f61eefff5b2 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -70,6 +70,7 @@ enum cpupower_cpu_vendor {X86_VENDOR_UNKNOWN = 0, X86_VENDOR_INTEL,
 #define CPUPOWER_CAP_IS_SNB		0x00000020
 #define CPUPOWER_CAP_INTEL_IDA		0x00000040
 #define CPUPOWER_CAP_AMD_RDPRU		0x00000080
+#define CPUPOWER_CAP_AMD_HW_PSTATE	0x00000100
 
 #define CPUPOWER_AMD_CPBDIS		0x02000000
 

Re: [PATCH 3/8] cpupower: Add CPUPOWER_CAP_AMD_HW_PSTATE cpuid caps flag

[Shuah Khan]

On 1/22/21 10:38 AM, Nathan Fontenot wrote:
> Add a check in get_cpu_info() for the ability to read frequencies
> from hardware and set the CPUPOWER_CAP_AMD_HW_PSTATE cpuid flag.
> The cpuid flag is set when CPUID_80000007_EDX[7] is set,
> which is all families >= 10h. The check excludes family 14h
> because HW pstate reporting was not implemented on family 14h.
> 
> This is intended to reduce family checks in the main code paths.
> 
> Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
> ---
>   tools/power/cpupower/utils/helpers/amd.c     |    6 +-----
>   tools/power/cpupower/utils/helpers/cpuid.c   |   12 +++++++++---
>   tools/power/cpupower/utils/helpers/helpers.h |    1 +
>   3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
> index 34368436bbd6..496844a20fe2 100644
> --- a/tools/power/cpupower/utils/helpers/amd.c
> +++ b/tools/power/cpupower/utils/helpers/amd.c
> @@ -94,11 +94,7 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
>   	union core_pstate pstate;
>   	unsigned long long val;
>   
> -	/* Only read out frequencies from HW when CPU might be boostable
> -	   to keep the code as short and clean as possible.
> -	   Otherwise frequencies are exported via ACPI tables.
> -	*/

Please update comment instead of deleting it.

> -	if (cpu_family < 0x10 || cpu_family == 0x14)
> +	if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_HW_PSTATE))
>   		return -1;
>   
>   	if (read_msr(cpu, MSR_AMD_PSTATE_LIMIT, &val))
> diff --git a/tools/power/cpupower/utils/helpers/cpuid.c b/tools/power/cpupower/utils/helpers/cpuid.c
> index f9a66a430b72..d577220a193b 100644
> --- a/tools/power/cpupower/utils/helpers/cpuid.c
> +++ b/tools/power/cpupower/utils/helpers/cpuid.c
> @@ -128,9 +128,15 @@ int get_cpu_info(struct cpupower_cpu_info *cpu_info)
>   	/* AMD or Hygon Boost state enable/disable register */
>   	if (cpu_info->vendor == X86_VENDOR_AMD ||
>   	    cpu_info->vendor == X86_VENDOR_HYGON) {
> -		if (ext_cpuid_level >= 0x80000007 &&
> -		    (cpuid_edx(0x80000007) & (1 << 9)))
> -			cpu_info->caps |= CPUPOWER_CAP_AMD_CPB;
> +		if (ext_cpuid_level >= 0x80000007) {
> +			if (cpuid_edx(0x80000007) & (1 << 9))
> +				cpu_info->caps |= CPUPOWER_CAP_AMD_CPB;
> +
> +			if ((cpuid_edx(0x80000007) & (1 << 7)) &&
> +			    cpu_info->family != 0x14)
> +				/* HW pstate was not implemented in family 0x14 */
> +				cpu_info->caps |= CPUPOWER_CAP_AMD_HW_PSTATE;
> +		}
>   
>   		if (ext_cpuid_level >= 0x80000008 &&
>   		    cpuid_ebx(0x80000008) & (1 << 4))
> diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
> index a84f85a9dbd2..5f61eefff5b2 100644
> --- a/tools/power/cpupower/utils/helpers/helpers.h
> +++ b/tools/power/cpupower/utils/helpers/helpers.h
> @@ -70,6 +70,7 @@ enum cpupower_cpu_vendor {X86_VENDOR_UNKNOWN = 0, X86_VENDOR_INTEL,
>   #define CPUPOWER_CAP_IS_SNB		0x00000020
>   #define CPUPOWER_CAP_INTEL_IDA		0x00000040
>   #define CPUPOWER_CAP_AMD_RDPRU		0x00000080
> +#define CPUPOWER_CAP_AMD_HW_PSTATE	0x00000100
>   
>   #define CPUPOWER_AMD_CPBDIS		0x02000000
>   
> 
> 

thanks,
-- Shuah

[PATCH 4/8] cpupower: Remove unused pscur variable.

[Nathan Fontenot]

The pscur variable is set but not uused, just remove it.

This may have previsously been set to validate the MSR_AMD_PSTATE_STATUS
MSR. With the addition of the CPUPOWER_CAP_AMD_HW_PSTATE cap flag this
is no longer needed since the cpuid bit to enable this cap flag also
validates that the MSR_AMD_PSTATE_STATUS MSR is present.

Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
---
 tools/power/cpupower/utils/helpers/amd.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
index 496844a20fe2..bd4db2e9a8a0 100644
--- a/tools/power/cpupower/utils/helpers/amd.c
+++ b/tools/power/cpupower/utils/helpers/amd.c
@@ -90,7 +90,7 @@ static int get_cof(int family, union core_pstate pstate)
 int decode_pstates(unsigned int cpu, unsigned int cpu_family,
 		   int boost_states, unsigned long *pstates, int *no)
 {
-	int i, psmax, pscur;
+	int i, psmax;
 	union core_pstate pstate;
 	unsigned long long val;
 
@@ -101,13 +101,6 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
 		return -1;
 
 	psmax = (val >> 4) & 0x7;
-
-	if (read_msr(cpu, MSR_AMD_PSTATE_STATUS, &val))
-		return -1;
-
-	pscur = val & 0x7;
-
-	pscur += boost_states;
 	psmax += boost_states;
 	for (i = 0; i <= psmax; i++) {
 		if (i >= MAX_HW_PSTATES) {

[PATCH 5/8] cpupower: Update family checks when decoding HW pstates

[Nathan Fontenot]

The family checks in get_cof() and get_did() need to use the
correct MSR format depending on the family. Add a cpupower
capability for using the pstatedef (family 17h and newer) to
control this instead of direct family checks.

Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
---
 tools/power/cpupower/utils/helpers/amd.c     |    8 ++++----
 tools/power/cpupower/utils/helpers/cpuid.c   |    6 +++++-
 tools/power/cpupower/utils/helpers/helpers.h |    1 +
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
index bd4db2e9a8a0..519a21e92666 100644
--- a/tools/power/cpupower/utils/helpers/amd.c
+++ b/tools/power/cpupower/utils/helpers/amd.c
@@ -45,10 +45,10 @@ static int get_did(int family, union core_pstate pstate)
 {
 	int t;
 
-	if (family == 0x12)
-		t = pstate.val & 0xf;
-	else if (family == 0x17 || family == 0x18)
+	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_PSTATEDEF)
 		t = pstate.pstatedef.did;
+	else if (family == 0x12)
+		t = pstate.val & 0xf;
 	else
 		t = pstate.pstate.did;
 
@@ -61,7 +61,7 @@ static int get_cof(int family, union core_pstate pstate)
 	int fid, did, cof;
 
 	did = get_did(family, pstate);
-	if (family == 0x17 || family == 0x18) {
+	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_PSTATEDEF) {
 		fid = pstate.pstatedef.fid;
 		cof = 200 * fid / did;
 	} else {
diff --git a/tools/power/cpupower/utils/helpers/cpuid.c b/tools/power/cpupower/utils/helpers/cpuid.c
index d577220a193b..db2e88ceb67b 100644
--- a/tools/power/cpupower/utils/helpers/cpuid.c
+++ b/tools/power/cpupower/utils/helpers/cpuid.c
@@ -133,9 +133,13 @@ int get_cpu_info(struct cpupower_cpu_info *cpu_info)
 				cpu_info->caps |= CPUPOWER_CAP_AMD_CPB;
 
 			if ((cpuid_edx(0x80000007) & (1 << 7)) &&
-			    cpu_info->family != 0x14)
+			    cpu_info->family != 0x14) {
 				/* HW pstate was not implemented in family 0x14 */
 				cpu_info->caps |= CPUPOWER_CAP_AMD_HW_PSTATE;
+
+				if (cpu_info->family >= 0x17)
+					cpu_info->caps |= CPUPOWER_CAP_AMD_PSTATEDEF;
+			}
 		}
 
 		if (ext_cpuid_level >= 0x80000008 &&
diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index 5f61eefff5b2..e4dc44ced770 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -71,6 +71,7 @@ enum cpupower_cpu_vendor {X86_VENDOR_UNKNOWN = 0, X86_VENDOR_INTEL,
 #define CPUPOWER_CAP_INTEL_IDA		0x00000040
 #define CPUPOWER_CAP_AMD_RDPRU		0x00000080
 #define CPUPOWER_CAP_AMD_HW_PSTATE	0x00000100
+#define CPUPOWER_CAP_AMD_PSTATEDEF	0x00000200
 
 #define CPUPOWER_AMD_CPBDIS		0x02000000
 

[PATCH 6/8] cpupower: Condense pstate enabled bit checks in decode_pstates()

[Nathan Fontenot]

The enabled bit (bit 63) is common for all families so we can remove
the multiple enabled checks based on family and have a common check
for HW pstate enabled.

Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
---
 tools/power/cpupower/utils/helpers/amd.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
index 519a21e92666..20694c3f367b 100644
--- a/tools/power/cpupower/utils/helpers/amd.c
+++ b/tools/power/cpupower/utils/helpers/amd.c
@@ -110,9 +110,9 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
 		}
 		if (read_msr(cpu, MSR_AMD_PSTATE + i, &pstate.val))
 			return -1;
-		if ((cpu_family == 0x17) && (!pstate.pstatedef.en))
-			continue;
-		else if (!pstate.pstate.en)
+
+		/* The enabled bit (bit 63) is common for all families */
+		if (!pstate.pstatedef.en)
 			continue;
 
 		pstates[i] = get_cof(cpu_family, pstate);

[PATCH 7/8] cpupower: Remove family arg to decode_pstates()

[Nathan Fontenot]

The decode_pstates() routine no longer uses the CPU family and
the caleed routines (get_cof() and get_did()) can grab the family
from the global cpupower_cpu_info struct. These update removes
passing the family arg to all these routines.

Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
---
 tools/power/cpupower/utils/cpufreq-info.c    |    3 +--
 tools/power/cpupower/utils/helpers/amd.c     |   19 +++++++++----------
 tools/power/cpupower/utils/helpers/helpers.h |    9 ++++-----
 3 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/tools/power/cpupower/utils/cpufreq-info.c b/tools/power/cpupower/utils/cpufreq-info.c
index 6efc0f6b1b11..f9895e31ff5a 100644
--- a/tools/power/cpupower/utils/cpufreq-info.c
+++ b/tools/power/cpupower/utils/cpufreq-info.c
@@ -186,8 +186,7 @@ static int get_boost_mode_x86(unsigned int cpu)
 	if ((cpupower_cpu_info.vendor == X86_VENDOR_AMD &&
 	     cpupower_cpu_info.family >= 0x10) ||
 	     cpupower_cpu_info.vendor == X86_VENDOR_HYGON) {
-		ret = decode_pstates(cpu, cpupower_cpu_info.family, b_states,
-				     pstates, &pstate_no);
+		ret = decode_pstates(cpu, b_states, pstates, &pstate_no);
 		if (ret)
 			return ret;
 
diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
index 20694c3f367b..01bb85121216 100644
--- a/tools/power/cpupower/utils/helpers/amd.c
+++ b/tools/power/cpupower/utils/helpers/amd.c
@@ -41,13 +41,13 @@ union core_pstate {
 	unsigned long long val;
 };
 
-static int get_did(int family, union core_pstate pstate)
+static int get_did(union core_pstate pstate)
 {
 	int t;
 
 	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_PSTATEDEF)
 		t = pstate.pstatedef.did;
-	else if (family == 0x12)
+	else if (cpupower_cpu_info.family == 0x12)
 		t = pstate.val & 0xf;
 	else
 		t = pstate.pstate.did;
@@ -55,19 +55,19 @@ static int get_did(int family, union core_pstate pstate)
 	return t;
 }
 
-static int get_cof(int family, union core_pstate pstate)
+static int get_cof(union core_pstate pstate)
 {
 	int t;
 	int fid, did, cof;
 
-	did = get_did(family, pstate);
+	did = get_did(pstate);
 	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_PSTATEDEF) {
 		fid = pstate.pstatedef.fid;
 		cof = 200 * fid / did;
 	} else {
 		t = 0x10;
 		fid = pstate.pstate.fid;
-		if (family == 0x11)
+		if (cpupower_cpu_info.family == 0x11)
 			t = 0x8;
 		cof = (100 * (fid + t)) >> did;
 	}
@@ -76,8 +76,7 @@ static int get_cof(int family, union core_pstate pstate)
 
 /* Needs:
  * cpu          -> the cpu that gets evaluated
- * cpu_family   -> The cpu's family (0x10, 0x12,...)
- * boots_states -> how much boost states the machines support
+ * boost_states -> how much boost states the machines support
  *
  * Fills up:
  * pstates -> a pointer to an array of size MAX_HW_PSTATES
@@ -87,8 +86,8 @@ static int get_cof(int family, union core_pstate pstate)
  *
  * returns zero on success, -1 on failure
  */
-int decode_pstates(unsigned int cpu, unsigned int cpu_family,
-		   int boost_states, unsigned long *pstates, int *no)
+int decode_pstates(unsigned int cpu, int boost_states,
+		   unsigned long *pstates, int *no)
 {
 	int i, psmax;
 	union core_pstate pstate;
@@ -115,7 +114,7 @@ int decode_pstates(unsigned int cpu, unsigned int cpu_family,
 		if (!pstate.pstatedef.en)
 			continue;
 
-		pstates[i] = get_cof(cpu_family, pstate);
+		pstates[i] = get_cof(pstate);
 	}
 	*no = i;
 	return 0;
diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index e4dc44ced770..8a0c11c6ec63 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -127,8 +127,8 @@ extern struct pci_dev *pci_slot_func_init(struct pci_access **pacc,
 
 /* AMD HW pstate decoding **************************/
 
-extern int decode_pstates(unsigned int cpu, unsigned int cpu_family,
-			  int boost_states, unsigned long *pstates, int *no);
+extern int decode_pstates(unsigned int cpu, int boost_states,
+			  unsigned long *pstates, int *no);
 
 /* AMD HW pstate decoding **************************/
 
@@ -145,9 +145,8 @@ unsigned int cpuid_edx(unsigned int op);
 /* cpuid and cpuinfo helpers  **************************/
 /* X86 ONLY ********************************************/
 #else
-static inline int decode_pstates(unsigned int cpu, unsigned int cpu_family,
-				 int boost_states, unsigned long *pstates,
-				 int *no)
+static inline int decode_pstates(unsigned int cpu, int boost_states,
+				 unsigned long *pstates, int *no)
 { return -1; };
 
 static inline int read_msr(int cpu, unsigned int idx, unsigned long long *val)

[PATCH 8/8] cpupower: Add cpuid cap flag for MSR_AMD_HWCR support

[Nathan Fontenot]

Remove the family check for accessing the MSR_AMD_HWCR MSR and replace
it with a cpupower cap flag.

This update also allows for the removal of the local cpupower_cpu_info
variable in cpufreq_has_boost_support() since we no longer need it to
check the family.

Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>
---
 tools/power/cpupower/utils/helpers/cpuid.c   |    6 +++++-
 tools/power/cpupower/utils/helpers/helpers.h |    1 +
 tools/power/cpupower/utils/helpers/misc.c    |    7 +------
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/power/cpupower/utils/helpers/cpuid.c b/tools/power/cpupower/utils/helpers/cpuid.c
index db2e88ceb67b..72eb43593180 100644
--- a/tools/power/cpupower/utils/helpers/cpuid.c
+++ b/tools/power/cpupower/utils/helpers/cpuid.c
@@ -129,9 +129,13 @@ int get_cpu_info(struct cpupower_cpu_info *cpu_info)
 	if (cpu_info->vendor == X86_VENDOR_AMD ||
 	    cpu_info->vendor == X86_VENDOR_HYGON) {
 		if (ext_cpuid_level >= 0x80000007) {
-			if (cpuid_edx(0x80000007) & (1 << 9))
+			if (cpuid_edx(0x80000007) & (1 << 9)) {
 				cpu_info->caps |= CPUPOWER_CAP_AMD_CPB;
 
+				if (cpu_info->family >= 0x17)
+					cpu_info->caps |= CPUPOWER_CAP_AMD_CPB_MSR;
+			}
+
 			if ((cpuid_edx(0x80000007) & (1 << 7)) &&
 			    cpu_info->family != 0x14) {
 				/* HW pstate was not implemented in family 0x14 */
diff --git a/tools/power/cpupower/utils/helpers/helpers.h b/tools/power/cpupower/utils/helpers/helpers.h
index 8a0c11c6ec63..33ffacee7fcb 100644
--- a/tools/power/cpupower/utils/helpers/helpers.h
+++ b/tools/power/cpupower/utils/helpers/helpers.h
@@ -72,6 +72,7 @@ enum cpupower_cpu_vendor {X86_VENDOR_UNKNOWN = 0, X86_VENDOR_INTEL,
 #define CPUPOWER_CAP_AMD_RDPRU		0x00000080
 #define CPUPOWER_CAP_AMD_HW_PSTATE	0x00000100
 #define CPUPOWER_CAP_AMD_PSTATEDEF	0x00000200
+#define CPUPOWER_CAP_AMD_CPB_MSR	0x00000400
 
 #define CPUPOWER_AMD_CPBDIS		0x02000000
 
diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
index f9bcce9c72d5..fc6e34511721 100644
--- a/tools/power/cpupower/utils/helpers/misc.c
+++ b/tools/power/cpupower/utils/helpers/misc.c
@@ -16,16 +16,11 @@
 int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
 			int *states)
 {
-	struct cpupower_cpu_info cpu_info;
 	int ret;
 	unsigned long long val;
 
 	*support = *active = *states = 0;
 
-	ret = get_cpu_info(&cpu_info);
-	if (ret)
-		return ret;
-
 	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB) {
 		*support = 1;
 
@@ -34,7 +29,7 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
 		 * has Hardware determined variable increments instead.
 		 */
 
-		if (cpu_info.family == 0x17 || cpu_info.family == 0x18) {
+		if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) {
 			if (!read_msr(cpu, MSR_AMD_HWCR, &val)) {
 				if (!(val & CPUPOWER_AMD_CPBDIS))
 					*active = 1;

Re: [PATCH 0/8] cpupower: Updates and cleanup to support AMD Family 0x19

[Robert Richter]

On 22.01.21 11:38:28, Nathan Fontenot wrote:
> Updates to the cpupower command to add support for AMD family 0x19
> and cleanup the code to remove many of the family checks to hopefully
> make any future family updates easier.
> 
> The first couple of patches are simple updates to rename the structs
> in the msr_pstate union to better reflect current support and correcting
> the name of the CPUPOWER_CAP_AMD_CPB cpuid cap flag.
> 
> Patches 3, 5, and 8 update the family checks to either replace
> them with a new cpuid cap flag based off of cpuid checks or check for
> family >= 0x17 where removing the direct family check isn't possible.
> 
> The reamianing patches are cleanups to remove unneeded extra enabled bit
> checking, remove passing no longer used variables, and remove unused
> variables in decode_pstates().
> 
> ---
> 
> Nathan Fontenot (7):
>       cpupower: Update msr_pstate union struct naming
>       cpupower: Add CPUPOWER_CAP_AMD_HW_PSTATE cpuid caps flag
>       cpupower: Remove unused pscur variable.
>       cpupower: Update family checks when decoding HW pstates
>       cpupower: Condense pstate enabled bit checks in decode_pstates()
>       cpupower: Remove family arg to decode_pstates()
>       cpupower: Add cpuid cap flag for MSR_AMD_HWCR support
> 
> Robert Richter (1):
>       cpupower: Correct macro name for CPB caps flag

For the whole series:

Reviewed-by: Robert Richter <rrichter@amd.com>