[Skiboot] [PATCH Skiboot v1.2 3/3] Advertise the self-save and self-restore attributes in the device tree
Oliver O'Halloran
oohall at gmail.com
Fri Oct 11 12:03:18 AEDT 2019
On Thu, Oct 10, 2019 at 11:10 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 Linux kernel for that register.
Where did 2048 come from and why do we care about being consistent
with Linux? SPRs are either defined by the Power architecture or in
the CPU user manual. Whatever happens to be in Linux is *not*
authoritative and from the point of view of firmware we shouldn't be
relying on any specific behaviour from the OS.
> Signed-off-by: Pratik Rajesh Sampat <psampat at linux.ibm.com>
> ---
> hw/slw.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
> include/skiboot.h | 1 +
> 2 files changed, 73 insertions(+)
>
> diff --git a/hw/slw.c b/hw/slw.c
> index b79aaab3..d9c2d091 100644
> --- a/hw/slw.c
> +++ b/hw/slw.c
> @@ -22,6 +22,7 @@
> #include <opal-api.h>
> #include <nvram.h>
> #include <sbe-p8.h>
> +#include <bitmap.h>
>
> #include <p9_stop_api.H>
> #include <p8_pore_table_gen_api.H>
> @@ -753,6 +754,70 @@ 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(struct dt_node *power_mgt)
> +{
> + int i;
> + struct dt_node *self_restore, *self_save;
> + bitmap_t *self_restore_map, *self_save_map;
> + /* 32 times 64 bits needed to store a 2048 bits bitmask*/
> + const int bits_nr = 32;
If you're going to call something "bits_nr" then it had better be a
number of bits, and this isn't.
> + const uint64_t self_restore_regs[] = {
> + 0x130, // HSPRG0
> + 0x13E, // LPCR
> + 0x151, // HMEER
> + 0x3F0, // HID0
> + 0x3F1, // HID1
> + 0x3F4, // HID4
> + 0x3F6, // HID5
> + 0x7D0, // MSR
> + 0x357 // PSCCR
> + };
> +
> + const uint64_t self_save_regs[] = {
> + 0x130, // HSPRG0
> + 0x13E, // LPCR
> + 0x151, // HMEER
> + 0x7D0, // MSR
> + 0x357 // PSCCR
> + };
How'd you decide on these registers?
> + const int self_save_regs_nr = ARRAY_SIZE(self_save_regs);
> + const int self_restore_regs_nr = ARRAY_SIZE(self_restore_regs);
> +
> + self_save_map = zalloc(BITMAP_BYTES(0x800));
> + self_restore_map = zalloc(BITMAP_BYTES(0x800));
> +
> + for (i = 0; i < self_save_regs_nr; i++)
> + bitmap_set_bit(*self_save_map, self_save_regs[i]);
> +
> + for (i = 0; i < self_restore_regs_nr; i++)
> + bitmap_set_bit(*self_restore_map, self_restore_regs[i]);
Just use ARRAY_SIZE() in the loop tests. The constants aren't making
anything better.
> + self_restore = dt_new(power_mgt, "self-restore");
> + if (!self_restore) {
> + prerror("OCC: Failed to create self restore node");
> + return;
> + }
This leaks the memory allocated for self_save_map and
self_restore_map. dt_add_property() allocates it's own space and
copies the contents rather than re-using the existing storage. You
need to free them.
> + dt_add_property_cells(self_restore, "active", 0x1);
I don't see the point of this property. If firmware doesn't support
the feature then don't add the node. We need to handle that case from
the OS side anyway since old firmware won't be providing it.
If you're super-convinced that we need something like this, then use
the more standard device-tree "status" property.
> +
> + dt_add_property(self_restore, "sprn-bitmask", *self_restore_map,
> + bits_nr * sizeof(uint64_t));
Write the size as: 2048 / 8 or SPR_BITMAP_LENGTH / 8, or similar, and
use the same expression in the bitmap allocations above.
> +
> + self_save = dt_new(power_mgt, "self-save");
> + if (!self_save) {
> + prerror("OCC: Failed to create self save node");
> + return;
> + }
> + if (proc_gen == proc_gen_p9) {
> + dt_add_property_cells(self_save, "active", 0x1);
> +
> + dt_add_property(self_save, "sprn-bitmask", *self_save_map,
> + bits_nr * sizeof(uint64_t));
> + } else
> + dt_add_property_cells(self_save, "active", 0x0);
Skiboot style is to use {} around all the arms of an if statement if
any of them have a a pair of braces.
Nitpicks aside, how do we know libpore supports the self-restore? The
commit message says that it's a new feature but you're assuming that
all P9 systems support it. Is there some way to determine if it's
supported at runtime?
> +}
> +
> /* Add device tree properties to describe idle states */
> void add_cpu_idle_state_properties(void)
> {
> @@ -1543,6 +1608,7 @@ opal_call(OPAL_SLW_SELF_SAVE_REG, opal_slw_self_save_reg, 2);
> void slw_init(void)
> {
> struct proc_chip *chip;
> + struct dt_node *power_mgt;
>
> if (proc_chip_quirks & QUIRK_MAMBO_CALLOUTS) {
> wakeup_engine_state = WAKEUP_ENGINE_NOT_PRESENT;
> @@ -1568,4 +1634,10 @@ void slw_init(void)
> }
> }
> add_cpu_idle_state_properties();
> + 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");
> + return;
> + }
> + add_cpu_self_save_properties(power_mgt);
Either move the DT node lookup into add_cpu_self_save_properties() to
make it self-contained, or rework it to something like:
add_spr_bitmap(power_mgt, "self-save", self_save_spr_array);
add_spr_bitmap(power_mgt, "self-restore", self_restore_spr_array);
I don't really mind which you do, but right now it's a bit all over the place.
> }
> diff --git a/include/skiboot.h b/include/skiboot.h
> index 1aa8bf7c..e8f0f755 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -202,6 +202,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(struct dt_node *power_mgt);
> extern void lpc_rtc_init(void);
>
> /* flash support */
> --
> 2.21.0
>
More information about the Skiboot
mailing list