kernel: idle: introduce idle enter hook

[bjarki-andreasen]

To allow for custom SoC idle behavior, introduce hook executed
when idle thread is entered, and add the config
CONFIG_IDLE_DEFAULT_ROUTINE to allow excluding the current
"default" idle routine.

This addition introduces no functional change unless the hook
is explicitly enabled, and the default routine explicitly
disabled.

[pdgendt]

If it's SoC specific, maybe clearer and more consistent is

Suggested change

	void idle_enter_hook(void);
	void soc_idle_enter_hook(void);

[bjarki-andreasen]

I have no problem with naming it soc_idle_enter_hook, it was my first choice as well but since its in idle.c and the Kconfig for it is IDLE_HOOK_ENTER in Kconfig.idle, it felt more concrete to just name it idle :) I have no strong opinion though (well, between adding soc_ or not)

[gmarull]

any options executed by the default routine should depend on this option, right? e.g. policy stuff

[bjarki-andreasen]

well, optimistically if pm is enabled it would select the hook and implement a custom routine. Right now everything depends on the default, so it would practically need to be added everywhere, so I just leave it default to be disabled for now, until more stuff implements the hook than not :)

[gmarull]

I foresee this will be a source of confusion, people being able to enable options that do nothing is a bad idea.
Why don't we accept the PM subsystem is garbage and propose a new PM subsystem that evolves in parallel, as an experimental thing that will eventually replace what we have?

[cfriedt]

PM_NG or PM2? maybe AM? Naming things is hard. I like the parallel pm subsystem idea.

[kartben]

I foresee this will be a source of confusion, people being able to enable options that do nothing is a bad idea. Why don't we accept the PM subsystem is garbage and propose a new PM subsystem that evolves in parallel, as an experimental thing that will eventually replace what we have?

@gmarull I really want to remind you that calling something "garbage" is just outright disrespectful to the people and community members who worked on it. If I'm not mistaken neither you and I are native speakers, but I believe we both know there are many other words in the English language to qualify something that could use some improvement/refactoring/re-engineering, or even a complete rewrite, than "garbage".

[gmarull]

I think it's an accurate word in this case. I'm open to use any more polite synonyms.

[bjarki-andreasen]

To be clear, the solution in this PR is not a replacement of PM, its an "overlay"/ higher level abstraction which matches more than the system power state, we still need PM for systems with complex power states, but those states can be abstracted to "just" an event latency, which matches this PM_EVTC in this PR :)

[peter-mitsis]

Just a couple of questions for a little more info.

[peter-mitsis]

Question: With the introduction of Kconfig.idle, what about moving "IDLE_STACK_SIZE" to here as well?

[bjarki-andreasen]

sure :)

[peter-mitsis]

Point of clarification ... Does/should the setting of IDLE_ENTER_HOOK influence the setting of IDLE_DEFAULT_ROUTINE (or vice versa)? I ask, because I note that if both are set to 'n', then we get an idle loop that is essentially while (1)--not inherently wrong, but it certainly raises an eyebrow.

[bjarki-andreasen]

The "true" default idle loop would just be to spin the CPU in a while(1), that is, if idle logic has not been implemented for an SoC using the hook, don't idle.

Optimistically the IDLE_DEFAULT_ROUTINE will be deprecated, as SoC specific hooks are implemented over time. So the only options will be implement hook or spin :)

Various default routines can still be written and used, like one which just calls k_cpu_idle(), they would just be selected by calling them from the hook :)

void idle_enter_hook(void)
{
        idle_hook_default_cpu_idle();
}

[bjarki-andreasen]

Looking at this, maybe the default routine should actually just be an implementation of the idle_enter_hook() which selects IDLE_ENTER_HOOK

[andyross]

I'm confused, what does this do that the existing PM layer (or the platforms arch_cpu_idle()) can't do already? It really seems like we have this problem area covered already, no?

[bjarki-andreasen]

I'm confused, what does this do that the existing PM layer (or the platforms arch_cpu_idle()) can't do already? It really seems like we have this problem area covered already, no?

It allows for SoC specific idle logic / power management. We (nordic) are experimenting with a novel approach to power management (#83592) which requires the idle logic to be a bit less coupled to PM and arch_cpu_idle(). In the lowest latency configuration on nrf5340 for example, we don't idle the thread at all. We also don't use CPU states to manage latency (in the PM state sense, we do idle it with WFI in some cases).

[nashif]

It allows for SoC specific idle logic / power management. We (nordic) are experimenting with a novel approach to power management (#83592) which requires the idle logic to be a bit less coupled to PM and arch_cpu_idle(). In the lowest latency configuration on nrf5340 for example, we don't idle the thread at all. We also don't use CPU states to manage latency (in the PM state sense, we do idle it with WFI in some cases).

idle does more than just power management and you are basically disabling or want to be able to disable an essential part of how the kernel works (through CONFIG_IDLE_DEFAULT_ROUTINE), with or without power management enabled which would potentially would cause other problems.

I thought 83592 was about adding latency handling to device drivers, so I am confused why the idle thread is being changed to achive this.

if this is a novel approach, you probably want to provide a more complete story and show that this approach works and that such changes to the kernel are going to yield the expected results. True it is functionally not changing anything, but disabling essential code through a Kconfig can be very dangerous as well.

[bjarki-andreasen]

It allows for SoC specific idle logic / power management. We (nordic) are experimenting with a novel approach to power management (#83592) which requires the idle logic to be a bit less coupled to PM and arch_cpu_idle(). In the lowest latency configuration on nrf5340 for example, we don't idle the thread at all. We also don't use CPU states to manage latency (in the PM state sense, we do idle it with WFI in some cases).

idle does more than just power management and you are basically disabling or want to be able to disable an essential part of how the kernel works (through CONFIG_IDLE_DEFAULT_ROUTINE), with or without power management enabled which would potentially would cause other problems.

The idle thread is "just there" to have a context to switch to when all other threads are blocked. For the majority of SoCs, a simple while(1){} is all the idle thread needs to do. It happens to be a perfect place to determine if the SoC can be put into a low power state given no more work has to be done until the next event, which is why the call to PM is there.

All the CONFIG_IDLE_DEFAULT_ROUTINE does is add SoC specific quirks, and a call to PM. These quirks include manually swapping to a new thread when CPU exits idle if preemption is not supported for the idle thread, or manually swapping in SMP systems which don't have SCHED_IPI. This is logic which should ideally be moved to the SoCs.

The current approach to the idle thread routine is similar to having a single soc_init_hook() where every SoC is ifdefed in with their specific requirenments:

void soc_init_hook(void)
{
#if SOC_NRF9160 || SOC_ATSAM4E
        ...
#endif
        ...
}

see

zephyr/kernel/idle.c

Line 78 in 0728f5d

# if !defined(CONFIG_USE_SWITCH) || defined(CONFIG_SPARC)

I thought 83592 was about adding latency handling to device drivers, so I am confused why the idle thread is being changed to achive this.

Because the latency of device drivers often depends on their IRQ latency, which is affected by the state of the system, like CPU idle state, hence for a device driver to meet a latency for its event, like a comparator, it both has to configure its own latency, and its IRQ handlers latency.

if this is a novel approach, you probably want to provide a more complete story and show that this approach works and that such changes to the kernel are going to yield the expected results. True it is functionally not changing anything, but disabling essential code through a Kconfig can be very dangerous as well.

I will be added the full story to #83592 as it develops :) I will refactor the PR to make excluding the default routine "harder" to make it less likely to be disabled by accident :)

[ceolin]

I think this option is source of confusion and if multiple SoCs adopt it we will end up with a log of duplicated logic, like lock interruptions, triggering PM , calling arch specific code ...

Isn't possible to add enter / exit hooks to the idle logic ?

[ceolin]

Bringing visibility to the folks here, since it is somehow related #84049

[bjarki-andreasen]

I think this option is source of confusion and if multiple SoCs adopt it we will end up with a log of duplicated logic, like lock interruptions, triggering PM , calling arch specific code ...

Isn't possible to add enter / exit hooks to the idle logic ?

That is what this PR is doing? though it only adds an enter hook since exit hook is not meaningful in this context, given that the "exit hook" is simply when the enter hook exits, so its implicitly available if required.