Skip to content
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

Jump to bottom

[DNM] intel_adsp: ace: dynamic clock switching #8423

Closed
wants to merge 7 commits into from
Closed

[DNM] intel_adsp: ace: dynamic clock switching #8423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5183a8b
[DRAFT] west.yml: zephyr version update
tmleman Oct 31, 2023
1fe1d9a
config: ace: enable ARCH_CPU_IDLE_CUSTOM
tmleman Dec 21, 2023
08a4a94
zephyr: pm_runtime: integrate dynamic clock switching support
tmleman Dec 22, 2023
a17cd0f
ipc4: handler: stabilize clock during delayed ipc reply
tmleman Dec 22, 2023
2a46303
ipc4: restore DSP clock on ipc message receipt
tmleman Dec 22, 2023
b005224
zephyr: ll-schedule: restore clock after idle in scheduler
tmleman Dec 22, 2023
c11531b
config: ace: enable dynamic clock switching
tmleman Dec 22, 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
4 changes: 4 additions & 0 deletions app/boards/intel_adsp_ace15_mtpm.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,17 @@ CONFIG_PM_DEVICE_RUNTIME=y
CONFIG_PM_DEVICE_RUNTIME_EXCLUSIVE=n
CONFIG_PM_DEVICE_POWER_DOMAIN=y
CONFIG_PM_POLICY_CUSTOM=y
CONFIG_PM_STATE_CUSTOM_DATA=y
CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING=y
Copy link
Member

Choose a reason for hiding this comment

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

I strongly suggest to rename it (in zephyr) to CONFIG_ADSP_IDLE_DYNAMIC_CLOCK_SWITCHING or something like that to avoid confusion with all kconfigs about clocks and kcps


CONFIG_POWER_DOMAIN=y
CONFIG_POWER_DOMAIN_INTEL_ADSP=y

CONFIG_ADSP_IMR_CONTEXT_SAVE=y
CONFIG_ADSP_IDLE_CLOCK_GATING=y

CONFIG_ARCH_CPU_IDLE_CUSTOM=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, why do you need this? I don't see a custom idle handled added in this patch.

This setting is now enabled in upstream by default (the new icache-workaround), so no need to enable this on SOF side anymore.


# enable Zephyr drivers
CONFIG_ZEPHYR_NATIVE_DRIVERS=y
CONFIG_DAI=y
Expand Down
4 changes: 4 additions & 0 deletions app/boards/intel_adsp_ace20_lnl.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,16 @@ CONFIG_PM_DEVICE_RUNTIME=y
CONFIG_PM_DEVICE_RUNTIME_EXCLUSIVE=n
CONFIG_PM_DEVICE_POWER_DOMAIN=y
CONFIG_PM_POLICY_CUSTOM=y
CONFIG_PM_STATE_CUSTOM_DATA=y
CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING=y
CONFIG_ADSP_IMR_CONTEXT_SAVE=y
CONFIG_ADSP_IDLE_CLOCK_GATING=y

CONFIG_POWER_DOMAIN=y
CONFIG_POWER_DOMAIN_INTEL_ADSP=y

CONFIG_ARCH_CPU_IDLE_CUSTOM=y

# enable Zephyr drivers
CONFIG_ZEPHYR_NATIVE_DRIVERS=y
CONFIG_DAI=y
Expand Down
4 changes: 4 additions & 0 deletions src/ipc/ipc-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ static bool message_handler(const struct device *dev, void *arg, uint32_t data,

#if CONFIG_DEBUG_IPC_COUNTERS
increment_ipc_received_counter();
#endif
#if CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING
/* If the DSP has been in Idle we may need to restore previous clock. */
soc_adsp_clock_idle_exit();
#endif
ipc_schedule_process(ipc);

Expand Down
4 changes: 4 additions & 0 deletions src/ipc/ipc4/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,16 +536,20 @@ static int ipc_wait_for_compound_msg(void)
{
int try_count = 30;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we generalize this a bit? I guess this could be a Kconfig to "keep core clock high when responding to IPC".
The actual implementation will vary on different platforms, so ideally we'd have a Zephyr interface to communicate this constraints, but for the time being we might need glue code on SOF side (e.g. for Intel this is turned into pm_runtime call on CORE_HP_CLK).

I'm ok to also go with a TODO comment for now if there's no direct Zephyr interface yet.


pm_runtime_disable(CORE_HP_CLK, PLATFORM_PRIMARY_CORE_ID);
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed? Will this work correctly with KCPS which changes clock during IPC?

while (atomic_read(&msg_data.delayed_reply)) {
/* preventing clock switching in idle */
k_sleep(Z_TIMEOUT_US(250));

if (!try_count--) {
atomic_set(&msg_data.delayed_reply, 0);
ipc_cmd_err(&ipc_tr, "ipc4: failed to wait schedule thread");
pm_runtime_enable(CORE_HP_CLK, PLATFORM_PRIMARY_CORE_ID);
return IPC4_FAILURE;
}
}

pm_runtime_enable(CORE_HP_CLK, PLATFORM_PRIMARY_CORE_ID);
return IPC4_SUCCESS;
}

Expand Down
4 changes: 4 additions & 0 deletions src/schedule/zephyr_domain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ static void zephyr_domain_thread_fn(void *p1, void *p2, void *p3)
#endif

cycles0 = k_cycle_get_32();
#if CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING
/* If the DSP has been in Idle we may need to restore previous clock. */
soc_adsp_clock_idle_exit();
#endif
dt->handler(dt->arg);
cycles1 = k_cycle_get_32();

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: 3614d4cb88f4714c47af66d7053304092659fb43
revision: pull/66092/head
remote: zephyrproject

# Import some projects listed in zephyr/west.yml@revision
Expand Down
26 changes: 25 additions & 1 deletion zephyr/lib/pm_runtime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@
#include <zephyr/pm/policy.h>
#include <sof/ipc/driver.h>

#if CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING
#include <pm_data.h>

static struct intel_adsp_ace_pm_data pr_runtime_data;
#endif /* CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING */

LOG_MODULE_REGISTER(power, CONFIG_SOF_LOG_LEVEL);

/* 76cc9773-440c-4df9-95a8-72defe7796fc */
Expand Down Expand Up @@ -74,6 +80,12 @@ void platform_pm_runtime_enable(uint32_t context, uint32_t index)
tr_dbg(&power_tr, "removing prevent on d0i3 (lock is active=%d)",
pm_policy_state_lock_is_active(PM_STATE_RUNTIME_IDLE, PM_ALL_SUBSTATES));
break;
#if CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING
Copy link
Member

Choose a reason for hiding this comment

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

Btw, what's the long term plan to move this into Zephyr as I would expect we should all the platform PM logic in Zephyr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We won't be able to move everything related to PM to Zephyr. We need to prevent changes to the clock and dynamic power gating on the application side. What can definitely be moved to Zephyr is the custom power policy (several changes in this direction have already been made).

Copy link
Member

Choose a reason for hiding this comment

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

ack, but we should do that with Zephyr PM APIs. e.g. zephyr_pm_prevent_clock_gating(CLOCK_ID) so that eventually we have zero IP code in SOF and its all in Zephyr. I know we cant yet do this today, but just for the record that we may have to extend Zephyr APIs here.

Copy link
Member

Choose a reason for hiding this comment

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

zephyr_pm_prevent_clock_gating(CLOCK_ID)

btw, we have clock gating, this is about clock switching.

I was wondering if there is really a benefit from idle clock switching (with switching overhead) if we have hw clock gating anyway

case CORE_HP_CLK:
tr_info(&power_tr, "removing lock on clock switching");
intel_adsp_release_clock_switch_lock(pm_state_custom_data_get(PM_STATE_ACTIVE, 0));
break;
#endif /* CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING */
default:
break;
}
Expand All @@ -86,13 +98,25 @@ void platform_pm_runtime_disable(uint32_t context, uint32_t index)
tr_dbg(&power_tr, "putting prevent on d0i3");
pm_policy_state_lock_get(PM_STATE_RUNTIME_IDLE, PM_ALL_SUBSTATES);
break;
#if CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING
case CORE_HP_CLK:
tr_info(&power_tr, "putting lock on clock switching");
intel_adsp_acquire_clock_switch_lock(pm_state_custom_data_get(PM_STATE_ACTIVE, 0));
break;
#endif /* CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING */
default:
break;
}
}

void platform_pm_runtime_init(struct pm_runtime_data *prd)
{ }
{
#if CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING
tr_info(&power_tr, "setting PM runtime data");
pm_state_custom_data_set(PM_STATE_ACTIVE, 0, &pr_runtime_data);
prd->platform_data = &pr_runtime_data;
#endif /* CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING */
}

void platform_pm_runtime_get(enum pm_runtime_context context, uint32_t index,
uint32_t flags)
Expand Down
Loading