Fixes for SOF with Zephyr on i.MX #4524

iuliana-prodan · 2021-07-21T23:23:41Z

This pull request fixes some issues we have on i.MX.
Please check each commit, but here's a summary of these fixes:

i.MX uses dma_domain, so keep the ll_schedule from SOF, until we extend the zephyr_ll for DMA_IRQ
on i.MX the DMA interrupts are routed via IRQ_STEER. In order for this to work we need to make any second level interrupts handling go through interrupt-irqsteer.c
for i.MX we need to use this temporary fix for SOF_SCHEDULE_LL_DMA

With this pull request we can now run successfully, on i.MX, playback and record scenarios with Zephyr.

Building imx is critical to test changes like thesofproject#4524 Note not signing imx means sof-imx.ldc is staged for deployment but no imx firmware is, example: $ scripts/xtensa-build-zephyr.sh imx apl build-sof-staging ├── sof │ ├── community │ │ └── sof-apl.ri │ ├── sof-apl.ldc │ └── sof-imx.ldc └── tools └── sof-logger Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb

The "unwrapped message" checkpatch warnings seems easy to fix.

zephyr/wrapper.c

marc-hb · 2021-07-22T05:11:13Z

src/include/sof/schedule/ll_schedule.h

It's still not easy to test this without knowing which --board argument to pass to west (see #4525 ) but at least I now see exactly what you're doing and I can simulate it, thx.

I've explained how I tested it with both xtensa-build-zephyr.sh and west commands: see here
The only problem is that the pull request from Zephyr's repo is not yet merged, so you'll need to use a specific branch: https://github.com/iuliana-prodan/zephyr/commits/imx_sof_with_zephy

marc-hb

Due to the relative #ifdef and CMake configuration complexity (e.g. : ll_tr problem in #4503 ) this should not be merged before anyone including CI can build it, see incomplete attempt in #4525

zephyr/CMakeLists.txt

iuliana-prodan · 2021-07-22T08:39:46Z

The "unwrapped message" checkpatch warnings seems easy to fix.

How can I fix it?
Usually, in kernel development, when mentioning a commit id and message it has to be on one line.
This kind of checkpatch warnings were accepted.

How can I wrap it?

marc-hb · 2021-07-22T13:33:03Z

Usually, in kernel development, when mentioning a commit id and message it has to be on one line.

That does not seem to be true, try:
./scripts/checkpatch.pl --strict -g 9d84c15

It would make the line exceed the limit most of the time.

lgirdwood

@iuliana-prodan probably best if we put all the wrapping in the zephyr directory as this will help a lot when it comes to transferring APIs to native Zephyr. This will also help if will help if we need to wrap other RTOS code since we can have a wrapper dir for each (and mean less conditional code in core places).

src/drivers/interrupt.c

iuliana-prodan · 2021-07-23T16:24:24Z

Usually, in kernel development, when mentioning a commit id and message it has to be on one line.

That does not seem to be true, try:
./scripts/checkpatch.pl --strict -g 9d84c15

It would make the line exceed the limit most of the time.

Fix it!

marc-hb · 2021-07-23T19:20:47Z

Please add this to PR. As long as you don't update SUPPORTED_PLATFORMS yet it won't have any CI effect yet hence it won't break anything but it will help local testing and cherry-picks.

@@ -110,6 +111,9 @@ build()
 			icl)
 				PLAT_CONFIG='intel_adsp_cavs20'
 				;;
+			imx8)
+				PLAT_CONFIG='nxp_adsp_imx8'
+				;;
 			tgl-h)
 				PLAT_CONFIG='intel_adsp_cavs25'

lyakh · 2021-07-26T07:08:17Z

src/drivers/interrupt.c

If you have to re-spin this PR to address @marc-hb 's comment, could you also add to this comment, that because of this currently i.MX with Zephyr has to use both wrapper.c and this file which causes name collisions. That's why you need these defines. And below where you #undef for the first time, please add a comment saying, that that will now call the version from wrapper.c
@lgirdwood in fact this seems like a good candidate for an early native Zephyr API conversion: if we could switch over to native z_soc_irq_*() everywhere in the code with wrappers for XTOS builds, we wouldn't need this?

Good point. @lyakh @iuliana-prodan what's the effort in switching over the APIs for Intel and NXP code ?

@marc-hb the irq wrapping as discussed on call

lyakh · 2021-07-26T07:09:46Z

zephyr/CMakeLists.txt

if you respin this PR, please merge this with the previous commit to avoid breaking bisection, even though these platforms don't actually work with Zephyr...

iuliana-prodan · 2021-07-26T14:38:28Z

Please add this to PR. As long as you don't update SUPPORTED_PLATFORMS yet it won't have any CI effect yet hence it won't break anything but it will help local testing and cherry-picks.
@@ -110,6 +111,9 @@ build()
 			icl)
 				PLAT_CONFIG='intel_adsp_cavs20'
 				;;
+			imx8)
+				PLAT_CONFIG='nxp_adsp_imx8'
+				;;
 			tgl-h)
 				PLAT_CONFIG='intel_adsp_cavs25'

Added.

dbaluta · 2021-07-26T15:53:48Z

LGTM. Thanks Iulia. We should also get at least 1 approval from Intel folks :)

ll_tr conflict fixed

lgirdwood · 2021-07-26T18:51:33Z

@iuliana-prodan can you check CI qemu for imx - look like this breaks boot.

Remove zephyr_ll.c from mandatory files for building SOF with Zephyr and include it where necessary: in CAVS 1.5, CAVS 1.8, CAVS 2.0 and CAVS 2.5. While here, add ll_schedule for BROADWELL and BAYTRAIL when building SOF with Zephyr. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

For now, zephyr_ll is limited to timer_domain. For i.MX we use dma_domain, so keep the ll_schedule from SOF, until we extend the zephyr_ll for DMA_IRQ. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

On i.MX the DMA interrupts are routed via IRQ_STEER. In order for this to work we need to: - make any second level interrupts handling go through interrupt-irqsteer.c; - use first level interrupt handling from wrapper.c. TODO: Implement a driver for the IRQ_STEER in Zephyr, to replace the legacy code (interrupt-irqsteer.c). Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

…CHEDULE_LL_DMA In XTOS SOF, ipc_send_queued_msg() is run by task_main_primary_core(). In Zephyr we need to schedule ipc_send_queued_msg() using a notifier triggered by the periodic ll_scheduler. This is similar to commit c194125 ("zephyr: add notifier_register(ipc_send_queued_msg) in task_main_start()") For i.MX we need to use this temporary fix for SOF_SCHEDULE_LL_DMA, also. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

Add imx8 platform for local testing. TODO: Update xtensa-build-zephyr.sh when Zephyr repo is updated with imx8 support. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

iuliana-prodan · 2021-07-26T19:17:07Z

@iuliana-prodan can you check CI qemu for imx - look like this breaks boot.

I've rebased to latest main branch (commit 57ee04f) also to restart CI.
I have no idea why is failing. :(
Let's wait for the new results.

iuliana-prodan · 2021-07-26T21:38:11Z

@iuliana-prodan can you check CI qemu for imx - look like this breaks boot.

I've rebased to latest main branch (commit 57ee04f) also to restart CI.
I have no idea why is failing. :(
Let's wait for the new results.

All green with one exception: "On-device Test Failed:APL_UP2_NOCODEC_ZEPHYR: kernel boot fail!".
But this test was failing without this pull request, also.

marc-hb · 2021-07-27T06:23:36Z

I've rebased to latest main branch (commit 57ee04f) also to restart CI.

BTW you don't need to rebase to restart CI if you don't want to create git range-diff noise which github doesn't know how separate (unlike say Gerrit). Instead you can just do an empty git commit --amend and force-push. The new commit date is enough to change the SHA1 and restart ALL CI engines (unlike magic keywords like SOFCI TEST that restarts only Jenkins)

Most (all?) CI engines test the constantly moving pull/4524/merge (e.g. https://github.com/thesofproject/sof/commits/ebfd72540fda right now), they do NOT test the static pull/4524/head so this is another wrong reason to rebase.

"You need to rebase" is probably the most common CI misconception. The only time rebasing is required is (of course) when there is a git conflict to resolve, as shown by Github.

lgirdwood · 2021-07-27T13:43:40Z

Looks like we are good now.

iuliana-prodan requested review from dbaluta, lbetlej, lgirdwood, mmaka1, mrajwa and plbossart as code owners July 21, 2021 23:23

iuliana-prodan requested review from kv2019i, lyakh, marc-hb and paulstelian97 July 21, 2021 23:24

iuliana-prodan mentioned this pull request Jul 21, 2021

zephyr: sof-logger #4503

Merged

marc-hb mentioned this pull request Jul 22, 2021

xtensa-build-zephyr: add imx #4525

Closed

marc-hb reviewed Jul 22, 2021

View reviewed changes

zephyr/wrapper.c Outdated Show resolved Hide resolved

marc-hb reviewed Jul 22, 2021

View reviewed changes

marc-hb previously requested changes Jul 22, 2021

View reviewed changes

lyakh reviewed Jul 22, 2021

View reviewed changes

zephyr/CMakeLists.txt Outdated Show resolved Hide resolved

lgirdwood reviewed Jul 23, 2021

View reviewed changes

src/drivers/interrupt.c Outdated Show resolved Hide resolved

iuliana-prodan force-pushed the play_record_with_zephyr branch from 8161c74 to 4dc506b Compare July 23, 2021 16:23

iuliana-prodan requested a review from marc-hb July 23, 2021 16:26

iuliana-prodan force-pushed the play_record_with_zephyr branch from 4dc506b to aba724d Compare July 23, 2021 16:29

iuliana-prodan requested a review from lyakh July 23, 2021 16:29

lyakh reviewed Jul 26, 2021

View reviewed changes

iuliana-prodan force-pushed the play_record_with_zephyr branch from aba724d to 2cdbaf2 Compare July 26, 2021 14:37

dbaluta approved these changes Jul 26, 2021

View reviewed changes

lgirdwood approved these changes Jul 26, 2021

View reviewed changes

iuliana-prodan added 5 commits July 26, 2021 22:11

zephyr: imx: use ll_schedule instead of zephyr_ll

fedcc4c

For now, zephyr_ll is limited to timer_domain. For i.MX we use dma_domain, so keep the ll_schedule from SOF, until we extend the zephyr_ll for DMA_IRQ. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

xtensa-build-zephyr: add imx8 platform

111a1e5

Add imx8 platform for local testing. TODO: Update xtensa-build-zephyr.sh when Zephyr repo is updated with imx8 support. Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>

iuliana-prodan force-pushed the play_record_with_zephyr branch from 2cdbaf2 to 111a1e5 Compare July 26, 2021 19:12

lgirdwood merged commit 6556933 into thesofproject:main Jul 27, 2021

Fixes for SOF with Zephyr on i.MX #4524

Fixes for SOF with Zephyr on i.MX #4524

Uh oh!

Conversation

iuliana-prodan commented Jul 21, 2021

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-hb Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iuliana-prodan Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iuliana-prodan commented Jul 22, 2021

Uh oh!

marc-hb commented Jul 22, 2021

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iuliana-prodan commented Jul 23, 2021

Uh oh!

marc-hb commented Jul 23, 2021

Uh oh!

lyakh Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

lyakh Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

iuliana-prodan Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

iuliana-prodan commented Jul 26, 2021

Uh oh!

dbaluta commented Jul 26, 2021

Uh oh!

lgirdwood commented Jul 26, 2021

Uh oh!

iuliana-prodan commented Jul 26, 2021

Uh oh!

iuliana-prodan commented Jul 26, 2021

Uh oh!

marc-hb commented Jul 27, 2021

Uh oh!

lgirdwood commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marc-hb Jul 22, 2021 •

edited

Loading