toml: modularise TOML configuration #8490
Conversation
|
works together with zephyrproject-rtos/zephyr#65411 |
src/audio/aria/aria.toml
Outdated
| @@ -0,0 +1,25 @@ | |||
| // # Aria module config | |||
There was a problem hiding this comment.
heh, looks like GH is letting us know toml has a comment style.
There was a problem hiding this comment.
| // # Aria module config | |
| // Aria module config |
Don't give the impression that two "levels" of comments are possible. The C pre-processor either leaves comments in the source or it deletes them; it's not capable (or at least not trivial) to pass comments to the generated .toml file.
There was a problem hiding this comment.
this and the double brackets, looks like this is invalid
There was a problem hiding this comment.
this and the double brackets, looks like this is invalid
@cujomalainey the comment is just removed by the preprocessor. Double brackets are valid, they're used in our present sources
There was a problem hiding this comment.
Ping?
@marc-hb sure, we can remove hashes there, no strong opinion here
There was a problem hiding this comment.
Don't give the impression that two "levels" of comments are possible. The C pre-processor either leaves comments in the source or it deletes them; it's not capable (or at least not trivial) to pass comments to the generated .toml file.
I found a workaround:
Demo in https://github.com/marc-hb/sof/commits/toml-REM/
| 3, 0, 0, 0, 260, 3591000, 64, 85, 0, 0, 0, | ||
| 4, 0, 0, 0, 260, 4477000, 96, 128, 0, 0, 0, | ||
| 5, 0, 0, 0, 260, 7195000, 192, 192, 0, 0, 0] | ||
| index = __COUNTER__ |
| [fw_desc.header] | ||
| name = "ADSPFW" | ||
| load_offset = "0x30000" | ||
|
|
||
| [[module.entry]] | ||
| name = "BRNGUP" | ||
| uuid = "61EB0CB9-34D8-4F59-A21D-04C54C21D3A4" | ||
| affinity_mask = "0x1" | ||
| instance_count = "1" | ||
| domain_types = "0" | ||
| load_type = "0" | ||
| module_type = "0" | ||
| auto_start = "0" | ||
| index = __COUNTER__ | ||
|
|
||
| [[module.entry]] | ||
| name = "BASEFW" | ||
| uuid = "383B9BE2-3518-4DB0-8891-B1470A8914F8" | ||
| affinity_mask = "3" | ||
| instance_count = "1" | ||
| domain_types = "0" | ||
| load_type = "0" | ||
| module_type = "0" | ||
| auto_start = "0" | ||
| index = __COUNTER__ |
There was a problem hiding this comment.
basefw should probably be a generic separate file ?
There was a problem hiding this comment.
yes, I think so too. Any suggestion where to put it?
| [adsp] | ||
| name = "tgl" | ||
| image_size = IMAGE_SIZE | ||
| alias_mask = "0xE0000000" | ||
|
|
||
| [[adsp.mem_zone]] | ||
| type = "ROM" | ||
| base = "0x9F180000" | ||
| size = "0x00002000" | ||
| [[adsp.mem_zone]] | ||
| type = "IMR" | ||
| base = "0xB0000000" | ||
| size = "0x1000000" | ||
| [[adsp.mem_zone]] | ||
| type = "HP-SRAM" | ||
| base = "0xBE000000" | ||
| size = "0x800000" | ||
| [[adsp.mem_zone]] | ||
| type = "LP-SRAM" | ||
| base = "0xBE800000" | ||
| size = "0x40" | ||
|
|
||
| [[adsp.mem_alias]] | ||
| type = "uncached" | ||
| base = "0x9E000000" | ||
| [[adsp.mem_alias]] | ||
| type = "cached" | ||
| base = "0xBE000000" | ||
|
|
||
| [cse] | ||
| partition_name = "ADSP" | ||
| [[cse.entry]] | ||
| name = "ADSP.man" | ||
| offset = "0x5c" | ||
| length = "0x464" | ||
| [[cse.entry]] | ||
| name = "cavs0015.met" | ||
| offset = "0x4c0" | ||
| length = "0x70" | ||
| [[cse.entry]] | ||
| name = "cavs0015" | ||
| offset = "0x540" | ||
| length = "0x0" // # calculated by rimage | ||
|
|
||
| [css] | ||
|
|
||
| [signed_pkg] | ||
| name = "ADSP" | ||
| [[signed_pkg.module]] | ||
| name = "cavs0015.met" |
There was a problem hiding this comment.
This manifest part is Intel signing specific so could be vendor/intel/manifest ?
There was a problem hiding this comment.
under rimage or in sof root? We need to come up with good locations for these parts, yes
There was a problem hiding this comment.
e.g. rimage/configs/vendor/intel/
| @@ -1939,6 +1939,9 @@ static int parse_module(const toml_table_t *toml, struct parse_ctx *pctx, | |||
| if (ret < 0) | |||
| return err_key_parse("auto_start", NULL); | |||
|
|
|||
| parse_uint32_key(mod_entry, &ctx_entry, "index", 1, &ret); | |||
There was a problem hiding this comment.
nno, I think it fails without this - it says unhandled key in toml. Maybe I can add it before TOML changes - the error is ignored anyway, maybe that would work, but in principle they're closely related
There was a problem hiding this comment.
ok, best to inline comment this in rimage then.
There was a problem hiding this comment.
@lyakh can you explain the issue this solves?
Does this imply a "Flag Day" where all .toml files have to become pre-processed?
How come rimage notices the pre-processing at all? It should never know.
EDIT: I see it seems related to the __COUNTER__
There was a problem hiding this comment.
@marc-hb yes, it is related to the __COUNTER__ and that one is related to
[module]
count = N
|
Thank you, Guennadi, this is part of our OKR for modules, you did for all of us , @andrula-song @singalsu |
lyakh
requested review from
a team,
tlissows,
singalsu,
RanderWang,
abonislawski,
pblaszko,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
|
LGTM |
Ack - this turned out to be faster than expected :) |
|
This can't work because https://sof-ci.01.org/sofpr/PR8490/build15027/build/tgl_zph_error.txt I think @lyakh once told me he does not use The internal Zephyr CI has yet other ways to invoke rimage. I once had to implement zephyrproject-rtos/zephyr@d98a7c2f8ddb for that.
The too many ways the .toml files can be passed unfortunately make this a complicated problem to solve. Discussion just started in zephyrproject-rtos/zephyr#65411 |
This comment was marked as off-topic.
This comment was marked as off-topic.
This fault is not related to the new build service, this PR test was run with the old CI flow. But new build service can handle this scarious well. Let's trigger the CI test again with the new CI flow. |
|
SOFCI TEST |
|
@marc-hb , the new process is that if the build fails, on-device testing will not be conducted. |
Rename: - tgl-cavs.toml to tgl.toml - tgl-h-cavs.toml to tgl-h.toml Remove the IPC3/IPC4 switch added by commit 6f71808 ("xtensa-build-zephyr.py: add ipc4 build support for tgl") This brings back consistency which is required for the .toml split (thesofproject#8490) Signed-off-by: Marc Herbert <[email protected]>
Let west/sign.py find the appropriate .toml file. This prepares for .toml modularization thesofproject#8490 Signed-off-by: Marc Herbert <[email protected]>
|
@lyakh , do you plan to merge this in 2023? with:zephyrproject-rtos/zephyr@85f9710 |
|
lyakh
force-pushed
the
toml-wip
branch
5 times, most recently
from
228ff07
to
3ed732b
Compare
Split TOML configuration files into platform and module parts. Use the C preprocessor to merge them back together. Signed-off-by: Guennadi Liakhovetski <[email protected]>
|
Looks ready to go, need a clean run of the mandatory CIs. |
|
@lgirdwood good to merge |
|
For a change, suspend/resume test Does not seem related but happened in this PR so archiving the dump here. |
marc-hb
added
the
suspend-resume
Rename: - tgl-cavs.toml to tgl.toml - tgl-h-cavs.toml to tgl-h.toml Remove the IPC3/IPC4 switch added by commit 6f71808 ("xtensa-build-zephyr.py: add ipc4 build support for tgl") This brings back consistency which is required for the .toml split (#8490) Signed-off-by: Marc Herbert <[email protected]>
Let west/sign.py find the appropriate .toml file. This prepares for .toml modularization #8490 Signed-off-by: Marc Herbert <[email protected]>
Rename: - tgl-cavs.toml to tgl.toml - tgl-h-cavs.toml to tgl-h.toml Remove the IPC3/IPC4 switch added by commit 6f71808 ("xtensa-build-zephyr.py: add ipc4 build support for tgl") This brings back consistency which is required for the .toml split (thesofproject#8490) Signed-off-by: Marc Herbert <[email protected]> (cherry picked from commit ded019b)
Let west/sign.py find the appropriate .toml file. This prepares for .toml modularization thesofproject#8490 Signed-off-by: Marc Herbert <[email protected]> (cherry picked from commit d661aad)
Rename: - tgl-cavs.toml to tgl.toml - tgl-h-cavs.toml to tgl-h.toml Remove the IPC3/IPC4 switch added by commit 6f71808 ("xtensa-build-zephyr.py: add ipc4 build support for tgl") This brings back consistency which is required for the .toml split (#8490) Signed-off-by: Marc Herbert <[email protected]> (cherry picked from commit ded019b)
Let west/sign.py find the appropriate .toml file. This prepares for .toml modularization #8490 Signed-off-by: Marc Herbert <[email protected]> (cherry picked from commit d661aad)
Rename: - tgl-cavs.toml to tgl.toml - tgl-h-cavs.toml to tgl-h.toml Remove the IPC3/IPC4 switch added by commit 6f71808 ("xtensa-build-zephyr.py: add ipc4 build support for tgl") This brings back consistency which is required for the .toml split (thesofproject#8490) Signed-off-by: Marc Herbert <[email protected]> (cherry picked from commit ded019b)
Let west/sign.py find the appropriate .toml file. This prepares for .toml modularization thesofproject#8490 Signed-off-by: Marc Herbert <[email protected]> (cherry picked from commit d661aad)
Split TOML configuration files into platform and module parts. Use the C preprocessor to merge them back together.