audio: make clock control optional in SOF Zephyr builds (plus remove clk platform code for TGL as example) #9670
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kv2019i
requested review from
lyakh,
iuliana-prodan,
dbaluta,
abonislawski,
lgirdwood,
plbossart,
mmaka1 and
lbetlej
as code owners
kv2019i
commented
Nov 21, 2024
| ret = core_kcps_adjust(cpu_get_id(), PRIMARY_CORE_BASE_CPS_USAGE); | ||
| #else |
There was a problem hiding this comment.
@abonislawski @serhiy-katsyuba-intel Can you double-check this? It seems unnecessary to set the DSP frequency from SOF side if there is no dynamic control (the Zephyr platform code should choose the default speed).
There was a problem hiding this comment.
@abonislawski any comment?
| trace_point(TRACE_BOOT_PLATFORM_SCHED); | ||
| scheduler_init_edf(); | ||
|
|
||
| /* init low latency timer domain and scheduler */ | ||
| sof->platform_timer_domain = timer_domain_init(sof->platform_timer, PLATFORM_DEFAULT_CLOCK); | ||
| sof->platform_timer_domain = timer_domain_init(sof->platform_timer, 0); |
There was a problem hiding this comment.
The "clk" parameter has no use on Zephyer, so might as well pass a 0.
There was a problem hiding this comment.
Done in today's PR revision.
UPDATE: comment really udpated in the second PR revision of today
lyakh
reviewed
Nov 22, 2024
| /* | ||
| * make sure that the DSP is running full speed for the duration of | ||
| * library loading | ||
| */ | ||
| ret = core_kcps_adjust(cpu_get_id(), CLK_MAX_CPU_HZ / 1000); | ||
| if (ret < 0) | ||
| goto err_dma_buffer; | ||
| #endif |
There was a problem hiding this comment.
maybe a dummy inline core_kcps_adjust() implementation instead?
There was a problem hiding this comment.
@lyakh Tried this approach, but it gets a bit ugly as CLK_MAX_CPU_HZ is not defined either. Or we want to drop the requirement to define such values on SOF side.
| ret = core_kcps_adjust(cpu_get_id(), PRIMARY_CORE_BASE_CPS_USAGE); | ||
| #else | ||
| ret = core_kcps_adjust(cpu_get_id(), CLK_MAX_CPU_HZ / 1000); | ||
| #endif | ||
| if (ret < 0) | ||
| return ret; |
There was a problem hiding this comment.
please, move this check under #ifdef and remove ret initialisation
There was a problem hiding this comment.
Done in today's PR revision.
UPDATE: missed the ret initialization, will update.
| trace_point(TRACE_BOOT_PLATFORM_SCHED); | ||
| scheduler_init_edf(); | ||
|
|
||
| /* init low latency timer domain and scheduler */ | ||
| sof->platform_timer_domain = timer_domain_init(sof->platform_timer, PLATFORM_DEFAULT_CLOCK); | ||
| sof->platform_timer_domain = timer_domain_init(sof->platform_timer, 0); |
Add an explicit include for rtos/clk.h in code that is using CLK_ definitions. Currently this dependency is pulled in via platform.h, but better to have dependency recorded in the compilation unit that is actually using the interface. Signed-off-by: Kai Vehmanen <[email protected]>
Remove the use of to clock_get_freq() in platform_dai_timestamp(). This is at best unnecessary dependency and at worst a subtle bug if the timebase for default platform clock (of clock_get_freq()) and sof_cycle_get_64() are different. Add sys_cycle_get_64_rate() to get the clock rate. This is in practise CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC, but better to have this in a separate function and defined in the same place in rtos/timer.h as sof_cycle_get_64() in case some SOF target requires customization in how Zephyr k_cycle_get_64() is used. Signed-off-by: Kai Vehmanen <[email protected]>
Library manager and dynamic CPU clock control can be independently enabled/disabled in the build. To allow the clock control code to be removed from the build, make the calls to kcps_*() conditional in the library manager code. Signed-off-by: Kai Vehmanen <[email protected]>
kv2019i
force-pushed
the
202411-clk-rework-step1
branch
from
04c2b19 to
375fc86
Compare
kv2019i
changed the title
Dynamic CPU clock control is not enabled in all ACE builds. Make the kcps_*() calls conditional in the platform initialization code. The default clock frequency at boot should be set by platform code in Zephyr for each platform. Link: thesofproject#9541 Signed-off-by: Kai Vehmanen <[email protected]>
Mark the few places in generic SOF code where SOF clock control interface is used. These cases are few as most usage has traditionally been in XTOS drivers and platform code. In Zephyr builds these are not used, making the clock interface mostly unnecessary. The one bigger exception is CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL feature for dynamically adjusting the DSP clock frequency based on IPC messages and audio pipeline configuration. This is an optional feature not used by all targets, so the requirement to have a clock abstraction implemented, should also be optional. Remaining uses are for IPC4 base firmware attributes and some informational use in logging. None of these are e.g. required by SOF Linux driver for any essential functionality, so can be disabled without side-effects. As the rtos/clk.h interfaces are still used in many places in platform code, this patch adds a new transition tool in form of CONFIG_SOF_ZEPHYR_NO_SOF_CLOCK Kconfig option. This allows to incrementally transition targets to not use the clock framework. In longer term, the remaining uses will be transitioned to use Zephyr clock-control.h directly Signed-off-by: Kai Vehmanen <[email protected]>
The platform headers should not be included directly. Replace platform/clk.h include with rtos/clk.h. This patch allows to remove dependencies from platform.h files. Signed-off-by: Kai Vehmanen <[email protected]>
Take benefit of the ability to build SOF Zephyr without SOF clock interface and drop all the clock related definitions from TGL platform code. Signed-off-by: Kai Vehmanen <[email protected]>
kv2019i
force-pushed
the
202411-clk-rework-step1
branch
from
375fc86 to
f6d7d4c
Compare
lyakh
approved these changes
Nov 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is first step in solving in simplifying clock management code in SOF Zephyr builds ( #9541 ).
This PR does the following
The big elephant in the room is cpu-clk-manager.c. This is an optional feature, currently only used by Intel, but is also the only legitimate use of the clock interface. IOW, this code really needs to know the available CPU/DSP frequencies, be able to get the current frequency and set it. And ideally do this in a way that works on multiple platforms.
UPDATE: some update on this, a clock management framework is under discussion in Zephyr that would meet the needs of SOF cpu-clk-manager, see more details in #9541 (comment)
The next step for me would be to see how to reimplement cpu-clk-manager.c directly on top of Zephyr clock-control.h interface. There are parts available (clock-control.h and Intel "intel,adsp-shim-clkctl"" DT), but not all so this is likely to require Zephyr-side development.