Skip to content

platform: ace: notifying about idle thread readiness #7174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kv2019i merged 2 commits into from
May 11, 2023
Merged

platform: ace: notifying about idle thread readiness #7174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec7cea2
zephyr: cpu: secondary core idle thread init
tmleman Mar 30, 2023
69c559f
platform: ace: notifying about idle thread readiness
tmleman Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/platform/intel/ace/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <ipc/info.h>
#include <kernel/abi.h>
#include <rtos/clk.h>
#include <sof/lib/cpu.h>

#include <sof_versions.h>
#include <stdint.h>
Expand Down Expand Up @@ -74,6 +75,11 @@ int platform_boot_complete(uint32_t boot_message)
return ipc_platform_send_msg(&msg);
}

static struct pm_notifier pm_state_notifier = {
.state_entry = NULL,
.state_exit = cpu_notify_state_exit,
};

/* Runs on the primary core only */
int platform_init(struct sof *sof)
{
Expand Down Expand Up @@ -114,6 +120,9 @@ int platform_init(struct sof *sof)
if (ret < 0)
return ret;

/* register power states entry / exit notifiers */
pm_notifier_register(&pm_state_notifier);

/* initialize the host IPC mechanisms */
trace_point(TRACE_BOOT_PLATFORM_IPC);
ipc_init(sof);
Expand Down
2 changes: 1 addition & 1 deletion west.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ manifest:

- name: zephyr
repo-path: zephyr
revision: 3e02d48e4ead9978d10ee760c640bf55873f6e95
revision: aba3b12e311b4779338406fe3e7c4c54f655d0cd
remote: zephyrproject

# Import some projects listed in zephyr/west.yml@revision
Expand Down
10 changes: 8 additions & 2 deletions zephyr/include/sof/lib/cpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,16 @@

#include <stdbool.h>

//#include <zephyr/sys/arch_interface.h>

#include <zephyr/arch/arch_inlines.h>

#if CONFIG_PM

#include <zephyr/pm/pm.h>

void cpu_notify_state_exit(enum pm_state state);

#endif /* CONFIG_PM */

/* let the compiler optimise when in single core mode */
#if CONFIG_MULTICORE && CONFIG_SMP

Expand Down
24 changes: 22 additions & 2 deletions zephyr/lib/cpu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,26 @@ static FUNC_NORETURN void secondary_init(void *arg)
#if CONFIG_ZEPHYR_NATIVE_DRIVERS
#include <sof/trace/trace.h>
#include <rtos/wait.h>
#include <zephyr/pm/pm.h>

LOG_MODULE_DECLARE(zephyr, CONFIG_SOF_LOG_LEVEL);

extern struct tr_ctx zephyr_tr;

/* notifier called after every power state transition */
void cpu_notify_state_exit(enum pm_state state)
{
if (state == PM_STATE_SOFT_OFF) {
#if CONFIG_MULTICORE
if (!cpu_is_primary(arch_proc_id())) {
/* Notifying primary core that secondary core successfully exit the D3
* state and is back in the Idle thread.
*/
atomic_set(&ready_flag, 1);
}
#endif
}
}

int cpu_enable_core(int id)
{
/* only called from single core, no RMW lock */
Expand All @@ -79,7 +93,13 @@ int cpu_enable_core(int id)
return 0;

#if ZEPHYR_VERSION(3, 0, 99) <= ZEPHYR_VERSION_CODE
z_init_cpu(id);
/* During kernel initialization, the next pm state is set to ACTIVE. By checking this
* value, we determine if this is the first core boot, if not, we need to skip idle thread
* initialization. By reinitializing the idle thread, we would overwrite the kernel structs
* and the idle thread stack.
*/
if (pm_state_next_get(id)->state == PM_STATE_ACTIVE)
z_init_cpu(id);
Copy link
Collaborator

@kv2019i kv2019i Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is an open-coded of z_smp_start_cpu() (see comment above), I don't fully understand why we have a different implementation here, and if this is correct, why this is not needed in the Zephyr kernel/smp.c version. Wouldn't the same problem be there? Or is this specific to a configuration where state is saved to persistant memory? If yes, this would need to be ifdef'ed out so this applies only to applicable platforms.

Copy link
Contributor Author

@tmleman tmleman May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how zephyr is handling it but right now it's not working properly. After secondary core exits D3 it will repeat the path that led him to this state. Power will not be shut off so eventually it will return to the Idle state but this is not correct behavior. Maybe zephyr doesn't support situation in which each core can be turned off and on multiple times.

Copy link
Collaborator

@kv2019i kv2019i May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nashif @teburd @dcpleung @andyross ^^ trying to understand why z_smp_start_cpu() does not need this?

#endif

atomic_clear(&start_flag);
Expand Down