Add support for AMD ACP_6_0 ADSP #79796

DINESHKUMARK1 · 2024-10-14T11:52:58Z

https://builds.zephyrproject.io/zephyr/pr/79796/docs/boards/amd/acp_6_0_adsp/doc/index.html

Add support for ACP_6_0 to build Sound Open Firmware with Zephyr OS on DSP.
The build target is acp_6_0. It has one Xtensa HiFi5 core, RAM & Storage: 1.75 MB HP SRAM / 512 KB IRAM/DRAM

Audio Interfaces:
1 x SP (I2S, PCM),
1 x BT (I2S, PCM),
1 x HS(I2S, PCM),
DMIC as audio interfaces.
Please check each commit message.

With these commits I have a successful build.
I obtain the zephyr.ri image which boots the DSP and loads the topology.

snd_sof_amd_acp_6_0 0000:03:00.5: booting DSP firmware
snd_sof_amd_acp_6_0 0000:03:00.5: Firmware info: version 2:11:99-29faa
snd_sof_amd_acp_6_0 0000:03:00.5: Firmware: ABI 3:29:1 Kernel ABI 3:23:0

sof@target:~$ aplay -l
**** List of PLAYBACK Hardware Devices ****
card 2: sofacp_6_0dsp [sof-acp_6_0-dsp], device 0: I2SHS () []
Subdevices: 1/1
Subdevice #0: subdevice #0
card 2: sofacp_6_0dsp [sof-acp_6_0-dsp], device 1: I2SHS_VIRTUAL () []
Subdevices: 1/1
Subdevice #0: subdevice #0

DINESHKUMARK1 · 2024-10-14T11:58:02Z

Due to the local repository being deleted, I was unable to reopen the original PR (79534 ). Therefore, I have raised a new PR for this issue

dcpleung · 2024-10-15T16:04:06Z

Could you squash all the "update" commits to the ones originally introduced those files?

DINESHKUMARK1 · 2024-10-16T10:49:32Z

Could you squash all the "update" commits to the ones originally introduced those files?

The changes will be updated to the original commit once the CI test failure is resolved.

DINESHKUMARK1 · 2024-10-17T14:10:29Z

Could you squash all the "update" commits to the ones originally introduced those files?

The changes will be updated to the original commit once the CI test failure is resolved.

Consolidate all updates into original commits.

DINESHKUMARK1 · 2024-10-22T09:20:26Z

Hi there! Could you please provide an update on the status of this PR?

dcpleung · 2024-10-22T16:34:24Z

Could you rearrange the commit so the SoC commit goes before the board one? It would help with git bisect. If there is a board config with a non-existant SoC config, twister would probably not be very happy about it.

Also, since there is a BSD license mixed in the files, you will need to go through the approval process.

kartben · 2024-10-29T04:38:17Z

@DINESHKUMARK1 if the PR is not actually ready to be reviewed it might be better for you to mark it as draft. Otherwise reviewers (myself included) never know when you're actually expecting them to look at the PR to actually approve it
Also, I would really encourage you to test your documentation changes locally as it looks like doc tweaks might be one of the reasons why you're pushing new edits
See https://docs.zephyrproject.org/latest/contribute/documentation/generation.html

DINESHKUMARK1 · 2024-10-29T04:44:26Z

+1 for docs

It seems the documentation could use some additional information. Otherwise, the current details should be sufficient for the end user.

kartben · 2024-10-29T10:04:18Z

@DINESHKUMARK1 if the PR is not actually ready to be reviewed it might be better for you to mark it as draft. Otherwise reviewers (myself included) never know when you're actually expecting them to look at the PR to actually approve it Also, I would really encourage you to test your documentation changes locally as it looks like doc tweaks might be one of the reasons why you're pushing new edits See docs.zephyrproject.org/latest/contribute/documentation/generation.html

@DINESHKUMARK1 again, can you please test your changes locally, or put the PR back to draft mode so that we know it's not ready to be reviewed? Thanks!

DINESHKUMARK1 · 2024-10-29T11:30:54Z

@DINESHKUMARK1 if the PR is not actually ready to be reviewed it might be better for you to mark it as draft. Otherwise reviewers (myself included) never know when you're actually expecting them to look at the PR to actually approve it Also, I would really encourage you to test your documentation changes locally as it looks like doc tweaks might be one of the reasons why you're pushing new edits See docs.zephyrproject.org/latest/contribute/documentation/generation.html

@DINESHKUMARK1 again, can you please test your changes locally, or put the PR back to draft mode so that we know it's not ready to be reviewed? Thanks!

This is the final edit and is ready for review. Additional information can be added in future PRs as we have few more commits to submit. I think, the current documentation should be sufficient for this PR.

DINESHKUMARK1 · 2024-11-04T05:57:34Z

@DINESHKUMARK1 if the PR is not actually ready to be reviewed it might be better for you to mark it as draft. Otherwise reviewers (myself included) never know when you're actually expecting them to look at the PR to actually approve it Also, I would really encourage you to test your documentation changes locally as it looks like doc tweaks might be one of the reasons why you're pushing new edits See docs.zephyrproject.org/latest/contribute/documentation/generation.html

@DINESHKUMARK1 again, can you please test your changes locally, or put the PR back to draft mode so that we know it's not ready to be reviewed? Thanks!

This is the final edit and is ready for review. Additional information can be added in future PRs as we have few more commits to submit. I think, the current documentation should be sufficient for this PR.

Acknowledging PR review request, PR ready for review. Let me know if any additional information is needed.

DINESHKUMARK1 · 2024-11-07T08:32:04Z

could the reviewers help with the review to progress further with next steps.

kartben

could the reviewers help with the review to progress further with next steps.

I made a comment a couple weeks ago about the documentation being on the light side, and it is now even lighter, so please try to improve it a bit, I left some comments. Also, please look at making sure the board can be used with Zephyr SDK and is tested/testable in CI
Thanks

boards/amd/acp_6_0_adsp/doc/index.rst

kartben · 2024-11-07T09:35:46Z

boards/amd/acp_6_0_adsp/doc/index.rst

+For debugging purposes, use xt-gdb. For xcc debugging,
+please contact the AMD developers’ debug tools team or utilize Cadence debugging tools.


"please contact the AMD developers’ debug tools team" is not necessarily appropriate for the documentation of an upstream open source project IMO. Please consider dropping this.

Please don't just mark items as resolved. It's up to the original reviewer to check if the comment has been correctly addressed
If you really want to close it (e.g. if request has been applied verbatim) then please add a comment explaining what you did

kartben · 2024-11-07T09:38:08Z

boards/amd/acp_6_0_adsp/doc/index.rst

+System requirements
+*******************
+
+Xtensa Toolchain
+----------------


This seems to imply that the Xtensa toolchain is a requirement which I don't think is the case judging by the text that follows.

Suggested change

System requirements

*******************

Xtensa Toolchain

----------------

System requirements

*******************

Xtensa Toolchain (optional)

---------------------------

That being said, it does not seem possible to build with the Zephyr SDK - maybe SOF module needs updating?? https://github.com/zephyrproject-rtos/sof/tree/zephyr/src/platform
(really not familiar with this so it might be something else)

... In file included from /Users/kartben/zephyrproject/zephyr/include/zephyr/arch/xtensa/arch.h:29, from /Users/kartben/zephyrproject/zephyr/include/zephyr/arch/cpu.h:27, from /Users/kartben/zephyrproject/zephyr/include/zephyr/kernel_includes.h:36, from /Users/kartben/zephyrproject/zephyr/include/zephyr/kernel.h:17, from /Users/kartben/zephyrproject/zephyr/include/zephyr/pm/device.h:11, from /Users/kartben/zephyrproject/zephyr/kernel/include/kernel_offsets.h:7, from /Users/kartben/zephyrproject/zephyr/arch/xtensa/core/offsets/offsets.c:7: /Users/kartben/zephyrproject/modules/hal/xtensa/include/xtensa/config/core.h:51:10: fatal error: xtensa/config/core-matmap.h: No such file or directory 51 | #include <xtensa/config/core-matmap.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. ...

zephyrproject-rtos/hal_xtensa#33 this PR might update SOF modules.

kartben · 2024-11-07T09:40:39Z

boards/amd/acp_6_0_adsp/doc/index.rst

+Supported Features
+==================
+
+The following hardware features are supported:
+
+-----------+------------+-------------------------------------+
+| Interface | Controller | Driver/Component                    |
+===========+============+=====================================+
+| I2S       | on-chip    | interrupt controller                |
+-----------+------------+-------------------------------------+
+
+System Clock
+------------
+
+This board configuration uses a system clock frequency of @ 200 - 800MHz.


as mentioned before, please try to follow the board template... This should come earlier, as a subsection of the Hardware heading.

This hasn't been addressed. Please check board template and other boards documentation. Also please note how comment being made usually clearly indicate which Line range they apply to -- here, for example, it's not just the system clock section I was referring to... (and as you can see this is a very good example of why one should mark conversations as resolved themselves :))

Currently Zephyr is being enabled on AMD reference/proprietary boards. So, we couldn't add more data as part of boards documentation. We don't have any generic boards or evaluation boards to provide more information currently. We will plan to add more information as and when possible for everyone's usage. We have just done the basic enablement of Zephyr, still we have many changes to add, so we will plan to update the documentation

kartben · 2024-11-07T09:47:01Z

boards/amd/acp_6_0_adsp/doc/index.rst

+-----------+------------+-------------------------------------+
+| Interface | Controller | Driver/Component                    |
+===========+============+=====================================+
+| I2S       | on-chip    | interrupt controller                |


I am not sure what you mean here? Is I2S actually supported? and why "interrupt controller"?

kartben · 2024-11-07T09:50:28Z

boards/amd/acp_6_0_adsp/board.yml

you also need an acp_6_0_adsp.yml file so that the board is tested in CI (which would also allow to catch the apparent issue of it not currently working with the Zephyr SDK)

henrikbrixandersen · 2024-11-07T09:55:52Z

could the reviewers help with the review to progress further with next steps.

We are currently in feature freeze for Zephyr v4.0.0, so most developers are focusing on testing, bug fixes, documentation updates etc. at the moment. I am sure reviews will be ticking in once v4.0.0 is released.

andyross

Sorry, I missed this when it came by the first time. Notes and nitpicks, nothing particularly fatal.

andyross · 2024-11-13T15:53:31Z

soc/amd/acp_6_0/adsp/_soc_inthandlers.h

+ * dispatcher will handle exactly one flagged interrupt, in numerical
+ * order (low bits first) and will return a mask of that bit that can
+ * then be cleared by the calling code.  Unrecognized bits for the
+ * level will invoke an error handler.


Please investigate CONFIG_XTENSA_GEN_HANDLERS=y. A few legacy platforms still have these hand-generated files because they were editted, but we now run the generator script against the platform core-isa.h at build time.

andyross · 2024-11-13T15:55:18Z

soc/amd/acp_6_0/adsp/include/adsp/cache.h

+
+#ifndef __COMMON_ADSP_CACHE_H__
+#define __COMMON_ADSP_CACHE_H__
+#include <xtensa/hal.h>


What tries to include this? Seems weird for a platform-level header to be empty, and in particular for it to act only to include non-Zephyr APIs.

andyross · 2024-11-13T15:57:45Z

soc/amd/acp_6_0/adsp/include/adsp/io.h

+
+static inline uint32_t io_reg_read(uint32_t reg)
+{
+	return sys_read32(reg);


This looks like a vestigial SOF compatibility shim? Nothing in the Zephyr tree should be using these calls. Grepping around I think you got this via cut paste from imx? Really this doesn't belong here, move this to the SOF platform layer.

andyross · 2024-11-13T15:59:41Z

soc/amd/acp_6_0/adsp/memory.h

+#define MEM_RESET_TEXT_SIZE             0x400
+#define MEM_RESET_LIT_SIZE              0x8
+#define XCHAL_RESET_VECTOR_PADDR_IRAM   0x7F000000
+#define XCHAL_WINDOW_VECTORS_PADDR_IRAM 0x7F000400


Standard complaint: memory layout and register addresses really want to live in device tree and not be hard-coded in a big header. I understand that's not the way it works in SOF upstream, so some amount of friction is inevitable with early work on new platforms. But really this isn't the right way to do things.

andyross · 2024-11-13T16:01:15Z

soc/amd/acp_6_0/Kconfig.defconfig

+
+if SOC_ACP_6_0
+config DCACHE_LINE_SIZE
+default 128


Style: lines under a "config" should be indented with a single tab.

andyross · 2024-11-13T16:03:15Z

boards/amd/acp_6_0_adsp/acp_6_0_adsp.yml

+arch: xtensa
+toolchain:
+  - zephyr
+  - xcc


FWIW, pretty much every other SOF platform is moving towards xt-clang at this point. The last Cadence toolchain version that supports xcc is now over three years old.

The acp_6_0 includes support for xcc, and for upcoming boards, we could migrate to xt-clang.

andyross · 2024-11-13T16:10:35Z

boards/amd/acp_6_0_adsp/acp_6_0_adsp_defconfig

+CONFIG_OUTPUT_SYMBOLS=y
+CONFIG_MULTI_LEVEL_INTERRUPTS=n
+CONFIG_2ND_LEVEL_INTERRUPTS=n
+CONFIG_BUILD_OUTPUT_BIN=n


This one should be forced off at the arch layer, no? If you're getting a zephyr.bin (which is mostly only meaningful on cortex-M) there's a bug somewhere we should fix. It's not supposed to be something every board needs to turn off.

andyross · 2024-11-13T16:12:19Z

boards/amd/acp_6_0_adsp/acp_6_0_adsp_defconfig

+CONFIG_MULTI_LEVEL_INTERRUPTS=n
+CONFIG_2ND_LEVEL_INTERRUPTS=n
+CONFIG_BUILD_OUTPUT_BIN=n
+CONFIG_CLEANUP_INTERMEDIATE_FILES=y


This one should come out. The intermediate files are actually useful for a lot of things. This setting exists for e.g. CI applications that need to have hundreds or thousands of build trees in cloud filesystems with limited storage. Generic builds should use the defaults, it's not something the board layer is supposed to care about.

Add myself and basavaraj as codeowners for ACP_6_0 related files for SOF with Zephyr OS. Signed-off-by: DineshKumar Kalva <[email protected]>

Add support for signing acp_6_0 SOF with Zephyr images with rimage. Signed-off-by: DineshKumar Kalva <[email protected]>

Add a common part for AMD board ACP_6_0_ADSP. Add support for ACP_6_0_ADSP BOARD, which represents ACP_6_0 soc. This has a 1 Xtensa HiFi5 core, with 200-800MHz 1.75 MB HP SRAM / 512 KB IRAM/DRAM, 1 x SP (I2S, PCM), 1 x BT (I2S, PCM), 1 x HS(I2S, PCM), DMIC as audio interfaces. Signed-off-by: DineshKumar Kalva <[email protected]>

Create a acp_6_0_adsp board support for the Audio DSP on ACP soc. Signed-off-by: DineshKumar Kalva <[email protected]>

boards/amd/acp_6_0_adsp/doc/index.rst

DINESHKUMARK1 · 2024-11-18T09:37:43Z

boards/amd/acp_6_0_adsp/acp_6_0_adsp.yml

+arch: xtensa
+toolchain:
+  - zephyr
+  - xcc


The acp_6_0 includes support for xcc, and for upcoming boards, we could migrate to xt-clang.

boards/amd/acp_6_0_adsp/doc/index.rst

DINESHKUMARK1 · 2024-11-18T09:45:15Z

boards/amd/acp_6_0_adsp/acp_6_0_adsp_defconfig

We’re generating zephyr.ri instead of zephyr.bin, so CONFIG_BUILD_OUTPUT_BIN is unset. This configuration is no longer necessary

andyross

I'm out of complaints

github-actions · 2024-11-19T22:53:49Z

Hi @DINESHKUMARK1!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

zephyrbot added area: West West utility area: Xtensa Xtensa Architecture labels Oct 14, 2024

zephyrbot requested review from andyross, carlescufi, ceolin, dcpleung, mbolivar-ampere, nashif, pdgendt and swinslow October 14, 2024 11:53

zephyrbot assigned dcpleung Oct 14, 2024

DINESHKUMARK1 force-pushed the main branch 4 times, most recently from a426f76 to a3b1641 Compare October 14, 2024 13:11

DINESHKUMARK1 force-pushed the main branch 2 times, most recently from 2531094 to a6d9b83 Compare October 17, 2024 05:23

kv2019i mentioned this pull request Oct 17, 2024

Build SOF with zephyr on acp_6_0 platform thesofproject/sof#9578

Merged

DINESHKUMARK1 force-pushed the main branch 3 times, most recently from 793042d to 219f9fe Compare October 17, 2024 14:04

DINESHKUMARK1 force-pushed the main branch from 219f9fe to f341936 Compare October 21, 2024 04:16

DINESHKUMARK1 force-pushed the main branch from f341936 to 4e0e8ec Compare October 23, 2024 04:58

zephyrbot added the platform: Xilinx Xilinx label Oct 23, 2024

DINESHKUMARK1 force-pushed the main branch from be8a311 to e9f423b Compare October 29, 2024 04:19

DINESHKUMARK1 requested a review from kartben October 29, 2024 05:49

DINESHKUMARK1 force-pushed the main branch 2 times, most recently from f28fc7b to 6c94229 Compare October 29, 2024 09:24

dcpleung previously approved these changes Nov 4, 2024

View reviewed changes

kartben requested changes Nov 7, 2024

View reviewed changes

DINESHKUMARK1 dismissed dcpleung’s stale review via f6fae39 November 11, 2024 13:48

DINESHKUMARK1 force-pushed the main branch from 6c94229 to f6fae39 Compare November 11, 2024 13:48

DINESHKUMARK1 requested a review from kartben November 13, 2024 05:34

DINESHKUMARK1 force-pushed the main branch 2 times, most recently from a32dabd to ebe57d5 Compare November 13, 2024 11:54

andyross reviewed Nov 13, 2024

View reviewed changes

DINESHKUMARK1 added 4 commits November 18, 2024 14:48

CODEOWNERS: add codeowner for SOF with Zephyr on AMD ACP_6_0.

6a5ef68

Add myself and basavaraj as codeowners for ACP_6_0 related files for SOF with Zephyr OS. Signed-off-by: DineshKumar Kalva <[email protected]>

west: sign: add support for AMD acp_6_0_adsp board.

f1a9f67

Add support for signing acp_6_0 SOF with Zephyr images with rimage. Signed-off-by: DineshKumar Kalva <[email protected]>

Board: amd : add board support for the Audio DSP on ACP_6_0 soc.

0a77129

Create a acp_6_0_adsp board support for the Audio DSP on ACP soc. Signed-off-by: DineshKumar Kalva <[email protected]>

DINESHKUMARK1 force-pushed the main branch from c594488 to 0a77129 Compare November 18, 2024 09:18

DINESHKUMARK1 commented Nov 18, 2024

View reviewed changes

andyross approved these changes Nov 19, 2024

View reviewed changes

dcpleung approved these changes Nov 19, 2024

View reviewed changes

kartben approved these changes Nov 19, 2024

View reviewed changes

nashif merged commit 749192a into zephyrproject-rtos:main Nov 19, 2024
35 checks passed

		For debugging purposes, use xt-gdb. For xcc debugging,
		please contact the AMD developers’ debug tools team or utilize Cadence debugging tools.

Add support for AMD ACP_6_0 ADSP #79796

Add support for AMD ACP_6_0 ADSP #79796

Conversation

DINESHKUMARK1 commented Oct 14, 2024 • edited Loading

DINESHKUMARK1 commented Oct 14, 2024

dcpleung commented Oct 15, 2024

DINESHKUMARK1 commented Oct 16, 2024

DINESHKUMARK1 commented Oct 17, 2024

DINESHKUMARK1 commented Oct 22, 2024

dcpleung commented Oct 22, 2024

kartben commented Oct 29, 2024

DINESHKUMARK1 commented Oct 29, 2024

kartben commented Oct 29, 2024

DINESHKUMARK1 commented Oct 29, 2024 • edited Loading

DINESHKUMARK1 commented Nov 4, 2024 • edited Loading

DINESHKUMARK1 commented Nov 7, 2024

kartben left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikbrixandersen commented Nov 7, 2024

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyross left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024

DINESHKUMARK1 commented Oct 14, 2024 •

edited

Loading

DINESHKUMARK1 commented Oct 29, 2024 •

edited

Loading

DINESHKUMARK1 commented Nov 4, 2024 •

edited

Loading

kartben left a comment •

edited

Loading