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.

Sign up for GitHub

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

Jump to bottom

qspi erase optimization #576

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

qspi erase optimization #576

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
95b8aa3
qspi block erase
eh2k Mar 25, 2023
2461b7b
Update qspi.cpp
eh2k Apr 24, 2023
bbd0f95
USE_STM32_USB_HOST, USE_STM32_USB_DEVICE
eh2k Jan 12, 2023
7e11f64
USBH https://github.com/STMicroelectronics/STM32CubeH7 fd79630
eh2k Jan 17, 2023
244ec3a
USBH_MSC_Yield
eh2k May 6, 2023
3832801
bootloader_v61
eh2k Nov 26, 2023
bc1e4ff
fix lds ranges + makefile ext
eh2k Nov 30, 2023
106b869
dsy_pin_cmp cost args
eh2k Dec 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 35 additions & 25 deletions src/per/qspi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class QSPIHandle::Impl

QSPIHandle::Result Erase(uint32_t start_addr, uint32_t end_addr);

QSPIHandle::Result EraseSector(uint32_t address);
QSPIHandle::Result EraseCmd(uint32_t address, uint32_t instruction);

uint32_t GetPin(size_t pin);

Expand Down Expand Up @@ -330,39 +330,49 @@ QSPIHandle::Impl::Write(uint32_t address, uint32_t size, uint8_t* buffer)
QSPIHandle::Result QSPIHandle::Impl::Erase(uint32_t start_addr,
uint32_t end_addr)
{
uint32_t block_addr;
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;
}
Copy link
Contributor

@ndonald2 ndonald2 Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@eh2k eh2k Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ndonald2 ndonald2 Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@eh2k eh2k Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndonald2,

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.

}
else
{
uint32_t block_addr;
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)
{
block_addr = start_addr & 0x0FFFFFFF;
if(EraseCmd(block_addr, SECTOR_ERASE_CMD) != QSPIHandle::Result::OK)
{
ERR_RECOVERY(Status::E_HAL_ERROR);
}
start_addr += block_size;
}
start_addr += block_size;
}
return QSPIHandle::Result::OK;
}


QSPIHandle::Result QSPIHandle::Impl::EraseSector(uint32_t address)
QSPIHandle::Result QSPIHandle::Impl::EraseCmd(uint32_t address,
uint32_t instruction
= SECTOR_ERASE_CMD)
{
uint8_t use_qpi = 0;
QSPI_CommandTypeDef s_command;
if(use_qpi)
{
s_command.InstructionMode = QSPI_INSTRUCTION_4_LINES;
s_command.Instruction = SECTOR_ERASE_QPI_CMD;
s_command.AddressMode = QSPI_ADDRESS_4_LINES;
}
else
{
s_command.InstructionMode = QSPI_INSTRUCTION_1_LINE;
s_command.Instruction = SECTOR_ERASE_CMD;
s_command.AddressMode = QSPI_ADDRESS_1_LINE;
}
s_command.InstructionMode = QSPI_INSTRUCTION_1_LINE;
s_command.Instruction = instruction;
s_command.AddressMode = QSPI_ADDRESS_1_LINE;
s_command.AddressSize = QSPI_ADDRESS_24_BITS;
s_command.AlternateByteMode = QSPI_ALTERNATE_BYTES_NONE;
s_command.DataMode = QSPI_DATA_NONE;
Expand Down Expand Up @@ -898,7 +908,7 @@ QSPIHandle::Result QSPIHandle::Erase(uint32_t start_addr, uint32_t end_addr)

QSPIHandle::Result QSPIHandle::EraseSector(uint32_t address)
{
return pimpl_->EraseSector(address);
return pimpl_->EraseCmd(address, SECTOR_ERASE_CMD);
}

void* QSPIHandle::GetData(uint32_t offset)
Expand Down