Re: [PATCH v2] cpuidle/powernv : Add Description for cpuidle state
On Tue, Jun 05, 2018 at 02:24:39PM +0530, Abhishek wrote: > > > On 06/04/2018 05:15 PM, Akshay Adiga wrote: > > On Mon, Jun 04, 2018 at 07:04:14PM +1000, Benjamin Herrenschmidt wrote: > > > Is this a new property ? I'm not fan of adding yet another of those > > > silly arrays. > > > > > > I would say this is the right time now to switch over to a node per > > > state instead, as we discussed with Vaidy. > > It is not a new property. Name was being used for description as description > was not present in device tree. A skiboot patch adding description to device > tree have been posted. This patch reads those description instead of copying > name itself into description. And we fall back to reading name into > description to not break the comaptibility with older firmware. >From a cpuidle point of view this is a exisiting property, but for powernv there was no device-tree property describing this. Abhishek has added the following skiboot patch for adding description for each idle state in device-tree . https://patchwork.ozlabs.org/patch/924879/ I agree this can go into new device tree format which each idle state as a node. Probably i will roll this patch into mine in the next version. > > Thanks > Abhishek > > > I posted the node based device tree here : > > skiboot patch : https://patchwork.ozlabs.org/patch/923120/ > > kernel patch : https://lkml.org/lkml/2018/5/30/1146 > > > > Do you have any inputs for this design ? > > > > > Additionally, while doing that, we can provide the versioning mechanism > > > I proposed so we can deal with state specific issues and erratas. > > > > > > Cheers, > > > Ben. > > > >
Re: [PATCH v2] cpuidle/powernv : Add Description for cpuidle state
On 06/04/2018 05:15 PM, Akshay Adiga wrote: On Mon, Jun 04, 2018 at 07:04:14PM +1000, Benjamin Herrenschmidt wrote: Is this a new property ? I'm not fan of adding yet another of those silly arrays. I would say this is the right time now to switch over to a node per state instead, as we discussed with Vaidy. It is not a new property. Name was being used for description as description was not present in device tree. A skiboot patch adding description to device tree have been posted. This patch reads those description instead of copying name itself into description. And we fall back to reading name into description to not break the comaptibility with older firmware. Thanks Abhishek I posted the node based device tree here : skiboot patch : https://patchwork.ozlabs.org/patch/923120/ kernel patch : https://lkml.org/lkml/2018/5/30/1146 Do you have any inputs for this design ? Additionally, while doing that, we can provide the versioning mechanism I proposed so we can deal with state specific issues and erratas. Cheers, Ben.
Re: [PATCH v2] cpuidle/powernv : Add Description for cpuidle state
On Mon, Jun 04, 2018 at 07:04:14PM +1000, Benjamin Herrenschmidt wrote: > Is this a new property ? I'm not fan of adding yet another of those > silly arrays. > > I would say this is the right time now to switch over to a node per > state instead, as we discussed with Vaidy. I posted the node based device tree here : skiboot patch : https://patchwork.ozlabs.org/patch/923120/ kernel patch : https://lkml.org/lkml/2018/5/30/1146 Do you have any inputs for this design ? > > Additionally, while doing that, we can provide the versioning mechanism > I proposed so we can deal with state specific issues and erratas. > > Cheers, > Ben. >
Re: [PATCH v2] cpuidle/powernv : Add Description for cpuidle state
On Mon, 2018-06-04 at 13:47 +0530, Abhishek Goel wrote:
> + if (of_property_read_string_array(power_mgt,
> + "ibm,cpu-idle-state-descs", descs, dt_idle_states) < 0) {
> + of_property_read_string_array(power_mgt,
> + "ibm,cpu-idle-state-names", descs,
> dt_idle_states);
> + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-descs in
> DT\n"
> + "Name will be used for description\n");
> + }
>
> /*
Is this a new property ? I'm not fan of adding yet another of those
silly arrays.
I would say this is the right time now to switch over to a node per
state instead, as we discussed with Vaidy.
Additionally, while doing that, we can provide the versioning mechanism
I proposed so we can deal with state specific issues and erratas.
Cheers,
Ben.
[PATCH v2] cpuidle/powernv : Add Description for cpuidle state
Names of cpuidle states were being used for description of states
in POWER as no descriptions were added in device tree. This patch
reads description for idle states which have been added in device
tree.
The description for idle states in case of POWER can be printed
using "cpupower monitor -l" or "cpupower idle-info".
Signed-off-by: Abhishek Goel
---
The skiboot patch which adds description for idle states in device tree
can be found here: https://patchwork.ozlabs.org/patch/924879/
drivers/cpuidle/cpuidle-powernv.c | 19 +++
include/linux/cpuidle.h | 2 +-
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-powernv.c
b/drivers/cpuidle/cpuidle-powernv.c
index 1a8234e706bc..08d8b0953a14 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -133,7 +133,7 @@ static int stop_loop(struct cpuidle_device *dev,
static struct cpuidle_state powernv_states[CPUIDLE_STATE_MAX] = {
{ /* Snooze */
.name = "snooze",
- .desc = "snooze",
+ .desc = "idle polling state",
.exit_latency = 0,
.target_residency = 0,
.enter = snooze_loop },
@@ -206,6 +206,7 @@ static int powernv_cpuidle_driver_init(void)
}
static inline void add_powernv_state(int index, const char *name,
+const char *desc,
unsigned int flags,
int (*idle_fn)(struct cpuidle_device *,
struct cpuidle_driver *,
@@ -215,7 +216,7 @@ static inline void add_powernv_state(int index, const char
*name,
u64 psscr_val, u64 psscr_mask)
{
strlcpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN);
- strlcpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN);
+ strlcpy(powernv_states[index].desc, desc, CPUIDLE_DESC_LEN);
powernv_states[index].flags = flags;
powernv_states[index].target_residency = target_residency;
powernv_states[index].exit_latency = exit_latency;
@@ -250,6 +251,7 @@ static int powernv_add_idle_states(void)
u64 psscr_val[CPUIDLE_STATE_MAX];
u64 psscr_mask[CPUIDLE_STATE_MAX];
const char *names[CPUIDLE_STATE_MAX];
+ const char *descs[CPUIDLE_STATE_MAX];
u32 has_stop_states = 0;
int i, rc;
u32 supported_flags = pnv_get_supported_cpuidle_states();
@@ -311,6 +313,13 @@ static int powernv_add_idle_states(void)
pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in
DT\n");
goto out;
}
+ if (of_property_read_string_array(power_mgt,
+ "ibm,cpu-idle-state-descs", descs, dt_idle_states) < 0) {
+ of_property_read_string_array(power_mgt,
+ "ibm,cpu-idle-state-names", descs,
dt_idle_states);
+ pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-descs in
DT\n"
+ "Name will be used for description\n");
+ }
/*
* If the idle states use stop instruction, probe for psscr values
@@ -414,10 +423,11 @@ static int powernv_add_idle_states(void)
target_residency = 100;
/* Add NAP state */
add_powernv_state(nr_idle_states, "Nap",
+ "stop processor execution",
CPUIDLE_FLAG_NONE, nap_loop,
target_residency, exit_latency, 0, 0);
} else if (has_stop_states && !stops_timebase) {
- add_powernv_state(nr_idle_states, names[i],
+ add_powernv_state(nr_idle_states, names[i], descs[i],
CPUIDLE_FLAG_NONE, stop_loop,
target_residency, exit_latency,
psscr_val[i], psscr_mask[i]);
@@ -434,11 +444,12 @@ static int powernv_add_idle_states(void)
target_residency = 30;
/* Add FASTSLEEP state */
add_powernv_state(nr_idle_states, "FastSleep",
+ "Core and L2 clock gating",
CPUIDLE_FLAG_TIMER_STOP,
fastsleep_loop,
target_residency, exit_latency, 0, 0);
} else if (has_stop_states && stops_timebase) {
- add_powernv_state(nr_idle_states, names[i],
+ add_powernv_state(nr_idle_states, names[i], descs[i],
CPUIDLE_FLAG_TIMER_STOP, stop_loop,
