Misc. Zephyrization fixups #9731
Conversation
andyross
requested review from
kv2019i,
lyakh,
iuliana-prodan,
dbaluta,
abonislawski,
lgirdwood,
plbossart,
mmaka1 and
lbetlej
as code owners
andyross
force-pushed
the
zephyr-fixups-1224
branch
from
971622b to
f6f9bde
Compare
|
Quick respin to fix a dropped file change. And now I see that this breaks IPC3 fuzzing somewhere. Will fix. |
andyross
force-pushed
the
zephyr-fixups-1224
branch
2 times, most recently
from
132fbdd to
9b37e3f
Compare
There was a problem hiding this comment.
Thanks @andyross . 90% of the PR is completely fine and ready to go. The XTENSA_TOOLS_VERSION change seems a bit orthogonal to other fixes so maybe could go in as a separate PR. Other than that, only one issue that affects NXP IMX configs (see inline comment), but rest looks nice!
This is also timely and good opportunity to discuss about our transition layers. I'll make a separate discussion page entry on that. I have some further cleanups in progress for our "transitory tools", btu I'm now a bit hesitent to push them through as they may cause trouble like you had with the DMA interface naming changes.
| @@ -287,7 +287,7 @@ static int primary_core_init(int argc, char *argv[], struct sof *sof) | |||
| interrupt_init(sof); | |||
| #endif /* __ZEPHYR__ */ | |||
|
|
|||
| #ifdef CONFIG_ZEPHYR_LOG | |||
| #if defined(CONFIG_ZEPHYR_LOG) && !defined(CONFIG_LOG_MODE_MINIMAL) | |||
There was a problem hiding this comment.
Ack, this has been missed. Minimal support indeed a good addition!
| @@ -97,6 +97,9 @@ enum dma_cb_status { | |||
| DMA_CB_STATUS_END, | |||
| }; | |||
|
|
|||
| #define SOF_DMA_CB_STATUS_RELOAD DMA_CB_STATUS_RELOAD | |||
| #define SOF_DMA_CB_STATUS_END DMA_CB_STATUS_END | |||
|
|
|||
There was a problem hiding this comment.
Apologies @andyross , I was testing with all AMD/MTK/NXP/Intel targets in upstream, but right, definitions not used in upstream were very likely missed.
| @@ -5,7 +5,6 @@ CONFIG_DCACHE_LINE_SIZE=128 | |||
| CONFIG_DYNAMIC_INTERRUPTS=y | |||
There was a problem hiding this comment.
@andyross Ack. CONFIG_TRACE was working with Zephyr at some point, but basicly as Intel has moved to native logging, this code is likely to have bitrotted. For Intel, we are ok to remove it. It was useful at start, but now that there is plenty of infra available in Zephyr (and plenty of examples in SOF how to use it), I don't think it's the worth keeping around anymore. And if it doesn't work anymore, seems like a clear case.
| depends on COMP_FIR | ||
| depends on COMP_IIR | ||
| depends on COMP_TDFB | ||
| depends on COMP_FIR || COMP_IIR || COMP_TDFB |
That's the idea. It's just the two drivers: dummy_dma and afe. The former might have a solution from NXP already, but if not it's pretty simple as I read it. The latter is half way there already: it sources all its config from DTS on the Zephyr side, but then translates it to the legacy config struct for use by the existing driver code. Shouldn't be too hard. But for sure the first submission is going to be ZEPHYR_NATIVE_DRIVERS=n and IPC3. I'm hammering out all the existing workaround right now and hope to have the submission out today. |
Zephyr builds don't use the older trace protocol, but for now still need to work with IPC3 kernel layers that expect it to be present. Just return a noop success from trace messages when not configured, otherwise the -EINVAL will propagate to the other side and abort the firmware load. Signed-off-by: Andy Ross <andyross@google.com>
When CONFIG_LOG_MODE_MINIMAL is chosen this function doesn't exist. Minimal logging is actually really useful when debugging: it's a synchronous logging mode (especially useful for winstream logging where it becomes safe against DSP hangs or latchups) using the same logging API, but with no buffering/filtering/timestamping, and is backed by the platform printk() implementation. We want to support this. Signed-off-by: Andy Ross <andyross@google.com>
andyross
force-pushed
the
zephyr-fixups-1224
branch
from
37ed8d7 to
f30a345
Compare
This header may be included in situations where there is no log context declared, but still needs to expand a tr_err() macro. Declare the "zephyr" context as a default. (Though really this is not a runtime error and should probably be handled with a BUILD_ASSERT()). Also, there was a growing "enumerate all the Zephyr platforms" preprocessor #if expression that can be simplified to just the IMX8M device that needs special treatment. Signed-off-by: Andy Ross <andyross@google.com>
These symbols got renamed in commit 1d73fb3 ("treewide: prefix RTOS lib/dma.h DMA_ definitions with SOF_DMA_"), but these spots were apparently dead code and missed it. They're being resurrected, so fix them up. Signed-off-by: Andy Ross <andyross@google.com>
Things had gotten a little messy here. The CPU_MASK_PIN_ONLY and SMP_BOOT_DELAY options are in fact not generic to all Zephyr builds; they both depend on SMP, and in fact don't exist otherwise, producing a build failure if the board defconfig doesn't turn it off. Create an application "Kconfig" file (which SOF hasn't used so far), which allows the dependencies for these settings to be more cleanly expressed. Similarly CONFIG_TRACE=y won't build with Zephyr, and all boards were turning it off independently. Move that to the prj.conf level. And likewise CONFIG_ZEPHYR_LOG, while not strictly required for a Zephyr build, is pervasively enabled (except on ACP6? What does that device use for logging?), so it belongs in the global file. Signed-off-by: Andy Ross <andyross@google.com>
This DMA driver was using timer_get_system() to generate timestamps, which is the Xtensa CCOUNT timer in legacy SOF but doesn't exist at all in Zephyr (well, you can read the SR directly). Use sof_cycle_get_64(), which is the portable API based on the clock hardware. Other DMA drivers do just fine with this. Signed-off-by: Andy Ross <andyross@google.com>
The older SOF allocator would return a valid pointer into the heap for a zero length block, but Zephyr's sys_heap returns a NULL. That's not an error and it will free() as a noop. Signed-off-by: Andy Ross <andyross@google.com>
At the start of probe(), the expectation is that component objects go to COMP_STATE_INIT. This was being skipped. That used to be benign, I believe because the struct was allocated via a path that set it already. But now it fails with the Zephyr integration. Signed-off-by: Andy Ross <andyross@google.com>
The intent of the dependencies on this choice seems to be that they apply only to certain components so the selection should be hidden if they aren't in use. But the logic was wrong: instead of enabling it if any component was enabled that needed it, it would hide the selection unless ALL such components were enabled. Which doesn't build. Fix. (Also I note the comment implies that other kconfigs should probably be in the list too, but not adding them out of conservatism as this is, again, just a hygiene thing.) Signed-off-by: Andy Ross <andyross@google.com>
andyross
force-pushed
the
zephyr-fixups-1224
branch
from
f30a345 to
442420b
Compare
|
@lgirdwood good to merge :) |
Some changes needed by an imminent Mediatek Zephyrization PR, largely of the form "make this older code build with Zephyr while we untangle it from the legacy layers".
Most of these shouldn't be controversial, so I've left them in a long series for simplicity. Let me know if there's something people want to split out for independent review.
In particular please look carefully at the Kconfig changes. I'm 90%+ sure that this is pure cleanup that doesn't change any existing configs, but it's not possible for me to test (or even build) all platforms, so some help by the relevant maintainers would be appreciated.