[BUG] make CONFIG_SOF_ZEPHYR_STRICT_HEADERS the default

[kv2019i]

Describe the bug/enhancement
When rtos include layer was added in #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
• xtos/sof/compiler_attributes.h -- zephyr/toolchain.h defines some but not all definitions SOF codebase needs, fork the definitions? rtos: branch compiler_attributes.h for XTOS and Zephyr #9489
• xtos/sof/compiler_info.h
• xtos/sof/trace/ -- no blockers, no use in Zephyr
• xtos/sof/list.h -- why in RTOS layer, this seems like a SOF specific SW API that is used very widely xtos: move list.h to application interface #9466
• xtos/sof/lib/agent.h -- not used in Zephyr builds, any need in the future? xtos: move sof/lib/agent.h to application interface #9490
• xtos/sof/lib/cpu.h -- no blockers, can be replaced with Zephyr
• xtos/sof/lib/dai.h -- no blockers, can be replaced rtos: add Zephyr implementation of sof/lib/dai.h #9457
• xtos/sof/lib/dma.h -- a complex case, lot of definitions still used plus lot of unncessary stuff for Zephyr builds
• xtos/sof/lib/io.h -- no blockers, can be replaced
• xtos/sof/lib/mailbox.h -- complex case, dependencies to SRAM_INBOX/OUTPUT definitions in platform layer
• xtos/sof/lib/memory.h -- all definitions in platform layer, in intermedia, Zephyr version can use platform layer still
• xtos/sof/lib/mm_heap.h -- medium complexity, mostly unused in Zephyr but some key definitions zephyr: sof/lib/mm_heap.h added Zephyr version #9467
• 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 xtos: move sof/lib/perf_cnt.h to application interface #9494
• xtos/sof/lib/pm_runtime.h -- medium complexity, mostly unnecessary in Zephyr but some usage remains for Intel cAVS2.5 zephyr: decouple from platform pm_runtime.h interface #9495
• xtos/sof/lib/shim.h -- no blockers, can be replaced, directly uses platform layer platform/xtos: remove shim.h interface as it's no longer needed #9461
• xtos/rtos/alloc.h -- widely used interface in SOF, has audio extensions so no direct replacement in Zephyr
• xtos/rtos/atomic.h
• xtos/rtos/bit.h
• xtos/rtos/cache.h
• xtos/rtos/clk.h
• xtos/rtos/idc.h
• xtos/rtos/init.h
• xtos/rtos/interrupt.h
• xtos/rtos/kernel.h
• xtos/rtos/mutex.h zephyr: add rtos/mutex.h #9488
• xtos/rtos/panic.h
• xtos/rtos/sof.h
• xtos/rtos/spinlock.h
• xtos/rtos/string.h
• xtos/rtos/string_macro.h
• xtos/rtos/symbol.h
• xtos/rtos/task.h
• xtos/rtos/timer.h
• xtos/rtos/wait.h

Screenshots or console output

[kv2019i]

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

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

[kv2019i]

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

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

[LaurentiuM1234]

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.

[kv2019i]

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.

[LaurentiuM1234]

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.