[Skiboot] [PATCH v3 3/4] Advertise the self-save and self-restore attributes in the device tree
Oliver O'Halloran
oohall at gmail.com
Mon Feb 10 13:13:37 AEDT 2020
On Fri, Feb 7, 2020 at 3:21 PM Pratik Rajesh Sampat
<psampat at linux.ibm.com> wrote:
>
> Support for self save and self restore interface is advertised in the
> device tree, along with the list of SPRs it supports for each.
>
> The Special Purpose Register identification is encoded in a 2048 bitmask
> structure, where each bit signifies the identification key of that SPR
> which is consistent with that of the POWER architecture set for that
> register.
Please answer the questions I asked the last time I reviewed this:
https://patchwork.ozlabs.org/patch/1174835/#2278701
Since then I've also forgotten what the difference between
self-restore and the existing API is. So can you add something to
doc/power-management.rst to describe the relationship between the
current API, self-restore, and self-save? The new device-tree bindings
need to be documented in doc/device-tree/ibm,opal/power-mgt/ too. The
cover letter of the series covers some of that, but actual
documentation is sorely needed.
> Signed-off-by: Pratik Rajesh Sampat <psampat at linux.ibm.com>
> ---
> hw/slw.c | 83 +++++++++++++++++++++++++++++++++
> include/skiboot.h | 1 +
> libpore/p8_pore_table_gen_api.H | 1 +
> 3 files changed, 85 insertions(+)
>
> diff --git a/hw/slw.c b/hw/slw.c
> index 5efa2168..087a8619 100644
> --- a/hw/slw.c
> +++ b/hw/slw.c
> @@ -23,12 +23,15 @@
> #include <nvram.h>
> #include <sbe-p8.h>
> #include <xive.h>
> +#include <bitmap.h>
>
> #include <p9_stop_api.H>
> #include <p8_pore_table_gen_api.H>
> #include <sbe_xip_image.h>
>
> static uint32_t slw_saved_reset[0x100];
> +#define MAX_RESET_PATCH_SIZE 64
unused?
> +#define SPR_BITMAP_LENGTH 2048
>
> static bool slw_current_le = false;
>
> @@ -750,6 +753,84 @@ static void slw_late_init_p9(struct proc_chip *chip)
> }
> }
>
> +/* Add device tree properties to determine self-save | restore */
> +void add_cpu_self_save_properties()
should be: void add_cpu_self_save_properties(void)
> +{
> + int i;
> + struct dt_node *self_restore, *self_save, *power_mgt;
> + bitmap_t *self_restore_map, *self_save_map;
declarations should be sorted reverse christmas tree order. i.e.
longest at the top, shortest at the bottom.
> +
> + const uint64_t self_restore_regs[] = {
> + P8_SPR_HRMOR,
> + P8_SPR_HMEER,
> + P8_SPR_PMICR,
> + P8_SPR_PMCR,
> + P8_SPR_HID0,
> + P8_SPR_HID1,
> + P8_SPR_HID4,
> + P8_SPR_HID5,
> + P8_SPR_HSPRG0,
> + P8_SPR_LPCR,
> + P8_SPR_PSSCR,
> + P8_MSR_MSR
> + };
> +
> + const uint64_t self_save_regs[] = {
> + P9_STOP_SPR_DAWR,
> + P9_STOP_SPR_HSPRG0,
> + P9_STOP_SPR_LDBAR,
> + P9_STOP_SPR_LPCR,
> + P9_STOP_SPR_PSSCR,
> + P9_STOP_SPR_MSR,
> + P9_STOP_SPR_HRMOR,
> + P9_STOP_SPR_HMEER,
> + P9_STOP_SPR_PMCR,
> + P9_STOP_SPR_PTCR
> + };
Again, where did these sets of registers come from?
> + self_save_map = zalloc(BITMAP_BYTES(SPR_BITMAP_LENGTH));
> + self_restore_map = zalloc(BITMAP_BYTES(SPR_BITMAP_LENGTH));
In general I prefer people avoid doing allocations until they're
actually needed. If any of the checks below fail then we've done a
bunch of work for no reason so... do the checks first?
> + for (i = 0; i < ARRAY_SIZE(self_save_regs); i++)
> + bitmap_set_bit(*self_save_map, self_save_regs[i]);
> +
> + for (i = 0; i < ARRAY_SIZE(self_restore_regs); i++)
> + bitmap_set_bit(*self_restore_map, self_restore_regs[i]);
> +
> + power_mgt = dt_find_by_path(dt_root, "/ibm,opal/power-mgt");
> + if (!power_mgt) {
> + prerror("OCC: dt node /ibm,opal/power-mgt not found\n");
> + goto bail;
> + }
> +
> + self_restore = dt_new(power_mgt, "self-restore");
> + if (!self_restore) {
> + prerror("OCC: Failed to create self restore node");
> + goto bail;
> + }
> + dt_add_property_string(self_restore, "status", "enabled");
> +
> + dt_add_property(self_restore, "sprn-bitmask", *self_restore_map,
> + SPR_BITMAP_LENGTH / 8);
> +
> + self_save = dt_new(power_mgt, "self-save");
> + if (!self_save) {
> + prerror("OCC: Failed to create self save node");
> + goto bail;
> + }
> + if (proc_gen == proc_gen_p9) {
> + dt_add_property_string(self_save, "status", "enabled");
> +
> + dt_add_property(self_save, "sprn-bitmask", *self_save_map,
> + SPR_BITMAP_LENGTH / 8);
If skiboot is compiled as little endian this is going to break. It
works today because we always build skiboot BE, but that may not
always be true.
> + } else {
> + dt_add_property_string(self_save, "status", "disabled");
I still don't understand why we're adding anything to the DT if the
feature isn't supported.
> + }
> +bail:
> + free(self_save_map);
> + free(self_restore_map);
> +}
> +
> /* Add device tree properties to describe idle states */
> void add_cpu_idle_state_properties(void)
> {
> @@ -1563,4 +1644,6 @@ void slw_init(void)
> }
> }
> add_cpu_idle_state_properties();
> + if (has_deep_states)
> + add_cpu_self_save_properties();
> }
> diff --git a/include/skiboot.h b/include/skiboot.h
> index 072ce589..676211ad 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -209,6 +209,7 @@ extern void early_uart_init(void);
> extern void homer_init(void);
> extern void slw_init(void);
> extern void add_cpu_idle_state_properties(void);
> +extern void add_cpu_self_save_properties(void);
> extern void lpc_rtc_init(void);
>
> /* flash support */
> diff --git a/libpore/p8_pore_table_gen_api.H b/libpore/p8_pore_table_gen_api.H
> index 63081ca5..d72fee90 100644
> --- a/libpore/p8_pore_table_gen_api.H
> +++ b/libpore/p8_pore_table_gen_api.H
> @@ -233,6 +233,7 @@ enum {
> P8_SPR_HRMOR = 313,
> P8_SPR_HMEER = 337,
> P8_SPR_PMICR = 852,
> + P8_SPR_PSSCR = 855,
> P8_SPR_PMCR = 884,
> P8_SPR_HID0 = 1008,
> P8_SPR_HID1 = 1009,
> --
> 2.24.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list