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.

Sign up for GitHub

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

audio: base_fw: add platform layer to hw_config data #9012

Merged
merged 1 commit into from
Apr 17, 2024
+80 −11
Merged

audio: base_fw: add platform layer to hw_config data #9012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions src/audio/base_fw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <sof/ut.h>
#include <sof/tlv.h>
#include <ipc4/base_fw.h>
#include <ipc4/base_fw_platform.h>
#include <ipc4/pipeline.h>
#include <ipc4/logging.h>
#include <sof_versions.h>
Expand Down Expand Up @@ -129,6 +130,7 @@ static int basefw_config(uint32_t *data_offset, char *data)
static int basefw_hw_config(uint32_t *data_offset, char *data)
{
struct sof_tlv *tuple = (struct sof_tlv *)data;
uint32_t plat_data_offset = 0;
uint32_t value;

tlv_value_uint32_set(tuple, IPC4_CAVS_VER_HW_CFG, HW_CFG_VERSION);
Expand All @@ -147,19 +149,11 @@ static int basefw_hw_config(uint32_t *data_offset, char *data)
tlv_value_uint32_set(tuple, IPC4_TOTAL_PHYS_MEM_PAGES_HW_CFG, value);

tuple = tlv_next(tuple);
tlv_value_uint32_set(tuple, IPC4_HP_EBB_COUNT_HW_CFG, PLATFORM_HPSRAM_EBB_COUNT);

tuple = tlv_next(tuple);
/* 2 DMIC dais */
value = DAI_NUM_SSP_BASE + DAI_NUM_HDA_IN + DAI_NUM_HDA_OUT +
DAI_NUM_ALH_BI_DIR_LINKS + 2;
tlv_value_uint32_set(tuple, IPC4_GATEWAY_COUNT_HW_CFG, value);

tuple = tlv_next(tuple);
tlv_value_uint32_set(tuple, IPC4_LP_EBB_COUNT_HW_CFG, PLATFORM_LPSRAM_EBB_COUNT);
/* add platform specific tuples */
platform_basefw_hw_config(&plat_data_offset, (char *)tuple);
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't this break non-Intel platforms? Do you need a weak version? Or is this all Intel-only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lyakh This is just like all functions in platform.h, all platforms need to provide one. Let me refresh the series with "posix" platform support, I missed that and that causes some of the CI checks to fail.

The one design open I had (and still have to some extent), is whether to make this a "bigger thing", e.g. something like a "platform/ipc-data.h". But I ended up not doing that and just putting thees in top-level platform.h as the longterm goal is to minimize platform interface (drop all platform/clk.h, platfrom/dma.h as these can all be handled via Zephyr interfaces, we just can't drop them yet as some platfroms still use XTOS). Especially as @lgirdwood is still not convinced we need platform.h at all (see #7248).

One alternative would be just to add src/ipc/ipc4/basefw_intel.c and not try to abstract this now. I'd still want to keep base_fw.c clean of vendor specific parts, so it is easier to use IPC4 for all SOF targets.


tuple = tlv_next(tuple);
*data_offset = (int)((char *)tuple - data);
*data_offset = (int)((char *)tuple - data) + plat_data_offset;

return 0;
}
Expand Down
25 changes: 25 additions & 0 deletions src/include/ipc4/base_fw_platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/* SPDX-License-Identifier: BSD-3-Clause
*
* Copyright(c) 2024 Intel Corporation.
*
* Author: Kai Vehmanen <[email protected]>
*/

/**
* \file include/ipc4/base_fw_platform.h
* \brief Platform specific IPC4 base firmware functionality.
*/

#ifndef __SOF_IPC4_BASE_FW_PLATFORM_H__
#define __SOF_IPC4_BASE_FW_PLATFORM_H__

/**
* \brief Platform specific routine to add data tuples to HW_CONFIG
* structure sent to host via IPC.
* \param[out] data_offset data offset after tuples added
* \parma[in] data pointer where to add new entries
* \return 0 if successful, error code otherwise.
*/
int platform_basefw_hw_config(uint32_t *data_offset, char *data);

#endif
32 changes: 32 additions & 0 deletions src/platform/intel/ace/lib/base_fw_platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// SPDX-License-Identifier: BSD-3-Clause
//
// Copyright(c) 2024 Intel Corporation.
//
// Author: Kai Vehmanen <[email protected]>

#include <rtos/string.h>
#include <sof/tlv.h>
#include <sof/lib/dai.h>
#include <ipc4/base_fw.h>

int platform_basefw_hw_config(uint32_t *data_offset, char *data)
{
struct sof_tlv *tuple = (struct sof_tlv *)data;
uint32_t value;

tlv_value_uint32_set(tuple, IPC4_HP_EBB_COUNT_HW_CFG, PLATFORM_HPSRAM_EBB_COUNT);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To @lgirdwood your question: Not the best example of platform specific data in SOF, but let's use this as example. This is a tuple only needed on Intel platforms (e.g. Linux SOF driver has no need for this info), so even if we could get the data from Zephyr interfaces (or DTS), the decision to put this piece of data into a HW_CONFIG IPC response, is something only SOF built for Intel platforms should do.

Currently in SOF, the way to trigger build target specific behaviour of SOF is via the platform name -- this is our top-level build target. The confusing bit is that in XTOS, platform was also used to abstract "soc" and "board", but these are not used when SOF is built with Zephyr.

So that brings as back to IPC4 base_fw variation. As SOF is built separately for e..g ACE and NXP platforms, we can use this choice (made at built time) to variate the base_fw implementation in a more scalable way.

Things that are purely hw properties, like CPU core count, can be handled completely in generic code as 1) this information is added to HW_CONFIG response always, 2) the information can be queried with standard Zephyr interface.

Copy link
Member

Choose a reason for hiding this comment

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

We should put this in src/ipc/ipc4/intel.c and export several C functions that can be called where vendor data has to be installed (otherwise the are all static inline vendor_add_memory() and compile out).
The input data for IPC4_HP_EBB_COUNT_HW_CFG, PLATFORM_HPSRAM_EBB_COUNT has to come from DT, but this could be a secondary PR later on (we could have a comment saying this today).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lgirdwood That's what this patch now does, ipc4/intel.c code in src/platform/intel/ace/lib/base_fw_platform.c and generic stub in src/platform/posix/base_fw_platform.c.

We can keep this simpler and put this in ipc4/intel.c, but that means we need to maintain a platform/vendor layer in base_fw.h/base_fw_vendor.h and have ifdefs in src/ipc. Current PR keeps the ifdefs out of main src/ipc and ipc/audio trees.

I'm ok with both and can update the PR. There is more code and data to move once the approach is agreed, hw_config is just a start).


tuple = tlv_next(tuple);
/* 2 DMIC dais */
value = DAI_NUM_SSP_BASE + DAI_NUM_HDA_IN + DAI_NUM_HDA_OUT +
DAI_NUM_ALH_BI_DIR_LINKS + 2;
tlv_value_uint32_set(tuple, IPC4_GATEWAY_COUNT_HW_CFG, value);

tuple = tlv_next(tuple);
tlv_value_uint32_set(tuple, IPC4_LP_EBB_COUNT_HW_CFG, PLATFORM_LPSRAM_EBB_COUNT);

tuple = tlv_next(tuple);
*data_offset = (int)((char *)tuple - data);

return 0;
}
15 changes: 15 additions & 0 deletions src/platform/posix/base_fw_platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: BSD-3-Clause
//
// Copyright(c) 2024 Intel Corporation.
//
// Author: Kai Vehmanen <[email protected]>

#include <stdint.h>
#include <ipc4/base_fw_platform.h>

int platform_basefw_hw_config(uint32_t *data_offset, char *data)
{
*data_offset = 0;

return 0;
}
3 changes: 3 additions & 0 deletions zephyr/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ if (CONFIG_SOC_SERIES_INTEL_CAVS_V25)
# Platform sources
zephyr_library_sources(
${SOF_PLATFORM_PATH}/intel/cavs/platform.c
${SOF_PLATFORM_PATH}/intel/ace/lib/base_fw_platform.c
${SOF_PLATFORM_PATH}/tigerlake/lib/clk.c
lib/pm_runtime.c
lib/clk.c
Expand All @@ -183,6 +184,7 @@ if (CONFIG_SOC_SERIES_INTEL_ADSP_ACE)
# Platform sources
zephyr_library_sources(
${SOF_PLATFORM_PATH}/intel/ace/platform.c
${SOF_PLATFORM_PATH}/intel/ace/lib/base_fw_platform.c
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally you would use the oldest platform's name, so intel/cavs/lib/base_fw_platform.c for both cavs and ace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ujfalusi Ack. If I can tune this in follow-up? I anticipate the ace one to be the full implementation and thus I put the main implementation here under ace/lib. For cavs we can probably go with a minimal implementation even, to keep the Linux SOF driver happy, but just wanted some example for cavs here as well for this PR.

lib/pm_runtime.c
lib/clk.c
lib/dma.c
Expand Down Expand Up @@ -345,6 +347,7 @@ zephyr_library_sources_ifdef(CONFIG_ZEPHYR_POSIX
${SOF_PLATFORM_PATH}/posix/dai.c
${SOF_PLATFORM_PATH}/posix/ipc.c
${SOF_PLATFORM_PATH}/posix/posix.c
${SOF_PLATFORM_PATH}/posix/base_fw_platform.c
)

zephyr_library_sources_ifdef(CONFIG_LIBRARY
Expand Down