Re: [RESEND PATCH v2 1/3] powerpc/powernv: Interface to define support and preference for a SPR

[Pratik Sampat]

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@xxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
> RP