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.

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

[FEATURE][Zephyr] implement clk.h with native Zephyr interface and drop platform clk.h #9541

Open
kv2019i opened this issue Oct 3, 2024 · 6 comments
Labels
enhancement New feature or request Zephyr Issues only observed with Zephyr integrated

Comments

@kv2019i
Copy link
Collaborator

kv2019i commented Oct 3, 2024

Is your feature request related to a problem? Please describe.
Implement the SOF clk.h interface without depending on SOF platform interface and rather implement the functions using direct calls to Zephyr. This allows to move the platform specifics to Zephyr side and reduce the work needed to be done on SOF side to add new platforms.

Part of #5794

Additional context
Primary use in code base is for clock_get_freq() (e.g. in basefw.c). Most of the other uses are in XTOS drivers, which can be ignored for Zephyr builds. All in all, it's a good question whether clk.h interface is needed at all for application use and whether the clk.h could be just dropped (and only used for XTOS driver and platform code).

IOW, is there any need to query the CPU frequency from application code? After all, clock gating and scaling can be used and any logic dependent on exact frequency, is potentially sensitive to platform differences.

@kv2019i kv2019i added the enhancement New feature or request label Oct 3, 2024
@lgirdwood lgirdwood added this to the v2.12 milestone Oct 3, 2024
@kv2019i
Copy link
Collaborator Author

kv2019i commented Oct 4, 2024

Ok this is somewhat more complex as src/lib/cpu-clk-manager.c has non-trivial use of clk.h and this is used on many native Zephyr SOF platforms.

@lgirdwood
Copy link
Member

Ok this is somewhat more complex as src/lib/cpu-clk-manager.c has non-trivial use of clk.h and this is used on many native Zephyr SOF platforms.

Ok, pls do create a new generic application header for cpu-clk manager and we can transition to that and remove the old headers.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Nov 19, 2024

Further observations of current design:

  • src/lib/clk.h still used in Zephyr builds (initialized by platform layer)
  • SOF clk.h provides interfaces to enumerate platform DSP clocks
    - Zephyr clock_control.h and DTS can describe invididual clocks but AFAIK there is not way to query and enumerate core clocks
  • SOF clk.h provides interfaces to use SOF notifier to get async events on DSP core freq change
    - no matching fucnction in Zephyr, but also no actual users of this in SOF Zephyr builds, so maybe can be dropped
  • Intel platforms use zephyr/dts/bindings/clock/intel,adsp-shim-clkctl.yaml schema to describe the DT data
  • platform clk.h provides platform specific defaults to KCPS control (like PRIMARY_CORE_BASE_CPS_USAGE)
  • sof/zephyr/lib/clk.c provides a mapping from SOF to zephyr interface, but is "DEVICE_DT_GET(DT_NODELABEL(clkctl))" generic enough?

Ripping out the cpu-clk-manager usage to its own interface might still be the best course of action, but above is stll useful to input even if we go down on this path.

@lgirdwood
Copy link
Member

Ripping out the cpu-clk-manager usage to its own interface might still be the best course of action, but above is stll useful to input even if we go down on this path.

We need to rip out the SOF side code that deals with clock IP and clock change notifications, i.e. any RTOS service/API should be used and the SOF code should be a client that asks for clock changes.

Agree, I dont think we have any notifier usage for clock change so can be dropped.

kv2019i added a commit to kv2019i/sof that referenced this issue Nov 21, 2024
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]>
@kv2019i
Copy link
Collaborator Author

kv2019i commented Nov 22, 2024

Some study into implementing clock management on top of Zephyr:

Next step would be to clean up the Intel ACE platform code in SOF to use more Zephyr definitions.

kv2019i added a commit to kv2019i/sof that referenced this issue Nov 25, 2024
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]>
kv2019i added a commit to kv2019i/sof that referenced this issue Nov 25, 2024
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]>
lgirdwood pushed a commit that referenced this issue Nov 26, 2024
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: #9541
Signed-off-by: Kai Vehmanen <[email protected]>
@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 13, 2024

Feature cutoff for v2.12. The remaining work has complex dependencies to Zephyr, so I won't move this to 2.13 directly. Needs to be separately planned, when SOF can intercept the clock framework.

@kv2019i kv2019i removed this from the v2.12 milestone Dec 13, 2024
@kv2019i kv2019i added the Zephyr Issues only observed with Zephyr integrated label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Zephyr Issues only observed with Zephyr integrated
Projects
None yet
Development

No branches or pull requests

2 participants