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

nuttx/esp32s3: apply ibus/dbus adjustment to internal ram 1 as well #3421

Merged
merged 1 commit into from
May 14, 2024

Conversation

yamt
Copy link
Collaborator

@yamt yamt commented May 13, 2024

No description provided.

@@ -19,6 +19,11 @@
#define IRAM0_CACHE_ADDRESS_HIGH 0x44000000
#define IRAM_ATTR locate_data(".iram1")

#define INTERNAL_SRAM_1_DBUS_ADDRESS_LOW 0x3fc88000
#define INTERNAL_SRAM_1_DBUS_ADDRESS_HIGH 0x3fcf0000
Copy link
Contributor

Choose a reason for hiding this comment

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

INTERNAL_SRAM_1_DBUS_ADDRESS_HIGH is not used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right. it's just for completeness.

@@ -182,6 +187,11 @@ os_get_dbus_mirror(void *ibus)
if (in_ibus_ext(ibus)) {
return (void *)((uint8 *)ibus - MEM_DUAL_BUS_OFFSET);
}
else if (INTERNAL_SRAM_1_IBUS_ADDRESS_LOW <= (uintptr_t)ibus
&& (uintptr_t)ibus < INTERNAL_SRAM_1_IBUS_ADDRESS_HIGH) {
return (void *)((uintptr_t)ibus - INTERNAL_SRAM_1_IBUS_ADDRESS_LOW
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need special handling like this for the mirrored memmory allocated by os_mmap and os_mumap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes.
for some configurations, os_mmap can return memory from the internal ram 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will the special handling be implemented in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry, which special handling are you talking about?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move out these arch specific impls from common place?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, which special handling are you talking about?

I mean if the os_mmap returns memory from the internal ram1, does that also need convert it into its mirrored addr once the feature enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

up_textheap_memalign already returns i-bus address.

i have no idea about the WASM_MEM_DUAL_BUS_MIRROR block in os_mmap.
to me, it seems like a dead code because CONFIG_ARCH_USE_TEXT_HEAP is always set for
esp32s3.
i guess it's different for the private fork of nuttx?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we move out these arch specific impls from common place?

i guess it can be eventually moved to nuttx.

Copy link
Contributor

Choose a reason for hiding this comment

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

to me, it seems like a dead code because CONFIG_ARCH_USE_TEXT_HEAP is always set for

OK, agree to refactor this into nuttx.

@dongsheng28849455
Copy link
Contributor

LGTM

@wenyongh wenyongh merged commit b1529bc into bytecodealliance:main May 14, 2024
377 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants