qspi erase optimization #576
Conversation
|
Hi @eh2k thanks for the PR! This is definitely a useful update! |
There was a problem hiding this comment.
Hi @eh2k, thanks so much for this contribution!
I'm assisting Electrosmith with some PR reviews. I have been doing some testing of this change this morning and it does seem to be working as intended, and makes a huge difference in erase performance, so that's awesome!
I left one inline comment requesting a change regarding the strategy for minimizing the number of erased sectors, please let me or @stephenhensley know if you have any questions about that.
Also, as part of this PR would you mind taking a stab at updating the doc comment for QSPIHandle::Erase? Currently it says Erasures will happen by 4K, 32K or 64K increments. which is obviously not true (wasn't true before either 😅).
Thanks so much!
src/per/qspi.cpp
Outdated
| uint32_t block_size = IS25LP080D_SECTOR_SIZE; // 4kB blocks for now. | ||
| // 64kB chunks for now. | ||
| start_addr = start_addr - (start_addr % block_size); | ||
| while(end_addr > start_addr) | ||
| if((end_addr - start_addr) >= IS25LP080D_BLOCK_SIZE) | ||
| { | ||
| block_addr = start_addr & 0x0FFFFFFF; | ||
| if(EraseSector(block_addr) != QSPIHandle::Result::OK) | ||
| uint32_t block_addr; | ||
| uint32_t block_size = IS25LP080D_BLOCK_SIZE; | ||
| start_addr = start_addr - (start_addr % block_size); | ||
| while(end_addr > start_addr) | ||
| { | ||
| ERR_RECOVERY(Status::E_HAL_ERROR); | ||
| block_addr = start_addr & 0x0FFFFFFF; | ||
| if(EraseCmd(block_addr, BLOCK_ERASE_CMD) != QSPIHandle::Result::OK) | ||
| { | ||
| ERR_RECOVERY(Status::E_HAL_ERROR); | ||
| } | ||
| start_addr += block_size; | ||
| } |
There was a problem hiding this comment.
By checking that the requested erase range is larger than a block only once at the start of the method, and then looping over block erase commands, that means as long as the requested erase range is larger than a block then we are only erasing in blocks, not in a combo of blocks and sectors.
For example if an erase of an address range corresponding to 68 kB (one block + one sector) is requested, with this change we will be erasing a minimum of 128kB (two blocks) which isn't obvious and may in some cases inadvertently erase data that wasn't meant to be erased.
Would you be able to rewrite this a little bit so it's taking that into account and erasing the minimum possible amount of data while still erasing at a granularity of blocks when applicable?
Ideally, the method would also be smart about rounding down to the nearest sector if the requested erase range is greater than a block but the start address doesn't align to be within the first sector of a block. E.g. if we request an erase starting at offset 0x8000 with a length >= 1 block, we would erase the sector at offset 0x8000 before erasing the block at offset 0x10000 (if applicable) rather than rounding down to erase the entire block starting at 0x0.
There was a problem hiding this comment.
I have added the following constrain to the if block ((end_addr - start_addr) % IS25LP080D_BLOCK_SIZE) == 0.
Thus the BLOCK_ERASE should only take place if the memory area is exactly multiple of BLOCK_SIZE.
Personally, I would prefer to tag the Erase method OBSOLETE, and replace it with a public EraseCmd.
By the way - the fix_style.sh makes too many modifications in my environment - which in turn are reported in the github CI.
There was a problem hiding this comment.
I have added the following constrain to the if block ((end_addr - start_addr) % IS25LP080D_BLOCK_SIZE) == 0.
Thus the BLOCK_ERASE should only take place if the memory area is exactly multiple of BLOCK_SIZE
This definitely mitigates the over-erase issue, but it also means that unusually sized erases will still be slow. As a compromise, what about moving the size check to determine block vs sector erase to inside the while loop, so we're comparing the remaining erase range each time rather than the entire original range, to decide whether to erase a block or sector?
Personally, I would prefer to tag the Erase method OBSOLETE, and replace it with a public EraseCmd.
I can see where you're coming from. However given that would (eventually) be a breaking change I would imagine that would require a little more review and caution.
By the way - the fix_style.sh makes too many modifications in my environment - which in turn are reported in the github CI.
If I remember correctly this is a known problem (happens to me too). Don't worry about it, as long as the files your changes are in pass the lint check we're good.
There was a problem hiding this comment.
As a companion to the more generalized public Erase method and the existing public EraseSector method, there's also the option of adding a public EraseBlock method that calls into EraseCmd to erase the whole block at the specified offset.
There was a problem hiding this comment.
Ok, an public EraseBlock method would also be a good alternative. The Erase method, in general, is and was a bit dangerous and confusing if you don't know what's going on internally - the address range had to correspond to the 4KB sectors, and could not be arbitrary...
There was a problem hiding this comment.
thanks for your feedback.
I just looked at the EraseSector method again - actually any address checking is missing there. If we want to introduce an additional EraseBlock method, we have to consider that there is a BLOCK_ERASE_CMD (64KB) and a BLOCK_ERASE_32K_CMD. In short, I want to contribute only improvements that I really use and where I can say that it works - all other special cases with arbitrary addresses I do not want to handle.
eh2k
force-pushed
the
master
branch
2 times, most recently
from
b554ba9
to
087179a
Compare
Hello,
Erasing larger memory areas takes a long time, because in the current implementation only single 4kb sectors are erased in a loop.
In this pull request, depending on the erasing memory range, either the previous SECTOR_ERASE_CMD (4kb) or the much faster BLOCK_ERASE_CMD (64kb) instruction is used. See QSPIHandle::Impl::Erase...
The issue was also discussed here https://forum.electro-smith.com/t/external-flash-erase-speed/1268/6.
eh2k