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

Add the new platform ACP_7_0 #9351

Merged
merged 9 commits into from
Sep 11, 2024
Merged

Add the new platform ACP_7_0 #9351

merged 9 commits into from
Sep 11, 2024

Conversation

saisurya-ch
Copy link
Member

@saisurya-ch saisurya-ch commented Aug 7, 2024

  • Add the new platform support for ACP_7_0.
  • Add built support for xt-clang compiler.

marc-hb added a commit to marc-hb/sof that referenced this pull request Aug 7, 2024
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 <marc.herbert@intel.com>
marc-hb
marc-hb previously requested changes Aug 7, 2024
Copy link
Collaborator

@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.

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

SUPPORTED_PLATFORMS+=( acp_6_3 acp_7_0 )

# Platforms support xt-clang compiler build
CLANG_BUILD_PLATFORMS=( acp_7_0 )
Copy link
Collaborator

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

Copy link
Member Author

@saisurya-ch saisurya-ch Aug 9, 2024

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.

Copy link
Collaborator

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.

@@ -253,6 +266,7 @@ do
printf 'PATH=%s\n' "$PATH"
( set -x # log the main commands and their parameters
cmake -DTOOLCHAIN="$TOOLCHAIN" \
-DCOMPILER="$COMPILER" \
Copy link
Collaborator

@marc-hb marc-hb Aug 7, 2024

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.

Copy link
Member Author

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" \

>
-imacros${CONFIG_H_PATH}
)
else()
Copy link
Collaborator

@marc-hb marc-hb Aug 7, 2024

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.

Copy link
Member Author

@saisurya-ch saisurya-ch Aug 9, 2024

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.

Copy link
Collaborator

@marc-hb marc-hb Aug 9, 2024

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.)

@@ -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})
Copy link
Collaborator

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

XCC="xt-clang"
else
XCC="xt-xcc"
fi
Copy link
Collaborator

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;
Copy link
Collaborator

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.

Copy link
Member Author

@saisurya-ch saisurya-ch Aug 9, 2024

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?

Copy link
Collaborator

@marc-hb marc-hb Aug 9, 2024

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;
Copy link
Collaborator

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
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Collaborator

@marc-hb marc-hb Aug 19, 2024

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?

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

@saisurya-ch saisurya-ch Aug 28, 2024

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 .

Copy link
Member Author

@saisurya-ch saisurya-ch Aug 28, 2024

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

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dcpleung , can you please help transfer that question to newer, much smaller and focused #9413? It's hard enough to get attention to tricky issues like this, but it's much harder buried deep down this big, platform-specific PR named "ACP 7.0"

Copy link
Collaborator

@kv2019i kv2019i left a 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)
*/
Copy link
Collaborator

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

Copy link
Collaborator

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
Copy link
Collaborator

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.

kv2019i pushed a commit that referenced this pull request Aug 9, 2024
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 <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this pull request Aug 9, 2024
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 <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this pull request Aug 10, 2024
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 <marc.herbert@intel.com>
int_did_val = (uint32_t)(fraction_val * 100.0f);
fraction_val = (float)(int_did_val / 100.0f);

if (fraction_val == 0.0f)
Copy link
Collaborator

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

Copy link
Collaborator

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.

{
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

superfluous init

{
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


clk7_clk_pll_req_u_t clk7_clk_pll_req;

clk7_clk_pll_req.u32all = 0;
Copy link
Collaborator

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

kv2019i pushed a commit that referenced this pull request Aug 14, 2024
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 <marc.herbert@intel.com>
@saisurya-ch saisurya-ch reopened this Aug 14, 2024
@marc-hb marc-hb dismissed their stale review August 14, 2024 16:57

stale review

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 14, 2024

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 Show resolved Hide resolved

void acp_7_0_reg_wait(void)
{
int test_count = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

superfluous init

clk7_spll_field_2_t clk7_spll_field;
uint32_t spinediv = 1;
float fract_part = 0.0f;
float final_refclk = 0.0f;
Copy link
Collaborator

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

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);
Copy link
Collaborator

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

clock_freq, clock_type);

fraction_val = (float)(clock_freq / (float)1000000.0f);
clock_freq = (clock_freq / 1000000);
Copy link
Collaborator

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 /=

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

volatile clk7_clk1_dfs_status_u_t dfs_status;
volatile uint32_t updated_clk = 0;
float did = 0.0f;
float fraction_val = 0.0f;
Copy link
Collaborator

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

dfs_cntl.bitfields.CLK1_DIVIDER = 0x7F;
bypass_cntl.bitfields.CLK1_BYPASS_DIV = 0xF;
} else {
did = (float)(boot_ref_clk / (float)fraction_val);
Copy link
Collaborator

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

@kv2019i kv2019i left a 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)
*/
Copy link
Collaborator

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?

@saisurya-ch
Copy link
Member Author

saisurya-ch commented Aug 28, 2024

Last three commits need rewording of the git commit message.

you mean the git commit prefixes such as feature, fix , refactor. right ?

@kv2019i
Copy link
Collaborator

kv2019i commented Aug 28, 2024

Last three commits need rewording of the git commit message.

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:

work/sof$ git log --pretty=oneline src/arch/xtensa/ |head -20
ee5efdf5b9ef8996cebd2b95d159edef77c91656 src/arch/xtensa/CMakeLists.txt: drop bogus, non-portable -Wl,-EL
a2338e51035dfeb5eea520391486fa6d4aa3f2e0 rembrandt_defconfig: remove duplicate CONFIG_COMP_DCBLOCK=n
83f69f144fe8e1d770d78effdba971e46d27e4d7 mt8195: Disable GOOGLE AEC MOCK by default
a25c70e183f17e05bbb915c3c62a3ee97917491c amd: enable probe functionality config for AMD/Renoir platform.
a732b3bbd379797fff20485a64da7b4238db776f amd: add probe functionality in AMD/vangogh platform.
eb3fcc67b80cd7de6b71df81093c04dee61a2d5d amd: add probe functionality in AMD/rembrandt platform.
5cf1c9c0d67c09437bff0db6e3bda4c7a1b0fbb5 topology: Add Waves codec to MT8188 topology
2e5703d7d7b3438189285e7ca00898aa14caac59 amd: acp_6_3: add build support for ACP_6_3
2c4d02df1e952ad234a0ac2d006e152cba6048aa src: arch: xtensa: configs: add ACP_6_3 defconfig
2e7296f17f7082b7de77889e86335f529d90c948 cmake: add new ${RIMAGE_TOP} constant
54dd8ecf193732879f54786d88a0fe5cc89e02a3 arch: xtensa: remove tigerlake platform option
049329be0b52f102b16f04924664012441bcda81 arch: xtensa: configs: remove Intel Tiger Lake defconfigs
51556ddd63b0b7562716a3ba03a7a43b13ebffe5 audio: sof: remove -fno-inline-functions option for cavs2.5 and ace
b1683ec1d6efb6c922ea66c03668f965d4626740 amd: vangogh: add build support for vangogh
da39550a4f1304c99bc3f75fc90b08fa7dcabf50 src: arch: xtensa: configs: add defconfig
a8967f784cc18896513ce8a5002ca170b2971d69 platform: amd: create common platform code for different amd platforms
9d51b5c10f6011d12a61781113c38200f033c80e audio: delete switch component
138a9d018755ed04475f63a1a360c7af05eb8a82 smart_amp: modify the Kconfig hierarchy
c7af3c5b3be5bc374faafc9d3a4d598fce2d6efa Kconfig: don't fall back on CONFIG_TIGERLAKE
820fadae3930d1335bc22fda98633cc08c49b22d xtensa/CMakeLists.txt: simplify CONFIG_RENOIR/REMBRANDT logic

@saisurya-ch
Copy link
Member Author

Last three commits need rewording of the git commit message.

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:

work/sof$ git log --pretty=oneline src/arch/xtensa/ |head -20
ee5efdf5b9ef8996cebd2b95d159edef77c91656 src/arch/xtensa/CMakeLists.txt: drop bogus, non-portable -Wl,-EL
a2338e51035dfeb5eea520391486fa6d4aa3f2e0 rembrandt_defconfig: remove duplicate CONFIG_COMP_DCBLOCK=n
83f69f144fe8e1d770d78effdba971e46d27e4d7 mt8195: Disable GOOGLE AEC MOCK by default
a25c70e183f17e05bbb915c3c62a3ee97917491c amd: enable probe functionality config for AMD/Renoir platform.
a732b3bbd379797fff20485a64da7b4238db776f amd: add probe functionality in AMD/vangogh platform.
eb3fcc67b80cd7de6b71df81093c04dee61a2d5d amd: add probe functionality in AMD/rembrandt platform.
5cf1c9c0d67c09437bff0db6e3bda4c7a1b0fbb5 topology: Add Waves codec to MT8188 topology
2e5703d7d7b3438189285e7ca00898aa14caac59 amd: acp_6_3: add build support for ACP_6_3
2c4d02df1e952ad234a0ac2d006e152cba6048aa src: arch: xtensa: configs: add ACP_6_3 defconfig
2e7296f17f7082b7de77889e86335f529d90c948 cmake: add new ${RIMAGE_TOP} constant
54dd8ecf193732879f54786d88a0fe5cc89e02a3 arch: xtensa: remove tigerlake platform option
049329be0b52f102b16f04924664012441bcda81 arch: xtensa: configs: remove Intel Tiger Lake defconfigs
51556ddd63b0b7562716a3ba03a7a43b13ebffe5 audio: sof: remove -fno-inline-functions option for cavs2.5 and ace
b1683ec1d6efb6c922ea66c03668f965d4626740 amd: vangogh: add build support for vangogh
da39550a4f1304c99bc3f75fc90b08fa7dcabf50 src: arch: xtensa: configs: add defconfig
a8967f784cc18896513ce8a5002ca170b2971d69 platform: amd: create common platform code for different amd platforms
9d51b5c10f6011d12a61781113c38200f033c80e audio: delete switch component
138a9d018755ed04475f63a1a360c7af05eb8a82 smart_amp: modify the Kconfig hierarchy
c7af3c5b3be5bc374faafc9d3a4d598fce2d6efa Kconfig: don't fall back on CONFIG_TIGERLAKE
820fadae3930d1335bc22fda98633cc08c49b22d xtensa/CMakeLists.txt: simplify CONFIG_RENOIR/REMBRANDT logic

Done

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 29, 2024

This depends on the resolution of #9413 -> #9442 first.

@marc-hb marc-hb marked this pull request as draft August 29, 2024 18:42
@marc-hb marc-hb marked this pull request as ready for review September 6, 2024 13:48
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"
Copy link
Collaborator

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)

Copy link
Member Author

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 <saisurya.chakkaveeravenkatanaga@amd.com>
Add defconfig for ACP_7_0 platform.

Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
Add ACP_7_0 platform topology.

Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
Add acp_7_0 toml file to support sof-acp_7_0.ri binary build

Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
Add build support to enable ACP_7_0 platform.

Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
- Add ACP_7_0 platform support

Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
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 <saisurya.chakkaveeravenkatanaga@amd.com>
src: alloc: Remove unused get memory size functions.

Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
amd: Remove inclusion of unrequired header files.

Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>
@saisurya-ch saisurya-ch reopened this Sep 8, 2024
Copy link
Member

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

@bhiregoudar
Copy link

@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.
Can we get this PR merged? we have some pending follow up patches in line.

@kv2019i kv2019i merged commit d00561e into thesofproject:main Sep 11, 2024
69 of 92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants