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

[Pratik Sampat <psampat@linux.ibm.com>]

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.