-
Notifications
You must be signed in to change notification settings - Fork 20
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: add SoC overlay for intel_ace20_lnl #19
zephyr: add SoC overlay for intel_ace20_lnl #19
Conversation
This PR adds support for the Intel Lunarlake and ACE2.0 SoC derived Audio DSP platforms. Signed-off-by: Jaroslaw Stelter <[email protected]>
cc: @lrgirdwo , @kv2019i , @teburd , @andyross , @plbossart, @dbaluta, @aborisovich , @abonislawski, ... |
FYI: @iuliana-prodan |
@jxstelter consider adding reviewers to PR and the one with write access too |
Wait, I remember I had to cherry-pick this in my workspace compile SOF LNL with the Zephyr SDK: Otherwise: Yet we're now compiling SOF routinely without this? Weird... |
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.
Was going to nitpick that this didn't match the Zephyr-side board name ("intel_adsp_ace20_lnl"), but it does match the SOC name, so if there's an inconsistency to whine about we've already made the mistake.
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.
Mystery solved, sort of.
This PR used to fix a fatal error: xtensa/config/core-isa.h: No such file or directory
error when compiling with zephyr-sdk-0.15.2/xtensa-intel_s1000_zephyr-elf
But now that Zephyr has moved to zephyr-sdk-0.16.1
, this xtensa PR is not required to compile LNL any more. Why? Because LNL now uses zephyr-sdk-0.16.1/xtensa-intel_ace15_mtpm_zephyr-elf/
and modules/hal/xtensa/zephyr/soc/intel_ace15_mtpm/xtensa/config/core-isa.h
and a lot of (all?) other intel_ace15_mtpm
files. This xtensa PR is now totally ignored by SOF+LNL compilation.
This feels... wrong? Blocking this PR until we get to the bottom of this. Whatever the correct solution is, we never want to merge something that is never even compiled!
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.
just clicked the wrong option
So it looks like I don't have the power to "request" changes in this repo, only to "suggest" changes (even though Github makes me believe the former, never mind). Please don't merge this PR until we get to the bottom of this cc: @dcpleung |
Looks like this comes out of CONFIG_SOC_TOOLCHAIN_NAME, which has a global default in soc/xtensa/intel_adsp/ace/Kconfig.defconfig.series of "intel_ace15_mtpm". That seems dangerous to me, we shouldn't default something like this at all, or if we do it should be to something like "this_is_not_a_hal_soc" or whatever. Each SOC should set it affirmatively. Regardless, this specific PR looks to me like it should merge. It's just upstream Cadence headers in a standard location. The bug is in Zephyr. |
I think having DoA code in the main branch that is never compiled by any CI or anyone could do more harm than having no code at all. For instance, someone could very much waste a fair amount of time staring or even fiddling with these headers before realizing they're not used at all. This being said, happy for this to be merged once at least one person has this solution (or any even better one) successfully tested in their workspace and the corresponding PR shared as at least a draft:
If there isn't at least one vaguely working prototype capable of using these headers on at least one person's workstation then what's the point of having them in the main branch? |
The BTW, |
MTL and LNL uses the same core and toolchain. But this could change for next version of ACE. |
Hm... this has a different XCHAL_BUILD_UNIQUE_ID than the existing ace15 files (0x863b0 vs. 0xa315c). Are they the same or not? If Cadence generated separate files for these parts, we probably want to use the files as shipped. Or if not we should carefully diff the results and document exactly what we're doing somewhere, no? |
Mostly copy/paste/diverge but not 100% My dirt cheap, magical copy/paste/diverge measurement tool: cd modules/hal/xtensa
git fetch upstream pull/19/head
git cherry-pick FETCH_HEAD
rsync -a zephyr/soc/intel_ace20_lnl/ zephyr/soc/intel_ace15_mtpm/
git diff --stat
zephyr/soc/intel_ace15_mtpm/xtensa/config/core-isa.h | 226 +++++++++++++++-------------------------------------------------------
zephyr/soc/intel_ace15_mtpm/xtensa/config/core-matmap.h | 3 +-
zephyr/soc/intel_ace15_mtpm/xtensa/config/specreg.h | 5 +-
zephyr/soc/intel_ace15_mtpm/xtensa/config/system.h | 35 +++++++----
zephyr/soc/intel_ace15_mtpm/xtensa/config/tie-asm.h | 266 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
zephyr/soc/intel_ace15_mtpm/xtensa/config/tie.h | 12 ++--
6 files changed, 241 insertions(+), 306 deletions(-)
wc -l zephyr/soc/intel_ace20_lnl/xtensa/config/*
586 zephyr/soc/intel_ace20_lnl/xtensa/config/core-isa.h
317 zephyr/soc/intel_ace20_lnl/xtensa/config/core-matmap.h
99 zephyr/soc/intel_ace20_lnl/xtensa/config/specreg.h
273 zephyr/soc/intel_ace20_lnl/xtensa/config/system.h
376 zephyr/soc/intel_ace20_lnl/xtensa/config/tie-asm.h
188 zephyr/soc/intel_ace20_lnl/xtensa/config/tie.h
1839 total
|
FWIW, essentially all the software-visible definitions (and all the ones Zephyr cares about[1]) are in core-isa.h, which is 100% simple macro definitions with no code content. Running them through It looks like this is the same set of tunables run through two different versions of the Xtensa core generator tooling. I don't see anything incompatible, though there are more deltas than I'd like. So... I guess I'd agree that these represent compatible hardware and we should be using the same files. Which also means we probably don't want these here in the Zephyr HAL as they won't be used. I'm going to remove my +1. These files should remain with the Xtensa toolchain distributables, which will need and provide them. Zephyr builds using the SDK don't. [1] The other stuff has to do with code generation, and nothing in Zephyr or the HAL is a compiler. I won't say literally nothing in the HAL code cares about the other headers, but our use of the HAL is at this point vestigial anyway and I'd view any such divergence as something we shouldn't be using anyway.
|
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.
Switch vote, unless we decide we want to use these after all (in which case feel free to override)
Also, in case someone thinks there's a mistake: the intel_ace15_mtl file does indeed identify itself in XCHAL_CORE_ID as "ace10" (wrong version!), and the intel_ace20_lnl file as "mtl" (wrong hardware name!). Can someone please ask the hardware design people to be more rigorous about naming these things? :) |
If MTL and LNL are the same core, maybe we can skip the LNL overlay and just reuse the MTL one? We can then use the same toolchain on Zephyr SDK without the need to add another one. |
MTL has 3 cores and LNL 5 |
But the overlay itself does not specify how many CPU cores there are. Let me re-phrase, if individual CPU core of MTL and LNL are the same, we do not need another overlay. |
Closing this as this is not needed. |
This PR adds support for the Intel Lunarlake and ACE2.0
SoC derived Audio DSP platforms.
Signed-off-by: Jaroslaw Stelter [email protected]