mmap: Add write functionality #70
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@enjoy-digital should the CSR functionality also be tested? If so, is there any recommended approach? |
m-byte
force-pushed
the
psram
branch
2 times, most recently
from
103acba
to
4840368
Compare
With this change, it is possible to memory map e.g. PSRAM. When setting with_write to "csr", it can also be used to support faster flash writing via etherbone. For flash, only False and "csr" should be used, as the some other logic or application needs to make sure that the flash is erased ahead of time. True is only meant for use with RAM chips. Signed-off-by: Matthias Breithaupt <m.breithaupt@vogl-electronic.com>
With this change, it is possible compensate the memory map origin using offset. E.g. for spi flash, the following line would suffice `self.comb += self.spiflash_core.mmap.offset.eq(self.bus.regions.get(name, None).origin)` Signed-off-by: Matthias Breithaupt <m.breithaupt@vogl-electronic.com>
This change prevents accidental shorts during the wait period in QSPI, as the SoC should not be driving data lines while waiting for the memory chip to get ready. Signed-off-by: Matthias Breithaupt <m.breithaupt@vogl-electronic.com>
Add a minimal test for mmap write Signed-off-by: Matthias Breithaupt <m.breithaupt@vogl-electronic.com>
Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
|
@enjoy-digital do you think you would find some time to review this soon? |
|
@m-byte: Thanks for the PR and sorry for the delay...! I'll review it before the end of the week. |
enjoy-digital
approved these changes
Jul 31, 2024
There was a problem hiding this comment.
Thanks @m-byte, this make sense and the code look fine, let's merge. Thanks also for the test-bench, really appreciated and help a lot for future maintenance!
|
Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this change, it is possible to memory map e.g. PSRAM. When setting with_write to "csr", it can also be used to support faster flash writing via etherbone. For flash, only False and "csr" should be used, as the some other logic or application needs to make sure that the flash is erased ahead of time. True is only meant for use with RAM chips.
There are two bugs that this also addresses:
Addresses are now masked to flash size or addr_width if no size is set. I'm not 100% happy with this solution, as it might cause issues with non power of 2 flashes (do those exist). However, without this, I would get flash writes to the wishbone address rather than offsets inside the flash. So I'm open to better alternatives. Optimum would be subtracting the base bus address, but I haven't found any clean way of accessing it.I found a way to wire the origin through, so now this can be fixed by wiring through the corresponding addresses (described in the commit message and I will create a PR for LiteX, once this is merged)Let me know if they should be part of separate
commits/PRs.Currently, tests are still to be added.This should fix #25