powernv:stop: Use psscr_val,mask provided by firmware

From: "Gautham R. Shenoy" <[email protected]>

In the current code for powernv_add_idle_states, there is a lot of code duplication while initializing an idle state in powernv_states table.

Add an inline helper function to populate the powernv_states[] table for a given idle state. Invoke this for populating the "Nap", "Fastsleep" and the stop states in powernv_add_idle_states.

Signed-off-by Gautham R. Shenoy <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 82 +++++++++++++++++++++++----------------
 include/linux/cpuidle.h           |  1 +
 2 files changed, 49 insertions(+), 34 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +1 -0

index 7fe442c..11b22b9 100644
@@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void)
        return 0;
 }
 
+static inline void add_powernv_state(int index, const char *name,

drivers/cpuidle/cpuidle-powernv.c +5 -0

+
+       stop_psscr_table[index] = psscr_val;
+}
+
 static int powernv_add_idle_states(void)
 {
        struct device_node *power_mgt;

@@ -236,6 +258,7 @@ static int powernv_add_idle_states(void)
                "ibm,cpu-idle-state-residency-ns", residency_ns, dt_idle_states);
 
        for (i = 0; i < dt_idle_states; i++) {
+               unsigned int exit_latency, target_residency;
                /*
                 * If an idle state has exit latency beyond
                 * POWERNV_THRESHOLD_LATENCY_NS then don't use it

@@ -244,27 +267,30 @@ static int powernv_add_idle_states(void)
                if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS)
                        continue;
 

drivers/cpuidle/cpuidle-powernv.c +7 -13

                        /* Add NAP state */
-                       strcpy(powernv_states[nr_idle_states].name, "Nap");
-                       strcpy(powernv_states[nr_idle_states].desc, "Nap");
-                       powernv_states[nr_idle_states].flags = 0;
-                       powernv_states[nr_idle_states].target_residency = 100;
-                       powernv_states[nr_idle_states].enter = nap_loop;
+                       add_powernv_state(nr_idle_states, "Nap",
+                                         CPUIDLE_FLAG_NONE, nap_loop,
+                                         target_residency, exit_latency, 0);
                } else if ((flags[i] & OPAL_PM_STOP_INST_FAST) &&
                                !(flags[i] & OPAL_PM_TIMEBASE_STOP)) {
-                       strncpy(powernv_states[nr_idle_states].name,
-                               names[i], CPUIDLE_NAME_LEN);
-                       strncpy(powernv_states[nr_idle_states].desc,
-                               names[i], CPUIDLE_NAME_LEN);
-                       powernv_states[nr_idle_states].flags = 0;
-
-                       powernv_states[nr_idle_states].enter = stop_loop;
-                       stop_psscr_table[nr_idle_states] = psscr_val[i];
+                       add_powernv_state(nr_idle_states, names[i],
+                                         CPUIDLE_FLAG_NONE, stop_loop,
+                                         target_residency, exit_latency,
+                                         psscr_val[i]);
                }
 
                /*

drivers/cpuidle/cpuidle-powernv.c +8 -21

                        /* Add FASTSLEEP state */
-                       strcpy(powernv_states[nr_idle_states].name, "FastSleep");
-                       strcpy(powernv_states[nr_idle_states].desc, "FastSleep");
-                       powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP;
-                       powernv_states[nr_idle_states].target_residency = 300000;
-                       powernv_states[nr_idle_states].enter = fastsleep_loop;
+                       add_powernv_state(nr_idle_states, "FastSleep",
+                                         CPUIDLE_FLAG_TIMER_STOP,
+                                         fastsleep_loop,
+                                         target_residency, exit_latency, 0);
                } else if ((flags[i] & OPAL_PM_STOP_INST_DEEP) &&
                                (flags[i] & OPAL_PM_TIMEBASE_STOP)) {
-                       strncpy(powernv_states[nr_idle_states].name,
-                               names[i], CPUIDLE_NAME_LEN);
-                       strncpy(powernv_states[nr_idle_states].desc,
-                               names[i], CPUIDLE_NAME_LEN);
-
-                       powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP;
-                       powernv_states[nr_idle_states].enter = stop_loop;
-                       stop_psscr_table[nr_idle_states] = psscr_val[i];
+                       add_powernv_state(nr_idle_states, names[i],
+                                         CPUIDLE_FLAG_TIMER_STOP, stop_loop,
+                                         target_residency, exit_latency,
+                                         psscr_val[i]);
                }
 #endif
-               powernv_states[nr_idle_states].exit_latency =
-                               ((unsigned int)latency_ns[i]) / 1000;
-
-               if (!rc) {
-                       powernv_states[nr_idle_states].target_residency =
-                               ((unsigned int)residency_ns[i]) / 1000;
-               }
-
                nr_idle_states++;
        }
 out:

include/linux/cpuidle.h +1 -1

index bb31373..c4e10f8 100644
@@ -62,6 +62,7 @@ struct cpuidle_state {
 };
 
 /* Idle State Flags */
+#define CPUIDLE_FLAG_NONE       (0x00)
 #define CPUIDLE_FLAG_COUPLED   (0x02) /* state applies to multiple cpus */
 #define CPUIDLE_FLAG_TIMER_STOP (0x04)  /* timer is stopped on this state */
 
--

1.9.4

Quoted 23 lines · Gautham R. Shenoy

From: "Gautham R. Shenoy" <[email protected]>

In the current code for powernv_add_idle_states, there is a lot of code duplication while initializing an idle state in powernv_states table.

Add an inline helper function to populate the powernv_states[] table for a given idle state. Invoke this for populating the "Nap", "Fastsleep" and the stop states in powernv_add_idle_states.

Signed-off-by Gautham R. Shenoy <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 82 +++++++++++++++++++++++----------------
 include/linux/cpuidle.h           |  1 +
 2 files changed, 49 insertions(+), 34 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +0 -0

index 7fe442c..11b22b9 100644
@@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void)
        return 0;
 }
 

drivers/cpuidle/cpuidle-powernv.c static int powernv_cpuidle_driver_init(void) · line 180

Quoted 11 lines

drivers/cpuidle/cpuidle-powernv.c +11 -0

+static inline void add_powernv_state(int index, const char *name,
+                                    unsigned int flags,
+                                    int (*idle_fn)(struct cpuidle_device *,
+                                                   struct cpuidle_driver *,
+                                                   int),
+                                    unsigned int target_residency,
+                                    unsigned int exit_latency,
+                                    u64 psscr_val)
+{
+       strncpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN);
+       strncpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN);

If the supplied name is equal to CPUIDLE_NAME_LEN then strncpy() won't terminate the string. The least annoying fix is to memset() the whole structure to zero and set n to CPUIDLE_NAME_LEN - 1.

Quoted 23 lines · Gautham R. Shenoy

From: "Gautham R. Shenoy" <[email protected]>

In the current code for powernv_add_idle_states, there is a lot of code duplication while initializing an idle state in powernv_states table.

Add an inline helper function to populate the powernv_states[] table for a given idle state. Invoke this for populating the "Nap", "Fastsleep" and the stop states in powernv_add_idle_states.

Signed-off-by Gautham R. Shenoy <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 82 +++++++++++++++++++++++----------------
 include/linux/cpuidle.h           |  1 +
 2 files changed, 49 insertions(+), 34 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +0 -0

index 7fe442c..11b22b9 100644
@@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void)
        return 0;
 }
 

drivers/cpuidle/cpuidle-powernv.c static int powernv_cpuidle_driver_init(void) · line 180

Quoted 11 lines

drivers/cpuidle/cpuidle-powernv.c +11 -0

+static inline void add_powernv_state(int index, const char *name,
+                                    unsigned int flags,
+                                    int (*idle_fn)(struct cpuidle_device *,
+                                                   struct cpuidle_driver *,
+                                                   int),
+                                    unsigned int target_residency,
+                                    unsigned int exit_latency,
+                                    u64 psscr_val)
+{
+       strncpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN);
+       strncpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN);

If the supplied name is equal to CPUIDLE_NAME_LEN then strncpy() won't terminate the string. The least annoying fix is to memset() the whole structure to zero and set n to CPUIDLE_NAME_LEN - 1.

drivers/cpuidle/cpuidle-powernv.c static int powernv_cpuidle_driver_init(void) · line 187

Quoted 8 lines

drivers/cpuidle/cpuidle-powernv.c +7 -0

+       powernv_states[index].flags = flags;
+       powernv_states[index].target_residency = target_residency;
+       powernv_states[index].exit_latency = exit_latency;
+       powernv_states[index].enter = idle_fn;
+
+       if (idle_fn != stop_loop)
+               return;

This can probably be deleted. The nap/sleep loops don't look at the psscr setting and you've passed in a dummy value of zero anyway.

Quoted 20 lines

drivers/cpuidle/cpuidle-powernv.c +5 -0

+
+       stop_psscr_table[index] = psscr_val;
+}
+
 static int powernv_add_idle_states(void)
 {
        struct device_node *power_mgt;

@@ -236,6 +258,7 @@ static int powernv_add_idle_states(void)
                "ibm,cpu-idle-state-residency-ns", residency_ns, dt_idle_states);
 
        for (i = 0; i < dt_idle_states; i++) {
+               unsigned int exit_latency, target_residency;
                /*
                 * If an idle state has exit latency beyond
                 * POWERNV_THRESHOLD_LATENCY_NS then don't use it

@@ -244,27 +267,30 @@ static int powernv_add_idle_states(void)
                if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS)
                        continue;
 

drivers/cpuidle/cpuidle-powernv.c static int powernv_add_idle_states(void) · line 279

Quoted 10 lines

drivers/cpuidle/cpuidle-powernv.c +8 -0

+               exit_latency = ((unsigned int)latency_ns[i]) / 1000;
+               target_residency = (!rc) ? ((unsigned int)residency_ns[i]) : 0;
+               /*
+                * Firmware passes residency values in ns.
+                * cpuidle expects it in us.
+                */
+               target_residency /= 1000;
+
                /*
                 * Cpuidle accepts exit_latency and target_residency in us.

This comment says the same thing as the one above.

drivers/cpuidle/cpuidle-powernv.c static int powernv_add_idle_states(void) · line 283

Quoted 5 lines

drivers/cpuidle/cpuidle-powernv.c +1 -0

                 * Use default target_residency values if f/w does not expose it.
                 */
                if (flags[i] & OPAL_PM_NAP_ENABLED) {
+                       target_residency = 100;

Hmm, the above comment says that we should only use the default value for target_residency if firmware hasn't provided a value. Is there a reason we always use the hard coded value?

Quoted 28 lines

drivers/cpuidle/cpuidle-powernv.c +7 -13

                        /* Add NAP state */
-                       strcpy(powernv_states[nr_idle_states].name, "Nap");
-                       strcpy(powernv_states[nr_idle_states].desc, "Nap");
-                       powernv_states[nr_idle_states].flags = 0;
-                       powernv_states[nr_idle_states].target_residency = 100;
-                       powernv_states[nr_idle_states].enter = nap_loop;
+                       add_powernv_state(nr_idle_states, "Nap",
+                                         CPUIDLE_FLAG_NONE, nap_loop,
+                                         target_residency, exit_latency, 0);
                } else if ((flags[i] & OPAL_PM_STOP_INST_FAST) &&
                                !(flags[i] & OPAL_PM_TIMEBASE_STOP)) {
-                       strncpy(powernv_states[nr_idle_states].name,
-                               names[i], CPUIDLE_NAME_LEN);
-                       strncpy(powernv_states[nr_idle_states].desc,
-                               names[i], CPUIDLE_NAME_LEN);
-                       powernv_states[nr_idle_states].flags = 0;
-
-                       powernv_states[nr_idle_states].enter = stop_loop;
-                       stop_psscr_table[nr_idle_states] = psscr_val[i];
+                       add_powernv_state(nr_idle_states, names[i],
+                                         CPUIDLE_FLAG_NONE, stop_loop,
+                                         target_residency, exit_latency,
+                                         psscr_val[i]);
                }
 
                /*

drivers/cpuidle/cpuidle-powernv.c static int powernv_add_idle_states(void) · line 303

Quoted 4 lines

drivers/cpuidle/cpuidle-powernv.c +1 -0

@@ -274,32 +300,20 @@ static int powernv_add_idle_states(void)
 #ifdef CONFIG_TICK_ONESHOT
                if (flags[i] & OPAL_PM_SLEEP_ENABLED ||
                        flags[i] & OPAL_PM_SLEEP_ENABLED_ER1) {
+                       target_residency = 300000;

Same comment as above.

Quoted 49 lines

drivers/cpuidle/cpuidle-powernv.c +8 -21

                        /* Add FASTSLEEP state */
-                       strcpy(powernv_states[nr_idle_states].name, "FastSleep");
-                       strcpy(powernv_states[nr_idle_states].desc, "FastSleep");
-                       powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP;
-                       powernv_states[nr_idle_states].target_residency = 300000;
-                       powernv_states[nr_idle_states].enter = fastsleep_loop;
+                       add_powernv_state(nr_idle_states, "FastSleep",
+                                         CPUIDLE_FLAG_TIMER_STOP,
+                                         fastsleep_loop,
+                                         target_residency, exit_latency, 0);
                } else if ((flags[i] & OPAL_PM_STOP_INST_DEEP) &&
                                (flags[i] & OPAL_PM_TIMEBASE_STOP)) {
-                       strncpy(powernv_states[nr_idle_states].name,
-                               names[i], CPUIDLE_NAME_LEN);
-                       strncpy(powernv_states[nr_idle_states].desc,
-                               names[i], CPUIDLE_NAME_LEN);
-
-                       powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP;
-                       powernv_states[nr_idle_states].enter = stop_loop;
-                       stop_psscr_table[nr_idle_states] = psscr_val[i];
+                       add_powernv_state(nr_idle_states, names[i],
+                                         CPUIDLE_FLAG_TIMER_STOP, stop_loop,
+                                         target_residency, exit_latency,
+                                         psscr_val[i]);
                }
 #endif
-               powernv_states[nr_idle_states].exit_latency =
-                               ((unsigned int)latency_ns[i]) / 1000;
-
-               if (!rc) {
-                       powernv_states[nr_idle_states].target_residency =
-                               ((unsigned int)residency_ns[i]) / 1000;
-               }
-
                nr_idle_states++;
        }
 out:

include/linux/cpuidle.h +1 -0

index bb31373..c4e10f8 100644
@@ -62,6 +62,7 @@ struct cpuidle_state {
 };
 
 /* Idle State Flags */
+#define CPUIDLE_FLAG_NONE       (0x00)
 #define CPUIDLE_FLAG_COUPLED   (0x02) /* state applies to multiple cpus */
 #define CPUIDLE_FLAG_TIMER_STOP (0x04)  /* timer is stopped on this state */
 

Quoted 3 lines

--
1.9.4

Looks good otherwise.

Reviewed-by Oliver O'Halloran <[email protected]>

From: "Gautham R. Shenoy" <[email protected]>

Hi,

This is the second iteration of the patchset to use the psscr_val and psscr_mask provided by the firmware for each of the stop states.

The previous version can be found here:
https://lkml.org/lkml/2016/9/29/45

The main changes in this version are:

1) Add a helper function in powernv-cpuidle.c to help initialize the powernv_states cpuidle table.

2) Handle the older firmware which populates only the Requested Level (RL) fields of the psscr and psscr_mask in the device tree. This patchset ensures that in the presence of the older firmware, the other fields of PSSCR are initalized to sane default values.

Synopsis
==========
In the current implementation, the code for ISA v3.0 stop implementation has a couple of shortcomings.

a) The code hand-codes the values for ESL,EC,TR,MTL bits of PSSCR and uses only the RL field from the firmware. While this is not incorrect, since the hand-coded values are legitimate, it is not a very flexible design since the firmware has the capability to communicate these values via the "ibm,cpu-idle-state-psscr" and "ibm,cpu-idle-state-psscr-mask" properties. In case where the firmware provides values for these fields that is different from the hand-coded values, the current code will not work as intended.

b) Due to issue a), the current code assumes that ESL=EC=1 for all the stop states and hence the wakeup from the stop instruction will happen at 0x100, the system-reset vector. However, the ISA v3.0 allows the ESL=EC=0 behaviour where the corresponding stop-state loses no state and wakes up from the subsequent instruction. The current code doesn't handle this case.

This patch series addresses these issues.

The first patch in the series renames the existing IDLE_STATE_ENTER_SEQ macro to IDLE_STATE_ENTER_SEQ_NORET. It reuses the name IDLE_STATE_ENTER_SEQ for entering into stop-states which wake up at the subsequent instruction.

The second patch adds a helper function in cpuidle-powernv.c for initializing entries of the powernv_states[] table that is passed to the cpu-idle core. This eliminates some of the code duplication in the function that discovers and initializes the stop states.

The third patch in the series fixes issues a) and b) by ensuring that the psscr-value and the psscr-mask provided by the firmware are what will be used to set a particular stop state. It also adds support for handling wake-up from stop states which were entered with ESL=EC=0.

The third patch also handles the older firmware which sets only the Requested Level (RL) field in the psscr and psscr-mask exposed in the device tree. In the presence of such older firmware, this patch will set the default sane values for for remaining PSSCR fields (i.e PSLL, MTL, ESL, EC, and TR).

The skiboot patch populates all the relevant fields in the PSSCR values and the mask for all the stop states can be found here:
https://lists.ozlabs.org/pipermail/skiboot/2016-September/004869.html

The patches are based on top of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git fixes

Gautham R. Shenoy (3):
powernv:idle: Add IDLE_STATE_ENTER_SEQ_NORET macro cpuidle:powernv: Add helper function to populate powernv idle states.
powernv: Pass PSSCR value and mask to power9_idle_stop