sof [BUG] make CONFIG_SOF_ZEPHYR_STRICT

Describe the bug/enhancement When rtos include layer was added in https://github.com/thesofproject/sof/pull/6161 , CONFIG_SOF_ZEPHYR_STRICT_HEADERS was added as a transitory tool.

This is still in place and can cause confusing issues as both XTOS and Zephyr headers are added to the include path by default.

To Reproduce Build target, check .config

Reproduction Rate 100%

Expected behavior OS include paths not mixed in build.

Impact Slows down development of new features.

Environment SOF main as of 2024-04-09

Work breakdown

Following subtasks identified (breakdown started in v2.11 cycle):

[ ] xtos headers used to hook in memory layout definitions in platform layer, platform/lib/memory.h for SRAM_INBOX/OUTBOUT etl al
[x] xtos/sof/compiler_attributes.h -- zephyr/toolchain.h defines some but not all definitions SOF codebase needs, fork the definitions? https://github.com/thesofproject/sof/pull/9489
[x] xtos/sof/compiler_info.h
[x] xtos/sof/init.h -- https://github.com/thesofproject/sof/pull/9592
[x] xtos/sof/trace/ -- no blockers, no use in Zephyr
[x] xtos/sof/trace/preproc.h -- used in SOF trace.h still -- https://github.com/thesofproject/sof/pull/9603
[x] xtos/sof/list.h -- why in RTOS layer, this seems like a SOF specific SW API that is used very widely https://github.com/thesofproject/sof/pull/9466
[x] xtos/sof/lib/agent.h -- not used in Zephyr builds, any need in the future? https://github.com/thesofproject/sof/pull/9490
[x] xtos/sof/lib/cpu.h -- no blockers, can be replaced with Zephyr
[x] xtos/sof/lib/dai.h -- no blockers, can be replaced https://github.com/thesofproject/sof/pull/9457
[x] xtos/sof/lib/dma.h -- a complex case, lot of definitions still used plus lot of unncessary stuff for Zephyr builds https://github.com/thesofproject/sof/pull/9560
[x] xtos/sof/lib/io.h -- no blockers, can be replaced
[x] xtos/sof/lib/mailbox.h -- complex case, dependencies to SRAM_INBOX/OUTPUT definitions in platform layer https://github.com/thesofproject/sof/pull/9562
[x] xtos/sof/lib/memory.h -- all definitions in platform layer, in intermedia, Zephyr version can use platform layer still https://github.com/thesofproject/sof/pull/9552
[x] xtos/sof/lib/mm_heap.h -- medium complexity, mostly unused in Zephyr but some key definitions https://github.com/thesofproject/sof/pull/9467
[x] xtos/sof/lib/perf_cnt.h -- problematic, why is this in the RTOS layer, shouldn't this be agnostic SOF service? move back to app layer https://github.com/thesofproject/sof/pull/9494
[x] xtos/sof/lib/pm_runtime.h -- medium complexity, mostly unnecessary in Zephyr but some usage remains for Intel cAVS2.5 https://github.com/thesofproject/sof/pull/9495
[x] xtos/sof/lib/shim.h -- no blockers, can be replaced, directly uses platform layer https://github.com/thesofproject/sof/pull/9461
[x] xtos/rtos/alloc.h -- widely used interface in SOF, has audio extensions so no direct replacement in Zephyr
[x] xtos/rtos/atomic.h
[x] xtos/rtos/bit.h
[x] xtos/rtos/cache.h
[x] xtos/rtos/clk.h
[x] xtos/rtos/idc.h
[x] xtos/rtos/init.h
[x] xtos/rtos/interrupt.h
[x] xtos/rtos/kernel.h
[x] xtos/rtos/mutex.h https://github.com/thesofproject/sof/pull/9488
[x] xtos/rtos/panic.h
[x] xtos/rtos/sof.h
[x] xtos/rtos/spinlock.h
[x] xtos/rtos/string.h
[x] xtos/rtos/string_macro.h
[x] xtos/rtos/symbol.h
[x] xtos/rtos/task.h
[x] xtos/rtos/timer.h
[x] xtos/rtos/wait.h
[ ] make CONFIG_SOF_ZEPHYR_STRICT_HEADERS=y at least for one SOF target
[ ] CONFIG_SOF_ZEPHYR_STRICT_HEADERS=y for all SOF Zephyr targets

Screenshots or console output

Apr 09 '24 15:04 kv2019i

Not sure I can tackle this in time for v2.10, but assigning for myself, as nobody else is assigned.

May 28 '24 10:05 kv2019i

Update for v2.10: spent some time on this trying to make one Intel target build in strict headers mode, but could not complete the task. There is still quite a lot of use of e..g sof/lib/mailbox.h and sof/lib/memory.h in surprising places in audio code that needs to be cleaned up first. Also we have some SOF side functions list SOF list.h and perf_cnt -- for these, move to some common header might be easier. For now moving to v2.11 -- might be able to complete some of this work in June, but won't backport to 2.10 release (as no functional benefit for the release).

Jun 05 '24 15:06 kv2019i

Work breakdown added to the issue description with already done worked marked as done.

Sep 10 '24 13:09 kv2019i

2.11 cutoff day. Some progress with this. Work breakdown has been done and shared as a task list in bug description. At end of 2.11 cycle, we have 23 out of 35 identified tasks completed. Pushing the remaining work to v2.12.

Sep 13 '24 07:09 kv2019i

Just thinking out loud here regarding xtos/sof/lib/mailbox.h: would it be better to define the mailbox regions inside the DT overlays and have a generic mailbox.h defining all the mailbox macros based on the DT?

i.e: something like:

chosen {
    sof,mbox-dspbox = &dspbox;
    sof,mbox-hostbox = &hostbox;
    sof,mbox-stream = &stream
};

sof-mailbox {
    dspbox: memory@cafebabe {
        reg = <0xcafebabe 0x1000>;
    };

   hostbox: memory@deadbabe {
        reg = <0xdeadbabe 0x1000>;
    };

   stream: memory@babebabe {
        reg = <0xbabebabe 0x1000>;
   };
};

The new mailbox header would look something like:

#define BUILD_FAIL_IF_MBOX_NOT_DEFINED(mbox_chosen)
    BUILD_ASSERT(DT_NODE_EXISTS(mbox_chosen), "mandatory mbox <insert name here via stringify> not defined"))

#define MAILBOX_HOSTBOX_BASE DT_REG_ADDR(DT_CHOSEN(sof_mbox_hostbox))
#define MAILBOX_HOSTBOX_SIZE DT_REG_SIZE(DT_CHOSEN(sof_mbox_hostbox))

... define all required macros this way ...

/* these should help ppl figure out if they're missing some definitions */
BUILD_FAIL_IF_MBOX_NOT_DEFINED(sof_mbox_hostbox)
BUILD_FAIL_IF_MBOX_NOT_DEFINED(sof_mbox_dspbox)
BUILD_FAIL_IF_MBOX_NOT_DEFINED(sof_mbox_stream)

/* the following chunk would allow platforms to overwrite the macros and add their custom data if need be */
#ifdef CONFIG_PLATFORM_HAS_CUSTOM_MBOX_DATA
/* TODO: include platform-specific header here */
#endif /* CONFIG_PLATFORM_HAS_CUSTOM_MBOX_DATA */

One obvious problem that I see with this is that the messages from the failed BUILD_ASSERT statements are kinda ugly but if you scroll through them the warning string should tell you exactly what you're missing.

Also, I'm not sure how well this would play out on Intel platforms. I've been messing around with this idea on nxp platforms and I think it might work out. Also, not sure how good of an idea is to add application-specific chosen nodes....

This would also get rid of having to add the platform-specific mailbox.h unless you really need to.

Sep 19 '24 12:09 LaurentiuM1234

Thanks @LaurentiuM1234 . mailbox.h is one of the more complex cases. With many platform interfaces, once you move to use native Zephyr drivers, most of the platform interface usage gets removed automatically (makes sense, talking to hw should go via drivers). But with mailbox we actually have quite a bit of use remaining in generic SOF code.

I think the DT overlay would work. In fact for Intel platforms, the mailbox addresses are actually already defined via DT entries in soc/src/platform/meteorlake/include/platform/lib/memory.h: DT_REG_ADDR(DT_PHANDLE(MEM_WINDOW_NODE(n), memory)) -> WIN_BASE -> SRAM_FOO_BASE -> MAILBOX_FOO_BASE

That's a good question whether mandating a SOF side DT overlay adds value anymore or do we get same result by leaving a minimal platform layer for mailbox.h. And for e.g. Intel this turns into simple mapping to a DT entry already defined in Zephyr that is same for all Intel platforms.

Another approach might be to improve and scale up the "adsp_host_ipc" DT interface used by zephyr/soc/intel/intel_adsp/common/ipc.c . This is essentially the driver interface to talk to host and I think for longterm it would be better to move all current mailbox use to this interface (or maybe have two, one for IPC send/receiv and one to write to shared debug area). Majority of mailbox.h use is stll just to implement IPC to host, with a smaller set of use for debug (and some could clearly be replaced with other Zephyr mechanisms). This would also make it easier to use SOF on platforms that don't have a similar mapped memory between host and the DSP. E.g. you send IPC via ipc_send_message() Zephyr call, but implementation details are driver specific.

Sep 20 '24 09:09 kv2019i

Another approach might be to improve and scale up the "adsp_host_ipc" DT interface used by zephyr/soc/intel/intel_adsp/common/ipc.c . This is essentially the driver interface to talk to host and I think for longterm it would be better to move all current mailbox use to this interface (or maybe have two, one for IPC send/receiv and one to write to shared debug area). Majority of mailbox.h use is stll just to implement IPC to host, with a smaller set of use for debug (and some could clearly be replaced with other Zephyr mechanisms). This would also make it easier to use SOF on platforms that don't have a similar mapped memory between host and the DSP. E.g. you send IPC via ipc_send_message() Zephyr call, but implementation details are driver specific.

I like the idea of using a driver for the IPC stuff. One aparent flaw I've noticed with our current mailbox implementation is that it's not really MMU friendly. On imx93 (ARM64 architecture, MMU enabled) we had to deal with this by adding some static phys-virt mappings inside mmu_regions.c. I'm thinking that, ideally, this shouldn't be required. Instead, we should have an IPC driver handling this stuff via device_map calls on the mbox regions. Additionally, this would make it easier to port SOF to new architectures if need be.

If we are to add such a driver in Zephyr (or extend the one used by Intel) assuming we're going to be using Zephyr's mbox API, we also need to switch our mailbox unit driver (talking about the imx one) to Zephyr (we already have 2 of them, but not sure how "complete" they are - @iuliana-prodan may be able to comment on this). Nevertheless, it's some extra work, but it must be done at some point.

Sep 20 '24 10:09 LaurentiuM1234

Added: xtos/sof/trace/preproc.h

Oct 09 '24 14:10 kv2019i

With #9627 all work for this done, marking this as closed.

Oct 31 '24 08:10 kv2019i

@LaurentiuM1234 wrote:

@kv2019i wrote:

Another approach might be to improve and scale up the "adsp_host_ipc" DT interface used by zephyr/soc/intel/ intel_adsp/common/ipc.c . This is essentially the driver interface to talk to host and I think for longterm it would be

I like the idea of using a driver for the IPC stuff. One aparent flaw I've noticed with our current mailbox implementation is that it's not really MMU friendly. On imx93 (ARM64 architecture, MMU enabled) we had to deal with this by adding some static phys-virt mappings inside mmu_regions.c. I'm thinking that, ideally, this shouldn't be required. Instead, we

I filed now a proper enhancement ticket for this https://github.com/thesofproject/sof/issues/9697 Rest of platform layer starts to be simplified now -- a few tough nuts to crack left, mailbox.h is one of them.

Nov 29 '24 17:11 kv2019i

[BUG] make CONFIG_SOF_ZEPHYR_STRICT_HEADERS the default