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

atomic xtensa build fail #33857

Closed
hongshui3000 opened this issue Mar 30, 2021 · 3 comments · Fixed by #33964
Closed

atomic xtensa build fail #33857

hongshui3000 opened this issue Mar 30, 2021 · 3 comments · Fixed by #33964
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@hongshui3000
Copy link
Contributor

hongshui3000 commented Mar 30, 2021

Describe the bug
Not all xtensa architectures support the s32c1i and SCOMPARE1 registers
code:

#if defined(CONFIG_ATOMIC_OPERATIONS_BUILTIN)
/* Default.  See this file for the Doxygen reference: */
#include <sys/atomic_builtin.h>
#elif defined(CONFIG_ATOMIC_OPERATIONS_ARCH)-----------------------------------------------------------here
/* Some architectures need their own implementation */
# ifdef CONFIG_XTENSA
/* Not all Xtensa toolchains support GCC-style atomic intrinsics */
# include <arch/xtensa/atomic_xtensa.h>
# endif
#else
/* Generic-but-slow implementation based on kernel locking and syscalls */
#include <sys/atomic_c.h>
#endif

static ALWAYS_INLINE
atomic_val_t xtensa_cas(atomic_t *addr, atomic_val_t oldval,
			atomic_val_t newval)
{
	__asm__ volatile("wsr %1, SCOMPARE1; s32c1i %0, %2, 0" ---------------------------------------------here
			 : "+r"(newval), "+r"(oldval) : "r"(addr) : "memory");

	return newval; /* got swapped with the old memory by s32c1i */
}

log:

atomic_xtensa.h:35: Error: unknown opcode or format name 's32c1i'
atomic_xtensa.h:35: Error: invalid register 'SCOMPARE1' for 'wsr' instruction
atomic_xtensa.h:35: Error: unknown opcode or format name 's32c1i'
xt-xcc.exe ERROR:  C:/usr/xtensa/XtDevTools/install/tools/RI-2020.5-win32/XtensaTools/bin/xt-as.exe returned non-zero status 1

At least the code needs to be judged as follows,or distinguish the architecture used

if (XCHAL_HAVE_S32C1)
..............

The kconfig I saw opens the ATOMIC_OPERATIONS_ARCH configuration by default.

zephyr\arch\Kconfig:

config XTENSA
	bool
	select ARCH_IS_SET
	select HAS_DTS
	select USE_SWITCH
	select USE_SWITCH_SUPPORTED
	select ATOMIC_OPERATIONS_ARCH          -----------------------------------------------------------here
	help
	  Xtensa architecture

SECOND:
I found another problem when I enabled ATOMIC_OPERATIONS_BUILTIN

config SOC_SERIES_MYSOC
	bool "Xtensa hifi4 core"
	select XTENSA
	select XTENSA_HAL
	select ATOMIC_OPERATIONS_BUILTIN 

log:

In file included from E:/work/zephyr/include/sys/atomic.h:29,
                 from E:/work/zephyr/include/kernel_includes.h:21,
                 from E:/work/zephyr/include/kernel.h:17,
                 from E:/work/zephyr/include/init.h:11,
                 from E:/work/zephyr/include/device.h:29,
                 from E:/work/zephyr/kernel/include/kernel_offsets.h:6,
                 from E:/work/zephyr/arch/xtensa/core/offsets/offsets.c:7:
E:/work/zephyr/include/sys/atomic_builtin.h:304:29: error: syscalls/atomic.h: No such file or directory

Under the build directory I only found "ZCAN-sdk\build\zephyr\include\generated\syscalls\atomic_c.h" but no "atomic.h"
CONFIG_ATOMIC_OPERATIONS_C and CONFIG_ATOMIC_OPERATIONS_BUILTIN and CONFIG_ATOMIC_OPERATIONS_ARCH are enabled by me at the same time. but there is no warning that they cannot be enabled at the same time

code:

#ifdef CONFIG_ATOMIC_OPERATIONS_C
#include <syscalls/atomic.h>
#endif

My current change plan can build the code: del "select ATOMIC_OPERATIONS_ARCH" & CONFIG_XTENSA_NO_IPC=y & CONFIG_ATOMIC_OPERATIONS_C=y

zephyr\arch\Kconfig:

config XTENSA
	bool
	select ARCH_IS_SET
	select HAS_DTS
	select USE_SWITCH
	select USE_SWITCH_SUPPORTED
	help
	  Xtensa architecture
@hongshui3000 hongshui3000 added the bug The issue is a bug, or the PR is fixing a bug label Mar 30, 2021
@galak galak added the priority: low Low impact/importance bug label Mar 30, 2021
@andyross
Copy link
Contributor

Does your platform really not have S32C1? That is the Xtensa atomic operations primitive, without it you have no hardware support and need to fall back to a much slower irq_lock() implementation in ATOMIC_OPERATIONS_C. ATOMIC_OPERATIONS_BUILTIN just falls back to the gcc implementations, which are essentially identical to our own and also based on S32C1I -- they won't work for you (the only reason we have our own is that older xcc toolchains don't have the gcc builtins).

As far as selecting this automatically based on core-isa.h constants, that would be nice but difficult, as kconfig sits upstream of the C compiler. You need the kconfig output header defined before you can build a C file against core-isa.h. The Zephyr model is that the platform layer decides on software configuration.

@hongshui3000
Copy link
Contributor Author

hongshui3000 commented Mar 31, 2021

@andyross
The biggest problem I encounter at the moment is that I have to change the code of Zephyr(Kconfig) in order to compile and pass.
If I change this, it will affect other platforms.
I cannot make ATOMIC_OPERATIONS_ARCH not selected through configuration?????
zephyr\arch\Kconfig:

config XTENSA
	bool
	select ARCH_IS_SET
	select HAS_DTS
	select USE_SWITCH
	select USE_SWITCH_SUPPORTED
        select ATOMIC_OPERATIONS_ARCH          -----------------------------------------------------------del here
	help
	  Xtensa architecture

as long as CONFIG_XTENSA is configured, select ATOMIC_OPERATIONS_ARCH will be selected. So my suggestion is not to choose here, but to give others the right to choose.
Otherwise, the unsupported S32C1 platform cannot be built through
This choice should not be determined by the architecture, it should be determined by the respective platform (soc or board).
Because the same architecture may not support ATOMIC_OPERATIONS_BUILTIN

dcpleung added a commit to dcpleung/zephyr that referenced this issue Apr 1, 2021
Usually, GCC builtin or arch-specific atomic functions are being
used. The corresponding kconfigs are "selected" by architecture
or SoC kconfigs. CONFIG_ATOMIC_OPERATIONS_C is usually used to
override the GCC built-in or arch-specific atomic functions when
these two are not supported. So change the priority of #include
so that C version is included first if selected, and skips
the inline versions of the other two variants. Or else there
will be two compiled versions of atomic functions: inline version
and the compiled C version. Note that the arch-specific version
and builtin are swapped, so the builtin one is now the default.

Fixes zephyrproject-rtos#33857

Signed-off-by: Daniel Leung <[email protected]>
nashif pushed a commit that referenced this issue Apr 2, 2021
Usually, GCC builtin or arch-specific atomic functions are being
used. The corresponding kconfigs are "selected" by architecture
or SoC kconfigs. CONFIG_ATOMIC_OPERATIONS_C is usually used to
override the GCC built-in or arch-specific atomic functions when
these two are not supported. So change the priority of #include
so that C version is included first if selected, and skips
the inline versions of the other two variants. Or else there
will be two compiled versions of atomic functions: inline version
and the compiled C version. Note that the arch-specific version
and builtin are swapped, so the builtin one is now the default.

Fixes #33857

Signed-off-by: Daniel Leung <[email protected]>
@jcmvbkbc
Copy link
Contributor

jcmvbkbc commented Apr 2, 2021

Does your platform really not have S32C1? That is the Xtensa atomic operations primitive

@andyross to be more precise that is one of the possible xtensa atomic operations primitive. There's also exclusive access option with l32ex/s32ex/getex/clrex opcodes and XCHAL_HAVE_EXCLUSIVE core-isa.h macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants