Thanks for the review.
The support just signifies what is default. Self restore is known to be
supported by legacy systems.

I'll mention a comment saying that this can change when the system is
initialized.


On 13/01/20 1:14 pm, Ram Pai wrote:
On Mon, Jan 13, 2020 at 09:15:07AM +0530, Pratik Rajesh Sampat wrote:
Define a bitmask interface to determine support for the Self Restore,
Self Save or both.

Also define an interface to determine the preference of that SPR to
be strictly saved or restored or encapsulated with an order of preference.

The preference bitmask is shown as below:
----------------------------
|... | 2nd pref | 1st pref |
----------------------------
MSB                       LSB

The preference from higher to lower is from LSB to MSB with a shift of 8
bits.
Example:
Prefer self save first, if not available then prefer self
restore
The preference mask for this scenario will be seen as below.
((SELF_RESTORE_STRICT << PREFERENCE_SHIFT) | SELF_SAVE_STRICT)
---------------------------------
|... | Self restore | Self save |
---------------------------------
MSB                             LSB

Finally, declare a list of preferred SPRs which encapsulate the bitmaks
for preferred and supported with defaults of both being set to support
legacy firmware.

This commit also implements using the above interface and retains the
legacy functionality of self restore.

Signed-off-by: Pratik Rajesh Sampat <psam...@linux.ibm.com>
---
  arch/powerpc/platforms/powernv/idle.c | 327 +++++++++++++++++++++-----
  1 file changed, 271 insertions(+), 56 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/idle.c 
b/arch/powerpc/platforms/powernv/idle.c
index 78599bca66c2..2f328403b0dc 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -32,9 +32,106 @@
  #define P9_STOP_SPR_MSR 2000
  #define P9_STOP_SPR_PSSCR      855

+/* Interface for the stop state supported and preference */
+#define SELF_RESTORE_TYPE    0
+#define SELF_SAVE_TYPE       1
+
+#define NR_PREFERENCES    2
+#define PREFERENCE_SHIFT  4
+#define PREFERENCE_MASK   0xf
+
+#define UNSUPPORTED         0x0
+#define SELF_RESTORE_STRICT 0x1
+#define SELF_SAVE_STRICT    0x2
+
+/*
+ * Bitmask defining the kind of preferences available.
+ * Note : The higher to lower preference is from LSB to MSB, with a shift of
+ * 4 bits.
+ * ----------------------------
+ * |    | 2nd pref | 1st pref |
+ * ----------------------------
+ * MSB                       LSB
+ */
+/* Prefer Restore if available, otherwise unsupported */
+#define PREFER_SELF_RESTORE_ONLY       SELF_RESTORE_STRICT
+/* Prefer Save if available, otherwise unsupported */
+#define PREFER_SELF_SAVE_ONLY          SELF_SAVE_STRICT
+/* Prefer Restore when available, otherwise prefer Save */
+#define PREFER_RESTORE_SAVE            ((SELF_SAVE_STRICT << \
+                                         PREFERENCE_SHIFT)\
+                                         | SELF_RESTORE_STRICT)
+/* Prefer Save when available, otherwise prefer Restore*/
+#define PREFER_SAVE_RESTORE            ((SELF_RESTORE_STRICT <<\
+                                         PREFERENCE_SHIFT)\
+                                         | SELF_SAVE_STRICT)
  static u32 supported_cpuidle_states;
  struct pnv_idle_states_t *pnv_idle_states;
  int nr_pnv_idle_states;
+/* Caching the lpcr & ptcr support to use later */
+static bool is_lpcr_self_save;
+static bool is_ptcr_self_save;
+
+struct preferred_sprs {
+       u64 spr;
+       u32 preferred_mode;
+       u32 supported_mode;
+};
+
+struct preferred_sprs preferred_sprs[] = {
+       {
+               .spr = SPRN_HSPRG0,
+               .preferred_mode = PREFER_RESTORE_SAVE,
+               .supported_mode = SELF_RESTORE_STRICT,
+       },
+       {
+               .spr = SPRN_LPCR,
+               .preferred_mode = PREFER_RESTORE_SAVE,
+               .supported_mode = SELF_RESTORE_STRICT,
+       },
+       {
+               .spr = SPRN_PTCR,
+               .preferred_mode = PREFER_SAVE_RESTORE,
+               .supported_mode = SELF_RESTORE_STRICT,
+       },
This confuses me.  It says SAVE takes precedence over RESTORE.
and than it says it is strictly 'RESTORE' only.

Maybe you should not initialize the 'supported_mode' ?
or put a comment somewhere here, saying this value will be overwritten
during system initialization?


Otherwise the code looks correct.

Reviewed-by: Ram Pai <linux...@us.ibm.com>
RP

Reply via email to