[Skiboot] [PATCH v3 3/6] slw: Add Power9 idle states to power-mgt dt node
Michael Neuling
mikey at neuling.org
Mon Jun 6 16:01:32 AEST 2016
This looks mostly ok to me with some minor comments below. It seems
to follow much of what we do currently
> POWER ISA v3 defines a new idle processor core mechanism. In summary,
> a) new instruction named stop is added. This instruction replaces
> instructions like nap, sleep, rvwinkle.
> b) new per thread SPR named PSSCR is added which controls the behavior
> of stop instruction. This SPR subsumes PMICR.
>
> This patch adds the supported idle states to power-mgt dt node.
>
> It also introduces ibm,cpu-idle-state-psscr and
> ibm,cpu-idle-state-psscr-mask entries which exposes the value to be
> written to PSSCR to enter a given stop state. These entries replaces
> POWER8's counterparts ibm,cpu-idle-state-pmicr and
> ibm,cpu-idle-state-pmicr-mask.
Are these backwards and forwards compatible with the kernel?
Or don't we have to worry since it'll be invalid boot an older kernel
on p9 anyway, or a newer p9 kernel without upgrading your firmware to
p9?
> Signed-off-by: Shreyas B. Prabhu <shreyas at linux.vnet.ibm.com>
> ---
> hw/slw.c | 178 ++++++++++++++++++++++++++++++++++++++++++-----
------
> include/opal-api.h | 10 +++
> 2 files changed, 153 insertions(+), 35 deletions(-)
>
> diff --git a/hw/slw.c b/hw/slw.c
> index 68b61ea80ccd..d391cd70cc98 100644
> --- a/hw/slw.c
> +++ b/hw/slw.c
> @@ -397,8 +397,12 @@ struct cpu_idle_states {
> u32 latency_ns;
> u32 residency_ns;
> u32 flags;
> - u64 pmicr;
> - u64 pmicr_mask;
> + /*
> + * Register value/mask used to select different idle states.
> + * PMICR in POWER8 and PSSCR in POWER9
> + */
> + u64 pm_ctrl_reg_val;
> + u64 pm_ctrl_reg_mask;
You might want to pack this struct a bit to ensure there aren't holes
in it. There is currently a 32bit hole between flags and
pm_ctrl_reg_val.
The same goes for some local variables in your functions.
> };
>
> /* Flag definitions */
> @@ -435,8 +439,8 @@ static struct cpu_idle_states
power7_cpu_idle_states[] = {
> | 0*OPAL_PM_SLEEP_ENABLED \
> | 0*OPAL_PM_WINKLE_ENABLED \
> | 0*IDLE_USE_PMICR,
> - .pmicr = 0,
> - .pmicr_mask = 0 },
> + .pm_ctrl_reg_val = 0,
> + .pm_ctrl_reg_mask = 0 },
> };
>
> static struct cpu_idle_states power8_cpu_idle_states[] = {
> @@ -451,8 +455,8 @@ static struct cpu_idle_states
power8_cpu_idle_states[] = {
> | 0*IDLE_LOSE_FULL_CONTEXT \
> | 1*OPAL_PM_NAP_ENABLED \
> | 0*IDLE_USE_PMICR,
> - .pmicr = 0,
> - .pmicr_mask = 0 },
> + .pm_ctrl_reg_val = 0,
> + .pm_ctrl_reg_mask = 0 },
> { /* fast sleep (with workaround) */
> .name = "fastsleep_",
> .latency_ns = 40000,
> @@ -465,8 +469,8 @@ static struct cpu_idle_states
power8_cpu_idle_states[] = {
> | 1*OPAL_PM_SLEEP_ENABLED_ER1 \
> | 0*IDLE_USE_PMICR, /* Not enabled until deep
> states are available */
> - .pmicr = IDLE_FASTSLEEP_PMICR,
> - .pmicr_mask = IDLE_SLEEP_PMICR_MASK },
> + .pm_ctrl_reg_val = IDLE_FASTSLEEP_PMICR,
> + .pm_ctrl_reg_mask = IDLE_SLEEP_PMICR_MASK },
> { /* Winkle */
> .name = "winkle",
> .latency_ns = 10000000,
> @@ -485,10 +489,93 @@ static struct cpu_idle_states
power8_cpu_idle_states[] = {
> | 1*OPAL_PM_WINKLE_ENABLED \
> | 0*IDLE_USE_PMICR, /* Currently choosing deep vs
> fast via EX_PM_GP1 reg
*/
> - .pmicr = 0,
> - .pmicr_mask = 0 },
> + .pm_ctrl_reg_val = 0,
> + .pm_ctrl_reg_mask = 0 },
> };
>
> +/*
> + * cpu_idle_states for POWER9
> + * Note latency_ns and residency_ns are estimated values for now.
> + */
> +static struct cpu_idle_states power9_cpu_idle_states[] = {
> + {
> + .name = "stop0",
> + .latency_ns = 300,
> + .residency_ns = 3000,
> + .flags = 0*IDLE_DEC_STOP \
> + | 0*IDLE_TB_STOP \
> + | 0*IDLE_LOSE_USER_CONTEXT \
> + | 0*IDLE_LOSE_HYP_CONTEXT \
> + | 0*IDLE_LOSE_FULL_CONTEXT \
> + | 1*OPAL_PM_STOP_INST_FAST,
> + .pm_ctrl_reg_val = 0,
> + .pm_ctrl_reg_mask = 0xF },
> + {
> + .name = "stop1",
> + .latency_ns = 5000,
> + .residency_ns = 50000,
> + .flags = 0*IDLE_DEC_STOP \
> + | 0*IDLE_TB_STOP \
> + | 1*IDLE_LOSE_USER_CONTEXT \
> + | 0*IDLE_LOSE_HYP_CONTEXT \
> + | 0*IDLE_LOSE_FULL_CONTEXT \
> + | 1*OPAL_PM_STOP_INST_FAST,
> + .pm_ctrl_reg_val = 1,
> + .pm_ctrl_reg_mask = 0xF },
> + {
> + .name = "stop2",
> + .latency_ns = 10000,
> + .residency_ns = 100000,
> + .flags = 0*IDLE_DEC_STOP \
> + | 0*IDLE_TB_STOP \
> + | 1*IDLE_LOSE_USER_CONTEXT \
> + | 0*IDLE_LOSE_HYP_CONTEXT \
> + | 0*IDLE_LOSE_FULL_CONTEXT \
> + | 1*OPAL_PM_STOP_INST_FAST,
> + .pm_ctrl_reg_val = 2,
> + .pm_ctrl_reg_mask = 0xF },
> +
> + {
> + .name = "stop4",
> + .latency_ns = 100000,
> + .residency_ns = 1000000,
> + .flags = 1*IDLE_DEC_STOP \
> + | 1*IDLE_TB_STOP \
> + | 1*IDLE_LOSE_USER_CONTEXT \
> + | 1*IDLE_LOSE_HYP_CONTEXT \
> + | 1*IDLE_LOSE_FULL_CONTEXT \
> + | 1*OPAL_PM_STOP_INST_DEEP,
> + .pm_ctrl_reg_val = 4,
> + .pm_ctrl_reg_mask = 0xF },
> +
> + {
> + .name = "stop8",
> + .latency_ns = 2000000,
> + .residency_ns = 20000000,
> + .flags = 1*IDLE_DEC_STOP \
> + | 1*IDLE_TB_STOP \
> + | 1*IDLE_LOSE_USER_CONTEXT \
> + | 1*IDLE_LOSE_HYP_CONTEXT \
> + | 1*IDLE_LOSE_FULL_CONTEXT \
> + | 1*OPAL_PM_STOP_INST_DEEP,
> + .pm_ctrl_reg_val = 0x8,
> + .pm_ctrl_reg_mask = 0xF },
> +
> +
> + {
> + .name = "stop11",
> + .latency_ns = 10000000,
> + .residency_ns = 100000000,
> + .flags = 1*IDLE_DEC_STOP \
> + | 1*IDLE_TB_STOP \
> + | 1*IDLE_LOSE_USER_CONTEXT \
> + | 1*IDLE_LOSE_HYP_CONTEXT \
> + | 1*IDLE_LOSE_FULL_CONTEXT \
> + | 1*OPAL_PM_STOP_INST_DEEP,
> + .pm_ctrl_reg_val = 0xB,
> + .pm_ctrl_reg_mask = 0xF },
Why 0, 1, 2, 4, 8 and 11? Why don't we use any of the others?
> +
> +};
> /* Add device tree properties to describe idle states */
> void add_cpu_idle_state_properties(void)
> {
> @@ -499,6 +586,7 @@ void add_cpu_idle_state_properties(void)
>
> bool can_sleep = true;
> bool has_slw = true;
> + bool has_stop_inst = false;
> u8 i;
>
> u32 supported_states_mask;
> @@ -508,8 +596,8 @@ void add_cpu_idle_state_properties(void)
> u32 *latency_ns_buf;
> u32 *residency_ns_buf;
> u32 *flags_buf;
> - u64 *pmicr_buf;
> - u64 *pmicr_mask_buf;
> + u64 *pm_ctrl_reg_val_buf;
> + u64 *pm_ctrl_reg_mask_buf;
>
> /* Variables to track buffer length */
> u8 name_buf_len;
> @@ -538,7 +626,12 @@ void add_cpu_idle_state_properties(void)
> */
> chip = next_chip(NULL);
> assert(chip);
> - if (chip->type == PROC_CHIP_P8_MURANO ||
> + if (chip->type == PROC_CHIP_P9_NIMBUS ||
> + chip->type == PROC_CHIP_P9_CUMULUS) {
> + states = power9_cpu_idle_states;
> + nr_states = ARRAY_SIZE(power9_cpu_idle_states);
> + has_stop_inst = true;
> + } else if (chip->type == PROC_CHIP_P8_MURANO ||
> chip->type == PROC_CHIP_P8_VENICE ||
> chip->type == PROC_CHIP_P8_NAPLES) {
> const struct dt_property *p;
> @@ -586,8 +679,8 @@ void add_cpu_idle_state_properties(void)
> latency_ns_buf = (u32 *) malloc(nr_states *
sizeof(u32));
> residency_ns_buf= (u32 *) malloc(nr_states * sizeof(u32));
> flags_buf = (u32 *) malloc(nr_states * sizeof(u32));
> - pmicr_buf = (u64 *) malloc(nr_states * sizeof(u64));
> - pmicr_mask_buf = (u64 *) malloc(nr_states *
sizeof(u64));
> + pm_ctrl_reg_val_buf = (u64 *) malloc(nr_states *
sizeof(u64));
> + pm_ctrl_reg_mask_buf = (u64 *) malloc(nr_states *
sizeof(u64));
malloc returns a void *, so you shouldn't need these casts.
We don't check any of these returns either, although this is at boot,
so if we can't malloc, something is really screwed, so probably not
worth bothering with
>
> name_buf_len = 0;
> num_supported_idle_states = 0;
> @@ -597,13 +690,18 @@ void add_cpu_idle_state_properties(void)
> * set. Use this to only add supported idle states to the
> * device-tree
> */
> - supported_states_mask = OPAL_PM_NAP_ENABLED;
> - if (can_sleep)
> - supported_states_mask |= OPAL_PM_SLEEP_ENABLED |
> - OPAL_PM_SLEEP_ENABLED_ER1;
> - if (has_slw)
> - supported_states_mask |= OPAL_PM_WINKLE_ENABLED;
> -
> + if (has_stop_inst) {
> + supported_states_mask = OPAL_PM_STOP_INST_FAST;
> + if (has_slw)
> + supported_states_mask |= OPAL_PM_STOP_INST_DEEP;
> + } else {
> + supported_states_mask = OPAL_PM_NAP_ENABLED;
> + if (can_sleep)
> + supported_states_mask |= OPAL_PM_SLEEP_ENABLED |
> + OPAL_PM_SLEEP_ENABLED_ER
1;
> + if (has_slw)
> + supported_states_mask |= OPAL_PM_WINKLE_ENABLED;
> + }
> for (i = 0; i < nr_states; i++) {
> /* For each state, check if it is one of the supported
states. */
> if (states[i].flags & supported_states_mask) {
> @@ -623,11 +721,11 @@ void add_cpu_idle_state_properties(void)
A couple of unrelated changes...
You can avoid the indentation here by changing the if statement to
if (!(states[i].flags & supported_states_mask))
continue;
also, the
strcpy(name_buf, states[i].name);
should be:
strncpy(name_buf, states[i].name, MAX_NAME_LEN);
> *flags_buf = cpu_to_fdt32(states[i].flags);
> flags_buf++;
>
> - *pmicr_buf = cpu_to_fdt64(states[i].pmicr);
> - pmicr_buf++;
> + *pm_ctrl_reg_val_buf =
cpu_to_fdt64(states[i].pm_ctrl_reg_val);
> + pm_ctrl_reg_val_buf++;
>
> - *pmicr_mask_buf = cpu_to_fdt64(states[i].pmicr);
> - pmicr_mask_buf++;
> + *pm_ctrl_reg_mask_buf =
cpu_to_fdt64(states[i].pm_ctrl_reg_mask);
> + pm_ctrl_reg_mask_buf++;
>
> /* Increment buffer length trackers */
> name_buf_len += strlen(states[i].name) + 1;
> @@ -640,9 +738,8 @@ void add_cpu_idle_state_properties(void)
> latency_ns_buf -= num_supported_idle_states;
> residency_ns_buf -= num_supported_idle_states;
> flags_buf -= num_supported_idle_states;
> - pmicr_buf -= num_supported_idle_states;
> - pmicr_mask_buf -= num_supported_idle_states;
> -
> + pm_ctrl_reg_val_buf -= num_supported_idle_states;
> + pm_ctrl_reg_mask_buf -= num_supported_idle_states;
> /* Create dt properties with the buffer content */
> dt_add_property(power_mgt, "ibm,cpu-idle-state-names", name_buf,
> name_buf_len* sizeof(char));
> @@ -652,18 +749,29 @@ void add_cpu_idle_state_properties(void)
> residency_ns_buf, num_supported_idle_states *
sizeof(u32));
> dt_add_property(power_mgt, "ibm,cpu-idle-state-flags",
flags_buf,
> num_supported_idle_states * sizeof(u32));
> - dt_add_property(power_mgt, "ibm,cpu-idle-state-pmicr",
pmicr_buf,
> - num_supported_idle_states * sizeof(u64));
> - dt_add_property(power_mgt, "ibm,cpu-idle-state-pmicr-mask",
> - pmicr_mask_buf, num_supported_idle_states *
sizeof(u64));
>
> + if (has_stop_inst) {
> + dt_add_property(power_mgt, "ibm,cpu-idle-state-psscr",
> + pm_ctrl_reg_val_buf,
> + num_supported_idle_states *
sizeof(u64));
> + dt_add_property(power_mgt, "ibm,cpu-idle-state-psscr-
mask",
> + pm_ctrl_reg_mask_buf,
> + num_supported_idle_states *
sizeof(u64));
> + } else {
> + dt_add_property(power_mgt, "ibm,cpu-idle-state-pmicr",
> + pm_ctrl_reg_val_buf,
> + num_supported_idle_states *
sizeof(u64));
> + dt_add_property(power_mgt, "ibm,cpu-idle-state-pmicr-
mask",
> + pm_ctrl_reg_mask_buf,
> + num_supported_idle_states *
sizeof(u64));
> + }
> assert(alloced_name_buf == name_buf);
> free(alloced_name_buf);
> free(latency_ns_buf);
> free(residency_ns_buf);
> free(flags_buf);
> - free(pmicr_buf);
> - free(pmicr_mask_buf);
> + free(pm_ctrl_reg_val_buf);
> + free(pm_ctrl_reg_mask_buf);
> }
>
> #ifdef __HAVE_LIBPORE__
> diff --git a/include/opal-api.h b/include/opal-api.h
> index 0b7b0bb9493b..4c99db58e2e9 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -175,6 +175,16 @@
> #define OPAL_PM_WINKLE_ENABLED 0x00040000
> #define OPAL_PM_SLEEP_ENABLED_ER1 0x00080000 /* with workaround
*/
>
> +/*
> + * Flags for stop states. Use 2 bits to distinguish between
> + * deep and fast states. Deep states result in full context
> + * loss thereby requiring slw to partially restore state
> + * whereas fast state can function without the presence of
> + * slw.
> + */
> +#define OPAL_PM_STOP_INST_FAST 0x00100000
> +#define OPAL_PM_STOP_INST_DEEP 0x00200000
> +
> #ifndef __ASSEMBLY__
>
> /* Other enums */
More information about the Skiboot
mailing list