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

Jump to bottom

Fixes for i.MX xt-clang build #7538

Merged
merged 3 commits into from
May 15, 2023
Merged

Fixes for i.MX xt-clang build #7538

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
edb4659
arch: xtensa: core.h: Add define for UINT32_C
Apr 28, 2023
480b24e
drivers: imx: interrupt: Use Zephyr wrapper definitions
Apr 28, 2023
bf137f3
west.yml: Upgrade zephyr to fa5117225a
May 4, 2023
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
18 changes: 18 additions & 0 deletions src/arch/xtensa/include/xtensa/config/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,24 @@
#ifndef XTENSA_CONFIG_CORE_H
#define XTENSA_CONFIG_CORE_H

/*
* This define is used by the new 2023 xt-clang toolchain and, while there are a few definitions
* (identical to this one) in various implementations such as newlib, none of them is in use when
* building SOF with Zephyr and XtensaTools.
*/

#if defined(__ZEPHYR__)

#ifndef __UINT32_C
#define __UINT32_C(x) x ## U
#endif

#ifndef UINT32_C
#define UINT32_C(x) __UINT32_C(x)
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

This #ifndef #define pattern is at best a necessary evil because the definition can change depending the #include order (recent example: 2512698) and the direction the wind blows.

So can you transfer some of the information and justification from the commit message to the comment here?

Copy link
Collaborator Author

@paulstelian97 paulstelian97 Apr 28, 2023 •
edited by marc-hb
Loading

Choose a reason for hiding this comment

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

The reason I had to put it in here in the first place, especially with the __ZEPHYR__ guard, is because of the include order. On XTOS builds (including i.MX8) without that guard this is included before newlib and newlib does not do this, which leads to conflicts.

I will copy the comment and resubmit, maybe today but if not on Tuesday. (edit: done)

Copy link
Collaborator

Choose a reason for hiding this comment

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

On XTOS builds (including i.MX8) without that guard this is included before newlib and newlib does not do this, which leads to conflicts.

What sort of conflicts? This is coming up again in non-Zephyr build #9351 (comment) ...

Maybe @keith-packard or @stephanosio could help us get out of this libc / toolchain nightmare?


#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this really a suitable header for this? This seems some compiler compatibility support, how about xtos/include/sof/compiler_attributes.h?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure the xtos/ subfolder isn't used in Zephyr builds; I want the opposite (something that only applies to Zephyr)


/* CONFIGURATION INDEPENDENT DEFINITIONS: */
#ifdef __XTENSA__
#include <xtensa/hal.h>
Expand Down
2 changes: 2 additions & 0 deletions src/drivers/imx/interrupt-generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ int irqstr_get_sof_int(int irqstr_int)

void platform_interrupt_init(void) {}

#ifndef __ZEPHYR__
void platform_interrupt_set(uint32_t irq)
{
arch_interrupt_set(irq);
Expand All @@ -45,6 +46,7 @@ void platform_interrupt_clear(uint32_t irq, uint32_t mask)
{
arch_interrupt_clear(irq);
}
#endif /* __ZEPHYR__ */

uint32_t platform_interrupt_get_enabled(void)
{
Expand Down
2 changes: 2 additions & 0 deletions src/drivers/imx/interrupt-irqsteer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,7 @@ void platform_interrupt_init(void)
interrupt_cascade_register(dsp_irq + i);
}

#ifndef __ZEPHYR__
void platform_interrupt_set(uint32_t irq)
{
if (interrupt_is_dsp_direct(irq))
Expand All @@ -462,6 +463,7 @@ void platform_interrupt_clear(uint32_t irq, uint32_t mask)
if (interrupt_is_dsp_direct(irq))
arch_interrupt_clear(irq);
}
#endif /* __ZEPHYR */

uint32_t platform_interrupt_get_enabled(void)
{
Expand Down
2 changes: 1 addition & 1 deletion west.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ manifest:

- name: zephyr
repo-path: zephyr
revision: aba3b12e311b4779338406fe3e7c4c54f655d0cd
revision: fa5117225af9c2167449d79cccd5f0e81bb10696
remote: zephyrproject

# Import some projects listed in zephyr/west.yml@revision
Expand Down
2 changes: 1 addition & 1 deletion zephyr/wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ void interrupt_unmask(uint32_t irq, unsigned int cpu)
{
/* TODO: how do we unmask on other cores with Zephyr APIs */
}
#endif

void platform_interrupt_set(uint32_t irq)
{
Expand All @@ -117,7 +118,6 @@ void platform_interrupt_clear(uint32_t irq, uint32_t mask)
{
/* handled by zephyr - needed for linkage */
}
#endif

/*
* Asynchronous Messaging Service
Expand Down