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

Feature: AT32F435/F437 flash programming #1547

Merged
merged 3 commits into from
Oct 29, 2023

Conversation

ALTracer
Copy link
Contributor

Detailed description

  • This can be considered a new feature, but in essence is old code cloned/ported/refactored to suit Artery targets.
  • The previous PR Feature: Identify AT32F435/437 #1540 attached stm32f1_flash routines to AT32F435 targets and this doesn't work.
  • This PR implements mandatory erase/write routines specifically for AT32F43x so that it's actually possible to reprogram target flash.

ARTERY AT32F435 (and F437) series chips have a flash controller which is similar in operation to stm32f1 but have its register base at 0x40023C00 like stm32f4. It has even sectors (all the same size, 2048 or 4096 byte pages) unlike most stm32f4 (or gd32f4) targets, and requires selecting sectors to erase by their address, not sector number. I considered using code in one of these target drivers but the difference are too significant.

I ended up porting relevant routines from stm32f1, incorporating runtime dual-bank split argument passing (for controller register base and for physical address split in internal flash) and updating naming/code style where I noticed it. Some of stm32f4 code was helpful but not directly applicable.

Tested on AT32F437ZMT7 (4032 KiB in 4 KiB pages) of AT-START-F437 by wiping and loading a glorified blinky project occupying 12 KiB using AT32IDE with its xpack gdb, (and in ST 10.3 GNU MCU Tools GDB, also Ubuntu/jammy gdb-multiarch).
Dual bank functionality tested by invoking hosted app to write a 3.5 MiB datasheet DS_AT32F435_F437_EN.pdf into the target, and then verify, both of which passed (at like 20 KiB/s speed).

Mass erase takes 7 seconds per 2 MiB bank, as the datasheet says, and BMF (blackpill-f411ce) indicates "progress" with dots twice per second. Reprogramming with BMF (or verifying compare-sections) displays performance of 15-19 KiB/s (15 for JTAG, 19 for SWDP), which is twice as fast compared to onboard AT-Link-EZ in CMSIS-DAP mode implementing SWD-only at "5000 kHz" when used with Arterytek-patched OpenOCD (sources?) from AT32IDE.

I did not aim to implement logic for option bytes ("USD, user data"), EPPS erase/program protection, SLib, NZW/ZW split manipulation and other Artery-specific features. These can be implemented later by interested contributors.

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

fixes #1542

@dragonmux dragonmux added the Enhancement General project improvement label Jul 19, 2023
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.

This is looking great. There are a couple of items we've caught in review, including a simplification of the Flash routines for a little extra space savings over the work you were sharing in Discord regarding the part_id switch-case and other bits.

With these items addressed, this looks good good to merge.

src/target/at32f43x.c Outdated Show resolved Hide resolved
src/target/at32f43x.c Outdated Show resolved Hide resolved
src/target/at32f43x.c Outdated Show resolved Hide resolved
src/target/at32f43x.c Outdated Show resolved Hide resolved
@dragonmux
Copy link
Member

Additional review note: Per the contribution guidelines, the target/ prefix on the target reference in the commit message is not required, at32f43x: instead of target/at32f43x: is sufficient as a prefix

src/target/at32f43x.c Outdated Show resolved Hide resolved
src/target/at32f43x.c Outdated Show resolved Hide resolved
src/target/at32f43x.c Outdated Show resolved Hide resolved
src/target/at32f43x.c Outdated Show resolved Hide resolved
src/target/at32f43x.c Outdated Show resolved Hide resolved
src/target/at32f43x.c Outdated Show resolved Hide resolved
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.

From a code perspective, this looks good. Please fix up your commit messages as noted in a previous review and apply the review fix items from the last 3 commits as fixups over the others in the series. We'll approve this for merge once that's done.

* lpc40xx_probe performs IAP calls specific to lpc40xx
  which crashes at32f435 targets.

* Rename to at32f40x_probe (the stm32f1-compatible flash ones)
* Create a separate translation unit src/target/at32f43x.c
  to implement flash operations differently (from stm32f1 or stm32f4)
* Move the at32f43_detect() here from src/target/stm32f1.c
  and tie it to new at32f43x_probe() instead
* Stub unimplemented operations with NULL pointers
* Code inspired by stm32f1 and partially stm32f4 target code
* Code heavily refactored to conform to modern target flash API
  (_prepare/_done, less looping and simpler _erase/_write)
* Most ops operate on their respective bank only, referencing
  the controller register offset from flash_priv
* _mass_erase still touches both banks, if present
* Rename old short arguments and variables on cloning, rename macros
@dragonmux dragonmux added this to the v2.0 release milestone Oct 16, 2023
@dragonmux dragonmux added the Bug Confirmed bug label Oct 16, 2023
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, we'll get this merged at the very start of the v2.0 merge window opening. Many thanks for your hard work on this contribution!

@dragonmux dragonmux merged commit 8715212 into blackmagic-debug:main Oct 29, 2023
6 checks passed
@ALTracer ALTracer deleted the at32f43x branch November 22, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bug Enhancement General project improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AT32F4 flash handling
3 participants