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

[Samuel Holland]

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?

Regards,
Samuel