-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
arch/xtensa: use arch atomic when enable iram heap #14805
base: master
Are you sure you want to change the base?
Conversation
[Experimental Bot, please feedback here] This PR appears to mostly meet the NuttX requirements, but is missing some key information. Missing/Needs Improvement:
Example of Improved Impact Section:
By providing more complete information in these sections, the PR will be much easier to review and merge. |
@@ -859,6 +859,7 @@ config ESP32_RTC_HEAP | |||
config ESP32_IRAM_HEAP | |||
bool "Use the rest of IRAM as a separate heap" | |||
select ARCH_HAVE_EXTRA_HEAPS | |||
select LIBC_ARCH_ATOMIC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LIBC_ARCH_ATOMIC has been removed while ago. am i missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was add in this PR #14803
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be enabled for ESP32-S2 and ESP32-S3 too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESP32-S2 and ESP32-S3 do not support IRAM HEAP, so there is no need
c599baa
to
74610b0
Compare
Hi @zyfeier , I'll test on our internal CI, thanks! |
Hi @zyfeier , I still couldn't make it work for
NuttX: 74610b0 (this PR's commit) Built/flashed with:
Compiler's version: Anything Am I missing here? |
@tmedicci From the disassembly file, it can be seen that the s32c1i instruction is still used,could you to check if -D__STDC_NO_ATOMICS__ was include in compile flag? |
Hi @zyfeier ! Thanks fo your quick response! In fact,
I had to set |
@tmedicci I found that when compiling esp32/common, scripts/Make.defs is included twice, which causes the flags in ARCHCFLAGS to be doubled. |
That's perfect! I was able to make it work (and, in fact, it fixes
|
@tmedicci wamr_wasi_debug build issue will fixed in PR #14827, and also i move the Make.def modify to PR #14855 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Thanks, @zyfeier !
@@ -188,3 +188,7 @@ endif() | |||
if(CONFIG_DEBUG_SYMBOLS) | |||
add_compile_options(${CONFIG_DEBUG_SYMBOLS_LEVEL}) | |||
endif() | |||
|
|||
if(CONFIG_LIBC_ARCH_ATOMIC) | |||
add_compile_options(-D__STDC_NO_ATOMICS__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why need add STDC_NO_ATOMICS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not use stdc atomic function, use arch_atomic when use iram heap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why do we check __STDC_NO_ATOMICS__
not CONFIG_LIBC_ARCH_ATOMIC
?
S32C1I instructions may target cached, cache-bypass, and data RAM memory locations. S32C1I instructions are not permitted to access memory addresses in data ROM, instruction memory or the address region allocated to the XLMI port. Attempts to direct the S32C1I at these addresses will cause an exception. Signed-off-by: zhangyuan29 <zhangyuan29@xiaomi.com>
Summary
This PR fixed EPS32 issue after enable #14465
Use arch atomic when enable iram heap.
S32C1I instructions may target cached, cache-bypass, and data RAM memory locations. S32C1I instructions are not permitted to access memory addresses in data ROM, instruction memory or the address region allocated to the XLMI port. Attempts to direct the S32C1I at these addresses will cause an exception.
Impact
ESP32 iram heap
Testing
esp32-devkit:sotest boot success