Skip to content

Fixes for SOF with Zephyr on i.MX#4524

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

Merged
lgirdwood merged 5 commits into from
Jul 27, 2021
Merged

Fixes for SOF with Zephyr on i.MX #4524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ac781c
zephyr: intel: remove zephyr_ll from mandatory files
iuliana-prodan Jul 23, 2021
fedcc4c
zephyr: imx: use ll_schedule instead of zephyr_ll
iuliana-prodan Jul 21, 2021
c01772e
zephyr: imx: enable second level interrupts handling
iuliana-prodan Jul 23, 2021
86c66f7
zephyr: wrapper: add notifier_register(ipc_send_queued_msg) for SOF_S…
iuliana-prodan Jul 20, 2021
111a1e5
xtensa-build-zephyr: add imx8 platform
iuliana-prodan Jul 26, 2021
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
4 changes: 4 additions & 0 deletions scripts/xtensa-build-zephyr.sh
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -114,6 +114,10 @@ build()
PLAT_CONFIG='intel_adsp_cavs25'
RIMAGE_KEY=modules/audio/sof/keys/otc_private_key_3k.pem
;;
imx8)
PLAT_CONFIG='nxp_adsp_imx8'
RIMAGE_KEY='' # no key needed for imx8
;;
*)
echo "Unsupported platform: $platform"
exit 1
Expand Down
4 changes: 4 additions & 0 deletions src/drivers/imx/interrupt-irqsteer.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -95,6 +95,10 @@ DECLARE_TR_CTX(irq_i_tr, SOF_UUID(irq_imx_uuid), LOG_LEVEL_INFO);
#define IRQSTR_INT_BIT(irq) ((irq) % 32)
#define IRQSTR_INT_MASK(irq) (1 << IRQSTR_INT_BIT(irq))

#if defined(__ZEPHYR__)
#define interrupt_get_irq mux_interrupt_get_irq
#endif

/* HW register access helper methods */

static inline void irqstr_write(uint32_t reg, uint32_t value)
Expand Down
54 changes: 53 additions & 1 deletion src/drivers/interrupt.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,21 @@ DECLARE_SOF_UUID("irq", irq_uuid, 0x1862d39a, 0x3a84, 0x4d64,

DECLARE_TR_CTX(irq_tr, SOF_UUID(irq_uuid), LOG_LEVEL_INFO);

/* For i.MX, when building SOF with Zephyr, we use wrapper.c,
* interrupt.c and interrupt-irqsteer.c which causes name
* collisions.
* In order to avoid this and make any second level interrupt
* handling go through interrupt-irqsteer.c define macros to
* rename the duplicated functions.
*/

@lyakhlyakhJul 26, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

@lgirdwoodlgirdwoodJul 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@lgirdwoodlgirdwoodJul 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@marc-hb the irq wrapping as discussed on call

#if defined(__ZEPHYR__) && defined(CONFIG_IMX)
#define interrupt_get_irq mux_interrupt_get_irq
#define interrupt_register mux_interrupt_register
#define interrupt_unregister mux_interrupt_unregister
#define interrupt_enable mux_interrupt_enable
#define interrupt_disable mux_interrupt_disable
Comment thread
lgirdwood marked this conversation as resolved.
Outdated
#endif

static SHARED_DATA struct cascade_root cascade_root;

static int interrupt_register_internal(uint32_t irq, void (*handler)(void *arg),
Expand DownExpand Up@@ -361,8 +376,18 @@ static int interrupt_register_internal(uint32_t irq, void (*handler)(void *arg),

/* no parent means we are registering DSP internal IRQ */
cascade = interrupt_get_parent(irq);
if (!cascade)
if (!cascade) {
#if defined(__ZEPHYR__) && defined(CONFIG_IMX)
/* undefine the macro so that interrupt_register()
* is resolved to the one from wrapper.c
*/
#undef interrupt_register

return interrupt_register(irq, handler, arg);
#else
return arch_interrupt_register(irq, handler, arg);
#endif
}

spin_lock_irq(&cascade->lock, flags);
ret = irq_register_child(cascade, irq, handler, arg, desc);
Expand All@@ -386,7 +411,16 @@ static void interrupt_unregister_internal(uint32_t irq, const void *arg,
/* no parent means we are unregistering DSP internal IRQ */
cascade = interrupt_get_parent(irq);
if (!cascade) {
#if defined(__ZEPHYR__) && defined(CONFIG_IMX)
/* undefine the macro so that interrupt_unregister()
* is resolved to the one from wrapper.c
*/
#undef interrupt_unregister

interrupt_unregister(irq, arg);
#else
arch_interrupt_unregister(irq);
#endif
return;
}

Expand All@@ -404,7 +438,16 @@ uint32_t interrupt_enable(uint32_t irq, void *arg)
if (cascade)
return irq_enable_child(cascade, irq, arg);

#if defined(__ZEPHYR__) && defined(CONFIG_IMX)
/* undefine the macro so that interrupt_enable()
* is resolved to the one from wrapper.c
*/
#undef interrupt_enable

return interrupt_enable(irq, arg);
#else
return arch_interrupt_enable_mask(1 << irq);
#endif
}

uint32_t interrupt_disable(uint32_t irq, void *arg)
Expand All@@ -416,5 +459,14 @@ uint32_t interrupt_disable(uint32_t irq, void *arg)
if (cascade)
return irq_disable_child(cascade, irq, arg);

#if defined(__ZEPHYR__) && defined(CONFIG_IMX)
/* undefine the macro so that interrupt_disable()
* is resolved to the one from wrapper.c
*/
#undef interrupt_disable

return interrupt_disable(irq, arg);
#else
return arch_interrupt_disable_mask(1 << irq);
#endif
}
11 changes: 11 additions & 0 deletions src/include/sof/drivers/interrupt.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -119,6 +119,17 @@ static inline struct cascade_root *cascade_root_get(void)
return sof_get()->cascade_root;
}

/* For i.MX, while building SOF with Zephyr use the interrupt_*
* functions from second level interrupt handling and IRQ_STEER.
*/
#if defined(__ZEPHYR__) && defined(CONFIG_IMX)
int mux_interrupt_get_irq(unsigned int irq, const char *cascade);
int mux_interrupt_register(uint32_t irq, void(*handler)(void *arg), void *arg);
void mux_interrupt_unregister(uint32_t irq, const void *arg);
uint32_t mux_interrupt_enable(uint32_t irq, void *arg);
uint32_t mux_interrupt_disable(uint32_t irq, void *arg);
#endif

int interrupt_register(uint32_t irq, void(*handler)(void *arg), void *arg);
void interrupt_unregister(uint32_t irq, const void *arg);
uint32_t interrupt_enable(uint32_t irq, void *arg);
Expand Down
2 changes: 1 addition & 1 deletion src/include/sof/schedule/ll_schedule.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -34,7 +34,7 @@ struct ll_task_pdata {
uint16_t skip_cnt; /**< how many times the task was skipped for execution */
};

#ifndef __ZEPHYR__
#if !defined(__ZEPHYR__) || defined(CONFIG_IMX)

@marc-hbmarc-hbJul 22, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@iuliana-prodaniuliana-prodanJul 22, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

int scheduler_init_ll(struct ll_schedule_domain *domain);

int schedule_task_init_ll(struct task *task,
Expand Down
5 changes: 5 additions & 0 deletions src/platform/imx8/platform.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -156,6 +156,11 @@ int platform_init(struct sof *sof)
sof->platform_timer = &timer;
sof->cpu_timers = &timer;

#ifdef __ZEPHYR__
/* initialize cascade interrupts before any usage */
interrupt_init(sof);
#endif

platform_interrupt_init();
platform_clock_init(sof);
scheduler_init_edf();
Expand Down
15 changes: 15 additions & 0 deletions src/schedule/dma_multi_chan_domain.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -23,6 +23,21 @@
#include <stddef.h>
#include <stdint.h>

/* For i.MX, when building SOF with Zephyr, we use wrapper.c,
* interrupt.c and interrupt-irqsteer.c which causes name
* collisions.
* In order to avoid this and make any second level interrupt
* handling go through interrupt-irqsteer.c define macros to
* rename the duplicated functions.
*/
#if defined(__ZEPHYR__) && defined(CONFIG_IMX)
#define interrupt_get_irq mux_interrupt_get_irq
#define interrupt_register mux_interrupt_register
#define interrupt_unregister mux_interrupt_unregister
#define interrupt_enable mux_interrupt_enable
#define interrupt_disable mux_interrupt_disable
#endif

struct dma_domain_data {
int irq;
struct pipeline_task *task;
Expand Down
38 changes: 37 additions & 1 deletion zephyr/CMakeLists.txt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -113,6 +113,11 @@ if (CONFIG_SOC_SERIES_INTEL_ADSP_BAYTRAIL)
${SOF_SRC_PATH}/schedule/dma_multi_chan_domain.c
)

# SOF core infrastructure - runs on top of Zephyr
zephyr_library_sources(
${SOF_SRC_PATH}/schedule/ll_schedule.c
)

@lyakhlyakhJul 26, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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-prodaniuliana-prodanJul 26, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done

set(PLATFORM "baytrail")
endif()

Expand All@@ -123,6 +128,11 @@ if (CONFIG_SOC_SERIES_INTEL_ADSP_BROADWELL)
${SOF_DRIVERS_PATH}/intel/haswell/ssp.c
)

# SOF core infrastructure - runs on top of Zephyr
zephyr_library_sources(
${SOF_SRC_PATH}/schedule/ll_schedule.c
)

set(PLATFORM "haswell")
endif()

Expand DownExpand Up@@ -177,6 +187,11 @@ if (CONFIG_SOC_SERIES_INTEL_CAVS_V15)
${SOF_PLATFORM_PATH}/apollolake/lib/clk.c
)

# SOF core infrastructure - runs on top of Zephyr
zephyr_library_sources(
${SOF_SRC_PATH}/schedule/zephyr_ll.c
)

set_source_files_properties(${SOF_PLATFORM_PATH}/apollolake/lib/power_down.S PROPERTIES COMPILE_FLAGS -DASSEMBLY)

set(PLATFORM "apollolake")
Expand DownExpand Up@@ -239,6 +254,11 @@ if (CONFIG_SOC_SERIES_INTEL_CAVS_V18)
#${SOF_PLATFORM_PATH}/intel/cavs/lps_pic_restore_vector.S
)

# SOF core infrastructure - runs on top of Zephyr
zephyr_library_sources(
${SOF_SRC_PATH}/schedule/zephyr_ll.c
)

zephyr_library_sources_ifdef(CONFIG_CAVS_LPS
${SOF_PLATFORM_PATH}/intel/cavs/lps_wait.c
)
Expand DownExpand Up@@ -302,6 +322,11 @@ if (CONFIG_SOC_SERIES_INTEL_CAVS_V20)
#${SOF_PLATFORM_PATH}/intel/cavs/lps_pic_restore_vector.S
)

# SOF core infrastructure - runs on top of Zephyr
zephyr_library_sources(
${SOF_SRC_PATH}/schedule/zephyr_ll.c
)

zephyr_library_sources_ifdef(CONFIG_CAVS_LPS
${SOF_PLATFORM_PATH}/intel/cavs/lps_wait.c
)
Expand DownExpand Up@@ -368,6 +393,11 @@ if (CONFIG_SOC_SERIES_INTEL_CAVS_V25)
#${SOF_PLATFORM_PATH}/intel/cavs/lps_pic_restore_vector.S
)

# SOF core infrastructure - runs on top of Zephyr
zephyr_library_sources(
${SOF_SRC_PATH}/schedule/zephyr_ll.c
)

zephyr_library_sources_ifdef(CONFIG_CAVS_LPS
${SOF_PLATFORM_PATH}/intel/cavs/lps_wait.c
)
Expand All@@ -389,6 +419,7 @@ if (CONFIG_SOC_SERIES_NXP_IMX8)
${SOF_DRIVERS_PATH}/imx/sai.c
${SOF_DRIVERS_PATH}/imx/ipc.c
${SOF_DRIVERS_PATH}/imx/esai.c
${SOF_DRIVERS_PATH}/imx/interrupt-irqsteer.c
)

# Platform sources
Expand All@@ -400,6 +431,12 @@ if (CONFIG_SOC_SERIES_NXP_IMX8)
${SOF_PLATFORM_PATH}/imx8/lib/memory.c
)

# SOF core infrastructure - runs on top of Zephyr
zephyr_library_sources(
${SOF_SRC_PATH}/schedule/ll_schedule.c
Comment thread
iuliana-prodan marked this conversation as resolved.
Outdated
${SOF_SRC_PATH}/drivers/interrupt.c
)

set(PLATFORM "imx8")
endif()

Expand DownExpand Up@@ -479,7 +516,6 @@ zephyr_library_sources(
${SOF_SRC_PATH}/schedule/schedule.c
${SOF_SRC_PATH}/schedule/dma_single_chan_domain.c
${SOF_SRC_PATH}/schedule/dma_multi_chan_domain.c
${SOF_SRC_PATH}/schedule/zephyr_ll.c
${SOF_SRC_PATH}/schedule/zephyr.c

# Bridge wrapper between SOF and Zephyr APIs - Will shrink over time.
Expand Down
54 changes: 31 additions & 23 deletions zephyr/wrapper.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -255,28 +255,6 @@ void heap_trace_all(int force)
const char irq_name_level2[] = "level2";
const char irq_name_level5[] = "level5";

/*
* CAVS IRQs are multilevel whereas BYT and BDW are DSP level only.
*
* For i.MX we use the IRQ_STEER
*/
int interrupt_get_irq(unsigned int irq, const char *cascade)
{
#if CONFIG_SOC_SERIES_INTEL_ADSP_BAYTRAIL ||\
CONFIG_SOC_SERIES_INTEL_ADSP_BROADWELL || \
CONFIG_IMX || \
CONFIG_LIBRARY
return irq;
#else
if (cascade == irq_name_level2)
return SOC_AGGREGATE_IRQ(irq, IRQ_NUM_EXT_LEVEL2);
if (cascade == irq_name_level5)
return SOC_AGGREGATE_IRQ(irq, IRQ_NUM_EXT_LEVEL5);

return SOC_AGGREGATE_IRQ(0, irq);
#endif
}

int interrupt_register(uint32_t irq, void(*handler)(void *arg), void *arg)
{
#ifdef CONFIG_DYNAMIC_INTERRUPTS
Expand DownExpand Up@@ -319,6 +297,29 @@ uint32_t interrupt_disable(uint32_t irq, void *arg)
}
#endif

/*
* i.MX uses the IRQ_STEER
*/
#if !CONFIG_IMX
/*
* CAVS IRQs are multilevel whereas BYT and BDW are DSP level only.
*/
int interrupt_get_irq(unsigned int irq, const char *cascade)
{
#if CONFIG_SOC_SERIES_INTEL_ADSP_BAYTRAIL ||\
CONFIG_SOC_SERIES_INTEL_ADSP_BROADWELL || \
CONFIG_LIBRARY
return irq;
#else
if (cascade == irq_name_level2)
return SOC_AGGREGATE_IRQ(irq, IRQ_NUM_EXT_LEVEL2);
if (cascade == irq_name_level5)
return SOC_AGGREGATE_IRQ(irq, IRQ_NUM_EXT_LEVEL5);

return SOC_AGGREGATE_IRQ(0, irq);
#endif
}

void interrupt_mask(uint32_t irq, unsigned int cpu)
{
/* TODO: how do we mask on other cores with Zephyr APIs */
Expand All@@ -343,6 +344,7 @@ void platform_interrupt_clear(uint32_t irq, uint32_t mask)
{
/* handled by zephyr - needed for linkage */
}
#endif

/*
* Timers.
Expand DownExpand Up@@ -609,8 +611,14 @@ int task_main_start(struct sof *sof)
/* init pipeline position offsets */
pipeline_posn_init(sof);

#if defined(CONFIG_IMX)
#define SOF_IPC_QUEUED_DOMAIN SOF_SCHEDULE_LL_DMA
#else
Comment thread
iuliana-prodan marked this conversation as resolved.
Outdated
#define SOF_IPC_QUEUED_DOMAIN SOF_SCHEDULE_LL_TIMER
#endif

/* Temporary fix for issue #4356 */
(void)notifier_register(NULL, scheduler_get_data(SOF_SCHEDULE_LL_TIMER),
(void)notifier_register(NULL, scheduler_get_data(SOF_IPC_QUEUED_DOMAIN),
NOTIFIER_ID_LL_POST_RUN,
ipc_send_queued_callback, 0);

Expand Down