[LaurentiuM1234]

This PR introduces all the necessary changes for running SOF using Zephyr and Jailhouse on i.MX93's A55 core.

Will update #7192 to contain a short description of our solution and all of its quirks. (EDIT: this is now up)

EDIT: Depends on Zephyr PR: zephyrproject-rtos/zephyr#56713

[paulstelian97]

Nice work, there's a few comments (the commit order one you need to deal with, the others are discussions/opinions).

And while it does look sketchy in some places, some of these things can be fixed later on too. I tried to point out what needs the attention right now.

Do we have a proof of concept (this actually working) as of this time?

[LaurentiuM1234]

@paulstelian97 Thanks for the feedback !

Regarding this:

Do we have a proof of concept (this actually working) as of this time?

I've made a status update here: #7192. Please check out my last comment, section STATUS. TL; DR: Yup this works.

[paulstelian97]

The other comment where you should never have a commit where build breaks with any of the defconfigs still applies. You don't want to break bisection!

[LaurentiuM1234]

The other comment where you should never have a commit where build breaks with any of the defconfigs still applies. You don't want to break bisection!

Thanks for pointing this out. Just tried building SOF with Zephyr for imx8 and tgl while incrementally adding all the patches here. Seems fine. As for i.MX93, this shouldn't be built anyways w/o all these patches.

[lyakh]

is this referring to idc_msg? Then maybe remove the empty line below? Or to the whole file?

[LaurentiuM1234]

It's referring to the whole file. Same goes for interrupt.h, dai.h and trace.h. Apart from the fact that they help silence compilation errors they serve no purpose in i.MX93's case ATM.

[paulstelian97]

The idc file specifically shouldn't really have been needed for any of the i.MX platforms, as IDC is Intel specific, and future multi-core DSPs from others will likely use different inter-core communication standards.

A future refactor should make sure to remove this file being needed from all non-CAVS platforms.

[kv2019i]

Ack @paulstelian97 . With Zephyr we don't really need IDC as a public interface in SOF anymore. It's now used to send messages between DSP cores, but with Zephyr we can now use Zephyr OS primitives (threads, semaphores, locks) to implement this.

It's just this cleanup will take time as we still have non-Zephyr platforms in SOF main. See #7248 for background.

[lgirdwood]

ack - cleanup will remove this incrementally.

[lyakh]

Maybe we could get rid of these ifdefs: either define a new function here, or maybe even (ab)use the boot_message parameter - it isn't used anywhere otherwise, but then you'd have to modify all implementations of platform_boot_complete()

[LaurentiuM1234]

ACK. Should also probably introduce a new function in wrapper.c. The ifdefs will still have to be used in said functions.

[lyakh]

hm, eventually maybe would be better to add this file to Zephyr, even if it's empty

[LaurentiuM1234]

This will probably have to be introduced to Zephyr at some point (maybe when drivers are moved there?). For now, if you think those defines are not deal-breaking I'd rather have those here than introducing an empty file to Zephyr.

EDIT: I can see how this would become bad if more arm64-based platforms w/o soc.h are introduced.

[dbaluta]

This is not imx specific but arm64 specific. So, I wonder if we could only introduce CONFIG_ARM64 and somehow figure out that we are on imx arm64 by checking for CONFIG_IMX && CONFIG_ARM64?

[lgirdwood]

Fyi @nashif has a Zephyr Cache API in the works that will abstact the arch cache ops with a generic API..

[LaurentiuM1234]

You're absolutely right. In fact, CONFIG_IMX_ARM64 should be removed completely and replaced by CONFIG_ARM64 (which exists in Zephyr). Possibly, an exception to this removal should be zephyr/lib/alloc.c where the heap is defined but since i.MX is the only board using ARM64 architecture we might as well replace this one too.

[lgirdwood]

@LaurentiuM1234 might be worths splitting this PR out a little, per feature for easier review.

[LaurentiuM1234]

@LaurentiuM1234 might be worths splitting this PR out a little, per feature for easier review.

Agreed. With the removal of CONFIG_IMX_ARM64, I can definitely see some changes worth submitting in other PRs (e.g: the cache API-related one).

[LaurentiuM1234]

UPDATE
Now that I've broken down this PR into several PRs and the Zephyr PR has already been merged will mark this PR as ready for review. Also, this PR now only introduces platform support for i.MX93.

Please see #7192 for PR dependency graph.

[paulstelian97]

No complaints (thus approve), but a little bit of discussion.

[paulstelian97]

The idc file specifically shouldn't really have been needed for any of the i.MX platforms, as IDC is Intel specific, and future multi-core DSPs from others will likely use different inter-core communication standards.

A future refactor should make sure to remove this file being needed from all non-CAVS platforms.

[paulstelian97]

I don't think you can share system vs runtime/buffer heaps can you? Or is my knowledge of them being structurally different (and thus mandatory to separate) only applicable on XTOS builds?

On XTOS, for reference, the system heap works very differently from the others (allocation is a pointer increment, as opposed to using a structure to keep track of allocated vs freed heap slots)

[kv2019i]

Currently these are mapped to a single heap in Zephyr SOF builds and the PLATFORM_HEAP_FOO definitions are implemented. Instead, we have a single heap implemented with sys_heap and the SOF heap type definitions are only used to control alloc time behaviour (and we maintain existing semantics here, e.g. SYSTEM allocs can never fail).

There is lot of activity in this area though. On Intel side, we recently added support for a separately managed persistant memory heap (SOF_MEM_CAPS_L3), and there's been active discussions about adding infra to support per-core heaps. But no PRs yet how to expose this via memory.h, so for now, we stick to implementing existing semantics to SOF app code. Platform code needs to provide necessary definitions both for XTOS and Zephyr builds.

[paulstelian97]

This is an interesting discussion to have -- how will Linux's own core frequency change affect Zephyr? Can it possibly mess with the playback in audible ways? Make it harder for the core to keep up if e.g. compress playback is happening?

[LaurentiuM1234]

Very good question, thanks for posing it. I'm not sure how altering the cluster's frequency from Linux's side would affect SOF's flow as I haven't tested it out but this is definitely something we need to watch out for.

[kv2019i]

Looks good! I think some of this code could be removed if/when we get further with platform interface cleanup -> #7248
... but alas, it's not complete yet, so now there's indeed some muddy waters in terms of what a platform needs to define.

If you want to minimize this code and only support Zephyr SOF build, you can take example of sof/src/platform/intel/ace/include/ace/ . It's not perfect yet, but it drops a lot of definitions that are not needed in Zephyr builds, resulting in much smaller set of code needed to add a new platform (on SOF side that is, of course the definitions need to be in Zephyr).

[kv2019i]

Ack @paulstelian97 . With Zephyr we don't really need IDC as a public interface in SOF anymore. It's now used to send messages between DSP cores, but with Zephyr we can now use Zephyr OS primitives (threads, semaphores, locks) to implement this.

It's just this cleanup will take time as we still have non-Zephyr platforms in SOF main. See #7248 for background.

[kv2019i]

Currently these are mapped to a single heap in Zephyr SOF builds and the PLATFORM_HEAP_FOO definitions are implemented. Instead, we have a single heap implemented with sys_heap and the SOF heap type definitions are only used to control alloc time behaviour (and we maintain existing semantics here, e.g. SYSTEM allocs can never fail).

There is lot of activity in this area though. On Intel side, we recently added support for a separately managed persistant memory heap (SOF_MEM_CAPS_L3), and there's been active discussions about adding infra to support per-core heaps. But no PRs yet how to expose this via memory.h, so for now, we stick to implementing existing semantics to SOF app code. Platform code needs to provide necessary definitions both for XTOS and Zephyr builds.

[LaurentiuM1234]

If you want to minimize this code and only support Zephyr SOF build, you can take example of sof/src/platform/intel/ace/include/ace/ . It's not perfect yet, but it drops a lot of definitions that are not needed in Zephyr builds, resulting in much smaller set of code needed to add a new platform (on SOF side that is, of course the definitions need to be in Zephyr).

Was thinking of introducing something similar for i.MX based on used architecture (or maybe even for all used architectures if there's a lot in common) but I think I'll leave this for after the drivers are ported to Zephyr as things may clear out.

[lgirdwood]

ack - cleanup will remove this incrementally.