ESP32-S3: Fix PSRAM start address calculation #2880
Conversation
esp-hal/src/soc/esp32s3/psram.rs
Outdated
| break; | ||
| } | ||
| } | ||
| let start = EXTMEM_ORIGIN + MMU_PAGE_SIZE * (mapped_pages + 1) as u32; |
There was a problem hiding this comment.
Is the + 1 required here? As far as I understand, if there are no pages mapped, then just use EXTMEM_ORIGIN as the start value? Or maybe I just don't understand the issue well enough :D.
I think adding some brackets around MMU_PAGE_SIZE * (mapped_pages + 1) might help for clarity.
There was a problem hiding this comment.
Given the bootloader mapped this
0 MAPPED to PAGE 0 at ORIGIN + 0x00000 - 0x0ffff
1 MAPPED to PAGE 1 at ORIGIN + 0x10000 - 0x1ffff
2 MAPPED to PAGE 2 at ORIGIN + 0x20000 - 0x2ffff
We would end-up with with mapped_pages = 2
Then we need (2 +1) * 0x10000 to get ORIGIN + 0x30000
But I agree it looks awkward - will change to make this less weird
|
Is there any way to test this? |
You mean in hil-testing? I guess it's hard to come up with a test case which is producing such a gap and any change to any of the crates involved can potentially remove the gap. We could force it via modified linker scripts but currently we don't have a way to override the linker scripts in tests |
esp-hal/src/soc/esp32s3/psram.rs
Outdated
| // calculate the PSRAM start address to map | ||
| let mut start = EXTMEM_ORIGIN; | ||
| // calculate the PSRAM start address to map - honor the fact there might be | ||
| // unmapped pages in between |
There was a problem hiding this comment.
To be quite honest, since I don't know anything about this problem, this comment doesn't tell me anything. Where are unmapped pages (i.e. what does "in between" mean here), and why are they a problem? I need to read the old code and the diff together to make sense of this and given that we don't have tests, I think we need some better explanations around this.
There was a problem hiding this comment.
I tried to explain things a bit better - happy to get suggestions to explain it better
There was a problem hiding this comment.
I simply don't understand this part of the system enough. I don't understand why we're counting flash pages or what the relationship is between ICACHE and PSRAM, when we seem to be mapping PSRAM for the data bus, which is backed by DCACHE.
I also don't know where anything related is documented, a pointer to that would also be helpful.
There was a problem hiding this comment.
mhhh there is not too much information about the MMU unfortunately - hope the link helps a bit
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Fixes a problem in calculating the PSRAM start address on ESP32-S3 when there is a gap in the mapped flash pages. See #2739 (reply in thread)
While it's probably a rare situation it apparently happens in the wild. At least the reproducer linked in the issue shows that
skip-changelog? I can add it if we think we should include itAt some point we probably should double check the linker scripts since creating that gap isn't necessary.
We might also want to reconsider using a fixed address instead, but this fix shouldn't harm in any way.
Testing
Getting into that situation is not easy however the code should be easy enough and all examples are still working