#393 STM32F2: Protecting OTP fails (code inverts lock value)
The STM32F2x flash driver correctly reads protection bytes for the OTP memory, but fails to write them. Looking at the code, it seems the stm32x_otp_protect read protect function uses the correct meaning of the lock bytes (0x00 == locked, any other value == unlocked), while stm32x_otp_protect actually tries to write 0xff to enable protection, which is incorrect.
See the code here: https://github.com/openocd-org/openocd/blob/91bd4313444c5a949ce49d88ab487608df7d6c37/src/flash/nor/stm32f2x.c#L510-L557
The documentation (PM0059: STM32F205/215, STM32F207/217 Flash programming manual) says:
The OTP area is divided into 16 OTP data blocks of 32 bytes and one lock OTP block of 16
bytes. The OTP data and lock blocks cannot be erased. The lock block contains 16 bytes
LOCKBi (0 ≤ i ≤ 15) to lock the corresponding OTP data block (blocks 0 to 15). Each OTP
data block can be programmed until the value 0x00 is programmed in the corresponding
OTP lock byte. The lock bytes must only contain 0x00 and 0xFF values, otherwise the OTP
bytes might not be taken into account correctly.
I've tested this on an F4, which uses the same driver (and has OTP that works the same way). Reading the lock bytes works correctly (this chip has one OTP sector locked already):
matthijs@dottie:~/docs/MKIT/3Devo/ChildbusBootloader$ openocd -f interface/stlink.cfg -f target/stm32f4x.cfg -c 'init; reset init; flash info 1'
Open On-Chip Debugger 0.12.0
[Snip more output]
#1 : stm32f2x at 0x1fff7800, size 0x00000200, buswidth 0, chipwidth 0
# 0: 0x00000000 (0x20 0kB) protected
# 1: 0x00000020 (0x20 0kB) not protected
# 2: 0x00000040 (0x20 0kB) not protected
# 3: 0x00000060 (0x20 0kB) not protected
[Snip more sectors]
STM32F4xx (Low Power) - Rev: Z
However locking sector 1 fails:
matthijs@dottie:~/docs/MKIT/3Devo/ChildbusBootloader$ openocd -f interface/stlink.cfg -f target/stm32f4x.cfg -c 'init; reset init; flash protect 1 1 1 on'
Open On-Chip Debugger 0.12.0
[Snip more output]
Error: failed setting protection for blocks 1 to 1
I've attached a complete log with -d3 to show some more details.
Looking at the code for this:
for (i = first; first <= last; i++) {
retval = target_read_u8(target, lock_base + i, &lock);
if (retval != ERROR_OK)
return retval;
if (lock)
continue;
lock = 0xff;
retval = target_write_u8(target, lock_base + i, lock);
if (retval != ERROR_OK)
return retval;
}
return ERROR_OK;
}
You can see that the if (lock) is inverted (it is locked when it is 0, not non-zero), so it skips any unlocked blocks and will only try to lock blocks that are already locked. You would expect it to then just silently fail and not raise an error, but (thanks for gdb to help me here), it turns out the loop is actually an infinite loop, since it checks first <= last instead of i <= last, so that's another bug. Since i does increment, this means it reads all the lock bytes, then system memory and then tries to read past the end of system memory (0x1fff8000) and fails, breaking the loop and returning error.
Looks like this code has been broken since when it was first introduced in f21c12abec and maybe was never tested (which is of course tricky with OTP...).
I will see if I can provide a patch to fix this - I have some sacrificial boards that I can use for testing.
I patched the code to fix this, and further found that:
- The code also did not do an unlock sequence and did not set the FLASH_PG bit, so the write would also silently fail.
- The code would allow protecting OTP sectors without requiring the otp enable command, as is required before doing regular writes to the main OTP memory. It would probably be better to require this, to minimize the chance of doing any unintentional OTP writes.
- The STM32l4x code also supports OTP, but that OTP hardware does not have separate protection bytes (instead, every 64-bit block is automatically protected from further writes once any bit is written to 0). So this issue does not exist in that code.
I've submitted a patch to fix this here: https://review.openocd.org/c/openocd/+/7612
I've also submitted three related patches, one to require enabling OTP writes before OTP protection can be set, one to fix a typo in a message and one to improve the message when trying to undo OTP protection (which is impossible).
The second patch is shown as having a merge conflict, but AFAICS that's only because it depends on the first patch and Gerrit does not take that into account. And the third patch shows a build failure, but that only seems to be because the first build was aborted because I pushed an update to a commit message before it was completed, a second build (in the same comment?) was successful AFAICS. I wanted to comment these things on the patchsets themselves in Gerrit, but I could not find any place where I could just leave a comment about the patchset. I'm really not used to using Gerrit for tracking patches, so my github-PR-mindset is probably approaching this wrong....