Re: [PATCH v6 0/3] powerpc/powernv: Introduce interface for self-restore support
Hello Gautham On 14/04/20 12:41 pm, Gautham R Shenoy wrote: Hello Pratik, On Thu, Mar 26, 2020 at 12:40:31PM +0530, Pratik Rajesh Sampat wrote: v5: https://lkml.org/lkml/2020/3/17/944 Changelog v5-->v6 1. Updated background, motivation and illuminated potential design choices 2. Re-organization of patch-set a. Split introducing preference for optimization from 1/1 to patch 3/3 b. Merge introducing self-save API and parsing the device-tree c. Introduce a supported mode called KERNEL_SAVE_RESTORE which outlines and makes kernel supported SPRs for save-restore more explicit [..snip..] Presenting the design choices in front of us: Design-Choice 1: A simple implementation is to just replace self-restore calls with self-save as it is direct super-set. Pros: A simple design, quick to implement Cons: * Breaks backward compatibility. Self-restore has historically been supported in the firmware and an old firmware running on an new kernel will be incompatible and deep stop states will be cut. * Furthermore, critical SPRs which need to be restored before 0x100 vector like HID0 are not supported by self-save. Design-Choice 2: Advertise both self-restore and self-save from OPAL including the set of registers that each support. The kernel can then choose which API to go with. For the sake of simplicity, in case both modes are supported for an SPR by default self-save would be called for it. Pros: * Backwards compatible Cons: Overhead in parsing device tree with the SPR list Possible optimization with Approach2: - There are SPRs whose values don't tend to change over time and invoking self-save on them, where the values are gotten each time may turn out to be inefficient. In that case calling a self-restore where passing the value makes more sense as, if the value is same, the memory location is not updated. SPRs that dont change are as follows: SPRN_HSPRG0, SPRN_LPCR, SPRN_PTCR, SPRN_HMEER, SPRN_HID0, We can just pick self-save wherever available and fallback to self-restore when self-save support is not avaiable for any SPR. The optimization that you mention here can be revisited if the additional latency due to self-save becomes observable (Note that both stop4 and stop5 have wakeup latency between 200-500us). Yes, Patch2 introduces picking self-save when available and using self-restore as fallback. Sure. The optimization of preference does certainly add an additional layer of complexity. As you mentioned, in the case the latency really begins to hurt us we can revisit this discussion. Thanks Pratik The values of PSSCR and MSR change at runtime and hence, the kernel cannot determine during boot time what their values will be before entering a particular deep-stop state. Therefore, a preference based interface is introduced for choosing between self-save or self-restore between for each SPR. The per-SPR preference is only a refinement of approach 2 purely for performance reasons. It can be dropped if the complexity is not deemed worth the returns. Patches Organization Design Choice 2 has been chosen as an implementation to demonstrate in the patch series. Patch1: Devises an interface which lists all the interested SPRs, along with highlighting the support of mode. It is an isomorphic patch to replicate the functionality of the older self-restore firmware for the new interface Patch2: Introduces the self-save API and leverages upon the struct interface to add another supported mode in the mix of saving and restoring. It also enforces that in case both modes are supported self-save is chosen over self-restore The commit also parses the device-tree and populate support for self-save and self-restore in the supported mask Patch3: Introduce an optimization to allow preference to choose between one more over the one when both both modes are supported. This optimization can allow for better performance for the SPRs that don't change in value and hence self-restore is a better alternative, and in cases when it is known for values to change self-save is more convenient. Pratik Rajesh Sampat (3): powerpc/powernv: Introduce interface for self-restore support powerpc/powernv: Introduce support and parsing for self-save API powerpc/powernv: Preference optimization for SPRs with constant values .../bindings/powerpc/opal/power-mgt.txt | 18 + arch/powerpc/include/asm/opal-api.h | 3 +- arch/powerpc/include/asm/opal.h | 1 + arch/powerpc/platforms/powernv/idle.c | 385 +++--- arch/powerpc/platforms/powernv/opal-call.c| 1 + 5 files changed, 351 insertions(+), 57 deletions(-) -- 2.17.1
Re: [PATCH v6 0/3] powerpc/powernv: Introduce interface for self-restore support
Hello Pratik, On Thu, Mar 26, 2020 at 12:40:31PM +0530, Pratik Rajesh Sampat wrote: > v5: https://lkml.org/lkml/2020/3/17/944 > Changelog > v5-->v6 > 1. Updated background, motivation and illuminated potential design > choices > 2. Re-organization of patch-set > a. Split introducing preference for optimization from 1/1 to patch 3/3 > b. Merge introducing self-save API and parsing the device-tree > c. Introduce a supported mode called KERNEL_SAVE_RESTORE which > outlines and makes kernel supported SPRs for save-restore more > explicit > [..snip..] > Presenting the design choices in front of us: > > Design-Choice 1: > > A simple implementation is to just replace self-restore calls with > self-save as it is direct super-set. > > Pros: > A simple design, quick to implement > > > Cons: > * Breaks backward compatibility. Self-restore has historically been > supported in the firmware and an old firmware running on an new > kernel will be incompatible and deep stop states will be cut. > * Furthermore, critical SPRs which need to be restored > before 0x100 vector like HID0 are not supported by self-save. > > Design-Choice 2: > > Advertise both self-restore and self-save from OPAL including the set > of registers that each support. The kernel can then choose which API > to go with. > For the sake of simplicity, in case both modes are supported for an > SPR by default self-save would be called for it. > > Pros: > * Backwards compatible > > Cons: > Overhead in parsing device tree with the SPR list > > Possible optimization with Approach2: > - > There are SPRs whose values don't tend to change over time and invoking > self-save on them, where the values are gotten each time may turn out to > be inefficient. In that case calling a self-restore where passing the > value makes more sense as, if the value is same, the memory location > is not updated. > SPRs that dont change are as follows: > SPRN_HSPRG0, > SPRN_LPCR, > SPRN_PTCR, > SPRN_HMEER, > SPRN_HID0, We can just pick self-save wherever available and fallback to self-restore when self-save support is not avaiable for any SPR. The optimization that you mention here can be revisited if the additional latency due to self-save becomes observable (Note that both stop4 and stop5 have wakeup latency between 200-500us). > > The values of PSSCR and MSR change at runtime and hence, the kernel > cannot determine during boot time what their values will be before > entering a particular deep-stop state. > > Therefore, a preference based interface is introduced for choosing > between self-save or self-restore between for each SPR. > The per-SPR preference is only a refinement of > approach 2 purely for performance reasons. It can be dropped if the > complexity is not deemed worth the returns. > > Patches Organization > > Design Choice 2 has been chosen as an implementation to demonstrate in > the patch series. > > Patch1: > Devises an interface which lists all the interested SPRs, along with > highlighting the support of mode. > It is an isomorphic patch to replicate the functionality of the older > self-restore firmware for the new interface > > Patch2: > Introduces the self-save API and leverages upon the struct interface to > add another supported mode in the mix of saving and restoring. It also > enforces that in case both modes are supported self-save is chosen over > self-restore > > The commit also parses the device-tree and populate support for > self-save and self-restore in the supported mask > > Patch3: > Introduce an optimization to allow preference to choose between one more > over the one when both both modes are supported. This optimization can > allow for better performance for the SPRs that don't change in value and > hence self-restore is a better alternative, and in cases when it is > known for values to change self-save is more convenient. > > Pratik Rajesh Sampat (3): > powerpc/powernv: Introduce interface for self-restore support > powerpc/powernv: Introduce support and parsing for self-save API > powerpc/powernv: Preference optimization for SPRs with constant values > > .../bindings/powerpc/opal/power-mgt.txt | 18 + > arch/powerpc/include/asm/opal-api.h | 3 +- > arch/powerpc/include/asm/opal.h | 1 + > arch/powerpc/platforms/powernv/idle.c | 385 +++--- > arch/powerpc/platforms/powernv/opal-call.c| 1 + > 5 files changed, 351 insertions(+), 57 deletions(-) > > -- > 2.17.1 >
