Messages in this thread |  | | Subject | Re: [RFC 1/3] powerpc/powernv: Interface to define support and preference for a SPR | From | Pratik Sampat <> | Date | Mon, 6 Jan 2020 15:16:51 +0530 |
| |
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.
|  |