Move platforms software configuration from Zephyr to SOF #5531
Conversation
aborisovich
requested review from
marc-hb,
lgirdwood,
plbossart,
mmaka1,
lbetlej and
dbaluta
as code owners
platforms/apl/overlay.conf
Outdated
| @@ -0,0 +1,7 @@ | |||
| CONFIG_APOLLOLAKE=y | |||
| CONFIG_INTEL_DMIC=y | |||
There was a problem hiding this comment.
Can you explain a little bit what is an overlay? Is just a fragment of config file that gets applied over default config?
There was a problem hiding this comment.
Exactly. It just applies one by one over existing Kconfig entries.
Let me just paste part of the build output logs:
Parsing /home/aborisov/workspace/software_configuration_pub/zephyrproject/zephyr/Kconfig Loaded configuration '/home/aborisov/workspace/software_configuration_pub/zephyrproject/zephyr/boards/xtensa/intel_adsp_cavs25/intel_adsp_cavs25_defconfig' Merged configuration '/home/aborisov/workspace/software_configuration_pub/zephyrproject/zephyr/samples/subsys/audio/sof/prj.conf' Merged configuration '/home/aborisov/workspace/software_configuration_pub/platforms/tgl/overlay.conf' Configuration saved to '/home/aborisov/workspace/software_configuration_pub/zephyrproject/build/zephyr/.config' Kconfig header saved to '/home/aborisov/workspace/software_configuration_pub/zephyrproject/build/zephyr/include/generated/autoconf.h'
This is TGL build example. So it basically:
- Gets zephyr board defconfig values
- Then applies prj.conf values on top of it
- Then applies our new overlay.conf on top of it
<etc... may be further overlay files provided that will just overwrite values that are repeated> - Final "summary" of Kconfig settings is written to autoconf.h header file.
There was a problem hiding this comment.
@aborisovich this is very good information. If you can add this please in the commit message would be great!
There was a problem hiding this comment.
overlays are documented in https://docs.zephyrproject.org/latest/application/index.html
scripts/xtensa-build-zephyr.py
Outdated
| @@ -32,24 +32,28 @@ | |||
| { | |||
| "name": "apl", | |||
| "PLAT_CONFIG": "intel_adsp_cavs15", | |||
| "CONFIG_OVERLAY": "overlay.conf", | |||
There was a problem hiding this comment.
This means that we won't support xtensa-build-zephyr.sh script anymore?
Cc: @marc-hb
There was a problem hiding this comment.
There were plans to stop support for xtensa-build-zephyr.sh but I do not know what is the status of CI migrations.
@marc-hb should I add this to .sh script too?
There was a problem hiding this comment.
@dbaluta plan is to have one script and the python version is easier to maintain and develop (supports Windows too). Are you using the bash script internally or with yocto - if so we need to create a transition plan.
There was a problem hiding this comment.
@lgirdwood thanks for the info. We are fine with using the python script with next SOF release. Should be v2.1 right?
BTW, any schedule when that will be released?
Later edit: sof-stable-v2.1 is already release, i think i might have missed the information. What is the target date for sof-stable-v2.2?
There was a problem hiding this comment.
SOF stable-v2.1 has been branched, it's too late to deprecate xtensa-build-zephyr.sh in 2.1
xtensa-build-zephyr.sh should absolutely be deprecated for 2.2, much earlier in fact. It will take a bit of time to make sure all CIs have switched but letting xtensa-build-zephyr.sh "bitrot" already now is a good way to put pressure on CI.
aborisovich
force-pushed
the
move-software-configuration-from-zephyr-to-sof
branch
from
d3afd19
to
0cd74f7
Compare
platforms/apl/overlay.conf
Outdated
| @@ -0,0 +1,7 @@ | |||
| CONFIG_APOLLOLAKE=y | |||
There was a problem hiding this comment.
@aborisovich please use platform prefix in the commit message. Also use imperative mood.
e.g platform: Move platforms software configuration from Zephyr to SOF
Another, thing is that you are creating a new directory src/platforms right? We already have src/platform can you somehow use this one instead?
There was a problem hiding this comment.
Sure I will reword in a moment.
Another, thing is that you are creating a new directory src/platforms right? We already have src/platform can you somehow use this one instead?
This is totally not related to the sources I do not think this is good idea.
There was a problem hiding this comment.
@aborisovich @lgirdwood yes, but having two directories named src/platform and src/platforms is very confusing. We need to find a better location for this conf files.
There was a problem hiding this comment.
This is not src/platforms.
It is just SOF_TOP/platforms.
We intent to add there also cmake files in future to reduce complexity of the building scripts for Zephyr.
There was a problem hiding this comment.
@aborisovich oh, ok! It is fine then.
|
@marc-hb @kv2019i @keqiaozhang fyi in case config updates needed for CI or testing scripts. |
There was a problem hiding this comment.
Thanks, the concept looks great. Some comments on the implementation details.
I'm not a fan of the poorly defined platforms/ name (see #3491), how about a more basic overlays/apl/?
Who knows, maybe we will have device tree or some other overlays in the future, could you check https://docs.zephyrproject.org/latest/application/index.html and the existing Zephyr source and find something more specific than overlay.conf? Something with kconfig in the name maybe?
Speaking of naming conventions, could someone from the core Zephyr team take a look at this? @andyross , @dcpleung , @MaureenHelm , other? Thanks in advance!
Also, to test this it would be good to compare the generated/.config files with and without this PR.
platforms/apl/overlay.conf
Outdated
| @@ -0,0 +1,7 @@ | |||
| CONFIG_APOLLOLAKE=y | |||
| CONFIG_INTEL_DMIC=y | |||
There was a problem hiding this comment.
overlays are documented in https://docs.zephyrproject.org/latest/application/index.html
scripts/xtensa-build-zephyr.py
Outdated
| @@ -32,24 +32,28 @@ | |||
| { | |||
| "name": "apl", | |||
| "PLAT_CONFIG": "intel_adsp_cavs15", | |||
| "CONFIG_OVERLAY": "overlay.conf", | |||
There was a problem hiding this comment.
SOF stable-v2.1 has been branched, it's too late to deprecate xtensa-build-zephyr.sh in 2.1
xtensa-build-zephyr.sh should absolutely be deprecated for 2.2, much earlier in fact. It will take a bit of time to make sure all CIs have switched but letting xtensa-build-zephyr.sh "bitrot" already now is a good way to put pressure on CI.
scripts/xtensa-build-zephyr.py
Outdated
| @@ -349,9 +357,16 @@ def build_platforms(): | |||
| if args.cmake_args: | |||
| build_cmd += args.cmake_args | |||
|
|
|||
| overlays = [] | |||
| if "CONFIG_OVERLAY" in platform_dict: | |||
| overlays.append(str(pathlib.Path(SOF_TOP, "platforms", platform, platform_dict["CONFIG_OVERLAY"]))) | |||
There was a problem hiding this comment.
I don't think it's possible to have an entirely empty Kconfig file, at the very least we need CONFIG_ICELAKE=y or equivalent.
So can you make overlay.conf the default filename and avoid the repetition? Something like this:
overlay_filename = platform_dict.get("CONFIG_OVERLAY", default="overlay.conf")
overlays.append(str(pathlib.Path(SOF_TOP, "platforms", platform, overlay_filename))There was a problem hiding this comment.
Yes sure. I was wondering if it is possible to build without Kconfig entries at all just by what is given in board files but if it is not lets remove it.
aborisovich
force-pushed
the
move-software-configuration-from-zephyr-to-sof
branch
from
0cd74f7
to
7687c1e
Compare
|
Push update:
|
aborisovich
force-pushed
the
move-software-configuration-from-zephyr-to-sof
branch
from
7687c1e
to
f908822
Compare
|
Push update:
|
I've read briefly that improvement article. I do agree that there is a mess currently in the sense you described there but I do not agree that using platform abstraction is bad approach. Freely speaking if we would build computer mice's then the Zephyr board would be lets say "Logitech Master Family" and platform abstraction would be "Logitech Master MX3" and overlay.conf would contain information whether it uses micro USB or USB-C port to charge.
I've searched over Zephyr docs and I believe overlays is the only way to go for us. Overlays are fine as we can concatanate them and the order of appearance resolves final value of the Kconfig entry. |
There was a problem hiding this comment.
Minor spelling issues "Moved software" in git message body, but otherwise looks great, thanks @aborisovich .
@dbaluta So software configuration is basicly application specific Kconfig options, and specifically from Zephyr point of view. The split is very complicated at this point, as we have drivers in both SOF and Zephyr trees, so a setting lke CONFIG_INTEL_SSP does not make much sense (this is not an application property, but rather a hw property that in Zephyr should be described in DT). But e.g. CONFIG_COMP_SRC_TINY is a better example. This kind of setting does not belong to Zephyr as this is not a hardware property, but rather application (=SOF) property. This patch allows us to more cleanly separate descriing hw (that should be in Zephyr tree) and describing SOF application configuration (that should be in SOF tree).
platforms/jsl/overlay.conf
Outdated
| CONFIG_ICELAKE=y | ||
| CONFIG_INTEL_DMIC=y | ||
| CONFIG_INTEL_SSP=y | ||
| CONFIG_LP_MEMORY_BANKS=1 |
There was a problem hiding this comment.
These are a bit problematic as these properties are not SW configuration, but HW properties that should be set on Zephyr side.
But alas, these are the SOF side definitions and they are defined in the "samples/" dir in Zephyr, so I guess better to move them back to SOF now.
There was a problem hiding this comment.
@kv2019i @aborisovich I wonder if we can fix this by a rename for some duplicated (in meaning) CONFIGs. i.e. CONFIG_XTOS_LP_MEMORY_BANKS as a subsequent PR ?
There was a problem hiding this comment.
@lgirdwood This is probably better done after the settings have been merged to SOF tree. Then you can make a single PR in SOF tree to change the names for both Zephyr and XTOS.
@aborisovich I think we are talking across each other a bit. I was only commenting about the directory and file names, not suggesting any alternative to overlays at all (I'm not knowledgeable enough to suggest alternatives to overlay).
If you look at the toolchain parameters right now you can see that toolchain configuration is not per "platform": the same toolchain configuration covers many different "platforms". This new overlay system and re-organization is a great opportunity to drop the pseudo per "platform" classification and reduce copy/paste/diverge (which causes accidents like b9d9719)
I think things like I haven't look at the Zephyr docs much but maybe they fail or didn't want to make the examples specific. |
aborisovich
changed the title
Marc the ambiguity of "platform" you talk about in this changeset is not an "issue" but a "plan". The plan was to create some build.cmake file in platforms// directory and place there all the things related to the given platform like rimage signing schema or what toolchain to use (what would reduce number of "platform" word occurrences in the project by putting those things into one place. But maybe you are right - I should not create things in this changeset that are "meant for future". If the directory has only overlays inside for now lets call it overlays! With further refactorings we can change that name as needed if other things will be put there. |
aborisovich
force-pushed
the
move-software-configuration-from-zephyr-to-sof
branch
from
f908822
to
8ec32fd
Compare
|
Push update:
|
aborisovich
force-pushed
the
move-software-configuration-from-zephyr-to-sof
branch
2 times, most recently
from
e1dcafe
to
c08add2
Compare
|
Merge please? I don't think approve from somebody from Zephyr project is necessary. We use their system after all it matches their docs/tutorials so I think we are good to go. |
|
Looks good to me. Lets wait for @marc-hb approval and will merge it. |
There was a problem hiding this comment.
Sorry for not reviewing the shell script earlier, I think I found a small bug in it.
I don't think approve from somebody from Zephyr project is necessary. We use their system after all it matches their docs/tutorials so I think we are good to go.
Thank you for checking the Zephyr docs, really appreciated however docs are always lagging a bit behind and examples must always to be kept (too) generic. Also, this is a top-level directory meaning it would very painful to rename if needed, would rename many files with a near 100% chance of git conflict. So I have no objection but I would really like a nod from some Zephyr expert who's already done stuff like this. @andyross , @dcpleung could you help find someone? @mbolivar-nordic maybe?
Build-time configuration: what everyone loves to hate :-)
scripts/xtensa-build-zephyr.sh
Outdated
| @@ -236,6 +236,26 @@ build_platforms() | |||
| ;; | |||
| esac | |||
|
|
|||
| # You may override overlay name in the platform variables by setting CONFIG_OVERLAY | |||
There was a problem hiding this comment.
The unset below breaks that promise.
This happens because the same variable is used for both user input and as "local" variable. The fix is to considered user input sacred and keep it immutable. Simply use two different variables, something like this:
# default value if undefined
local config_overlay=${CONFIG_OVERLAY:-overlay.conf}(${parameter:-defaultvalue} expansion is standardized in https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html )
There was a problem hiding this comment.
Fixed, thanks Marc! Bash scripts are not my strong side ;-)
scripts/xtensa-build-zephyr.sh
Outdated
| unset CONFIG_OVERLAY | ||
|
|
||
| if ! [[ -d "$CONFIG_DIR" ]]; then | ||
| die 'Overlay configuration directory: %s does not exist, create one.\n' \ |
There was a problem hiding this comment.
The more concise and more common coding style is do || die which reads like plain English and (in this case) avoids the negation (the ! character is one of the very few that cannot be copied on the command line for interactive testing, try it). Negations are just one step away from double negations :-)
| die 'Overlay configuration directory: %s does not exist, create one.\n' \ | |
| test -d "$CONFIG_DIR" || | |
| die 'Overlay configuration directory: %s does not exist, create one.\n' \ |
Move software configuration from Zephyr samples directory to SOF platforms directory using Zephyr overlay mechanism. How it works: 1. Gets zephyr board defconfig values 2. Applies Zephyr samples prj.conf values on top of it 3. Then applies our new overlay.conf on top of it 4. Final "summary" of Kconfig settings is written to autoconf.h header Signed-off-by: Andrey Borisovich <[email protected]>
aborisovich
force-pushed
the
move-software-configuration-from-zephyr-to-sof
branch
from
c08add2
to
3f72788
Compare
|
Push update:
|
|
How are those overlays going to be used if you decide to build using west or cmake directly and not use the wrapper scripts? |
|
I guess I counta as a Zephyr voice. Can I give a wishywashy answer here, skip all the specifics, and say I think that overlays look just fine? There are a conceptually infinite number of ways to specify kconfig values for a Zephyr build. By default you use a single "prj.conf" file. You can use an alternate file instead via the CONF_FILE cmake variable. Overlays provide a somewhat more flexible mechanism. But remember it's also possible to do it in proper kconfig syntax (vs. the defconfig settings format used for prj.conf) in SOF's own kconfig tree by providing defaults for settings separate from their definitions. That is, you could see that you're building on a 2-core TGL platform via an "if" and then add a bunch of defaults for Zephyr/SOF variables in that block, etc.... We do that in Zephyr at the soc and board level routinely, though not for apps (because apps don't have their own kconfig trees, but SOF does!). Alternatively, I could see an architecture where SOF's build environment has a whole extra layer of configuration to auto-generate the prj.conf file from metaconfiguration too, and that doesn't sound so weird given the level of complexity. I guess what I'm saying is that application configuration is a job for application code, and Zephyr's role is just to accomodate the scheme applications think works best. Certainly if we're making this hard for you, we should view that as a bug and fix it. But it's not our place to tell you how this should work. :) |
|
So basically as @andyross indicated, its our business (SOFs) how we will generate the configuration Zephyr just gives interfaces to use. And I am not saying that overlays are final solution here especially that we have configs for non-zephyr platforms in other directory and other format. But overlays are the fastest/simplest way to do this for now by the book. |
Does anyone ever invoke CMake directly? Every single time I "peeled the onion" and stepped down to invoke CMake directly it was to debug some CMake-related or other build issue. I think the main reason every CMake project in the universe seems to have a CMake wrapper script (like for instance cmake -DWEST_PYTHON=/usr/bin/python3 -B build-apl -S zephyr/samples/subsys/audio/sof -GNinja -DBOARD=intel_adsp_cavs15 -DOVERLAY=stuff -DTOOLCHAIN=blah -DFOO=bar...
ninja -C build-apl
# switch branch or something
rm -rf build-apl
cmake -DWEST_PYTHON=/usr/bin/python3 -B build-apl -S zephyr/samples/subsys/audio/sof -GNinja -DBOARD=intel_adsp_cavs15 -DOVERLAY=stuff -DTOOLCHAIN=blah -DFOO=bar...It does not take long for the (real! Try I see this PR as (among others) a preliminary step towards solving part of this problem and removing some stuff from Now of course the other question is: why another layer of wrapper script (
So in the longer term maybe the last two can be transferred to west. In the shorter term, it's always faster to just dump your shell history into a script and the rest is history. Not sure about the first two. |
|
@nashif @andyross I think we can refine and tune the build process as we converge more and identify/solve the opens. Our target should be we can invoke west on Windows/Linux and build, sign and optionally install our board + configuration + toolchain + key with a single simple (or maybe 2) west command(s). We will get there. Probably a topic for next call. |
not talking about calling cmake directly, this is about having all this knowledge in cmake files, so you can have a project that is easily buidlable without wrappers and by invoking cmake or west directly without having yet another wrapper.
right, nobody will want to call this, but you are the application, and you can put all of this in the top level cmake file and skip all of this. again, I am not saying call cmake with all those options, you can let cmake know about this all in the cmake file and simplify things by just calling west or a simplified version of cmake command line if you do not want west.
not saying the overlays introduced here are bad, but if the only way to use them is through some custom wrapper, there is going to be a major disconnect if you try to go to the basics again.
Developers in zephyr do not use twister to build multiple images, this is our CI and Test script and I think there is a place for this, but I see this script being used for almost everything, like a one-stop shop, is never good idea. Why do I need to download zephyr if I have it already downloaded and setup for my needs? Why can't you invoke signing in cmake directly? My original question is about how those overlays are going to be used if someone decides to use twister, west or cmake directly and not the wrapper script, why can't they be integrated in the lowest possible layer (cmake), so that every level of wrappers use the same information. |
|
Thanks @nashif
You don't, it's an optional feature used mainly by CI.
I don't think a "one-stop shop" is always a bad idea and this one is only 400 lines long and getting smaller. I'm more worried about features being implemented in the wrong place or duplicating some existing tool but if it comes to that then I'd rather have these design mistakes all contained in a single small script rather than scattered across multiple and even smaller scripts. With a single script at least CI and developers use and maintain the same script and build the same thing (the most common CI problem)
If not using twister, how do developers build multiple platforms or multiple build configurations to make sure they didn't break any? It's notoriously easy to make "works4me"
It should be possible but
Thanks I understand better now. Very good question, exactly the type of questions I hoped people more familiar with Zephyr could help answer. How do other Zephyr projects solve this build configuration issue? Do they use Zephyr overlays too and if yes how do they make these overlays easy to pass to CMake without very long cmake command lines? |
|
To further simplify and consolidate things I suggest moving the sample from zephyr into the sof tree and change zephyr/module.yaml to point where samples are. This way Zephyr's will be aware of it and CI will build it, see https://github.com/zephyrproject-rtos/mcuboot/blob/main/zephyr/module.yml If you do it this way, you can put the overlays in the sample itself and will not have to tell cmake about them on the command line and put them in boards/ directory or in the CMakeFile.txt itself. |
Move software configuration from Zephyr samples directory to SOF platforms directory using Zephyr overlay mechanism.
Zephyr .conf files in /zephyr/samples/subsys/audio/sof will be deleted after this PR is integrated.
Temporary configuration duplication does not break anything.
This PR only moves configuration from Zephyr without any further refactorings, this will be done later.
Existing IPC4 overlay for TGL is merged with default overlay.conf achieving expected result.
Existing thread-race with PR5503: xtensa-build-all: add a separate target for JSL: added only overlay.conf for JSL (for now).
Signed-of-by: Andrey Borisovich [email protected]