Re: [PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
Hi Oliver, Thanks for reviewing the patch! On Tue, Nov 01, 2016 at 07:32:58PM +1100, Oliver O'Halloran wrote: > On Thu, Oct 27, 2016 at 7:35 PM, Gautham R. Shenoy >wrote: > > From: "Gautham R. Shenoy" > > > > 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 > > --- > > drivers/cpuidle/cpuidle-powernv.c | 82 > > +++ > > include/linux/cpuidle.h | 1 + > > 2 files changed, 49 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c > > b/drivers/cpuidle/cpuidle-powernv.c > > index 7fe442c..11b22b9 100644 > > --- a/drivers/cpuidle/cpuidle-powernv.c > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) > > return 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. I will change this to use strlcpy as Paul suggested in the reply. > > > + 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. The subsequent patch adds the missing bits to the psscr_val if we are running on the older firmware. But in any case, as you rightly pointed out we don't use psscr_table in nap/sleep loops. So this can go. > > > + > > + 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; > > > > + 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. Will clean this up to reflect the state of the code. > > > * 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? Ah, good catch! I should be setting target_residency to the default value only if it is not set by the firmware. Will fix this. > > > /* Add NAP state */ > > - strcpy(powernv_states[nr_idle_states].name, "Nap"); > > - strcpy(powernv_states[nr_idle_states].desc, "Nap"); > > -
Re: [PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
Hi Oliver, Thanks for reviewing the patch! On Tue, Nov 01, 2016 at 07:32:58PM +1100, Oliver O'Halloran wrote: > On Thu, Oct 27, 2016 at 7:35 PM, Gautham R. Shenoy > wrote: > > From: "Gautham R. Shenoy" > > > > 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 > > --- > > drivers/cpuidle/cpuidle-powernv.c | 82 > > +++ > > include/linux/cpuidle.h | 1 + > > 2 files changed, 49 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c > > b/drivers/cpuidle/cpuidle-powernv.c > > index 7fe442c..11b22b9 100644 > > --- a/drivers/cpuidle/cpuidle-powernv.c > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) > > return 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. I will change this to use strlcpy as Paul suggested in the reply. > > > + 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. The subsequent patch adds the missing bits to the psscr_val if we are running on the older firmware. But in any case, as you rightly pointed out we don't use psscr_table in nap/sleep loops. So this can go. > > > + > > + 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; > > > > + 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. Will clean this up to reflect the state of the code. > > > * 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? Ah, good catch! I should be setting target_residency to the default value only if it is not set by the firmware. Will fix this. > > > /* 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; > > -
Re: [PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
On Tue, Nov 01, 2016 at 07:32:58PM +1100, Oliver O'Halloran wrote: > On Thu, Oct 27, 2016 at 7:35 PM, Gautham R. Shenoy >wrote: > > From: "Gautham R. Shenoy" > > > > 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 > > --- > > drivers/cpuidle/cpuidle-powernv.c | 82 > > +++ > > include/linux/cpuidle.h | 1 + > > 2 files changed, 49 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c > > b/drivers/cpuidle/cpuidle-powernv.c > > index 7fe442c..11b22b9 100644 > > --- a/drivers/cpuidle/cpuidle-powernv.c > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) > > return 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. Or he could use strlcpy() instead of strncpy(). Paul.
Re: [PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
On Tue, Nov 01, 2016 at 07:32:58PM +1100, Oliver O'Halloran wrote: > On Thu, Oct 27, 2016 at 7:35 PM, Gautham R. Shenoy > wrote: > > From: "Gautham R. Shenoy" > > > > 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 > > --- > > drivers/cpuidle/cpuidle-powernv.c | 82 > > +++ > > include/linux/cpuidle.h | 1 + > > 2 files changed, 49 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c > > b/drivers/cpuidle/cpuidle-powernv.c > > index 7fe442c..11b22b9 100644 > > --- a/drivers/cpuidle/cpuidle-powernv.c > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) > > return 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. Or he could use strlcpy() instead of strncpy(). Paul.
Re: [PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
On Thu, Oct 27, 2016 at 7:35 PM, Gautham R. Shenoywrote: > From: "Gautham R. Shenoy" > > 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 > --- > drivers/cpuidle/cpuidle-powernv.c | 82 > +++ > include/linux/cpuidle.h | 1 + > 2 files changed, 49 insertions(+), 34 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle-powernv.c > b/drivers/cpuidle/cpuidle-powernv.c > index 7fe442c..11b22b9 100644 > --- a/drivers/cpuidle/cpuidle-powernv.c > +++ b/drivers/cpuidle/cpuidle-powernv.c > @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) > return 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. > + 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. > + > + 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; > > + 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. > * 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? > /* 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, > -
Re: [PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
On Thu, Oct 27, 2016 at 7:35 PM, Gautham R. Shenoy wrote: > From: "Gautham R. Shenoy" > > 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 > --- > drivers/cpuidle/cpuidle-powernv.c | 82 > +++ > include/linux/cpuidle.h | 1 + > 2 files changed, 49 insertions(+), 34 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle-powernv.c > b/drivers/cpuidle/cpuidle-powernv.c > index 7fe442c..11b22b9 100644 > --- a/drivers/cpuidle/cpuidle-powernv.c > +++ b/drivers/cpuidle/cpuidle-powernv.c > @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) > return 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. > + 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. > + > + 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; > > + 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. > * 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? > /* 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); > -
[PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
From: "Gautham R. Shenoy"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 --- drivers/cpuidle/cpuidle-powernv.c | 82 +++ include/linux/cpuidle.h | 1 + 2 files changed, 49 insertions(+), 34 deletions(-) diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c index 7fe442c..11b22b9 100644 --- a/drivers/cpuidle/cpuidle-powernv.c +++ b/drivers/cpuidle/cpuidle-powernv.c @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) return 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); + 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; + + 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; + 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. * Use default target_residency values if f/w does not expose it. */ if (flags[i] & OPAL_PM_NAP_ENABLED) { + target_residency = 100; /* 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]); } /* @@ -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 = 30;
[PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
From: "Gautham R. Shenoy" 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 --- drivers/cpuidle/cpuidle-powernv.c | 82 +++ include/linux/cpuidle.h | 1 + 2 files changed, 49 insertions(+), 34 deletions(-) diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c index 7fe442c..11b22b9 100644 --- a/drivers/cpuidle/cpuidle-powernv.c +++ b/drivers/cpuidle/cpuidle-powernv.c @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) return 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); + 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; + + 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; + 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. * Use default target_residency values if f/w does not expose it. */ if (flags[i] & OPAL_PM_NAP_ENABLED) { + target_residency = 100; /* 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]); } /* @@ -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 = 30; /* Add FASTSLEEP state */ -