Skiboot patches v4: https://lists.ozlabs.org/pipermail/skiboot/2020-February/016404.html
v3 patches: https://lkml.org/lkml/2020/1/13/333
Changelog
v3 --> v4
Device tree interface change: Cease to use the active property for
self-save and self-restore and use only presence/absence of the node to
signify the status.
Currently the stop-API supports a mechanism called as self-restore
which allows us to restore the values of certain SPRs on wakeup from a
deep-stop state to a desired value. To use this, the Kernel makes an
OPAL call passing the PIR of the CPU, the SPR number and the value to
which the SPR should be restored when that CPU wakes up from a deep
stop state.
Recently, a new feature, named self-save has been enabled in the
stop-api, which is an alternative mechanism to do the same, except
that self-save will save the current content of the SPR before
entering a deep stop state and also restore the content back on
waking up from a deep stop state.
This patch series aims at introducing and leveraging the self-save feature in
the kernel.
Now, as the kernel has a choice to prefer one mode over the other and
there can be registers in both the save/restore SPR list which are sent
from the device tree, a new interface has been defined for the seamless
handing of the modes for each SPR.
A list of preferred SPRs are maintained in the kernel which contains two
properties:
1. supported_mode: Helps in identifying if it strictly supports self
save or restore or both.
Initialized using the information from device tree.
2. preferred_mode: Calls out what mode is preferred for each SPR. It
could be strictly self save or restore, or it can also
determine the preference of mode over the other if both
are present by encapsulating the other in bitmask from
LSB to MSB.
Initialized statically.
Below is a table to show the Scenario::Consequence when the self save and
self restore modes are available or disabled in different combinations as
perceived from the device tree thus giving complete backwards compatibly
regardless of an older firmware running a newer kernel or vise-versa.
Support for self save or self-restore is embedded in the device tree,
along with the set of registers it supports.
SR = Self restore; SS = Self save
.-----------------------------------.----------------------------------------.
| Scenario | Consequence |
:-----------------------------------+----------------------------------------:
| Legacy Firmware. No SS or SR node | Self restore is called for all |
| | supported SPRs |
:-----------------------------------+----------------------------------------:
| SR: !active SS: !active | Deep stop states disabled |
:-----------------------------------+----------------------------------------:
| SR: active SS: !active | Self restore is called for all |
| | supported SPRs |
:-----------------------------------+----------------------------------------:
| SR: active SS: active | Goes through the preferences for each |
| | SPR and executes of the modes |
| | accordingly. Currently, Self restore is|
| | called for all the SPRs except PSSCR |
| | which is self saved |
:-----------------------------------+----------------------------------------:
| SR: active(only HID0) SS: active | Self save called for all supported |
| | registers expect HID0 (as HID0 cannot |
| | be self saved currently) |
:-----------------------------------+----------------------------------------:
| SR: !active SS: active | currently will disable deep states as |
| | HID0 is needed to be self restored and |
| | cannot be self saved |
'-----------------------------------'----------------------------------------'
Pratik Rajesh Sampat (3):
powerpc/powernv: Interface to define support and preference for a SPR
powerpc/powernv: Introduce Self save support
powerpc/powernv: Parse device tree, population of SPR support
arch/powerpc/include/asm/opal-api.h | 3 +-
arch/powerpc/include/asm/opal.h | 1 +
arch/powerpc/platforms/powernv/idle.c | 420 ++++++++++++++++++---
arch/powerpc/platforms/powernv/opal-call.c | 1 +
4 files changed, 367 insertions(+), 58 deletions(-)
--
2.17.1
Parse the device tree for nodes self-save, self-restore and populate
support for the preferred SPRs based what was advertised by the device
tree.
Signed-off-by: Pratik Rajesh Sampat <[email protected]>
Reviewed-by: Ram Pai <[email protected]>
---
arch/powerpc/platforms/powernv/idle.c | 82 +++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 97aeb45e897b..27dfadf609e8 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -1436,6 +1436,85 @@ static void __init pnv_probe_idle_states(void)
supported_cpuidle_states |= pnv_idle_states[i].flags;
}
+/*
+ * Extracts and populates the self save or restore capabilities
+ * passed from the device tree node
+ */
+static int extract_save_restore_state_dt(struct device_node *np, int type)
+{
+ int nr_sprns = 0, i, bitmask_index;
+ int rc = 0;
+ u64 *temp_u64;
+ u64 bit_pos;
+
+ nr_sprns = of_property_count_u64_elems(np, "sprn-bitmask");
+ if (nr_sprns <= 0)
+ return rc;
+ temp_u64 = kcalloc(nr_sprns, sizeof(u64), GFP_KERNEL);
+ if (of_property_read_u64_array(np, "sprn-bitmask",
+ temp_u64, nr_sprns)) {
+ pr_warn("cpuidle-powernv: failed to find registers in DT\n");
+ kfree(temp_u64);
+ return -EINVAL;
+ }
+ /*
+ * Populate acknowledgment of support for the sprs in the global vector
+ * gotten by the registers supplied by the firmware.
+ * The registers are in a bitmask, bit index within
+ * that specifies the SPR
+ */
+ for (i = 0; i < nr_preferred_sprs; i++) {
+ bitmask_index = preferred_sprs[i].spr / 64;
+ bit_pos = preferred_sprs[i].spr % 64;
+ if ((temp_u64[bitmask_index] & (1UL << bit_pos)) == 0) {
+ if (type == SELF_RESTORE_TYPE)
+ preferred_sprs[i].supported_mode &=
+ ~SELF_RESTORE_STRICT;
+ else
+ preferred_sprs[i].supported_mode &=
+ ~SELF_SAVE_STRICT;
+ continue;
+ }
+ if (type == SELF_RESTORE_TYPE) {
+ preferred_sprs[i].supported_mode |=
+ SELF_RESTORE_STRICT;
+ } else {
+ preferred_sprs[i].supported_mode |=
+ SELF_SAVE_STRICT;
+ }
+ }
+
+ kfree(temp_u64);
+ return rc;
+}
+
+static int pnv_parse_deepstate_dt(void)
+{
+ struct device_node *sr_np, *ss_np;
+ int rc = 0, i;
+
+ /* Self restore register population */
+ sr_np = of_find_node_by_path("/ibm,opal/power-mgt/self-restore");
+ if (!sr_np) {
+ pr_warn("opal: self restore Node not found");
+ } else {
+ rc = extract_save_restore_state_dt(sr_np, SELF_RESTORE_TYPE);
+ if (rc != 0)
+ return rc;
+ }
+ /* Self save register population */
+ ss_np = of_find_node_by_path("/ibm,opal/power-mgt/self-save");
+ if (!ss_np) {
+ pr_warn("opal: self save Node not found");
+ pr_warn("Legacy firmware. Assuming default self-restore support");
+ for (i = 0; i < nr_preferred_sprs; i++)
+ preferred_sprs[i].supported_mode &= ~SELF_SAVE_STRICT;
+ } else {
+ rc = extract_save_restore_state_dt(ss_np, SELF_SAVE_TYPE);
+ }
+ return rc;
+}
+
/*
* This function parses device-tree and populates all the information
* into pnv_idle_states structure. It also sets up nr_pnv_idle_states
@@ -1584,6 +1663,9 @@ static int __init pnv_init_idle_states(void)
return rc;
pnv_probe_idle_states();
+ rc = pnv_parse_deepstate_dt();
+ if (rc)
+ return rc;
if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
if (!(supported_cpuidle_states & OPAL_PM_SLEEP_ENABLED_ER1)) {
power7_fastsleep_workaround_entry = false;
--
2.17.1
Pratik Rajesh Sampat <[email protected]> writes:
> Parse the device tree for nodes self-save, self-restore and populate
> support for the preferred SPRs based what was advertised by the device
> tree.
These should be documented in:
Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 97aeb45e897b..27dfadf609e8 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -1436,6 +1436,85 @@ static void __init pnv_probe_idle_states(void)
> supported_cpuidle_states |= pnv_idle_states[i].flags;
> }
>
> +/*
> + * Extracts and populates the self save or restore capabilities
> + * passed from the device tree node
> + */
> +static int extract_save_restore_state_dt(struct device_node *np, int type)
> +{
> + int nr_sprns = 0, i, bitmask_index;
> + int rc = 0;
> + u64 *temp_u64;
> + u64 bit_pos;
> +
> + nr_sprns = of_property_count_u64_elems(np, "sprn-bitmask");
> + if (nr_sprns <= 0)
> + return rc;
Using <= 0 means zero SPRs is treated by success as the caller, is that
intended? If so a comment would be appropriate.
> + temp_u64 = kcalloc(nr_sprns, sizeof(u64), GFP_KERNEL);
> + if (of_property_read_u64_array(np, "sprn-bitmask",
> + temp_u64, nr_sprns)) {
> + pr_warn("cpuidle-powernv: failed to find registers in DT\n");
> + kfree(temp_u64);
> + return -EINVAL;
> + }
> + /*
> + * Populate acknowledgment of support for the sprs in the global vector
> + * gotten by the registers supplied by the firmware.
> + * The registers are in a bitmask, bit index within
> + * that specifies the SPR
> + */
> + for (i = 0; i < nr_preferred_sprs; i++) {
> + bitmask_index = preferred_sprs[i].spr / 64;
> + bit_pos = preferred_sprs[i].spr % 64;
This is basically a hand coded bitmap, see eg. BIT_WORD(), BIT_MASK() etc.
I don't think there's an easy way to convert temp_u64 into a proper
bitmap, so it's probably not worth doing that. But at least use the macros.
> + if ((temp_u64[bitmask_index] & (1UL << bit_pos)) == 0) {
> + if (type == SELF_RESTORE_TYPE)
> + preferred_sprs[i].supported_mode &=
> + ~SELF_RESTORE_STRICT;
> + else
> + preferred_sprs[i].supported_mode &=
> + ~SELF_SAVE_STRICT;
> + continue;
> + }
> + if (type == SELF_RESTORE_TYPE) {
> + preferred_sprs[i].supported_mode |=
> + SELF_RESTORE_STRICT;
> + } else {
> + preferred_sprs[i].supported_mode |=
> + SELF_SAVE_STRICT;
> + }
> + }
> +
> + kfree(temp_u64);
> + return rc;
> +}
> +
> +static int pnv_parse_deepstate_dt(void)
> +{
> + struct device_node *sr_np, *ss_np;
You never use these concurrently AFAICS, so you could just have a single *np.
> + int rc = 0, i;
> +
> + /* Self restore register population */
> + sr_np = of_find_node_by_path("/ibm,opal/power-mgt/self-restore");
I know the existing idle code uses of_find_node_by_path(), but that's
because it's old and crufty. Please don't add new searches by path. You
should be searching by compatible.
> + if (!sr_np) {
> + pr_warn("opal: self restore Node not found");
This warning and the others below will fire on all existing firmware
versions, which is not OK.
> + } else {
> + rc = extract_save_restore_state_dt(sr_np, SELF_RESTORE_TYPE);
> + if (rc != 0)
> + return rc;
> + }
> + /* Self save register population */
> + ss_np = of_find_node_by_path("/ibm,opal/power-mgt/self-save");
> + if (!ss_np) {
> + pr_warn("opal: self save Node not found");
> + pr_warn("Legacy firmware. Assuming default self-restore support");
> + for (i = 0; i < nr_preferred_sprs; i++)
> + preferred_sprs[i].supported_mode &= ~SELF_SAVE_STRICT;
> + } else {
> + rc = extract_save_restore_state_dt(ss_np, SELF_SAVE_TYPE);
> + }
> + return rc;
You're leaking references on all the device_nodes in here, you need
of_node_put() before exiting.
> +}
cheers
Thank you Michael for the review.
On 17/03/20 8:43 am, Michael Ellerman wrote:
> Pratik Rajesh Sampat <[email protected]> writes:
>> Parse the device tree for nodes self-save, self-restore and populate
>> support for the preferred SPRs based what was advertised by the device
>> tree.
> These should be documented in:
> Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt
Sure thing I'll add them.
>> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
>> index 97aeb45e897b..27dfadf609e8 100644
>> --- a/arch/powerpc/platforms/powernv/idle.c
>> +++ b/arch/powerpc/platforms/powernv/idle.c
>> @@ -1436,6 +1436,85 @@ static void __init pnv_probe_idle_states(void)
>> supported_cpuidle_states |= pnv_idle_states[i].flags;
>> }
>>
>> +/*
>> + * Extracts and populates the self save or restore capabilities
>> + * passed from the device tree node
>> + */
>> +static int extract_save_restore_state_dt(struct device_node *np, int type)
>> +{
>> + int nr_sprns = 0, i, bitmask_index;
>> + int rc = 0;
>> + u64 *temp_u64;
>> + u64 bit_pos;
>> +
>> + nr_sprns = of_property_count_u64_elems(np, "sprn-bitmask");
>> + if (nr_sprns <= 0)
>> + return rc;
> Using <= 0 means zero SPRs is treated by success as the caller, is that
> intended? If so a comment would be appropriate.
My bad, this is undesirable. This should be treated as a failure.
I'll fix this.
>> + temp_u64 = kcalloc(nr_sprns, sizeof(u64), GFP_KERNEL);
>> + if (of_property_read_u64_array(np, "sprn-bitmask",
>> + temp_u64, nr_sprns)) {
>> + pr_warn("cpuidle-powernv: failed to find registers in DT\n");
>> + kfree(temp_u64);
>> + return -EINVAL;
>> + }
>> + /*
>> + * Populate acknowledgment of support for the sprs in the global vector
>> + * gotten by the registers supplied by the firmware.
>> + * The registers are in a bitmask, bit index within
>> + * that specifies the SPR
>> + */
>> + for (i = 0; i < nr_preferred_sprs; i++) {
>> + bitmask_index = preferred_sprs[i].spr / 64;
>> + bit_pos = preferred_sprs[i].spr % 64;
> This is basically a hand coded bitmap, see eg. BIT_WORD(), BIT_MASK() etc.
>
> I don't think there's an easy way to convert temp_u64 into a proper
> bitmap, so it's probably not worth doing that. But at least use the macros.
Right. I'll use the macros for a cleaner abstraction.
>> + if ((temp_u64[bitmask_index] & (1UL << bit_pos)) == 0) {
>> + if (type == SELF_RESTORE_TYPE)
>> + preferred_sprs[i].supported_mode &=
>> + ~SELF_RESTORE_STRICT;
>> + else
>> + preferred_sprs[i].supported_mode &=
>> + ~SELF_SAVE_STRICT;
>> + continue;
>> + }
>> + if (type == SELF_RESTORE_TYPE) {
>> + preferred_sprs[i].supported_mode |=
>> + SELF_RESTORE_STRICT;
>> + } else {
>> + preferred_sprs[i].supported_mode |=
>> + SELF_SAVE_STRICT;
>> + }
>> + }
>> +
>> + kfree(temp_u64);
>> + return rc;
>> +}
>> +
>> +static int pnv_parse_deepstate_dt(void)
>> +{
>> + struct device_node *sr_np, *ss_np;
> You never use these concurrently AFAICS, so you could just have a single *np.
Sure, got rid of it.
>> + int rc = 0, i;
>> +
>> + /* Self restore register population */
>> + sr_np = of_find_node_by_path("/ibm,opal/power-mgt/self-restore");
> I know the existing idle code uses of_find_node_by_path(), but that's
> because it's old and crufty. Please don't add new searches by path. You
> should be searching by compatible.
>
Alright, I'll replace of_find_node_by_path() calls with of_find_compatible_node()
>> + if (!sr_np) {
>> + pr_warn("opal: self restore Node not found");
> This warning and the others below will fire on all existing firmware
> versions, which is not OK.
I'll get rid of the warnings. They seem unnecessary to me also now.
>> + } else {
>> + rc = extract_save_restore_state_dt(sr_np, SELF_RESTORE_TYPE);
>> + if (rc != 0)
>> + return rc;
>> + }
>> + /* Self save register population */
>> + ss_np = of_find_node_by_path("/ibm,opal/power-mgt/self-save");
>> + if (!ss_np) {
>> + pr_warn("opal: self save Node not found");
>> + pr_warn("Legacy firmware. Assuming default self-restore support");
>> + for (i = 0; i < nr_preferred_sprs; i++)
>> + preferred_sprs[i].supported_mode &= ~SELF_SAVE_STRICT;
>> + } else {
>> + rc = extract_save_restore_state_dt(ss_np, SELF_SAVE_TYPE);
>> + }
>> + return rc;
> You're leaking references on all the device_nodes in here, you need
> of_node_put() before exiting.
Got it. I'll clear the refcount before exitting.
>> +}
>
> cheers
Thanks!
Pratik