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: Flash blank check #1971

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ALTracer
Copy link
Contributor

Detailed description

  • This is a small new feature in the framework of Target Flash API.
  • The existing problem is it's hard to check how much of flash is empty (only via (gdb) x/16zw 0x08004000 etc.)
  • This PR solves it by implementing and providing a target remote monitor command to run a blank-check against a connected halted target.

Many alternative gdbservers already implement a similar mechanism -- openocd flash, pyocd, Segger JLinkGDBServer, etc.
I opted to reuse the whatever flash write buffer is allocated in BMP heap for fast block reads (like bmd_crc32). The memcmp could be optimized to run in 4 bytes per comparison, but for a quick PoC 1 byte is enough, I get I/O bounded on swdptap.c (100-150 KiB/s for FS probes).

Tested on BMDA for dual blackpills, as usual, over JTAG transport, example output is

$ ./build/blackmagic -j -f 8M -M blank_check
DPv0 detected based on JTAG IDCode
Blank 0x08020000+131072
Blank 0x08040000+131072
Blank 0x08060000+131072
Blank 0x08010000+65536
Has data at 0x08000000
Blank 0x08004000+16384
Blank 0x08008000+16384
Blank 0x0800c000+16384

Notice how it walks the singly linked flash bank list backwards, but walks eraseblocks forwards. I erased the BMF from DUT, keeping only the BMD bootloader on it (in first 16 KiB). Output in GDB is similar, but there are progress dots printed before some lines.

Needs testing on smaller probes. Future usage could be skipping vFlashErase-triggered erase delays if reading memory-mapped flash is faster than waiting on it to erase (break-even is different for genuine STM32 with true simultaneous mass-erase vs. The Compatibles which back it with some sequential slow QuadSPI-like flash). It could also have interesting interactions with mass-erase -> load (skips all slow sector erases and starts writing to empty flash immediately), eliminating some unresponsiveness.

Your checklist for this pull request

Closing issues

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 has some good ideas and we like the concept. The review items are mostly all about code style and clarity.

platform_timeout_s timeout;
platform_timeout_set(&timeout, 500);

while (len) {
Copy link
Member

Choose a reason for hiding this comment

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

We think this would be better written as a for loop - for (size_t offset = 0U; offset < len; offset += flash->writebufsize) - so there aren't any weird end of loop conditions and src and len get to be const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was for const size_t local_len = MIN(flash->writebufsize - offset, len); in case a flash bank capacity is not an even/integer multiple of writebufsize. If there is such a guarantee, then I think I can simplify into for-loop.

Copy link
Member

Choose a reason for hiding this comment

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

That is a guarantee you can depend on. Even if it wasn't, MIN(len - offset, flash->writebufsize) for local_len would still give you the right answer with the suggested restructure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this form then? 8c8db53

Copy link
Member

Choose a reason for hiding this comment

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

That looks considerably better!

platform_timeout_set(&timeout, 500);

while (len) {
const size_t offset = src % flash->writebufsize;
Copy link
Member

Choose a reason for hiding this comment

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

Making this a for loop makes this calculation disappear and eliminates an expensive % call

src/target/target_flash.c Outdated Show resolved Hide resolved
src/target/target_flash.c Outdated Show resolved Hide resolved
@ALTracer ALTracer force-pushed the feature/blank-check branch 2 times, most recently from 8c8db53 to aeb352c Compare October 24, 2024 09:30
@dragonmux dragonmux added the Enhancement General project improvement label Oct 26, 2024
* Reuse flash write buffer for block reads
* Compare against erased value and skip to next sector on mismatch
* Indicate progress via tc_printf() to gdb_if.c or BMDA stdout
@ALTracer ALTracer force-pushed the feature/blank-check branch from aeb352c to c1b2c0a Compare November 21, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement General project improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants