-
Notifications
You must be signed in to change notification settings - Fork 349
zephyr: stop using CONFIG_SOF to identify Zephyr build (w/ Zephyr Nov/20 update) #10335
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
zephyr: stop using CONFIG_SOF to identify Zephyr build (w/ Zephyr Nov/20 update) #10335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the dependency on CONFIG_SOF (which was defined in Zephyr's SOF module Kconfig) and replaces it with CONFIG_ZEPHYR_SOF_MODULE. This change prepares for the removal of the SOF module from the upstream Zephyr manifest.
Key changes:
- Replaced
CONFIG_SOFwithCONFIG_ZEPHYR_SOF_MODULEin CMakeLists.txt - Removed
if SOFwrapper from Kconfig file - Removed
CONFIG_SOF=ysetting from app/prj.conf
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| zephyr/Kconfig | Removed the conditional if SOF wrapper around SOF configuration options |
| zephyr/CMakeLists.txt | Updated build condition from CONFIG_SOF to CONFIG_ZEPHYR_SOF_MODULE |
| app/prj.conf | Removed explicit CONFIG_SOF=y setting as it's no longer needed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kv2019i some build issue in CI, do we need any CI changes ? |
|
There are issues with this still. I can't really fully explain what happens. Some CONFIG_FOO options are missing when other settings (defined in same Kconfig file) are present. E.g. .config for MTL build: So CONFIG_MAX_CORE_COUNT is set but not CONFG_CORE_COUNT, both Kconfig options defined in the smae file. Hmm. Will debug this more. |
|
Ok, wow, it seems this this the extra CONFIG_CORE_COUNT definition in arch/host now. I now start to realize why nobody else has cleaned this up. This is quite messed up. Let me put this to draft and try to push this commit together with #10334 . |
|
See dependencies in #10334 (comment) . PRs #10334 and #10319 need to be merged first, before this PR can go in. |
95f5897 to
84592ab
Compare
|
v2:
|
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i are we pending the Zephyr v4.3 west update before we merge ?
|
@lgirdwood wrote:
No, this can go in right now if the CI passes. |
|
@tmleman This now breaks the new ZTests: https://github.com/thesofproject/sof/actions/runs/19469971535/job/55714257536?pr=10335 It seems with the test, we want to not build without any of the SOF stuff, but I couldn't immediately figure out how to differentiate between ztest build and normal SOF build. In both cases CONFIG_ZTEST=y and CONFIG_ZEPHYR_SOF_MODULE=y . We could bring CONFIG_SOF back, but where to set it? I'll look at this tomorrow... (we can merge #10365 first ) |
84592ab to
a8c7a86
Compare
|
V3:
|
| Top-level build option to enable full build of the SOF application | ||
| on top of Zephyr. This should be set to no only for unit test and | ||
| such special build targets. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you retain the if here and the endif:
if SOF_FULL_ZEPHYR_APPLICATION
or better:
menuconfig SOF_FULL_ZEPHYR_APPLICATION
bool "Build full SOF Zephyr application"
default y
help
Top-level build option to enable full build of the SOF application
on top of Zephyr. This should be set to no only for unit test and
such special build targets.
if SOF_FULL_ZEPHYR_APPLICATION
...
endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ujfalusi I left this out on purposew. There are some Kconfigs that are useful to be availlable for SOF ztest type of uses and with PR, these can now be left defined (but set to "n" in the test configs if not needed).
It seems CI is all good with this as well, so I'd say the less ifdefs the better.
majunkier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked fw building with current and newest Zephyr SHA in west.yaml - both works.
|
All tests failing, triggering Intel jenkins CI once more before merge. |
|
SOFCI TEST |
Prepare for change in Zephyr to drop SOF module from the upstream Zephyr manifest. Remove use of CONFIG_SOF which is defined in work/zephyr/modules/Kconfig.sof, but will now be removed. Fix this by adding new CONFIG_SOF_FULL_ZEPHYR_APPLICATION on SOF side and use it similarly to old CONFIG_SOF. If SOF is built without Zephyr (e.g. testbench or cmocka build), this define will not be set. After this change, one must use the SOF work/sof/west.yml manifest to build SOF with Zephyr. This is used by all public instructions and CI jobs already. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
SOF builds previously relied on CONFIG_SOF to be externally defined in zephyr/modules/Kconfig.sof, but set in sof/app/prj.conf. This was removed in Zephyr, and now instead the new CONFIG_SOF_FULL_ZEPHYR_APPLICATION is set on SOF side. This must be set to 'no' for the ztest unit tests to ensure full SOF dependencies are not pulled into unit tests builds. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
a8c7a86 to
47c5e42
Compare
|
V4 update:
|
@kv2019i sorry, not sure I understand: CI only tests the merged result of all the changes in the PR, what difference does it make for the CI if you remove |
app/prj.conf
Outdated
| @@ -1,5 +1,5 @@ | |||
| CONFIG_SOF=y | |||
| CONFIG_BUILD_OUTPUT_BIN=n | |||
| CONFIG_SOF=y | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this hunk can be dropped, and the commit title should be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh It's just moved to the last patch. I have to split this into multiple patches not to break bisect. If I remove CONFIG_SOF in a different commit than the zepyr west.yaml, I break bisect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i I understand why you keep CONFIG_SOF=y but I don't think swapping the lines in the file is needed? ;-) So you could just drop this hunk and leave the file unchanged - first line CONFIG_SOF=y, second line CONFIG_BUILD_OUTPUT_BIN=n
Total of 798 commits. Includes removal of CONFIG_SOF in Zephyr. This commit has the matching change to sof/app/prj.conf. Other changes include: 3f6ef5043ce5 soc: intel_adsp: ace30: allow userspace to execute cold functions 18b34bb3d29d drivers: power_domain: intel_adsp: Refactor power management initialization 27180d2fc573 arch: riscv + xtensa + x86: workaround needed for LLVM linker 95b48cd5ba7a west_commands: do not depend on CONFIG_SOF 71ec5df8f31f intel_adsp: remove workaround for SOF setting core count 1a780f933e1d manifest: optional: remove sof from optional manifest 65d31ea8609e tests: intel_adsp/smoke: cast to signed ints before abs() e45808b9cd71 xtensa: mmu: add page table usage statistics a48345fccfdb xtensa: mmu/ptables: conserve memory by using COW on L2 tables 1e41db3ddd84 xtensa: mmu: no need for cache ops if page tables are not cached 501368601dc7 xtensa: mmu/ptables: rework TLB invalidation on L2 unmap 259be3d55929 xtensa: mmu: remove XTENSA_ prefix for page table array macros 1470d9ef7460 xtensa: mmu: move PTE macros into source file b6713c0145e8 xtensa: mmu: skip PTE SW field redirection 2bfcb20258f5 xtensa: mmu: unify PTE macros 11dca1208aa3 boards: xtensa: add support for DOIT ESP32 DevKit V1 3b2b513be37e drivers: wifi: nxp: Raise Wi-Fi mgmt events for soft AP start/stop. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
47c5e42 to
628afcd
Compare
|
V5 update:
|
|
@lrudyX Can you check? All tests that are run are passing, but a few tests fail to execute ("ConnectionRefusedError: [Errno 111] Connection refused" in the logs). This PR would be good to get in to allow other Zephyr PR's through. FYI @abonislawski @tmleman |
Scheduled for rerun. |
Prepare for change in Zephyr to drop SOF module from the upstream Zephyr manifest. Remove use of CONFIG_SOF which is defined in work/zephyr/modules/Kconfig.sof, but will now be removed.
Fix this by using CONFIG_ZEPHYR_SOF_MODULE instead. This is defined by the Zephyr build system when using the work/sof/west.yml for building SOF. If SOF is built without Zephyr (e.g. testbench or cmocka build), this define will not be set.
After this change, one must use the SOF work/sof/west.yml manifest to build SOF with Zephyr. This is used by all public instructions and CI jobs already.