[PATCH 1/2] lib: utils: Add fdt_add_cpu_idle_states() helper function

[Anup Patel]

On Thu, Dec 29, 2022 at 11:10 AM Samuel Holland <samuel at sholland.org> wrote:
>
> On 12/29/22 06:33, Yu-Chien Peter Lin wrote:
> > Hi Samuel,
> >
> > On Wed, Dec 28, 2022 at 01:11:23PM -0600, Samuel Holland wrote:
> >> Since the availability and latency properties of CPU idle states depend
> >> on the specific SBI HSM implementation, it is appropriate that the idle
> >> states are added to the devicetree at runtime by that implementation.
> >>
> >> This helper function adds a platform-provided array of idle states to
> >> the devicetree, following the SBI idle state binding. It makes some
> >> assumptions for simplicity, but these could be relaxed if needed.
> >>
> >> Signed-off-by: Samuel Holland <samuel at sholland.org>
> >> ---
> >>
> >>  include/sbi_utils/fdt/fdt_fixup.h | 23 +++++++++
> >>  lib/utils/fdt/fdt_fixup.c         | 80 +++++++++++++++++++++++++++++++
> >>  2 files changed, 103 insertions(+)
> >>
> >> diff --git a/include/sbi_utils/fdt/fdt_fixup.h b/include/sbi_utils/fdt/fdt_fixup.h
> >> index fb076ba..cab3f0f 100644
> >> --- a/include/sbi_utils/fdt/fdt_fixup.h
> >> +++ b/include/sbi_utils/fdt/fdt_fixup.h
> >> @@ -9,6 +9,29 @@
> >>  #ifndef __FDT_FIXUP_H__
> >>  #define __FDT_FIXUP_H__
> >>
> >> +struct sbi_cpu_idle_state {
> >> +    const char *name;
> >> +    uint32_t suspend_param;
> >> +    bool local_timer_stop;
> >> +    uint32_t entry_latency_us;
> >> +    uint32_t exit_latency_us;
> >> +    uint32_t min_residency_us;
> >> +    uint32_t wakeup_latency_us;
> >> +};
> >> +
> >> +/**
> >> + * Add CPU idle states to cpu nodes in the DT
> >> + *
> >> + * Add information about CPU idle states to the devicetree. This function
> >> + * assumes that CPU idle states are not already present in the devicetree, and
> >> + * that all CPU states are equally applicable to all CPUs.
> >> + *
> >> + * @param fdt: device tree blob
> >> + * @param states: array of idle state descriptions, ending with empty element
> >> + * @return zero on success and -ve on failure
> >> + */
> >> +int fdt_add_cpu_idle_states(void *dtb, const struct sbi_cpu_idle_state *state);
> >> +
> >>  /**
> >>   * Fix up the CPU node in the device tree
> >>   *
> >> diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
> >> index 41f6cbb..d9aa0b2 100644
> >> --- a/lib/utils/fdt/fdt_fixup.c
> >> +++ b/lib/utils/fdt/fdt_fixup.c
> >> @@ -18,6 +18,86 @@
> >>  #include <sbi_utils/fdt/fdt_pmu.h>
> >>  #include <sbi_utils/fdt/fdt_helper.h>
> >>
> >> +int fdt_add_cpu_idle_states(void *fdt, const struct sbi_cpu_idle_state *state)
> >> +{
> >> +    int cpu_node, cpus_node, err, idle_states_node;
> >> +    uint32_t count, phandle;
> >> +
> >> +    err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 1024);
> >> +    if (err < 0)
> >> +            return err;
> >> +
> >> +    err = fdt_find_max_phandle(fdt, &phandle);
> >> +    phandle++;
> >> +    if (err < 0)
> >> +            return err;
> >> +
> >> +    cpus_node = fdt_path_offset(fdt, "/cpus");
> >> +    if (cpus_node < 0)
> >> +            return cpus_node;
> >> +
> >> +    /* Create the idle-states node and its child nodes. */
> >> +    idle_states_node = fdt_add_subnode(fdt, cpus_node, "idle-states");
> >> +    if (idle_states_node < 0)
> >> +            return idle_states_node;
> >
> > It breaks platform final init when the “idle-states” already exists:
>
> Right, that is a documented limitation. I would expect there to be a
> single source of idle state information -- wherever the HSM
> implementation is. So my suggestion would be to remove idle-states from
> the static devicetree if you plan to use this function. If that is not
> desired, could you explain your scenario in a bit more detail?

I assume there are existing boards with DTBs in flash containing
"idle-states" DT node. Better to add a check if "idle-states" DT
node already exists.

Otherwise, this patch looks good to me.

Reviewed-by: Anup Patel <anup at brainfault.org>

Regards,
Anup

>
> Regards,
> Samuel
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi