Re: [PATCH v3 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
Hi Michael, On Wed, Nov 23, 2016 at 08:49:08PM +1100, Michael Ellerman wrote: > "Gautham R. Shenoy"writes: > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c > > b/drivers/cpuidle/cpuidle-powernv.c > > index 7fe442c..9240e08 100644 > > --- a/drivers/cpuidle/cpuidle-powernv.c > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > @@ -243,28 +262,31 @@ static int powernv_add_idle_states(void) > > */ > > if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) > > continue; > > + /* > > +* Firmware passes residency and latency values in ns. > > +* cpuidle expects it in us. > > +*/ > > + exit_latency = ((unsigned int)latency_ns[i]) / 1000; > > + target_residency = (!rc) ? ((unsigned int)residency_ns[i]) : 0; > > + target_residency /= 1000; > > Urk. > > Can you just do it normally: > > if (rc == 0) > target_residency = (unsigned int)residency_ns[i] / 1000; Wrote this in a non-standard manner since the normal way would exceed 80 chars. > > I also don't see why you need the cast? It is a remnant from the previous code. But I see your point, the cast is redundant, and removing it will make it possible to implement this in the normal manner without checkpatch warning about it. > > cheers > -- Thanks and Regards gautham.
Re: [PATCH v3 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
Hi Michael, On Wed, Nov 23, 2016 at 08:49:08PM +1100, Michael Ellerman wrote: > "Gautham R. Shenoy" writes: > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c > > b/drivers/cpuidle/cpuidle-powernv.c > > index 7fe442c..9240e08 100644 > > --- a/drivers/cpuidle/cpuidle-powernv.c > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > @@ -243,28 +262,31 @@ static int powernv_add_idle_states(void) > > */ > > if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) > > continue; > > + /* > > +* Firmware passes residency and latency values in ns. > > +* cpuidle expects it in us. > > +*/ > > + exit_latency = ((unsigned int)latency_ns[i]) / 1000; > > + target_residency = (!rc) ? ((unsigned int)residency_ns[i]) : 0; > > + target_residency /= 1000; > > Urk. > > Can you just do it normally: > > if (rc == 0) > target_residency = (unsigned int)residency_ns[i] / 1000; Wrote this in a non-standard manner since the normal way would exceed 80 chars. > > I also don't see why you need the cast? It is a remnant from the previous code. But I see your point, the cast is redundant, and removing it will make it possible to implement this in the normal manner without checkpatch warning about it. > > cheers > -- Thanks and Regards gautham.
Re: [PATCH v3 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
"Gautham R. Shenoy"writes: > diff --git a/drivers/cpuidle/cpuidle-powernv.c > b/drivers/cpuidle/cpuidle-powernv.c > index 7fe442c..9240e08 100644 > --- a/drivers/cpuidle/cpuidle-powernv.c > +++ b/drivers/cpuidle/cpuidle-powernv.c > @@ -243,28 +262,31 @@ static int powernv_add_idle_states(void) >*/ > if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) > continue; > + /* > + * Firmware passes residency and latency values in ns. > + * cpuidle expects it in us. > + */ > + exit_latency = ((unsigned int)latency_ns[i]) / 1000; > + target_residency = (!rc) ? ((unsigned int)residency_ns[i]) : 0; > + target_residency /= 1000; Urk. Can you just do it normally: if (rc == 0) target_residency = (unsigned int)residency_ns[i] / 1000; I also don't see why you need the cast? cheers
Re: [PATCH v3 2/3] cpuidle:powernv: Add helper function to populate powernv idle states.
"Gautham R. Shenoy" writes: > diff --git a/drivers/cpuidle/cpuidle-powernv.c > b/drivers/cpuidle/cpuidle-powernv.c > index 7fe442c..9240e08 100644 > --- a/drivers/cpuidle/cpuidle-powernv.c > +++ b/drivers/cpuidle/cpuidle-powernv.c > @@ -243,28 +262,31 @@ static int powernv_add_idle_states(void) >*/ > if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) > continue; > + /* > + * Firmware passes residency and latency values in ns. > + * cpuidle expects it in us. > + */ > + exit_latency = ((unsigned int)latency_ns[i]) / 1000; > + target_residency = (!rc) ? ((unsigned int)residency_ns[i]) : 0; > + target_residency /= 1000; Urk. Can you just do it normally: if (rc == 0) target_residency = (unsigned int)residency_ns[i] / 1000; I also don't see why you need the cast? cheers
[PATCH v3 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 | 83 ++- include/linux/cpuidle.h | 1 + 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c index 7fe442c..9240e08 100644 --- a/drivers/cpuidle/cpuidle-powernv.c +++ b/drivers/cpuidle/cpuidle-powernv.c @@ -167,6 +167,24 @@ 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) +{ + strlcpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN); + strlcpy(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; + stop_psscr_table[index] = psscr_val; +} + static int powernv_add_idle_states(void) { struct device_node *power_mgt; @@ -236,6 +254,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 @@ -243,28 +262,31 @@ static int powernv_add_idle_states(void) */ if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) continue; + /* +* Firmware passes residency and latency values in ns. +* cpuidle expects it in us. +*/ + exit_latency = ((unsigned int)latency_ns[i]) / 1000; + target_residency = (!rc) ? ((unsigned int)residency_ns[i]) : 0; + target_residency /= 1000; /* -* Cpuidle accepts exit_latency and target_residency in us. -* Use default target_residency values if f/w does not expose it. +* For nap and fastsleep, use default target_residency +* values if f/w does not expose it. */ if (flags[i] & OPAL_PM_NAP_ENABLED) { + if (!rc) + 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 +296,21 @@ static int powernv_add_idle_states(void) #ifdef CONFIG_TICK_ONESHOT if (flags[i] &
[PATCH v3 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 | 83 ++- include/linux/cpuidle.h | 1 + 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c index 7fe442c..9240e08 100644 --- a/drivers/cpuidle/cpuidle-powernv.c +++ b/drivers/cpuidle/cpuidle-powernv.c @@ -167,6 +167,24 @@ 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) +{ + strlcpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN); + strlcpy(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; + stop_psscr_table[index] = psscr_val; +} + static int powernv_add_idle_states(void) { struct device_node *power_mgt; @@ -236,6 +254,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 @@ -243,28 +262,31 @@ static int powernv_add_idle_states(void) */ if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) continue; + /* +* Firmware passes residency and latency values in ns. +* cpuidle expects it in us. +*/ + exit_latency = ((unsigned int)latency_ns[i]) / 1000; + target_residency = (!rc) ? ((unsigned int)residency_ns[i]) : 0; + target_residency /= 1000; /* -* Cpuidle accepts exit_latency and target_residency in us. -* Use default target_residency values if f/w does not expose it. +* For nap and fastsleep, use default target_residency +* values if f/w does not expose it. */ if (flags[i] & OPAL_PM_NAP_ENABLED) { + if (!rc) + 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 +296,21 @@ static int powernv_add_idle_states(void) #ifdef CONFIG_TICK_ONESHOT if (flags[i] & OPAL_PM_SLEEP_ENABLED || flags[i] &