-
Notifications
You must be signed in to change notification settings - Fork 349
Intel: remove XTOS support #7527
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
Conversation
paulstelian97
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.
Blocking until we can test that nothing breaks on non-Intel XTOS builds (it is not obvious that the changes are Intel-only)
kv2019i
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 think the github flows need to be updated here as well, or otherwise there will be many CI failures
@kv2019i yes, apparently PR CI is still attempting TGL XTOS builds, @lgirdwood @marc-hb do we need them? If yes, then obviously this PR cannot be merged |
src/arch/xtensa/CMakeLists.txt
Outdated
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 don't see the point of removing this part of the CMake code, it has absolutely nothing platform specific and we just don't know how companies outside Intel use rimage. I believe the point of this PR is to block everyone from building TGL+XTOS in this branch. This is already a large and significant change, please don't top it up with risky, unnecessary and platform-independent CMake code cleanups. Even less so considering the entire project has 0.6 CMake maintainers total, myself included.
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.
@marc-hb nobody else uses the bootloader - check CONFIG_XT_BOOT_LOADER
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.
nobody else uses the bootloader - check CONFIG_XT_BOOT_LOADER
Sure but what does removing this particular CMake code achieves? When the Linux kernel code removes support for old hardware it's neither for the purpose of saving disk space nor for aesthetics nor for "ideology". It's because old code gets in the way of updating APIs and testing changes. In other words Linux kernel code is removed when it solves an actual, specific maintenance problem. I don't see what specific problem is being solved by removing this CMake code here right now. If it helps in the future then why not; _then+ we can spend time making sure nothing got lost in the translation to python/zephyr or whatever.
I really want to minimize CMake churn as much as possible and considering I'm the only one (barely) maintaining sof/cmake code I think I deserve this. As a coincidence I've just performed some CMake archeology for #7546 and deleted CMake code there would have make that archeology even more painful that it already was. git blaming deleted code is really, really painful - especially when you don't even know it existed.
By the way we have many more, major rimage-related changes in flight right now (including one of yours)
Feel free to a "this is dead code now since..." comment, that would be useful.
Quoting from #7552 that I just submitted, #7552 and 34d7baa similarly revert some massive search/replace that backfired:
If we want to stop supporting something then let's keep things simple and remove it only where it actually gets in the way and where it actually becomes a problem. No need to search and scrub every reference to it from the face of the planet and accidentally make our life harder on release branches, create pointless git conflicts or increase maintenance time in some other way when trying to reduce it!
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.
@marc-hb sorry, TBH I don't understand your objection here. We are removing the only use case for this block of code. If we keep it we'll keep stumbling over it trying to identify what it's there for, until someone removes it. Why - simple - to reduce maintenance costs. I might miss some further dependencies, breakage possibilities or whatever. But here it seems pretty clear to me - that if branch will never be entered. So it can be removed.
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.
Why - simple - to reduce maintenance costs.
I'm the only one doing CMake maintenance right now and I gave you examples. I'm sorry you don't understand them.
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.
If we keep it we'll keep stumbling over it trying to identify what it's there for, until someone removes it. Why - simple - to reduce maintenance costs.
"we" has been only myself for the last few years since Janusz left. So please leave the estimation of maintenance costs to me and believe me when I say that I don't make my work more difficult just for fun.
installer/GNUmakefile
Outdated
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.
This line should probably be dropped.
installer/GNUmakefile
Outdated
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.
Could you try adding one AMD product?
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 think the github flows need to be updated here as well, or otherwise there will be many CI failures
@kv2019i yes, apparently PR CI is still attempting TGL XTOS builds, @lgirdwood @marc-hb do we need them? If yes, then obviously this PR cannot be merged
yep - lets make sure we update all flows. CI should not be doing any xtos builds for Intel platforms as Intel only uses Zephyr now..
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.
Could you try adding one AMD product?
@marc-hb can we make it a follow-up? I'd be happy to get this right with a minimum changes
.github/workflows/installer.yml
Outdated
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.
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.
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.
Right now i.MX8 IPC4 support is a discussion topic. I am not committing to doing it soon, BUT it is in my backlog, I am just lacking some information related to the firmware side of this conversion (for the kernel I have some patches ready, minus exact filenames)
I do expect Zephyr support to be done before the IPC4 migration.
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 do expect Zephyr support to be done before the IPC4 migration.
Thanks, this is the information relevant to this line. It means this Github Action will never need to build IPC4+XTOS ever again so this line can be removed as @lyakh currently does.
|
https://github.com/thesofproject/sof/actions/runs/4831781226/jobs/8609674416?pr=7527 just needs some adjustments to the expected output |
8e5445f to
da70d51
Compare
|
SOFCI TEST |
XTOS isn't supported any more for Intel platforms, remove support for it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
CI: two suspend-resume failures in https://sof-ci.01.org/sofpr/PR7527/build6996/devicetest/index.html - unrelated |
kv2019i
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.
Thanks @lyakh , passing CI now.
|
Thanks @lyakh , merging! |
XTOS isn't supported any more for Intel platforms, remove support for it.
If anybody knows what additional code is Intel-XTOS-specific and can be removed, please let me know.