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

kv2019i · 2024-04-09T10:20:55Z

First patch to address #8391 - more to come later.

Some of the HW_CONFIG fields are platform specific and cannot be filled with generic means in generic code.

Handle this data via a simple platform abstraction to hook platform specific code to HW_CONFIG handling. Move out existing code for Intel cAVS and ACE platforms.

kv2019i · 2024-04-09T10:23:13Z

@dbaluta @iuliana-prodan this might affect NXP targets for IPC4 not yet in main (but in long term make it easier for you). I'm planning to move more plat spescific data out from base_fw.c but wanted to share this earlier to get feedback on the approach.

lgirdwood

LGTM, but we long term we should be removing the platform specific files as Zephyr will abstract platform. This looks IPC specific.

kv2019i · 2024-04-10T07:43:42Z

@lgirdwood wrote:

LGTM, but we long term we should be removing the platform specific files as Zephyr will abstract platform. This looks IPC specific.

Ack, we are trackign this is #7248. To me it seems we have a layer of data that is above the RTOS layer and is different between vendors. Currently we use the "platform" layer for this in SOF. You pass a "platform name" to build script Personally I think we need the ability to vary the build contents. Some (most) of this is in terms of Kconfig settings and DTS overrides, but e.g. for the IPC4 tuples, we need content that requires custom code (at least for now). The platform layer in SOF seems like a natural fit.

kv2019i · 2024-04-10T07:46:50Z

Issue in CI on one of the Intel platforms, tagging with DNM while debugging. Please review the concept still.

kv2019i · 2024-04-10T08:35:58Z

V3:

added similar change for FW_CONFIG (easier probably to review in one go as FW_CONFIG has more vendor/platform specific bits)
fixed a bug in V2
remove unneeded ifdefs on IPC version in platform files

kv2019i · 2024-04-10T08:42:25Z

V4:

one more bugfix to V3 (sorry for the churn)

lyakh

something is weird here... same commit twice?..

lyakh · 2024-04-12T06:35:20Z

src/audio/base_fw.c

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


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

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

my oversight - the description looked very similar, I missed, that the two commits change different functions

kv2019i · 2024-04-12T08:45:08Z

@thesofproject/nxp Any thoughts on the high-level concept? So I guess the options would be:

this appraoch, extend platform to cover vendor specific bits of base_fw.c
not touch and add to platform.h, but add some new base_fw_intel.c
keep current base_fw.c and incrementally move pieces of Intel-specific code to Zephyr when possible (I'm personally not convinced we can move all, @lgirdwood thinks we can move more)

I think some abraction is needed pretty quick to keep base_fw.c clean (e..g we have things like #9031 coming all the time).

kv2019i · 2024-04-12T09:07:55Z

V5:

implement the new interface for "posix" platform

kv2019i · 2024-04-12T10:17:30Z

V6:

fix build warning with SDK gcc version

kv2019i · 2024-04-16T10:36:42Z

Changing to draft. It seems even with a fresh cycle of cleanup to the platform layer, there is still quite a bit of usage left in the tree, so adding these definitions to main platform.h will not scale. Also, I see many new PRs coming to base_fw.c, so I think the platform-specific layer will have to be grown later. IOW, we need to add some more explicit interface where to put platform specific basefw stuff (versus just putting them to platform.h).

lgirdwood · 2024-04-16T11:44:38Z

Changing to draft. It seems even with a fresh cycle of cleanup to the platform layer, there is still quite a bit of usage left in the tree, so adding these definitions to main platform.h will not scale. Also, I see many new PRs coming to base_fw.c, so I think the platform-specific layer will have to be grown later. IOW, we need to add some more explicit interface where to put platform specific basefw stuff (versus just putting them to platform.h).

Depends what you mean by platform specific code, SOF codebase needs to be generic and Zephyr will define the platform.

kv2019i · 2024-04-16T12:18:52Z

V7:

moved the new interface to a separate header (versus adding to main platform.h)
add implementations for all IPC4 targets
dropped the FW_CONFIG support for now, will add back once we have agreement on the approach
UPDATE: stub build works for me locally, WIP to figure out why fails in github workflow

FYI @thesofproject/nxp -- for your initial IPC4 work, you could link against posix base_fw_platform.c and get a stubbed implementation. This does not currently leave anything out e.g. needed by Linux SOF driver for IPC4.

kv2019i · 2024-04-16T12:37:18Z

src/platform/intel/ace/lib/base_fw_platform.c

+	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);


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.

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

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

kv2019i · 2024-04-16T21:29:01Z

Finally figured out the mismatch between github workflow and my local build for fuzzer build, even did an OS upgrade to local build machine to match the github workflow. In the end the rootcause I was testing the pushed version locally, while the github flow was testing against newer SOF with #8737 . That adds constructs to base_fw.h that no longer build with the posix target, and thus my 9012 fails.

Some of the IPC4 HW_CONFIG fields are platform specific and cannot be filled with generic code. Handle this functionality by separating platform specific parts of base_fw to a new base_fw_platform.h interface. Move out existing code for Intel cAVS and ACE platforms. Add a new stub implementation for posix platform. This posix stub can be also used as a starting point when adding IPC4 support to new platforms. This platform construct can be later used to move out other vendor and platform data out from base_fw.c. Signed-off-by: Kai Vehmanen <[email protected]>

kv2019i · 2024-04-16T21:45:26Z

V8:

fix the stub/fuzzer build when combined with telemetry: systick info #8737
may need rework if we go for simpler src/ipc/ipc4/inter.c (and not use src/platform for this)

ujfalusi

@kv2019i, the kernel is not using the HW_CONFIG, yet, probably ever, but I guess we will need similar platform dependent handling?
Do we have indication that the HW_CONFIG is Intel or posix or other vendor's to parse?

ujfalusi · 2024-04-17T11:46:33Z

zephyr/CMakeLists.txt

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


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

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

jsarha

LGTM

jsarha · 2024-04-17T16:25:44Z

Just occurred to me, that on basefw_set side the only supported tlv type looks pretty Intel specific too. Does not happen to be this PR, but it would make sense to move that to platform specific source file too.

kv2019i · 2024-04-18T08:27:33Z

@jsarha wrote:

Just occurred to me, that on basefw_set side the only supported tlv type looks pretty Intel specific too. Does not happen to be this PR, but it would make sense to move that to platform specific source file too.

Ack, a good idea. Also, the function signature could be improved. I now just took the existing code as-is from basefw so it is easier to remove the new layout, but I think this could be improved as well. These functions are now passed a "char *" pointer with no knowledge of size and it's very unobvious how it is handled the the functions don't overwite allocated buffer for the IPC response.

kv2019i requested review from tmleman and softwarecki April 9, 2024 10:20

kv2019i requested review from lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners April 9, 2024 10:20

kv2019i force-pushed the 202404-basefw-plat-hwconfig branch from b896576 to 3a658e0 Compare April 9, 2024 11:25

lgirdwood approved these changes Apr 9, 2024

View reviewed changes

kv2019i mentioned this pull request Apr 10, 2024

[FEATURE][Zephyr] Clarify expectations of platform.h for Zephyr SOF builds #7248

Open

kv2019i added the DNM Do Not Merge tag label Apr 10, 2024

kv2019i force-pushed the 202404-basefw-plat-hwconfig branch from 3a658e0 to 1bdef88 Compare April 10, 2024 08:34

kv2019i removed the DNM Do Not Merge tag label Apr 10, 2024

kv2019i force-pushed the 202404-basefw-plat-hwconfig branch from 1bdef88 to 4ed4eb8 Compare April 10, 2024 08:41

lyakh previously requested changes Apr 12, 2024

View reviewed changes

kv2019i force-pushed the 202404-basefw-plat-hwconfig branch from 4ed4eb8 to 8284a9d Compare April 12, 2024 09:07

kv2019i force-pushed the 202404-basefw-plat-hwconfig branch from 8284a9d to 0319566 Compare April 12, 2024 10:18

lgirdwood approved these changes Apr 12, 2024

View reviewed changes

kv2019i marked this pull request as draft April 16, 2024 10:34

kv2019i requested a review from dnikodem April 16, 2024 10:34

kv2019i force-pushed the 202404-basefw-plat-hwconfig branch from 0319566 to 4c5808c Compare April 16, 2024 12:12

kv2019i marked this pull request as ready for review April 16, 2024 12:21

kv2019i requested review from marc-hb, RanderWang, marcinszkudlinski and pblaszko as code owners April 16, 2024 12:21

kv2019i commented Apr 16, 2024

View reviewed changes

marc-hb removed their request for review April 16, 2024 20:40

kv2019i force-pushed the 202404-basefw-plat-hwconfig branch from 4c5808c to 478757b Compare April 16, 2024 21:45

lgirdwood approved these changes Apr 17, 2024

View reviewed changes

ujfalusi reviewed Apr 17, 2024

View reviewed changes

jsarha approved these changes Apr 17, 2024

View reviewed changes

lgirdwood merged commit 14c4e86 into thesofproject:main Apr 17, 2024
44 of 45 checks passed

kv2019i mentioned this pull request Apr 18, 2024

Audio: basefw: Implemented ipc4 libraries info get #9031

Merged

This was referenced Apr 18, 2024

audio: base_fw: add platform layer to fw_config data #9059

Closed

audio: base_fw: move platform specific code from base_fw.c #9060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kv2019i commented Apr 9, 2024 •

edited

Loading

kv2019i commented Apr 9, 2024

lgirdwood left a comment

kv2019i commented Apr 10, 2024

kv2019i commented Apr 10, 2024

kv2019i commented Apr 10, 2024

kv2019i commented Apr 10, 2024

lyakh left a comment

lyakh Apr 12, 2024

kv2019i Apr 12, 2024

kv2019i commented Apr 12, 2024

kv2019i commented Apr 12, 2024

kv2019i commented Apr 12, 2024

kv2019i commented Apr 16, 2024

lgirdwood commented Apr 16, 2024

kv2019i commented Apr 16, 2024 •

edited

Loading

kv2019i Apr 16, 2024

lgirdwood Apr 16, 2024

kv2019i Apr 16, 2024

kv2019i commented Apr 16, 2024

kv2019i commented Apr 16, 2024

ujfalusi left a comment

ujfalusi Apr 17, 2024

kv2019i Apr 17, 2024

jsarha left a comment

jsarha commented Apr 17, 2024

kv2019i commented Apr 18, 2024

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

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

Conversation

kv2019i commented Apr 9, 2024 • edited Loading

kv2019i commented Apr 9, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

kv2019i commented Apr 10, 2024

kv2019i commented Apr 10, 2024

kv2019i commented Apr 10, 2024

kv2019i commented Apr 10, 2024

lyakh left a comment

Choose a reason for hiding this comment

lyakh Apr 12, 2024

Choose a reason for hiding this comment

kv2019i Apr 12, 2024

Choose a reason for hiding this comment

kv2019i commented Apr 12, 2024

kv2019i commented Apr 12, 2024

kv2019i commented Apr 12, 2024

kv2019i commented Apr 16, 2024

lgirdwood commented Apr 16, 2024

kv2019i commented Apr 16, 2024 • edited Loading

kv2019i Apr 16, 2024

Choose a reason for hiding this comment

lgirdwood Apr 16, 2024

Choose a reason for hiding this comment

kv2019i Apr 16, 2024

Choose a reason for hiding this comment

kv2019i commented Apr 16, 2024

kv2019i commented Apr 16, 2024

ujfalusi left a comment

Choose a reason for hiding this comment

ujfalusi Apr 17, 2024

Choose a reason for hiding this comment

kv2019i Apr 17, 2024

Choose a reason for hiding this comment

jsarha left a comment

Choose a reason for hiding this comment

jsarha commented Apr 17, 2024

kv2019i commented Apr 18, 2024

kv2019i commented Apr 9, 2024 •

edited

Loading

kv2019i commented Apr 16, 2024 •

edited

Loading