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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89574ae
ipc: only build dma-copy.c if CONFIG_TRACE selected
kv2019i Oct 9, 2024
36949c8
platform: imx8ulp: conditionally include dma-trace.h
kv2019i Oct 9, 2024
1c043d8
ipc: ipc3: conditionally include dma-trace.h
kv2019i Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/ipc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@ add_local_sources(sof
is_zephyr(it_is)
if(it_is) ### Zephyr ###

zephyr_library_sources(
dma-copy.c
)
# dma-copy only used for dma-trace
zephyr_library_sources_ifdef(CONFIG_TRACE dma-copy.c)

if (CONFIG_SOC_SERIES_INTEL_CAVS_V25 OR CONFIG_SOC_SERIES_INTEL_ADSP_ACE)
zephyr_library_sources(
Expand Down
2 changes: 2 additions & 0 deletions src/ipc/ipc3/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

#include <sof/trace/dma-trace.h>
#endif
#include <sof/trace/trace.h>
#include <ipc/control.h>
#include <ipc/dai.h>
Expand Down
2 changes: 2 additions & 0 deletions src/platform/imx8ulp/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

#endif
#include <ipc/dai.h>
#include <ipc/header.h>
#include <ipc/info.h>
Expand Down
Loading