-
Notifications
You must be signed in to change notification settings - Fork 321
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
Add the new platform ACP_7_0 #9351
Conversation
saisurya-ch
commented
Aug 7, 2024
•
edited
Loading
edited
- Add the new platform support for ACP_7_0.
- Add built support for xt-clang compiler.
As already discussed in commit bcbcec7 ("cmake: drop binutils-specific '-Wl,-EL' option") and commit ee58fef ("smex/cmake: move -Wl,EL option to target_linker_options() for clang") and their corresponding reviews on GitHub, this binutils-specific, endianness option makes no difference and is causing clang compatibility issues. It's now getting in the way of thesofproject#9351 "Add the new platform ACP_7_0". I ran `./scripts/docker-run.sh ./scripts/xtensa-build-all.sh -a` with and without it and there was absolutely zero binary difference. I was added in 2019 by giant commit e0ba98d ("cmake: add CMakeLists for firmware build") without any rationale of why it would be needed. Signed-off-by: Marc Herbert <[email protected]>
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.
Some changes requested in build scripts, see below.
As usual I was curious about the copy/paste/diverge ratio so I quickly copied src/platform/amd/acp_7_0/
over src/platform/amd/acp_6_3/
and ran git diff --stat
. The duplication seems to be at least 85%
$ git diff --stat
src/platform/amd/acp_6_3/include/arch/xtensa/config/core-isa.h | 160 +++++++++++++++++++++++++++++++-----------------
src/platform/amd/acp_6_3/include/arch/xtensa/config/core-matmap.h | 282 ++++++++++---------------------------------------------------------------------------
src/platform/amd/acp_6_3/include/arch/xtensa/config/defs.h | 2 +-
src/platform/amd/acp_6_3/include/arch/xtensa/config/specreg.h | 11 ++--
src/platform/amd/acp_6_3/include/arch/xtensa/config/system.h | 6 +-
src/platform/amd/acp_6_3/include/arch/xtensa/config/tie-asm.h | 42 +++++++++----
src/platform/amd/acp_6_3/include/arch/xtensa/config/tie.h | 52 ++++++++--------
src/platform/amd/acp_6_3/include/arch/xtensa/tie/xt_datacache.h | 61 +++++++++----------
src/platform/amd/acp_6_3/include/platform/chip_offset_byte.h | 106 +++++++++++++++-----------------
src/platform/amd/acp_6_3/include/platform/chip_registers.h | 85 +++++++++++++-------------
src/platform/amd/acp_6_3/include/platform/drivers/interrupt.h | 5 +-
src/platform/amd/acp_6_3/include/platform/fw_scratch_mem.h | 7 +--
src/platform/amd/acp_6_3/include/platform/lib/memory.h | 11 ++--
src/platform/amd/acp_6_3/include/platform/platform.h | 5 +-
src/platform/amd/acp_6_3/lib/clk.c | 235 +++++++++++++++++++++++++++++++++++------------------------------------
src/platform/amd/acp_6_3/lib/dai.c | 104 +------------------------------
src/platform/amd/acp_6_3/lib/dma.c | 21 +------
src/platform/amd/acp_6_3/lib/memory.c | 5 +-
src/platform/amd/acp_6_3/platform.c | 5 +-
19 files changed, 459 insertions(+), 746 deletions(-)
wc -l $(find acp_6_3/ -type f | sort)
581 acp_6_3/acp_6_3.x.in
586 acp_6_3/acp_7_0.x.in
5 acp_6_3/CMakeLists.txt
714 acp_6_3/include/arch/xtensa/config/core-isa.h
317 acp_6_3/include/arch/xtensa/config/core-matmap.h
38 acp_6_3/include/arch/xtensa/config/defs.h
108 acp_6_3/include/arch/xtensa/config/specreg.h
262 acp_6_3/include/arch/xtensa/config/system.h
366 acp_6_3/include/arch/xtensa/config/tie-asm.h
211 acp_6_3/include/arch/xtensa/config/tie.h
132 acp_6_3/include/arch/xtensa/tie/xt_datacache.h
230 acp_6_3/include/platform/chip_offset_byte.h
1062 acp_6_3/include/platform/chip_registers.h
109 acp_6_3/include/platform/drivers/interrupt.h
120 acp_6_3/include/platform/fw_scratch_mem.h
185 acp_6_3/include/platform/lib/memory.h
101 acp_6_3/include/platform/platform.h
493 acp_6_3/lib/clk.c
8 acp_6_3/lib/CMakeLists.txt
296 acp_6_3/lib/dai.c
150 acp_6_3/lib/dma.c
95 acp_6_3/lib/memory.c
210 acp_6_3/platform.c
6379 total
scripts/xtensa-build-all.sh
Outdated
SUPPORTED_PLATFORMS+=( acp_6_3 acp_7_0 ) | ||
|
||
# Platforms support xt-clang compiler build | ||
CLANG_BUILD_PLATFORMS=( acp_7_0 ) |
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.
Instead of adding this new variable, define XCC
at the bottom of scripts/set_xtensa_params.sh
. Copy and modify the ZEPHYR_TOOLCHAIN_VARIANT
code which is already there.
# for xtensa-build-all.sh
case "$platform" in
acp_7_0)
XCC='xt-clang';;
*)
XCC='xcc';;
esac
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.
Have an issue If setting XCC
this way: In the build case of acp_7_0 GCC build (where XTENSA_TOOLS_ROOT is empty), XCC
sets to 'xt-clang' and later in file "scripts/xtensa-build-all.sh" at line 228 if [ "$XCC" == "xt-xcc" ] || [ "$XCC" == "xt-clang" ]
gets true and sets COMPILER="clang"
which is wrong. Actually need to set the COMPILER="gcc"
for which the XCC
need to be empty.
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, this required a bit more clean-up than I expected. I did it in #9358. That should work for you, please test and review.
scripts/xtensa-build-all.sh
Outdated
@@ -253,6 +266,7 @@ do | |||
printf 'PATH=%s\n' "$PATH" | |||
( set -x # log the main commands and their parameters | |||
cmake -DTOOLCHAIN="$TOOLCHAIN" \ | |||
-DCOMPILER="$COMPILER" \ |
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.
The COMPILER
name is not specific enough, see above.
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.
made it -DSOF_XT_COMPILER="$COMPILER" \
src/arch/xtensa/CMakeLists.txt
Outdated
> | ||
-imacros${CONFIG_H_PATH} | ||
) | ||
else() |
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.
Don't copy/paste/diverge 10 lines to make a single line difference (which took me way too long to spot).
If this difference was needed (it's not, see below) then you would do something like:
-Wall -Werror
${EL_option}
-Wmissing-prototypes
BUT, even simpler: I'm pretty sure this option is not needed AT ALL. Please try simply removing it and if that works ACP_7_0 then approve small #9353 which I just submitted and which should be merged first. Then rebase on top of 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.
yes, removing -Wl,-EL
works fine.
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.
Please wait until the code is actually changed and pushed before "resolving" conversations, otherwise it looks like this particular change is OK (it's not).
(Some people don't "resolve" conversations even after the code has changed. It's not required by this project and it does not really matter except when there are too many of them.)
scripts/cmake/xtensa-toolchain.cmake
Outdated
@@ -49,7 +49,7 @@ set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) | |||
# xt toolchain only partially follows gcc convention | |||
if(TOOLCHAIN STREQUAL "xt") | |||
set(XCC 1) | |||
set(CMAKE_C_COMPILER ${CROSS_COMPILE}xcc) | |||
set(CMAKE_C_COMPILER ${CROSS_COMPILE}${COMPILER}) |
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.
COMPILER
is an extremely common name, very high risk of conflicts and impossible to git grep
. Use a more specific name, maybe: SOF_XT_COMPILER
scripts/xtensa-build-all.sh
Outdated
XCC="xt-clang" | ||
else | ||
XCC="xt-xcc" | ||
fi |
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.
See above.
lock->lock = 0; | ||
result = 1; |
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.
The code change looks good but the commit message does not:
Remove the unused variable 'result' that is causing the GCC build error.
Which error? Which gcc? This has been successfully compiled by gcc since forever.
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.
[ 5%] Building ASM object src/arch/xtensa/xtos/CMakeFiles/xlevel4.dir/int-vector.S.o
[ 7%] Building ASM object src/arch/xtensa/xtos/CMakeFiles/xlevel4.dir/int-initlevel.S.o
[ 7%] Building ASM object src/arch/xtensa/xtos/CMakeFiles/xlevel3.dir/int-handler.S.o
[ 8%] Building ASM object src/arch/xtensa/hal/CMakeFiles/hal.dir/int_asm.S.o
[ 8%] Building ASM object src/arch/xtensa/hal/CMakeFiles/hal.dir/clock.S.o
[ 8%] Building C object src/arch/xtensa/hal/CMakeFiles/hal.dir/interrupts.c.o
[ 10%] Building ASM object src/arch/xtensa/xtos/CMakeFiles/xlevel5.dir/int-handler.S.o
[ 10%] Building ASM object src/arch/xtensa/xtos/CMakeFiles/xlevel3.dir/int-vector.S.o
[ 11%] No download step for 'smex_ep'
In file included from /home/sof/xtos/include/rtos/spinlock.h:16,
from /home/sof/xtos/include/rtos/sof.h:14,
from /home/sof/src/include/sof/debug/debug.h:16,
from /home/sof/src/init/ext_manifest.c:11:
/home/sof/src/arch/xtensa/include/arch/spinlock.h: In function 'arch_spin_unlock':
/home/sof/src/arch/xtensa/include/arch/spinlock.h:121:11: error: variable 'result' set but not used [-Werror=unused-but-set-variable]
121 | uint32_t result;
| ^~~~~~
[ 11%] No download step for 'rimage_ep'
cc1: all warnings being treated as errors
gmake[3]: *** [src/init/CMakeFiles/ext_manifest.dir/build.make:77: src/init/CMakeFiles/ext_manifest.dir/ext_manifest.c.o] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:2118: src/init/CMakeFiles/ext_manifest.dir/all] Error 2
gmake[2]: *** Waiting for unfinished jobs....
[ 12%] Building ASM object src/arch/xtensa/hal/CMakeFiles/hal.dir/memcopy.S.o
[ 13%] Building ASM object src/arch/xtensa/xtos/CMakeFiles/xtos.dir/core-restore.S.o
[ 14%] No update step for 'smex_ep'
[ 14%] Building ASM object src/arch/xtensa/xtos/CMakeFiles/xtos.dir/core-save.S.o
facing this error when building with GCC, not sure why this is not raised previously. Could that be due to GCC version?
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.
Before getting into what you don't know, simply start by adding to the commit message what you know already: just the error message and the gcc version that prints it. That could be enough.
@@ -433,7 +433,7 @@ static inline unsigned XTHAL_COMPARE_AND_SET( int *addr, int testval, int setva | |||
: "=a"(result) : "0" (setval), "a" (testval), "a" (addr) | |||
: "memory"); | |||
#elif XCHAL_HAVE_INTERRUPTS | |||
int tmp; | |||
int tmp=0; |
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 fix is not ACP70-specific at all so it must be in a separate, standalone commit. The error message should be in the commit message.
#if defined(CONFIG_ACP_7_0) | ||
|
||
#ifndef UINT32_C | ||
#define UINT32_C(x) x |
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.
Please add a comment explaining why this is needed.
Do you really need BOTH #ifdef ACP70
AND #ifndef UINT32_C
?
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.
Yes, Actually ACP_7_0 platform uses the new 2023 xt-clang toolchain for XCC build. That need the defination of macro UINT32_C.
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 realized there is already something similar a few lines above. So we have some toolchain-specific ugliness with four #ifdef
, none of which actually checks any toolchain stuff... This looks duplicated, messy and fragile. @paulstelian97 , @andyross, @dcpleung can you help maybe?
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 realized there is already something similar a few lines above. So we have some toolchain-specific ugliness with four
#ifdef
, none of which actually checks any toolchain stuff... This looks duplicated, messy and fragile. @paulstelian97 , @andyross, @dcpleung can you help maybe?
gentle reminder
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.
IIRC, the old gcc-based xcc needs this. But newer xt-clang should be fine. Are you including stdint.h
in correct place?
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.
You can always do:
#if defined(__XT_CLANG__) #include <xtensa/xtensa-types.h> #endif
to limit it to xt-clang.
Condition #if defined(__XT_CLANG__)
don't work( i.e., goes False) for GCC build. Both XCC and GCC builds require the inclusion of xtensa-types.h
.
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.
Including
xtensa-types.h
works. But need to add this file in the GCC build xtensa overlay tar balls(all old platforms) to avoid build errors. Otherwise need to include under conditional macro(CONFIG_ACP_7_0
).
Here I wanted to convey that inclusion of xtensa-types.h
in core.h file at line 59 as shown below works fine for both XCC and GCC builds. But had a concern of backward compatibility (build flow of existing platforms) i.e., this inclusion don't effect the existing platform's XCC build flow as all existing platform's Xtensa toolchains (have observed that in all existing AMD platforms) contain the header file xtensa-types.h
, but effects the GCC build flow(i.e., cause build errors that xtensa-types.h
is not found) of existing platform's (mostly all) tarball (toolchain) don't have file xtensa-types.h
. Please note that inclusion of this header is required for both XCC and GCC builds of the platform ACP_7_0.
line 57 onwards in file core.h
/* CONFIGURATION INDEPENDENT DEFINITIONS: */
#ifdef __XTENSA__
#include <xtensa/hal.h>
#include <xtensa/xtensa-types.h>
#include <xtensa/xtensa-versions.h>
#else
#include "../hal.h"
#include "../xtensa-versions.h"
#endif
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.
... or maybe just drop the
__ZEPHYR__
guard. It's not clear why @paulstelian97 added it in #7538.In any case this is a complex toolchain compatibility issue that definitely deserves a separate PR.
Created a separate PR for inclusion of xtensa-types.h file which has the macro UINT32_C.
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.
GCC would not have __XT_CLANG__
declared so that should not affect the build... no?
I think we need clarification here. There are XCC (based on old GCC) and XT-CLANG (based on LLVM) compilers, and there is GCC which is not provided by Cadence. XCC build would not have __XT_CLANG__
declared either. Or are you saying that older XT-CLANG toolchain are having problems with including xtensa-types.h
?
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.
Only minor things, please check inline.
@@ -118,10 +118,7 @@ static inline void arch_spin_unlock(struct k_spinlock *lock) | |||
*/ |
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.
proper git summary prefix please
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.
@saisurya-ch Can you update the git commit message to have proper prefix on the first line?
src/lib/alloc.c
Outdated
@@ -92,6 +93,7 @@ static inline uint32_t heap_get_size(struct mm_heap *heap) | |||
|
|||
return size; | |||
} | |||
#endif |
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.
Can you remove L75-96? I assume clang complains that they are not used. It seems these have been dead code for long, not used for anything, so can be removed. Please add a separate commit.
As already discussed in commit bcbcec7 ("cmake: drop binutils-specific '-Wl,-EL' option") and commit ee58fef ("smex/cmake: move -Wl,EL option to target_linker_options() for clang") and their corresponding reviews on GitHub, this binutils-specific, endianness option makes no difference and is causing clang compatibility issues. It's now getting in the way of #9351 "Add the new platform ACP_7_0". I ran `./scripts/docker-run.sh ./scripts/xtensa-build-all.sh -a` with and without it and there was absolutely zero binary difference. I was added in 2019 by giant commit e0ba98d ("cmake: add CMakeLists for firmware build") without any rationale of why it would be needed. Signed-off-by: Marc Herbert <[email protected]>
This is required to allow building XTOS with newer, clang-based, Cadence toolchains as just submitted for ACP_7_0 in thesofproject#9351 Signed-off-by: Marc Herbert <[email protected]>
This is required to allow building XTOS with newer, clang-based, Cadence toolchains as just submitted for ACP_7_0 in thesofproject#9351 Signed-off-by: Marc Herbert <[email protected]>
int_did_val = (uint32_t)(fraction_val * 100.0f); | ||
fraction_val = (float)(int_did_val / 100.0f); | ||
|
||
if (fraction_val == 0.0f) |
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 thought this wouldn't work, but after a brief test it does appear to work. Still, I'm not sure this would work always, maybe better do -1e-10 < x < 1e-10
or similar instead of == 0
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.
Even if some rounding error "breaks" it for some values, similar rounding errors can affect x < 1e-10
too.
0.01 does not exist in float
, it's more like: 0.00999999977648258209228515625
https://numeral-systems.com/ieee-754-converter/
In any case lines 227 and 228 are confusing and seem slower and unnecessary.
src/platform/amd/acp_7_0/lib/clk.c
Outdated
{ | ||
volatile clk7_clk_pll_pwr_req_t clk7_clk_pll_pwr_req; | ||
volatile clk7_clk_fsm_status_t clk7_clk_fsm_status; | ||
int count = 0; |
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.
superfluous init
src/platform/amd/acp_7_0/lib/clk.c
Outdated
{ | ||
volatile clk7_clk_pll_pwr_req_t clk7_clk_pll_pwr_req; | ||
volatile clk7_clk_fsm_status_t clk7_clk_fsm_status; | ||
int count = 0; |
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.
ditto
src/platform/amd/acp_7_0/lib/clk.c
Outdated
|
||
clk7_clk_pll_req_u_t clk7_clk_pll_req; | ||
|
||
clk7_clk_pll_req.u32all = 0; |
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.
clk7_clk_pll_req
is completely overwritten in the next line, no need to set any members in it
This is required to allow building XTOS with newer, clang-based, Cadence toolchains as just submitted for ACP_7_0 in #9351 Signed-off-by: Marc Herbert <[email protected]>
I think the much smaller, non-AMD cleanups can be merged much faster if you submit them in a separate Pull Request https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs |
src/platform/amd/acp_7_0/lib/clk.c
Outdated
|
||
void acp_7_0_reg_wait(void) | ||
{ | ||
int test_count = 0; |
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.
superfluous init
src/platform/amd/acp_7_0/lib/clk.c
Outdated
clk7_spll_field_2_t clk7_spll_field; | ||
uint32_t spinediv = 1; | ||
float fract_part = 0.0f; | ||
float final_refclk = 0.0f; |
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.
final_refclk
is always calculated, no need to initialise
src/platform/amd/acp_7_0/lib/clk.c
Outdated
tr_info(&acp_clk_tr, "acp_change_clock_notify clock_freq : %d clock_type : %d", | ||
clock_freq, clock_type); | ||
|
||
fraction_val = (float)(clock_freq / (float)1000000.0f); |
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.
using f
at the end already makes it a float
constant, don't think the type-cast is needed
src/platform/amd/acp_7_0/lib/clk.c
Outdated
clock_freq, clock_type); | ||
|
||
fraction_val = (float)(clock_freq / (float)1000000.0f); | ||
clock_freq = (clock_freq / 1000000); |
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.
superfluous parentheses and you can use /=
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.
Done
src/platform/amd/acp_7_0/lib/clk.c
Outdated
volatile clk7_clk1_dfs_status_u_t dfs_status; | ||
volatile uint32_t updated_clk = 0; | ||
float did = 0.0f; | ||
float fraction_val = 0.0f; |
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.
superfluous init. Please check them everywhere
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.
Done
src/platform/amd/acp_7_0/lib/clk.c
Outdated
dfs_cntl.bitfields.CLK1_DIVIDER = 0x7F; | ||
bypass_cntl.bitfields.CLK1_BYPASS_DIV = 0xF; | ||
} else { | ||
did = (float)(boot_ref_clk / (float)fraction_val); |
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.
all variables are already of type float
, no need for any type-casts
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.
Done
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.
Last three commits need rewording of the git commit message.
@@ -118,10 +118,7 @@ static inline void arch_spin_unlock(struct k_spinlock *lock) | |||
*/ |
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.
@saisurya-ch Can you update the git commit message to have proper prefix on the first line?
you mean the git commit prefixes such as feature, fix , refactor. right ? |
Component/area prefix (same convention as in Linux and Zephyr). E.g. for src/arc/xtensa, here's example from recent git history:
|
Done |
scripts/set_xtensa_params.sh
Outdated
XTENSA_CORE="ACP_6_3_HiFi5_PROD_Linux" | ||
HOST="xtensa-acp_6_3-elf" | ||
TOOLCHAIN_VER="RI-2021.6-linux" | ||
;; | ||
acp_7_0) | ||
PLATFORM="acp_7_0" |
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.
You can also remove PLATFORM now (and solve the small, new, corresponding git conflict)
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.
Done
Add support for ACP_7_0 in build scripts. Signed-off-by: SaiSurya Ch <[email protected]>
Add defconfig for ACP_7_0 platform. Signed-off-by: SaiSurya Ch <[email protected]>
Add ACP_7_0 platform topology. Signed-off-by: SaiSurya Ch <[email protected]>
Add acp_7_0 toml file to support sof-acp_7_0.ri binary build Signed-off-by: SaiSurya Ch <[email protected]>
Add build support to enable ACP_7_0 platform. Signed-off-by: SaiSurya Ch <[email protected]>
- Add ACP_7_0 platform support Signed-off-by: SaiSurya Ch <[email protected]>
Remove the unused variable 'result' that is causing the build error with GCC 11.4.0. error: variable 'result' set but not used [-Werror=unused-but-set-variable] Signed-off-by: SaiSurya Ch <[email protected]>
src: alloc: Remove unused get memory size functions. Signed-off-by: SaiSurya Ch <[email protected]>
amd: Remove inclusion of unrequired header files. Signed-off-by: SaiSurya Ch <[email protected]>
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.
@saisurya-ch fwiw, Intel has found that adding new platforms has been less effort after moving to Zephyr for SOF RTOS. Zephyr has a ton of platforms and a really refined process for adding new ones.
Thanks Liam, we are in process of supporting Zephyr, soon we may get code up streamed. |