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.

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

target/stm32f1: fix split-bank erase #1719

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

tlyu
Copy link
Contributor

@tlyu tlyu commented Jan 8, 2024

Add a missing bank_offset when checking for bank unlock completion.

Detailed description

Discovered while testing #1709 on a GD32F303CGT6 target. It probably affects all split-bank STM32F1 parts or compatibles, as long as they have split flash banks.

The check for flash bank unlock completion failed for bank 2, because it was checking the register for bank 1. This probably went unnoticed for quite a while, because bank 2 is likely used for application data storage instead of code.

Only tested against GD32F303CGT6, which has split flash banks. Testing could only occur in conjunction with #1709, with which it merged cleanly. I don't have access to any other affected targets.

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

Closing issues

N/A

Sorry, something went wrong.

Add a missing `bank_offset` when checking for bank unlock completion.
@dragonmux dragonmux added this to the v2.0 release milestone Jan 9, 2024
@dragonmux dragonmux added the Bug Confirmed bug label Jan 9, 2024
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

LGTM, and based on reading TRMs and the various PR threads on it, this looks like the right fix. We'll merge this once ALTracer's PR is merged. Thank you for the contribution!

Edit: After talking with ALTracer about order of operations, we're going to merge this one first so he can better test the other PR.

@dragonmux dragonmux merged commit 233066b into blackmagic-debug:main Jan 9, 2024
6 checks passed
@ALTracer
Copy link
Contributor

ALTracer commented Jan 9, 2024

Adding supporting evidence for this PR. The key to reproducing the behaviour was to shrink all loadable sections (via linkerscript) to target only bank 2.
An AT32F403ACGU7 target (as found on "WeActStudio.BlackPill", not to be confused with "MiniSTM32F4x1") used to trigger an Error erasing flash with vFlashErase packet. GDB message, because bank 1 had not been unlocked by the write into bank 2 key register or by prior operations (like loads). Or these BMDA diagnostic logs:

stm32f1_flash_erase: at 08080000
unlock failed, cr: 0x00000080
Erase failed at 8080000

So the Arterytek families of stm32f1-quacking flash targets were affected at least as far as #1685 .
Testing uploads on this commit confirms correct unlocking/checking now.

@tlyu tlyu deleted the fix/stm32f1-split-erase branch January 9, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants