Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up dma-trace.h and dma-trace.c dependencies #9595

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Oct 18, 2024

As part of RTOS header cleanups, I've noted a few incorrect dependencies between common code and DMA trace. This series has a few small fixes to add proper ifdef guards to code only needed for DMA trace (like dma-copy).

The dma_copy_new() implemented in dma-copy.c is only used by dma-trace.c,
so if CONFIG_TRACE=n, the file can be omitted in builds.

Signed-off-by: Kai Vehmanen <[email protected]>
If the platform is build with native Zephyr drivers, dma-trace.h
cannot be used as it relies on legacy XTOS DMA driver interface.
Make include of dma-trace.h conditional.

Signed-off-by: Kai Vehmanen <[email protected]>
If the SOF is built with native Zephyr drivers, dma-trace.h cannot be
used as it relies on legacy XTOS DMA driver interface. Make include of
dma-trace.h conditional.

Signed-off-by: Kai Vehmanen <[email protected]>
@@ -34,7 +34,9 @@
#include <sof/platform.h>
#include <rtos/string.h>
#include <rtos/string_macro.h>
#if CONFIG_TRACE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't put those ifdefs in dma_trace.h file? It would have a global effect, also if somebody use dma_trace.h in future

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcinszkudlinski That's an option. We have a lot of ifdefs hidden like this. In this case, we have longterm goal to move to Zephry and stop using dma-trace (as it only covers parts of events when using Zephyr), so I actually want to make it explicit ont he user side where this is still needed. There are not many cases (= two) in common code, so this is a small amount of ifdefs and is a nice reminder of the dependencies. But yeah, we can go both ways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so its to help track impact of where its used whilst we transition code to native Zephyr. I'm good with this now, but we should end up with a global fix as @marcinszkudlinski suggested once transition work is complete.

@@ -34,7 +34,9 @@
#include <sof/platform.h>
#include <rtos/string.h>
#include <rtos/string_macro.h>
#if CONFIG_TRACE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so its to help track impact of where its used whilst we transition code to native Zephyr. I'm good with this now, but we should end up with a global fix as @marcinszkudlinski suggested once transition work is complete.

@@ -25,7 +25,9 @@
#include <sof/schedule/ll_schedule.h>
#include <sof/schedule/ll_schedule_domain.h>
#include <rtos/sof.h>
#if CONFIG_TRACE
#include <sof/trace/dma-trace.h>
Copy link
Contributor

@LaurentiuM1234 LaurentiuM1234 Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to just drop the inclusion of dma-trace.h altogether if not too much of a hassle. 8ulp doesn't use trace anymore.

LE: nvm, we'll remove it later on as there's still some trace-related stuff there that needs to be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack @LaurentiuM1234 , works for me. I was testing all the upstream imx builds and this indeed was still required to be there.

@lgirdwood lgirdwood merged commit 4d34c29 into thesofproject:main Oct 22, 2024
44 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants