#437 Wrong sector size for w25q16jv / RP2040 pico
in line 305 of src/flash/nor/rp2040.c (current git) there is this comment:
/*
The RP2040 Boot ROM provides a _flash_range_erase() API call documented in Section 2.8.3.1.3:
https://datasheets.raspberrypi.org/rp2040/rp2040-datasheet.pdf
and the particular source code for said Boot ROM function can be found here:
https://github.com/raspberrypi/pico-bootrom/blob/master/bootrom/program_flash_generic.c
In theory, the function algorithm provides for erasing both a smaller "sector" (4096 bytes) and
an optional larger "block" (size and command provided in args).
*/
I have verified the erase function from the boot rom of the RP2040. That is the same function openocd uses here. And I found that it is able to erase a 4096 byte sector. So the comment is right.
I wrote something before and after and in a sector and then called _flash_range_erase() from the boot-rom to erase the one 4096 bytes sized sector and only the bytes in the sector have been erased. The data before and after were still there. So erasing 4096 bytes is not theory anymore.
I therefore think that line 55 in src/flash/nor/rp2040.c and line 123 (129?) in src/flash/nor/spi.c should be changed. They specify the sector size to 0x10000 (64k) and should be 1000(4k)
What else can I do to get this change merged, or is there a reason not to do this?
Most SPI flash do support various "sector" sizes, 4kB is sometimes called subsector. OpenOCD uses for bigger flash always the largest available size: Erasing a substancial part by subsector erase is usually much slower than by the larger sectors. E.g. for MT25QL512ABB: subsector 4kByte typ. 50ms, 64kByte typ. 150ms, so roughly factor 5. If you can't live with the larger sector size, these alternatives came to my mind:
1) Add corresponding subsector size/erase command to spi.c for all devices where applicable and a a suitable subsector erase command to the various flash drivers. Note that the "auto erase" option would still use the "old" erase command if you don't add a suitable strategy there.
2) Change all sector erase commands and sector sizes in spi.c to the appropriate subsector variants. But be prepared for some polite complaints that programming time increased by a factor of four ...
3) Add a special command to send arbitrary command bytes to the flash, see e.g. "stmqspi cmd" in the stmqspi flash driver or a command to override the flash parameters in spi.c like "stmqspi set" ibid. That might be useful for other purposes as well.
I’m not sure if I understood your answer. I might have been unclear in my description.
I now understand that it is not as easy as I suggested (just a change of 3 lines). I assumed that the setting was the smallest possible erase block. I did not realize that it also defines the only erase block size. Thank your for pointing that out to me. I have only a limited understanding of your source code. I probably had something in mind that is currently not implemented.
My use case for example is a firmware that is 65kB in size. According to your numbers that now takes two erase cycles of 64kB each. I would suggest to do only one cycle of 64kB and another cycle of 4kB. 2x64kB = 2x150ms = 300ms; 1x64kB+1x4kB = 150ms + 50ms = 200ms. The suggested variant would be 100ms or 1/3 faster.
The other issue is that my firmware uses the flash space behind the firmware. With the current scheme 63kB behind the firmware are erased with every programming. With the new scheme only 3kB would be erased alongside a firmware update. To be able to modify the data the firmware already has to align it to sector borders. So the 3kB will be lost anyway. But the additional 60kB could become better usable with this change.
I don’t understand why such a feature needs to be rolled out to all devices. I would assume that some devices only have one kind of sector. After implementing this feature the device information needs to be updated. The information about the big and little sectors needs to be added. If we keep all existing devices in the “has only one sector size” category then those devices would not get affected. With careful testing, device by device, we could then add more devices to the “has two sector sizes” group.
I can understand if you don’t want such a big change for such a limited use case.
There are provisions (e.g. in SFDP data) for up to 4 differerent (erase) sector sizes. And most (all?) higher capacity devices have differerent sector sizes to choose from. There are even devices where the large erase sector size can be switched by some non-volatile register. Infineon came up with such a "fancy" device ...
So, somewhat chaotic, I'd say.
Of course, it's possible to add an additional erase block size for only one device. But this is rather non-systematic and additionally it doesn't solve the problem: How would you select one of these sizes? Automatically, or by a different high level command? Depending on the usage model, one or the other choice (or even a combination, as you suggested) would fit better, so an automatic approach seems to be difficult.
That's why I'd suggest the third alternative. You could more or less copy-and-paste the relevant code and docs from e.g. stmqspi driver.
And the last point: "... we could then add ...". Who is "we"?
Automatically. Start with the biggest sizes. Once the rest of the firmware is too small for another block of the biggest size try with the next smaller size. Use the smallest size for the left over bytes.
I looked at the code but couldn't figure out the relevant parts. Therefore right now that is not possible for me to do.
The OpenOCD developer community and me.
But again: I understood that you (the OpenOCD developers) don't want this change. And that is OK for me.
Please note that there is a bunch of pending patches adding RP2350 support.
The new RP2xxx driver code uses fixed 4k sector size for both RP2040 and RP2350. See e.g.
8453: flash/nor/rp2040: improve flash write buffer size computation | https://review.openocd.org/c/openocd/+/8453
Checkout whole relation chain under
8461: tcl/target/rp2040: add flash size override and reset init event | https://review.openocd.org/c/openocd/+/8461
and give them a try.
In my understanding boot rom functions use 4K only, but I might be wrong. The code is here:
https://github.com/raspberrypi/pico-bootrom-rp2040/blob/master/bootrom/program_flash_generic.c
I tested both linked patches, the current git and the packaged version of my distro. But did not see significant differences. (log attached).
All versions show this warning:
same tests without the adapter speed warning. (0.12.0 is a bit slower than current git and patches)
But still with the crucial warning: