Re: [RESEND PATCH v2 1/3] powerpc/powernv: Interface to define support and preference for a SPR
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 --- 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_TYPE0 +#define SELF_SAVE_TYPE 1 + +#define NR_PREFERENCES2 +#define PREFERENCE_SHIFT 4 +#define PREFERENCE_MASK 0xf + +#define UNSUPPORTED 0x0 +#define SELF_RESTORE_STRICT 0x1 +#define SELF_SAVE_STRICT0x2 + +/* + * 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 RP
Re: [RESEND PATCH v2 1/3] powerpc/powernv: Interface to define support and preference for a SPR
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 > --- > 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_TYPE0 > +#define SELF_SAVE_TYPE 1 > + > +#define NR_PREFERENCES2 > +#define PREFERENCE_SHIFT 4 > +#define PREFERENCE_MASK 0xf > + > +#define UNSUPPORTED 0x0 > +#define SELF_RESTORE_STRICT 0x1 > +#define SELF_SAVE_STRICT0x2 > + > +/* > + * 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_ONLYSELF_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 RP