[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