Re: [v3 PATCH 1/3] powernv-cpufreq: Add helper to extract pstate from PMSR

[Gautham R Shenoy]

Hi Balbir,

On Sun, Dec 17, 2017 at 02:04:03PM +1100, Balbir Singh wrote:
> On Wed, Dec 13, 2017 at 5:57 PM, Gautham R. Shenoy
>  wrote:
> > From: "Gautham R. Shenoy" 
> >
> > On POWERNV platform, the fields for pstates in the Power Management
> > Status Register (PMSR) and the Power Management Control Register
> > (PMCR) are 8-bits wide. On POWER8 the pstates are negatively numbered
> > while on POWER9 they are positively numbered.
> >
> > The device-tree exports pstates as 32-bit entries. The device-tree
> > implementation sign-extends the 8-bit pstate values to obtain the
> > corresponding 32-bit entry.
> >
> > Eg: On POWER8, a pstate value 0x82 [-126] is represented in the
> > device-tree as 0xfff82 while on POWER9, the same value 0x82 [130]
> > is represented in the device-tree as 0x0082.
> >
> > The powernv-cpufreq driver implementation represents pstates using the
> > integer type. In multiple places in the driver, the code interprets
> > the pstates extracted from the PMSR as a signed byte and assigns it to
> > a integer variable to get the sign-extention.
> >
> > On POWER9 platforms which have greater than 128 pstates, this results
> > in the driver performing incorrect sign-extention, and thereby
> > treating a legitimate pstate (say 130) as an invalid pstates (since it
> > is interpreted as -126).
> >
> > This patch fixes the issue by implementing a helper function to
> > extract Pstates from PMSR register, and correctly sign-extend it to be
> > consistent with the values provided by the device-tree.
> >
> > Signed-off-by: Gautham R. Shenoy 
> > ---
> 
> This looks better

Thanks for the Review.

> 
> Acked-by: Balbir Singh 
> 
> Balbir Singh
> 

Re: [v3 PATCH 1/3] powernv-cpufreq: Add helper to extract pstate from PMSR

[Gautham R Shenoy]

Hi Balbir,

On Sun, Dec 17, 2017 at 02:04:03PM +1100, Balbir Singh wrote:
> On Wed, Dec 13, 2017 at 5:57 PM, Gautham R. Shenoy
>  wrote:
> > From: "Gautham R. Shenoy" 
> >
> > On POWERNV platform, the fields for pstates in the Power Management
> > Status Register (PMSR) and the Power Management Control Register
> > (PMCR) are 8-bits wide. On POWER8 the pstates are negatively numbered
> > while on POWER9 they are positively numbered.
> >
> > The device-tree exports pstates as 32-bit entries. The device-tree
> > implementation sign-extends the 8-bit pstate values to obtain the
> > corresponding 32-bit entry.
> >
> > Eg: On POWER8, a pstate value 0x82 [-126] is represented in the
> > device-tree as 0xfff82 while on POWER9, the same value 0x82 [130]
> > is represented in the device-tree as 0x0082.
> >
> > The powernv-cpufreq driver implementation represents pstates using the
> > integer type. In multiple places in the driver, the code interprets
> > the pstates extracted from the PMSR as a signed byte and assigns it to
> > a integer variable to get the sign-extention.
> >
> > On POWER9 platforms which have greater than 128 pstates, this results
> > in the driver performing incorrect sign-extention, and thereby
> > treating a legitimate pstate (say 130) as an invalid pstates (since it
> > is interpreted as -126).
> >
> > This patch fixes the issue by implementing a helper function to
> > extract Pstates from PMSR register, and correctly sign-extend it to be
> > consistent with the values provided by the device-tree.
> >
> > Signed-off-by: Gautham R. Shenoy 
> > ---
> 
> This looks better

Thanks for the Review.

> 
> Acked-by: Balbir Singh 
> 
> Balbir Singh
> 

Re: [v3 PATCH 1/3] powernv-cpufreq: Add helper to extract pstate from PMSR

[Balbir Singh]

On Wed, Dec 13, 2017 at 5:57 PM, Gautham R. Shenoy
 wrote:
> From: "Gautham R. Shenoy" 
>
> On POWERNV platform, the fields for pstates in the Power Management
> Status Register (PMSR) and the Power Management Control Register
> (PMCR) are 8-bits wide. On POWER8 the pstates are negatively numbered
> while on POWER9 they are positively numbered.
>
> The device-tree exports pstates as 32-bit entries. The device-tree
> implementation sign-extends the 8-bit pstate values to obtain the
> corresponding 32-bit entry.
>
> Eg: On POWER8, a pstate value 0x82 [-126] is represented in the
> device-tree as 0xfff82 while on POWER9, the same value 0x82 [130]
> is represented in the device-tree as 0x0082.
>
> The powernv-cpufreq driver implementation represents pstates using the
> integer type. In multiple places in the driver, the code interprets
> the pstates extracted from the PMSR as a signed byte and assigns it to
> a integer variable to get the sign-extention.
>
> On POWER9 platforms which have greater than 128 pstates, this results
> in the driver performing incorrect sign-extention, and thereby
> treating a legitimate pstate (say 130) as an invalid pstates (since it
> is interpreted as -126).
>
> This patch fixes the issue by implementing a helper function to
> extract Pstates from PMSR register, and correctly sign-extend it to be
> consistent with the values provided by the device-tree.
>
> Signed-off-by: Gautham R. Shenoy 
> ---

This looks better

Acked-by: Balbir Singh 

Balbir Singh

Re: [v3 PATCH 1/3] powernv-cpufreq: Add helper to extract pstate from PMSR

[Balbir Singh]

On Wed, Dec 13, 2017 at 5:57 PM, Gautham R. Shenoy
 wrote:
> From: "Gautham R. Shenoy" 
>
> On POWERNV platform, the fields for pstates in the Power Management
> Status Register (PMSR) and the Power Management Control Register
> (PMCR) are 8-bits wide. On POWER8 the pstates are negatively numbered
> while on POWER9 they are positively numbered.
>
> The device-tree exports pstates as 32-bit entries. The device-tree
> implementation sign-extends the 8-bit pstate values to obtain the
> corresponding 32-bit entry.
>
> Eg: On POWER8, a pstate value 0x82 [-126] is represented in the
> device-tree as 0xfff82 while on POWER9, the same value 0x82 [130]
> is represented in the device-tree as 0x0082.
>
> The powernv-cpufreq driver implementation represents pstates using the
> integer type. In multiple places in the driver, the code interprets
> the pstates extracted from the PMSR as a signed byte and assigns it to
> a integer variable to get the sign-extention.
>
> On POWER9 platforms which have greater than 128 pstates, this results
> in the driver performing incorrect sign-extention, and thereby
> treating a legitimate pstate (say 130) as an invalid pstates (since it
> is interpreted as -126).
>
> This patch fixes the issue by implementing a helper function to
> extract Pstates from PMSR register, and correctly sign-extend it to be
> consistent with the values provided by the device-tree.
>
> Signed-off-by: Gautham R. Shenoy 
> ---

This looks better

Acked-by: Balbir Singh 

Balbir Singh

[v3 PATCH 1/3] powernv-cpufreq: Add helper to extract pstate from PMSR

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" 

On POWERNV platform, the fields for pstates in the Power Management
Status Register (PMSR) and the Power Management Control Register
(PMCR) are 8-bits wide. On POWER8 the pstates are negatively numbered
while on POWER9 they are positively numbered.

The device-tree exports pstates as 32-bit entries. The device-tree
implementation sign-extends the 8-bit pstate values to obtain the
corresponding 32-bit entry.

Eg: On POWER8, a pstate value 0x82 [-126] is represented in the
device-tree as 0xfff82 while on POWER9, the same value 0x82 [130]
is represented in the device-tree as 0x0082.

The powernv-cpufreq driver implementation represents pstates using the
integer type. In multiple places in the driver, the code interprets
the pstates extracted from the PMSR as a signed byte and assigns it to
a integer variable to get the sign-extention.

On POWER9 platforms which have greater than 128 pstates, this results
in the driver performing incorrect sign-extention, and thereby
treating a legitimate pstate (say 130) as an invalid pstates (since it
is interpreted as -126).

This patch fixes the issue by implementing a helper function to
extract Pstates from PMSR register, and correctly sign-extend it to be
consistent with the values provided by the device-tree.

Signed-off-by: Gautham R. Shenoy 
---
 drivers/cpufreq/powernv-cpufreq.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index b6d7c4c..f46b60f 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -41,11 +41,9 @@
 #define POWERNV_MAX_PSTATES256
 #define PMSR_PSAFE_ENABLE  (1UL << 30)
 #define PMSR_SPR_EM_DISABLE(1UL << 31)
-#define PMSR_MAX(x)((x >> 32) & 0xFF)
+#define MAX_PSTATE_SHIFT   32
 #define LPSTATE_SHIFT  48
 #define GPSTATE_SHIFT  56
-#define GET_LPSTATE(x) (((x) >> LPSTATE_SHIFT) & 0xFF)
-#define GET_GPSTATE(x) (((x) >> GPSTATE_SHIFT) & 0xFF)
 
 #define MAX_RAMP_DOWN_TIME 5120
 /*
@@ -94,6 +92,7 @@ struct global_pstate_info {
 };
 
 static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
+u32 pstate_sign_prefix;
 static bool rebooting, throttled, occ_reset;
 
 static const char * const throttle_reason[] = {
@@ -148,6 +147,20 @@ enum throttle_reason_type {
bool wof_enabled;
 } powernv_pstate_info;
 
+static inline int extract_pstate(u64 pmsr_val, unsigned int shift)
+{
+   int ret = ((pmsr_val >> shift) & 0xFF);
+
+   if (!ret)
+   return ret;
+
+   return (pstate_sign_prefix | ret);
+}
+
+#define extract_local_pstate(x) extract_pstate(x, LPSTATE_SHIFT)
+#define extract_global_pstate(x) extract_pstate(x, GPSTATE_SHIFT)
+#define extract_max_pstate(x)  extract_pstate(x, MAX_PSTATE_SHIFT)
+
 /* Use following macros for conversions between pstate_id and index */
 static inline int idx_to_pstate(unsigned int i)
 {
@@ -278,6 +291,9 @@ static int init_powernv_pstates(void)
 
powernv_pstate_info.nr_pstates = nr_pstates;
pr_debug("NR PStates %d\n", nr_pstates);
+
+   pstate_sign_prefix = pstate_min & ~0xFF;
+
for (i = 0; i < nr_pstates; i++) {
u32 id = be32_to_cpu(pstate_ids[i]);
u32 freq = be32_to_cpu(pstate_freqs[i]);
@@ -438,17 +454,10 @@ struct powernv_smp_call_data {
 static void powernv_read_cpu_freq(void *arg)
 {
unsigned long pmspr_val;
-   s8 local_pstate_id;
struct powernv_smp_call_data *freq_data = arg;
 
pmspr_val = get_pmspr(SPRN_PMSR);
-
-   /*
-* The local pstate id corresponds bits 48..55 in the PMSR.
-* Note: Watch out for the sign!
-*/
-   local_pstate_id = (pmspr_val >> 48) & 0xFF;
-   freq_data->pstate_id = local_pstate_id;
+   freq_data->pstate_id = extract_local_pstate(pmspr_val);
freq_data->freq = pstate_id_to_freq(freq_data->pstate_id);
 
pr_debug("cpu %d pmsr %016lX pstate_id %d frequency %d kHz\n",
@@ -522,7 +531,7 @@ static void powernv_cpufreq_throttle_check(void *data)
chip = this_cpu_read(chip_info);
 
/* Check for Pmax Capping */
-   pmsr_pmax = (s8)PMSR_MAX(pmsr);
+   pmsr_pmax = extract_max_pstate(pmsr);
pmsr_pmax_idx = pstate_to_idx(pmsr_pmax);
if (pmsr_pmax_idx != powernv_pstate_info.max) {
if (chip->throttled)
@@ -645,8 +654,8 @@ void gpstate_timer_handler(struct timer_list *t)
 * value. Hence, read from PMCR to get correct data.
 */
val = get_pmspr(SPRN_PMCR);
-   freq_data.gpstate_id = (s8)GET_GPSTATE(val);
-   freq_data.pstate_id = (s8)GET_LPSTATE(val);
+   freq_data.gpstate_id = extract_global_pstate(val);
+   freq_data.pstate_id = extract_local_pstate(val);
if (freq_data.gpstate_id  == 

[v3 PATCH 1/3] powernv-cpufreq: Add helper to extract pstate from PMSR

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" 

On POWERNV platform, the fields for pstates in the Power Management
Status Register (PMSR) and the Power Management Control Register
(PMCR) are 8-bits wide. On POWER8 the pstates are negatively numbered
while on POWER9 they are positively numbered.

The device-tree exports pstates as 32-bit entries. The device-tree
implementation sign-extends the 8-bit pstate values to obtain the
corresponding 32-bit entry.

Eg: On POWER8, a pstate value 0x82 [-126] is represented in the
device-tree as 0xfff82 while on POWER9, the same value 0x82 [130]
is represented in the device-tree as 0x0082.

The powernv-cpufreq driver implementation represents pstates using the
integer type. In multiple places in the driver, the code interprets
the pstates extracted from the PMSR as a signed byte and assigns it to
a integer variable to get the sign-extention.

On POWER9 platforms which have greater than 128 pstates, this results
in the driver performing incorrect sign-extention, and thereby
treating a legitimate pstate (say 130) as an invalid pstates (since it
is interpreted as -126).

This patch fixes the issue by implementing a helper function to
extract Pstates from PMSR register, and correctly sign-extend it to be
consistent with the values provided by the device-tree.

Signed-off-by: Gautham R. Shenoy 
---
 drivers/cpufreq/powernv-cpufreq.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index b6d7c4c..f46b60f 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -41,11 +41,9 @@
 #define POWERNV_MAX_PSTATES256
 #define PMSR_PSAFE_ENABLE  (1UL << 30)
 #define PMSR_SPR_EM_DISABLE(1UL << 31)
-#define PMSR_MAX(x)((x >> 32) & 0xFF)
+#define MAX_PSTATE_SHIFT   32
 #define LPSTATE_SHIFT  48
 #define GPSTATE_SHIFT  56
-#define GET_LPSTATE(x) (((x) >> LPSTATE_SHIFT) & 0xFF)
-#define GET_GPSTATE(x) (((x) >> GPSTATE_SHIFT) & 0xFF)
 
 #define MAX_RAMP_DOWN_TIME 5120
 /*
@@ -94,6 +92,7 @@ struct global_pstate_info {
 };
 
 static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
+u32 pstate_sign_prefix;
 static bool rebooting, throttled, occ_reset;
 
 static const char * const throttle_reason[] = {
@@ -148,6 +147,20 @@ enum throttle_reason_type {
bool wof_enabled;
 } powernv_pstate_info;
 
+static inline int extract_pstate(u64 pmsr_val, unsigned int shift)
+{
+   int ret = ((pmsr_val >> shift) & 0xFF);
+
+   if (!ret)
+   return ret;
+
+   return (pstate_sign_prefix | ret);
+}
+
+#define extract_local_pstate(x) extract_pstate(x, LPSTATE_SHIFT)
+#define extract_global_pstate(x) extract_pstate(x, GPSTATE_SHIFT)
+#define extract_max_pstate(x)  extract_pstate(x, MAX_PSTATE_SHIFT)
+
 /* Use following macros for conversions between pstate_id and index */
 static inline int idx_to_pstate(unsigned int i)
 {
@@ -278,6 +291,9 @@ static int init_powernv_pstates(void)
 
powernv_pstate_info.nr_pstates = nr_pstates;
pr_debug("NR PStates %d\n", nr_pstates);
+
+   pstate_sign_prefix = pstate_min & ~0xFF;
+
for (i = 0; i < nr_pstates; i++) {
u32 id = be32_to_cpu(pstate_ids[i]);
u32 freq = be32_to_cpu(pstate_freqs[i]);
@@ -438,17 +454,10 @@ struct powernv_smp_call_data {
 static void powernv_read_cpu_freq(void *arg)
 {
unsigned long pmspr_val;
-   s8 local_pstate_id;
struct powernv_smp_call_data *freq_data = arg;
 
pmspr_val = get_pmspr(SPRN_PMSR);
-
-   /*
-* The local pstate id corresponds bits 48..55 in the PMSR.
-* Note: Watch out for the sign!
-*/
-   local_pstate_id = (pmspr_val >> 48) & 0xFF;
-   freq_data->pstate_id = local_pstate_id;
+   freq_data->pstate_id = extract_local_pstate(pmspr_val);
freq_data->freq = pstate_id_to_freq(freq_data->pstate_id);
 
pr_debug("cpu %d pmsr %016lX pstate_id %d frequency %d kHz\n",
@@ -522,7 +531,7 @@ static void powernv_cpufreq_throttle_check(void *data)
chip = this_cpu_read(chip_info);
 
/* Check for Pmax Capping */
-   pmsr_pmax = (s8)PMSR_MAX(pmsr);
+   pmsr_pmax = extract_max_pstate(pmsr);
pmsr_pmax_idx = pstate_to_idx(pmsr_pmax);
if (pmsr_pmax_idx != powernv_pstate_info.max) {
if (chip->throttled)
@@ -645,8 +654,8 @@ void gpstate_timer_handler(struct timer_list *t)
 * value. Hence, read from PMCR to get correct data.
 */
val = get_pmspr(SPRN_PMCR);
-   freq_data.gpstate_id = (s8)GET_GPSTATE(val);
-   freq_data.pstate_id = (s8)GET_LPSTATE(val);
+   freq_data.gpstate_id = extract_global_pstate(val);
+   freq_data.pstate_id = extract_local_pstate(val);
if (freq_data.gpstate_id  == freq_data.pstate_id) {