acp: add native Zephyr drivers for AMD ACP 7.X #10777
Conversation
|
Can one of the admins verify this patch?
|
|
Dependency: This PR depends on zephyrproject-rtos/zephyr#108859 for native Zephyr ACP 7.X driver enablement. |
|
will run the CI |
|
test this please |
aab5985 to
3cb685a
Compare
kv2019i
left a comment
There was a problem hiding this comment.
Some comments inline. A few questions about modifications of common code. I also wonder why you still need so many xtensa headers. I guess your platform code may have some dependencies, but other vendors have been able to drop these headers from SOF once drivers moved to Zephyr side.
| volume_set_ramp_channel_counter(cd, cd->channels); | ||
|
|
||
| volume_ramp_check(mod); | ||
| #if defined(CONFIG_AMD) |
There was a problem hiding this comment.
This looks a bit odd? Why would this be HW/AMD specific functionality?
There was a problem hiding this comment.
Thank you for the review.
This fix addresses an issue we observed on AMD platforms where the scale_vol function pointer is not reset after volume reaches its maximum value. When volume hits max, is_passthrough is set to true and scale_vol points to the passthrough function. On subsequent volume decreases, is_passthrough remains set and the correct gain function is never re-selected, so the volume change has no audible effect.
We've guarded this under CONFIG_AMD since this is where we observed, debugged, and verified the fix. We haven't been able to confirm whether other platforms encounter the same behavior, so we kept the guard conservative to avoid any unintended side effects on other platforms.
There was a problem hiding this comment.
@sneha-voona This sounds like a generic bug, so I think we can drop the "#ifdef" and apply for all. @thesofproject/nxp and @thesofproject/mediatek can you check (as you have usage of this IPC3 volume component).
| } | ||
|
|
||
| #if defined(CONFIG_AMD) | ||
| /* Free allocated DAI-specific data structures */ |
There was a problem hiding this comment.
This is looks a bit odd in common code, but I see matching code in ipc_dai_data_config(). Maybe add a comment that this is freeing the object allocated in ipc_dai_data_config().
There was a problem hiding this comment.
Thank you for the review.
Fixed in latest push.
| #if defined(CONFIG_SOC_ACP_7_X) | ||
| { SOF_IPC_FRAME_S24_4LE, SOF_IPC_FRAME_S32_LE, just_copy }, | ||
| { SOF_IPC_FRAME_S32_LE, SOF_IPC_FRAME_S24_4LE, pcm_convert_s32_to_s24_be }, | ||
| #else |
There was a problem hiding this comment.
Can you @singalsu check this? This is first time we have HW dependencies the the PCM converter list.
It would prefer if we could keep same interpretations of the sample formats. This patch suggest your hardware natively 24bit encoding where the audio samples are stored in the 3 most significant bytes. The format we use for this is SOF_IPC_FRAME_S24_4LE_MSB.
So in theory, you'd set the correct format in either DSP topology, or some of the HW specific code. E.g. for Intel, we overrode the sample format in sof/src/ipc/ipc3/dai.c:ipc_dai_data_config().
Then we could keep the converter table vendor neutral.
There was a problem hiding this comment.
Thanks for the review.
The ACP 7.X hardware DMA strips the LSB byte from the 32-bit container and transmits only the upper 3 bytes to the bus. So 24-bit samples need to be MSB-aligned (left-shifted by 8) in the 32-bit word before reaching the DMA — which is exactly what pcm_convert_s32_to_s24_be does.
The reason we handle this at the converter level rather than changing the format enum:
PipeWire/ALSA negotiates S24_LE or S32_LE and sends LSB-aligned data (S24_4LE). The host-side format negotiation expects this standard format.
The MSB alignment is a hardware-level requirement specific to how the ACP DMA feeds data to the bus, rather than a format property of the audio pipeline. The converter is the appropriate place to handle this.
Overriding the format in ipc_dai_data_config() would only change the format label without performing the actual bit-shift — the incoming data from the host is still LSB-aligned S24_4LE, so a real conversion step is still needed before the data reaches the DMA.
The #if defined(CONFIG_SOC_ACP_7_X) guard keeps this hardware-specific conversion isolated so it doesn't affect other platforms at all.
There was a problem hiding this comment.
There are two negotiations here, the host side (what ALSA sends to/from SOF/DSP) and DAI side (what SOF/DSP sends to/from audio interfaces). So how this should work (playback example):
- ALSA can send e.g. S24_LE to DSP (no change)
- DSP pipe can use intermediate format based on what modules support (either process S24_LE natively or convert to their own intermediate format, this is configured in DSP topology)
- at DSP output, another negotiation happens and if only a fixed format (e.g. S24_4LE), PCM converter will be setup to convert to the hw supported format
We already do this for some Intel hw audio interfaces and this has worked without having special cases in the PCM converter, so I'd expect same to work in your case for ACP.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new AMD ACP 7.x platform target to the SOF Zephyr build, adding platform/driver code, build-system wiring, rimage memory layout, and initial SoundWire topologies for rt721-based systems.
Changes:
- Adds a new
ACP_7_Xplatform option (Kconfig, defconfig, rimage config) and wires it into both SOF and Zephyr CMake builds. - Introduces ACP 7.x platform implementation (platform init/clocking headers) and an ACP 7.x-specific IPC driver for Zephyr builds.
- Adds new topology m4 templates and registers an ACP 7.x SoundWire topology in the topology build.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/lib/dma.c | Extends Zephyr DMA table for ACP 7.x and uses DT-provided SDW DMA channel count. |
| zephyr/CMakeLists.txt | Adds Zephyr build sources/includes for the new CONFIG_SOC_ACP_7_X platform. |
| tools/topology/topology1/sof-acp_7_x_sdw.m4 | New ACP 7.x SoundWire topology (rt721) pipelines. |
| tools/topology/topology1/sof-acp_7_x_sdw_volume_comp.m4 | Variant topology adding volume components. |
| tools/topology/topology1/sof-acp_7_x_sdw_24bit.m4 | Variant topology targeting 24-bit formats. |
| tools/topology/topology1/CMakeLists.txt | Registers ACP 7.x SDW topology for build generation. |
| tools/rimage/config/acp_7_x.toml | Adds rimage memory zones for ACP 7.x. |
| src/platform/Kconfig | Adds ACP_7_X platform selection and signing schema default. |
| src/platform/amd/CMakeLists.txt | Adds ACP 7.x platform subdirectory and gates common platform subdir inclusion. |
| src/platform/amd/acp_7_x/CMakeLists.txt | Adds ACP 7.x platform sources to the SOF build. |
| src/platform/amd/acp_7_x/platform.c | New ACP 7.x platform init/boot-complete implementation for Zephyr timer domain path. |
| src/platform/amd/acp_7_x/lib/CMakeLists.txt | Adds ACP 7.x platform library sources. |
| src/platform/amd/acp_7_x/lib/clk.c | New ACP 7.x clock management implementation. |
| src/platform/amd/acp_7_x/include/platform/platform.h | ACP 7.x platform definitions and panic/IRQ constants. |
| src/platform/amd/acp_7_x/include/platform/platform_misc.h | ACP 7.x register defs and DMA-related structs used by AMD native drivers. |
| src/platform/amd/acp_7_x/include/platform/lib/memory.h | ACP 7.x memory map and heap/mailbox layout. |
| src/platform/amd/acp_7_x/include/platform/fw_scratch_mem.h | ACP scratch memory layout used for mailbox/flags. |
| src/platform/amd/acp_7_x/include/arch/xtensa/tie/xt_datacache.h | Xtensa TIE header added for this core configuration. |
| src/platform/amd/acp_7_x/include/arch/xtensa/config/tie.h | Xtensa core configuration header for ACP 7.x target. |
| src/platform/amd/acp_7_x/include/arch/xtensa/config/tie-asm.h | Xtensa assembler configuration header for ACP 7.x target. |
| src/platform/amd/acp_7_x/include/arch/xtensa/config/system.h | Xtensa system configuration header for ACP 7.x target. |
| src/platform/amd/acp_7_x/include/arch/xtensa/config/specreg.h | Xtensa special register definitions for ACP 7.x target. |
| src/platform/amd/acp_7_x/include/arch/xtensa/config/defs.h | Xtensa base compatibility header for ACP 7.x target. |
| src/platform/amd/acp_7_x/include/arch/xtensa/config/core-matmap.h | Xtensa memory attribute mapping header for ACP 7.x target. |
| src/platform/amd/acp_7_x/include/arch/xtensa/config/core-isa.h | Xtensa core ISA configuration for ACP 7.x target. |
| src/ipc/ipc3/dai.c | Adds ACP 7.x SoundWire bookkeeping and frees AMD DAI per-device data on release. |
| src/include/sof/schedule/ll_schedule_domain.h | Changes LL timer period for AMD builds. |
| src/drivers/amd/CMakeLists.txt | Adds ACP 7.x driver subdir and gates common driver subdir inclusion. |
| src/drivers/amd/acp_7_x/CMakeLists.txt | Adds ACP 7.x driver sources. |
| src/drivers/amd/acp_7_x/ipc.c | New ACP 7.x IPC/IRQ handling implementation. |
| src/audio/volume/volume_ipc3.c | AMD-specific behavior updating volume processing function after config changes. |
| src/audio/pcm_converter/pcm_converter_generic.c | Adds ACP 7.x-specific S24/S32 conversion mapping. |
| src/arch/xtensa/configs/acp_7_x_defconfig | Adds SOF defconfig for ACP 7.x builds. |
| scripts/xtensa-build-zephyr.py | Adds build-script platform config for acp_7_x and marks it unsupported in sof_ri_info. |
| app/boards/acp_7_x_adsp.conf | Adds Zephyr board config enabling ACP 7.x native drivers (INTC/DMA/DAI SDW). |
| "acp_7_x" : PlatformConfig( | ||
| "amd", "acp_7_x_adsp/acp_7_x", | ||
| f"RI-2022.9{xtensa_tools_version_postfix}", | ||
| "ACP73x_HiFi5_NNE_PROD", |
There was a problem hiding this comment.
Fixed in latest push.
Updated XCHAL_CORE_ID in core-isa.h to "ACP73x_HiFi5_NNE_PROD" to match XTENSA_CORE. The core-isa.h value was incorrect, not the build script.
| schedule_task_init_edf(&ipc->ipc_task, SOF_UUID(ipc_task_amd_uuid), | ||
| &ipc_task_ops, ipc, 0, 0); | ||
| interrupt_disable(IRQ_EXT_IPC_LEVEL_3, 0); | ||
| interrupt_unregister(IRQ_EXT_IPC_LEVEL_3, ipc); | ||
| interrupt_register(IRQ_EXT_IPC_LEVEL_3, amd_irq_handler, ipc); | ||
| /* Enabling software interuppts */ | ||
| interrupt_enable(IRQ_EXT_IPC_LEVEL_3, ipc); |
There was a problem hiding this comment.
Fixed in latest push.
Changed interrupt_disable(IRQ_EXT_IPC_LEVEL_3, 0) to use ipc as the second argument, consistent with src/drivers/amd/common/ipc.c.
| SCRATCH_REG_OFFSET); | ||
|
|
||
| SOF_DEFINE_REG_UUID(ipc_task_amd); | ||
| extern volatile acp_scratch_mem_config_t *pscratch_mem_cfg; |
| #include <stdint.h> | ||
|
|
||
| #if defined(CONFIG_AMD) | ||
| #define LL_TIMER_PERIOD_US 500ULL /* 500us period for AMD ACP platforms */ |
There was a problem hiding this comment.
In the SOF tree, CONFIG_AMD is used exclusively for AMD ACP platforms — there are no non-ACP AMD platforms in this codebase. So CONFIG_AMD is the correct and sufficient guard for this use
| #if defined(CONFIG_SOC_ACP_7_X) | ||
| { SOF_IPC_FRAME_S24_4LE, SOF_IPC_FRAME_S32_LE, just_copy }, | ||
| { SOF_IPC_FRAME_S32_LE, SOF_IPC_FRAME_S24_4LE, pcm_convert_s32_to_s24_be }, | ||
| #else | ||
| { SOF_IPC_FRAME_S24_4LE, SOF_IPC_FRAME_S32_LE, pcm_convert_s24_to_s32 }, | ||
| { SOF_IPC_FRAME_S32_LE, SOF_IPC_FRAME_S24_4LE, pcm_convert_s32_to_s24 }, | ||
| #endif |
There was a problem hiding this comment.
Thanks for the review.
The ACP 7.X hardware DMA strips the LSB byte from the 32-bit container and transmits only the upper 3 bytes to the bus. So 24-bit samples need to be MSB-aligned (left-shifted by 8) in the 32-bit word before reaching the DMA — which is exactly what pcm_convert_s32_to_s24_be does.
The reason we handle this at the converter level rather than changing the format enum:
PipeWire/ALSA negotiates S24_LE or S32_LE and sends LSB-aligned data (S24_4LE). The host-side format negotiation expects this standard format.
The MSB alignment is a hardware-level requirement specific to how the ACP DMA feeds data to the bus, rather than a format property of the audio pipeline. The converter is the appropriate place to handle this.
Overriding the format in ipc_dai_data_config() would only change the format label without performing the actual bit-shift — the incoming data from the host is still LSB-aligned S24_4LE, so a real conversion step is still needed before the data reaches the DMA.
The #if defined(CONFIG_SOC_ACP_7_X) guard keeps this hardware-specific conversion isolated so it doesn't affect other platforms at all.
| #include <zephyr/drivers/dma.h> | ||
|
|
||
| #define PU_REGISTER_BASE (0x9FD00000 - 0x01240000) | ||
| #define PU_SCRATCH_REG_BASE (0x9FF00000 - 0x01250000) | ||
| #define ACP_FUTURE_REG_ACLK_0 0x12418E0 /* don't use - reserved for driver gsync */ |
There was a problem hiding this comment.
ACP 7.X is a Zephyr-only platform — there is no non-Zephyr build support for it. The unconditional Zephyr include is intentional and correct.
| #Required Topology for rt721 with ACP DMIC Card for ACP_7_X | ||
| # | ||
| # PCM Description DAI LINK DAI BE | ||
| # 0 HS Playback 0 SDW0-PIN0-PLAYBACK-SimpleJack AUDIO_TX | ||
| # 1 HS Capture 1 SDW0-PIN11-CAPTURE-SimpleJack AUDIO_RX | ||
| # 2 Speaker playback 2 SDW0-PIN4-PLAYBACK-SmartAmp BT_TX | ||
| # 4 SDW DMIC 4 SDW0-PIN15-CAPTURE-SmartMic HS_RX | ||
|
|
| #Required Topology for rt721 with ACP DMIC Card for ACP_7_X | ||
| # | ||
| # PCM Description DAI LINK DAI BE | ||
| # 0 HS Playback 0 SDW0-PIN0-PLAYBACK-SimpleJack AUDIO_TX | ||
| # 1 HS Capture 1 SDW0-PIN11-CAPTURE-SimpleJack AUDIO_RX | ||
| # 2 Speaker playback 2 SDW0-PIN4-PLAYBACK-SmartAmp BT_TX | ||
| # 4 SDW DMIC 4 SDW0-PIN15-CAPTURE-SmartMic HS_RX | ||
|
|
| #Required Topology for rt721 with ACP DMIC Card for ACP_7_X | ||
| # | ||
| # PCM Description DAI LINK DAI BE | ||
| # 0 HS Playback 0 SDW0-PIN0-PLAYBACK-SimpleJack AUDIO_TX | ||
| # 1 HS Capture 1 SDW0-PIN11-CAPTURE-SimpleJack AUDIO_RX | ||
| # 2 Speaker playback 2 SDW0-PIN4-PLAYBACK-SmartAmp BT_TX | ||
| # 4 SDW DMIC 4 SDW0-PIN15-CAPTURE-SmartMic HS_RX | ||
|
|
|
@sneha-voona I've added copilot for review as this is standard now, pls do respond and resolve any copilot comments. |
a21f149 to
f0c58ce
Compare
Add ACP_7_X platform support to xtensa-build-zephyr.py build script. Signed-off-by: Sneha Voona <sneha.voona@amd.com> Co-authored-by: DineshKumar Kalva <DineshKumar.Kalva@amd.com>
Add the Xtensa HiFi5 defconfig for the AMD ACP 7.X audio DSP. Signed-off-by: Sneha Voona <sneha.voona@amd.com> Co-authored-by: DineshKumar Kalva <DineshKumar.Kalva@amd.com>
Add the SoundWire topology M4 source file for AMD ACP 7.X (sof-acp_7_x_sdw.m4). The topology defines the SoundWire pipeline for native Zephyr, covering playback and capture paths for the SoundWire SW0 instance on ACP 7.X hardware. Signed-off-by: Sneha Voona <sneha.voona@amd.com>
Add the rimage toml configuration file for AMD ACP 7.X to enable signing and packaging of the sof-acp_7_x.ri firmware binary. The file specifies the adsp name and memory layout required by rimage to produce a correctly formatted image for the ACP 7.X audio DSP. Signed-off-by: Sneha Voona <sneha.voona@amd.com> Co-authored-by: DineshKumar Kalva <DineshKumar.Kalva@amd.com>
Add CMakeLists.txt files to integrate the ACP 7.X platform and driver sources into the SOF CMake build system. This covers the platform-level build (platform.c, lib/clk.c) and the driver-level build (ipc.c) under src/drivers/amd/acp_7_x/, enabling the ACP_7_X platform to be compiled when CONFIG_ACP_7_X is selected. Signed-off-by: Sneha Voona <sneha.voona@amd.com> Co-authored-by: DineshKumar Kalva <DineshKumar.Kalva@amd.com>
Add ACP_7_X platform support for native Zephyr drivers. Changes include: - Add platform-specific files for ACP_7_X native Zephyr support - Add ACP_7_X in zephyr/CMakeLists.txt with platform sources, include directories and PLATFORM variable - Add app/boards/acp_7_x_adsp.conf with ACP_7_X Kconfig options for native Zephyr drivers (DMA, DAI, interrupts) Signed-off-by: Sneha Voona <sneha.voona@amd.com> Co-authored-by: DineshKumar Kalva <DineshKumar.Kalva@amd.com>
f0c58ce to
22abd43
Compare
Add ACP_7_X SDW DMA channel support in zephyr/lib/dma.c for native Zephyr DMA operations. Update dai.c to set dai_index from dd->dai->index for AMD SDW DAI, add ACP_7_X SDW instance handling with per-instance pin index array, and free DAI-specific data on DMA release for AMD. Signed-off-by: Sneha Voona <sneha.voona@amd.com>
On AMD platforms, the scale_vol function pointer is not reset after volume reaches its maximum value (32). This causes decreasing volume after reaching max to have no effect, because is_passthrough remains set and scale_vol is never updated to the gain function. Fix by resetting is_passthrough to false and re-selecting scale_vol via set_volume_process() on every SOF_CTRL_CMD_VOLUME command under CONFIG_AMD, ensuring correct volume scaling in both directions. Changes: - src/arch/xtensa/configs/acp_7_x_defconfig: enable CONFIG_COMP_VOLUME for ACP_7_X platform - src/audio/volume/volume_ipc3.c: reset is_passthrough and call set_volume_process() after volume changes so scale_vol is re-selected correctly after max volume is reached - tools/topology/topology1/sof-acp_7_x_sdw_volume_comp.m4: add ACP_7_X SDW topology with volume pipelines for HS playback/capture, Speaker playback and DMIC capture paths Signed-off-by: Sneha Voona <sneha.voona@amd.com>
Changes include: - Add sample topology sof-acp_7_x_sdw_24bit.m4 for ACP_7_X SDW 24-bit pipelines supporting playback and capture paths - Select PCM conversion functions for S24_4LE<->S32_LE as per AMD hardware requirements under CONFIG_AMD, preserving existing behaviour for all other platforms Signed-off-by: Sneha Voona <sneha.voona@amd.com>
As per AMD internal hardware design requirement, the ACP DSP scheduler tick must run at 500 microseconds (2kHz) instead of the default 1ms (1kHz). Guard the 500us value with CONFIG_AMD so non-AMD platforms retain the existing 1ms default period unchanged. Signed-off-by: Sneha Voona <sneha.voona@amd.com>
22abd43 to
42a3bda
Compare
|
Hi @kv2019i @lgirdwood, Thank you for the review. All comments from both the Copilot review (requested by @lgirdwood) and @kv2019i have been addressed in the latest force-push. Also rebased on latest main to resolve the dai.c conflict from the recent DMA channel rework. Please take another look when you get a chance. Thank you! |
| @@ -0,0 +1,262 @@ | |||
| /* | |||
There was a problem hiding this comment.
Fwiw, you should not need these headers to be included now since they should go in the Zephyr xtensa HAL now.
This PR adds native Zephyr support for AMD Audio Co-Processor (ACP) version 7.X for handling Sound Wire IO.