Messages in this thread | | | Subject | Re: [RESEND PATCH v2 1/3] powerpc/powernv: Interface to define support and preference for a SPR | From | Pratik Sampat <> | Date | Mon, 13 Jan 2020 15:19:02 +0530 |
| |
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 <psampat@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 <linuxram@us.ibm.com> > RP
| |