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

[Pratik Sampat]

Hello Ram,

Thank you for your reviewing the patches.

> > +/* 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  8
> > +#define PREFERENCE_MASK   0xff
> > +
> > +#define UNSUPPORTED         0x0
> > +#define SELF_RESTORE_STRICT 0x01
> > +#define SELF_SAVE_STRICT    0x10
> > +
> > +/*
> > + * Bitmask defining the kind of preferences available.
> > + * Note : The higher to lower preference is from LSB to MSB, with a shift of
> > + * 8 bits.
> A minor comment.
>
> Is there a reason why shift is 8?  Shift of 4 must be sufficient,
> and a mask of '0xf' should do. And SELF_SAVE_STRICT can be 0x2.
Yes, you're right! We could do away with using fewer bits here.

> > +/* Caching the lpcr & ptcr support to use later */
> > +static bool is_lpcr_self_save;
> > +static bool is_ptcr_self_save;
> I understand why you need to track the status of PTCR register.
> But its not clear, why LPCR register's save status need to be tracked?
Normally it does not but LPCR was previously unsupported in self-restore
and the kernel saved and restored its value in context. Now that we have
support for saving LPCR automatically I believe we leverage it and
make sure the kernel does not do redundant work.

> > +
> > +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,
> > +	},
> > +	{
> > +		.spr = SPRN_HMEER,
> > +		.preferred_mode = PREFER_RESTORE_SAVE,
> > +		.supported_mode = SELF_RESTORE_STRICT,
> > +	},
> > +	{
> > +		.spr = SPRN_HID0,
> > +		.preferred_mode = PREFER_RESTORE_SAVE,
> > +		.supported_mode = SELF_RESTORE_STRICT,
> > +	},
> > +	{
> > +		.spr = P9_STOP_SPR_MSR,
> > +		.preferred_mode = PREFER_RESTORE_SAVE,
> > +		.supported_mode = SELF_RESTORE_STRICT,
> > +	},
> > +	{
> > +		.spr = P9_STOP_SPR_PSSCR,
> > +		.preferred_mode = PREFER_SAVE_RESTORE,
> > +		.supported_mode = SELF_RESTORE_STRICT,
> > +	},
> > +	{
> > +		.spr = SPRN_HID1,
> > +		.preferred_mode = PREFER_RESTORE_SAVE,
> > +		.supported_mode = SELF_RESTORE_STRICT,
> > +	},
> > +	{
> > +		.spr = SPRN_HID4,
> > +		.preferred_mode = PREFER_RESTORE_SAVE,
> > +		.supported_mode = SELF_RESTORE_STRICT,
> > +	},
> > +	{
> > +		.spr = SPRN_HID5,
> > +		.preferred_mode = PREFER_RESTORE_SAVE,
> > +		.supported_mode = SELF_RESTORE_STRICT,
> > +	}
> > +};
> What determines the list of registers tracked in this table?
>
>
> .snip..
This list is of the SPRs of all the registers that the kernel is interested in
at wakeup. It has been refactored out as a list from what the kernel used
previously in the kernel.