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

intel_adsp: ace: cpu clock management #8182

Conversation

tmleman
Copy link
Contributor

@tmleman tmleman commented Sep 11, 2023

Updated clock definitions for ACE family platforms.

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

I can only agree with this change, but the comments and values seem off?

app/overlays/mtl/fpga_overlay.conf Outdated Show resolved Hide resolved
@tmleman tmleman force-pushed the topic/upstream/pr/ace/clock_mng/def_update branch from e38a185 to 077d3c4 Compare September 14, 2023 08:08
@tmleman tmleman requested a review from plbossart September 14, 2023 08:09
@tmleman tmleman force-pushed the topic/upstream/pr/ace/clock_mng/def_update branch 2 times, most recently from 0dddd7e to a3d985d Compare September 18, 2023 11:07
@tmleman tmleman changed the title [DNM] intel_adsp: ace: cpu clock management intel_adsp: ace: cpu clock management Sep 18, 2023
@tmleman tmleman force-pushed the topic/upstream/pr/ace/clock_mng/def_update branch from a3d985d to c9e4707 Compare September 20, 2023 11:07
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Looks good except for two things: 1) we need a clean CI pass, and 2) please confirm we don't break git bisect.

@@ -51,7 +51,7 @@ static int basefw_config(uint32_t *data_offset, char *data)
tuple = tlv_next(tuple);
tlv_value_uint32_set(tuple,
IPC4_SLOW_CLOCK_FREQ_HZ_FW_CFG,
clock_get_freq(CPU_LPRO_FREQ_IDX));
clock_get_freq(CPU_LOWEST_FREQ_IDX));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, will this break bisect with the previous git commit? If yes, please merge this change to the Zephyr baseline update.

Note: we should really avoid these kind of changes and provide transition period with Zephyr (i.e. add new defines in Zephyr, merge to SOF, then clean up old from Zephyr once done). E.g. now this will block @dbaluta 's PR to update Zephyr as there a interface change in Intel code.
If there's no dependency, then this please copy this note paragraphi to /dev/null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have corrected the order of commits to make bisection possible.

You're right about changes in zephyr. In this case it was possible to introduce them there that way so the kernel could be updated without any changes in SOF. I will remember about this next time.

@tmleman tmleman force-pushed the topic/upstream/pr/ace/clock_mng/def_update branch from c9e4707 to fd594c0 Compare September 20, 2023 12:19
@kv2019i
Copy link
Collaborator

kv2019i commented Sep 20, 2023

Right, so now the remaining hurdle is this new build error on Intel MTL:

In file included from /zep_workspace/zephyr/include/zephyr/sys/mem_manage.h:104,
                 from /zep_workspace/zephyr/include/zephyr/sys/device_mmio.h:47,
                 from /zep_workspace/zephyr/include/zephyr/device.h:15,
                 from /zep_workspace/zephyr/drivers/mm/mm_drv_intel_adsp.h:17,
                 from /zep_workspace/zephyr/drivers/mm/mm_drv_intel_adsp_mtl_tlb.c:23:
/zep_workspace/zephyr/drivers/mm/mm_drv_intel_adsp_mtl_tlb.c: In function 'sys_mm_drv_mm_init':
/zep_workspace/zephyr/include/zephyr/sys/__assert.h:44:52: error: format '%p' expects argument of type 'void *', but argument 2 has type 'long unsigned int' [-Werror=format=] 

Affects #8235 as well. So we'd need a fix for this to be merged at the same time.

@dbaluta
Copy link
Collaborator

dbaluta commented Sep 20, 2023

Right, so now the remaining hurdle is this new build error on Intel MTL:
Affects #8235 as well. So we'd need a fix for this to be merged at the same time.

@kv2019i sent a patch here: zephyrproject-rtos/zephyr#62854

@tmleman tmleman force-pushed the topic/upstream/pr/ace/clock_mng/def_update branch 3 times, most recently from 4190a9f to d814fc2 Compare September 22, 2023 08:17
@tmleman tmleman force-pushed the topic/upstream/pr/ace/clock_mng/def_update branch from d814fc2 to 077bab2 Compare September 22, 2023 08:46
@kv2019i
Copy link
Collaborator

kv2019i commented Sep 22, 2023

Oh man, @tmleman @dbaluta so SOF CI build is ok and pr-device-test tests even seem to pass, but:

  • we have one mandatory Intel CI failing
  • all Windows builds fail (lost ninja, no idea what is causing this)
  • the sparse build is broken after the picolibc change

Considering I could merge PRs on Monday with all-green in the PR results page, this is pretty disappointing, but it is what it is.

It seems there are now multiple fails not related to actual SOF functionality, so I'd be inclined to merge this once mandatory CI checks passs, and file P1 bugs to track the sparse and windows build failures.

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 22, 2023

@marc-hb @lyakh Any insights on the sparse build fail? I couldn't make it work locally even without this PR, so I have no immediate insight how to go about to fix this...

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 22, 2023

all Windows builds fail (lost ninja, no idea what is causing this)

Fix submitted:

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 22, 2023

I couldn't make it work locally even without this PR

First you need a special version of sparse with @lyakh's fix. As always, check the build logs. In this case the "git clone sparse analyzer" step.

Then:

PATH=$PATH:$HOME/sparse

sof/scripts/xtensa-build-zephyr.py -p mtl --cmake-args=-DZEPHYR_SCA_VARIANT=sparse   --cmake-args=-DCONFIG_LOG_USE_VLA=n |& tee sparse.log

scripts/parse_sparse_output.sh < sparse.log

Same as what CI does, no CI secret.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 22, 2023

the sparse build is broken after the picolibc change
https://github.com/thesofproject/sof/actions/runs/6272123572/job/17033041652?pr=8182

Confirmed:

CONFIG_MINIMAL_LIBC=y in an overlay should be enough to avoid it. EDIT: merged.

@tmleman
Copy link
Contributor Author

tmleman commented Sep 25, 2023

Maybe we should wait for that too zephyrproject-rtos/zephyr#63004

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 25, 2023

@tmleman zephyrproject-rtos/zephyr#63004 now merged, can you update this PR?

This patch adds CPU_LOWEST_FREQ_IDX definition to keep compliance with
changes in base_fw.

Signed-off-by: Tomasz Leman <[email protected]>
This patch is changing value of slow clock in response for FwConfigGet
from LP clock to the lowest clock is section "slow clock".

Signed-off-by: Tomasz Leman <[email protected]>
ACE_1.5 and ACE_2.0 use only two clocks for DSP cores. First is WOVRCO and
second is ACE IPLL.

IPLL allows to configure it to work like LP RING Oscillator Clock or HP
RING Oscillator Clock. Currently, the driver does not allow this, so I
remove the frequency that cannot be achieved anyway.

Clocks frequencies:
WOV: 38.4 MHz
IPLL: 393.216 MHz

Signed-off-by: Tomasz Leman <[email protected]>
Changing max clock frequency for FPGA configuration.

Signed-off-by: Tomasz Leman <[email protected]>
Zepych update: total of 1048 commits.

Changes include:

b2f7ea0523 soc: xtensa/intel_adsp/ace: fix _end location
e560bd6b8c boards: intel_adsp: fix board compatible
b4998c357e mm_drv: tlb: Fix compile time warning
759e07bebe intel_adsp: move memory window setup to PRE_KERNEL_1
492517b918 west.yml: Update NXP HAL SDK to 2.14
a5d1fd9857 soc: adsp: clk: update clock switch flow
9656056b19 dts: adsp: ace20: remove lp clock
50f0e223e8 dts: adsp: ace15: remove lp clock
cf6d5f95b6 adsp: clk: ace: select ipll if wovrco is unavailable
2d835e1b29 dts: adsp: ace20: replace hp with ipll clock
dcecda859c dts: adsp: ace15: replace hp with ipll clock
2f2689e3d3 intel_adsp: ace15: shim: update wovrco request bit
ea9dd59460 yamllint: bindings: add ipll clock index
1ddabfa8d8 dai: intel: dmic: fix shadow variable
b26921d776 dai: intel: dmic: New functions for writing fir coefficients
cba9ec10c3 dai: intel: tgl: dmic: Refactor of dai_nhlt_dmic_dai_params_get function
c28e8ba9ba dai: intel: dmic: Add pdm_base and pdm_idx variables in blob parser
2452aaad50 dai: intel: dmic: Separate fir configuration code into function
f74fd8edaf dai: intel: ace: dmic: Add dai_dmic_start_fifo_packers function
76d03e798f dai: intel: ace: dmic: Using the WAIT_FOR macro in waiting functions
3fbaed4de9 dai: intel: ace: dmic: Refactor of dai_nhlt_dmic_dai_params_get function
d7672af838 dai: intel: dmic: Combine PDM registers definitions
8ea53d49b6 dai: intel: dmic: nhlt: Move debug print code to a separate functions
81944c5c62 dai: intel: dmic: Move definitions of nhlt structures to a new file

Signed-off-by: Tomasz Leman <[email protected]>
@tmleman tmleman force-pushed the topic/upstream/pr/ace/clock_mng/def_update branch from 077bab2 to 911f89d Compare September 25, 2023 17:04
@kv2019i
Copy link
Collaborator

kv2019i commented Sep 25, 2023

Nice, https://sof-ci.01.org/sofpr/PR8182/build13439/devicetest/index.html (all RED)

I guess zephyrproject-rtos/zephyr#63004 was not tested on hardware...? It seems none of the resulting binaries boot anymore.

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 26, 2023

I can repro the problem with a local build. DSP does not boot. But it's not zephyrproject-rtos/zephyr#63004 , if only that is reverted, boot still fails. More bisect is needed (plus it's not clear why some CI tests passed on this hardware).

@tmleman
Copy link
Contributor Author

tmleman commented Sep 27, 2023

I'm closing this PR. Those changes and zephyr version update are made here #8268.

@tmleman tmleman closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants