Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jxstelter
Copy link

This PR adds support for the Intel Lunarlake and ACE2.0
SoC derived Audio DSP platforms.

Signed-off-by: Jaroslaw Stelter Jaroslaw.Stelter@intel.com

This PR adds support for the Intel Lunarlake and ACE2.0
 SoC derived Audio DSP platforms.

 Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>
@jxstelter jxstelter marked this pull request as ready for review May 9, 2023 12:47
@marc-hb
Copy link

marc-hb commented May 9, 2023

@dbaluta
Copy link
Contributor

dbaluta commented May 9, 2023

FYI: @iuliana-prodan

@aborisovich
Copy link

@jxstelter consider adding reviewers to PR and the one with write access too

@marc-hb
Copy link

marc-hb commented May 30, 2023

Wait, I remember I had to cherry-pick this in my workspace compile SOF LNL with the Zephyr SDK:

Otherwise: fatal error: xtensa/config/core-isa.h: No such file or directory

Yet we're now compiling SOF routinely without this? Weird...

Copy link
Contributor

@andyross andyross left a 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.

Copy link

@marc-hb marc-hb left a 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!

Copy link

@marc-hb marc-hb left a 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

@marc-hb
Copy link

marc-hb commented May 30, 2023

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 intel_ace15_mtpm confusion. Whatever the correct solution is, we never want to merge something that is never even compiled!

cc: @dcpleung

@andyross
Copy link
Contributor

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.

@marc-hb
Copy link

marc-hb commented May 31, 2023

Regardless, this specific PR looks to me like it should merge. It's just upstream Cadence headers in a standard location.

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. core-isa.h not found is a very simple error message very quick to understand and search. Plus it's not like anyone is going to actually review these .h files. EDIT: @andyross did below.

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:

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

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?

@dcpleung
Copy link
Member

The XCHAL_CORE_ID mentions MTL. Is this a LNL specfic overlay? Or just a copy of MTL? If this is just a copy, we can re-use the MTL toolchain for LNL, and skip this overlay.

BTW, CONFIG_SOC_TOOLCHAIN_NAME will need to be split among SoC Kconfig when we introduce a new toolchain for next version of ACE.

@jxstelter
Copy link
Author

jxstelter commented May 31, 2023

The XCHAL_CORE_ID mentions MTL. Is this a LNL specfic overlay? Or just a copy of MTL? If this is just a copy, we can re-use the MTL toolchain for LNL, and skip this overlay.

BTW, CONFIG_SOC_TOOLCHAIN_NAME will need to be split among SoC Kconfig when we introduce a new toolchain for next version of ACE.

MTL and LNL uses the same core and toolchain. But this could change for next version of ACE.

@andyross
Copy link
Contributor

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?

@marc-hb
Copy link

marc-hb commented May 31, 2023

Is this a LNL specfic overlay? Or just a copy of MTL?

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

@andyross
Copy link
Contributor

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 gcc -E -dM and sorting/diffing the output, I get the deltas below.

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.

--- ace15.h	2023-05-31 09:18:00.443997588 -0700
+++ ace20.h	2023-05-31 09:18:04.817430619 -0700
-#define PRID_ID_BITS 4
-#define PRID_ID_MASK 0x0000000F
-#define PRID_ID_SHIFT 0
-#define XCHAL_BUILD_UNIQUE_ID 0x000A315C
+#define XCHAL_BUILD_UNIQUE_ID 0x000863B0
-#define XCHAL_CORE_ID "ace10_LX7HiFi4_2022_10"
+#define XCHAL_CORE_ID "mtl_LX7HiFi4"
-#define XCHAL_DCACHE_LINES_PER_TAG_LOG2 0
-#define XCHAL_DCACHE_SIZE_LOG2 15
-#define XCHAL_DCACHE_WAYS_LOG2 1
-#define XCHAL_EXCCAUSE_NUM 64
-#define XCHAL_FUSIONJ_SIMD32 0
-#define XCHAL_HAVE_APB 0
-#define XCHAL_HAVE_BALL 0
-#define XCHAL_HAVE_BALLAP 0
-#define XCHAL_HAVE_BRANCH_PREDICTION 0
-#define XCHAL_HAVE_CME_DOWNGRADES 0
-#define XCHAL_HAVE_CONNX_B10 0
-#define XCHAL_HAVE_CONNX_B20 0
-#define XCHAL_HAVE_CONNX_B_32B_MAC 0
-#define XCHAL_HAVE_CONNX_B_DP_VFPU 0
-#define XCHAL_HAVE_CONNX_B_DPX_VFPU 0
-#define XCHAL_HAVE_CONNX_B_HP_VFPU 0
-#define XCHAL_HAVE_CONNX_B_HPX_VFPU 0
-#define XCHAL_HAVE_CONNX_B_LDPC 0
-#define XCHAL_HAVE_CONNX_B_SP_VFPU 0
-#define XCHAL_HAVE_CONNX_B_SPX_VFPU 0
-#define XCHAL_HAVE_CONNX_B_TURBO 0
-#define XCHAL_HAVE_CONNX_B_VITERBI 0
-#define XCHAL_HAVE_CSR_PARITY 0
-#define XCHAL_HAVE_DCACHE_DYN_ENABLE 1
-#define XCHAL_HAVE_DRAMCFG 0
-#define XCHAL_HAVE_EXT_CA 0
-#define XCHAL_HAVE_FUNC_SAFETY 0
-#define XCHAL_HAVE_FUSIONJ 0
-#define XCHAL_HAVE_FUSIONJ6 0
-#define XCHAL_HAVE_FUSIONJ_DP_VFPU 0
-#define XCHAL_HAVE_FUSIONJ_SP_VFPU 0
-#define XCHAL_HAVE_FXLK 0
-#define XCHAL_HAVE_HIFI1 0
-#define XCHAL_HAVE_HIFI1_LOW_LATENCY_MAC_FMA 0
-#define XCHAL_HAVE_HIFI1_VFPU 0
-#define XCHAL_HAVE_ICACHE_DYN_ENABLE 1
+#define XCHAL_HAVE_IDMA_TRANSPOSE 0
-#define XCHAL_HAVE_IMPRECISE_EXCEPTIONS 0
-#define XCHAL_HAVE_IRAMCFG 0
-#define XCHAL_HAVE_ISB 0
-#define XCHAL_HAVE_ISL 0
-#define XCHAL_HAVE_ITB 0
-#define XCHAL_HAVE_KSL 0
-#define XCHAL_HAVE_L2 0
-#define XCHAL_HAVE_L2_CACHE 0
-#define XCHAL_HAVE_LX 1
-#define XCHAL_HAVE_NX 0
-#define XCHAL_HAVE_PDXNX 0
-#define XCHAL_HAVE_PSL 0
-#define XCHAL_HAVE_RNX 0
-#define XCHAL_HAVE_SECURE 0
-#define XCHAL_HAVE_SUPERGATHER 0
-#define XCHAL_HAVE_VISION_DP_VFPU 0
-#define XCHAL_HAVE_VISION_HP_VFPU_2XFMAC 0
-#define XCHAL_HAVE_VISION_SP_VFPU_2XFMAC 0
-#define XCHAL_HAVE_WWDT 0
-#define XCHAL_HAVE_XEA3 0
-#define XCHAL_HAVE_XEA5 0
+#define XCHAL_HAVE_XEAX 0
-#define XCHAL_HAVE_XNNE 0
-#define XCHAL_HW_CONFIGID0 0xC203B286
-#define XCHAL_HW_CONFIGID1 0x29C95BA2
+#define XCHAL_HW_CONFIGID0 0xC2B3FBFE
+#define XCHAL_HW_CONFIGID1 0x230863B0
-#define XCHAL_HW_MAX_VERSION 281070
-#define XCHAL_HW_MAX_VERSION_MAJOR 2810
-#define XCHAL_HW_MAX_VERSION_MICRO 0
-#define XCHAL_HW_MAX_VERSION_MINOR 7
-#define XCHAL_HW_MIN_VERSION 281070
-#define XCHAL_HW_MIN_VERSION_MAJOR 2810
-#define XCHAL_HW_MIN_VERSION_MICRO 0
-#define XCHAL_HW_MIN_VERSION_MINOR 7
+#define XCHAL_HW_MAX_VERSION 270012
+#define XCHAL_HW_MAX_VERSION_MAJOR 2700
+#define XCHAL_HW_MAX_VERSION_MINOR 12
+#define XCHAL_HW_MIN_VERSION 270012
+#define XCHAL_HW_MIN_VERSION_MAJOR 2700
+#define XCHAL_HW_MIN_VERSION_MINOR 12
+#define XCHAL_HW_REL_LX7_0 1
+#define XCHAL_HW_REL_LX7_0_12 1
-#define XCHAL_HW_REL_LX7_1 1
-#define XCHAL_HW_REL_LX7_1_7 1
-#define XCHAL_HW_VERSION 281070
-#define XCHAL_HW_VERSION_MAJOR 2810
-#define XCHAL_HW_VERSION_MICRO 0
-#define XCHAL_HW_VERSION_MINOR 7
-#define XCHAL_HW_VERSION_NAME "LX7.1.7"
+#define XCHAL_HW_VERSION 270012
+#define XCHAL_HW_VERSION_MAJOR 2700
+#define XCHAL_HW_VERSION_MINOR 12
+#define XCHAL_HW_VERSION_NAME "LX7.0.12"
-#define XCHAL_ICACHE_SIZE_LOG2 14
-#define XCHAL_ICACHE_WAYS_LOG2 1
-#define XCHAL_INTTYPE_MASK_BREAKIN 0x00000000
-#define XCHAL_INTTYPE_MASK_COR_ECC_ERR 0x00000000
-#define XCHAL_INTTYPE_MASK_DBG_REQUEST 0x00000000
-#define XCHAL_INTTYPE_MASK_ETIE 0x00000000
-#define XCHAL_INTTYPE_MASK_FXLK 0x00000000
-#define XCHAL_INTTYPE_MASK_L2_ERR 0x00000000
-#define XCHAL_INTTYPE_MASK_L2_STATUS 0x00000000
-#define XCHAL_INTTYPE_MASK_TRAX 0x00000000
-#define XCHAL_INTTYPE_MASK_WWDT 0x00000000
-#define XCHAL_ISB_VADDR 0
-#define XCHAL_ITB_VADDR 0
-#define XCHAL_L1SCACHE_ACCESS_SIZE 0
-#define XCHAL_L1SCACHE_BANKS 1
-#define XCHAL_L1SCACHE_SIZE 0
-#define XCHAL_L1SCACHE_SIZE_LOG2 0
-#define XCHAL_L1SCACHE_WAYS 1
-#define XCHAL_L1SCACHE_WAYS_LOG2 0
-#define XCHAL_L1VCACHE_SIZE 0
-#define XCHAL_MPU_LOCK 0
-#define XCHAL_NUM_CORES_IN_CLUSTER 0
-#define XCHAL_RESET_VECTOR_PADDR XCHAL_RESET_VECTOR0_PADDR
-#define XCHAL_RESET_VECTOR_VADDR XCHAL_RESET_VECTOR0_VADDR
+#define XCHAL_RESET_VECTOR_PADDR 0x1FF80000
+#define XCHAL_RESET_VECTOR_VADDR 0x1FF80000
-#define XCHAL_SW_MICRO_VERSION 1410000
-#define XCHAL_SW_MINOR_VERSION 1410000
-#define XCHAL_SW_VERSION 1410000
-#define XCHAL_SW_VERSION_MAJOR 14000
-#define XCHAL_SW_VERSION_MICRO 0
-#define XCHAL_SW_VERSION_MINOR 10
+#define XCHAL_SW_VERSION 1200012
-#define XCHAL_UNIFIED_LOADSTORE 0
-#define XTENSA_CORE_CONFIGURATION_H_ 
+#define _XTENSA_CORE_CONFIGURATION_H 

Copy link
Contributor

@andyross andyross left a 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)

@andyross
Copy link
Contributor

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? :)

@dcpleung
Copy link
Member

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.

@plbossart
Copy link

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

@dcpleung
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants