periph_flashpage: Make pagewise API optional by bergzand · Pull Request #15412 · RIOT-OS/RIOT

bergzand · 2020-11-09T16:02:29Z

Contribution description

flashpage currently requires pagewise implementation with an optional
extension for per block writes (flashpage_raw). Most implementations
with flashpage_raw implement the pagewise access via the flashpage_raw
functions. This commit makes the flashpage raw the main access method
and adds an extension feature for the pagewise access.

The functions and defines are renamed to reflect this. The API is also
extended with a dedicated function for erasing a sector.

Testing procedure

Preferably every CPU family should be tested with tests/periph_flashpage

Issues/PRs references

~~Depends on #15410~~

aabadie · 2020-11-09T17:14:19Z

When I see the HAS_PERIPH_FLASHPAGE_PAGEWISE new name, I even wonder why the base feature is called periph_flashpage, why not simply rather call it periph_flash ?
Then the new pagewise variant would be called periph_flash_pagewise.

cpu/cc2538/periph/flashpage.c

cpu/msp430_common/periph/flashpage.c

fjmolinas · 2020-11-09T17:39:05Z

samr21-xpro (sam0) OK

>

> test_last_raw

>  test_last_raw
wrote raw short buffer to last flash page
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
>

openmote-b (cc2538) OK

> test_last_raw

>  test_last_raw
wrote raw short buffer to last flash page
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
>

dwm1001 (nrf5x) OK

> test_last_raw

>  test_last_raw
wrote raw short buffer to last flash page
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
>

nucleo-l152re (stm32) OK

test_last_raw

>  test_last_raw
wrote raw short buffer to last flash page
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
>

frdm-kw41z (kinetis) OK

> test_last_raw

>  test_last_raw
wrote raw short buffer to last flash page
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
>

z1 (msp430) OK (with patch)

> test_last_raw

>  test_last_raw
The last page holds the ISR vector, so test page 101
wrote raw short buffer to last flash page
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
> help
 help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test_last_raw        Write and verify raw short write on last page available
>

fjmolinas · 2020-11-09T17:41:16Z

@basilfx @benpicco have efm32 I think.

bergzand · 2020-11-09T18:14:52Z

why not simply rather call it periph_flash ?
Then the new pagewise variant would be called periph_flash_pagewise.

I fully agree! Anything against this? I can do the rename tomorrow during the sprint day.

aabadie · 2020-11-09T18:21:31Z

Anything against this?

Only thing I see is that it would be a massive api change: I think that all functions in the API and files, and includes would have to be renamed as well (flashpage_write => flash_write, periph/flashpage.h => periph/flash.h etc).
But I'm not against it.

bergzand · 2020-11-09T18:29:19Z

Only thing I see is that it would be a massive api change: I think that all functions in the API and files, and includes would have to be renamed as well (flashpage_write => flash_write, periph/flashpage.h => periph/flash.h etc).

I already had to rename all functions because of the flashpage_write ⇒ flashpage_write_page, I think I can manage a few more renames :)

bergzand · 2020-11-10T09:13:31Z

rebased to exclude the commit from #15410

bergzand · 2020-11-10T09:49:11Z

Only thing I see is that it would be a massive api change: I think that all functions in the API and files, and includes would have to be renamed as well (flashpage_write => flash_write, periph/flashpage.h => periph/flash.h etc).

Decided offline to postpone this to a future PR.

bergzand · 2020-11-10T20:57:00Z

needs a rebase wink

Rebased!

benpicco

looks good, mostly mechanical translations

benpicco · 2020-11-10T23:06:36Z

cpu/nrf52/include/cpu_conf.h

 * block is always FLASHPAGE_SIZE.
 */
-#define FLASHPAGE_RAW_BLOCKSIZE    (4U)
+#define FLASHPAGE_BLOCK_SIZE    (4U)


Since you are doing a rename anyway, how about calling this FLASHPAGE_WRITE_BLOCK_SIZE

benpicco · 2020-11-10T23:15:32Z

cpu/cc2538/periph/flashpage.c

-        flashpage_write_raw(page_addr, data, FLASHPAGE_SIZE);
+        flashpage_write(page_addr, data, FLASHPAGE_SIZE);
    }
 }


We can now have a generic flashpage_write_page() implementation in drivers/periph_common/flashpage.c

void flashpage_write_page(unsigned page, const void *data) { assert((unsigned) page < FLASHPAGE_NUMOF); flashpage_erase(page); /* write page */ if (data != NULL) { flashpage_write(flashpage_addr(page), data, FLASHPAGE_SIZE); } }

Good idea. I can add this here (or in a follow up) and guard it to only include it when FLASHPAGE_SIZE is available (it is not available on the stm32f{2,4,7}) and allow CPU implementations to override it if they have an optimized version available.

Done and deduplicated

cpu/cc2538/Makefile.features

cpu/cc2538/include/cpu_conf.h

benpicco · 2020-11-11T16:01:03Z

Please squash!

bergzand · 2020-11-11T16:48:47Z

Please squash!

Squashed!

benpicco

looks good to me.
I check all the patches and there are only mechanical changes / renames.

flashpage currently requires pagewise implementation with an optional extension for per block writes (flashpage_raw). Most implementations with flashpage_raw implement the pagewise access via the flashpage_raw functions. This commit makes the flashpage raw the main access method and adds an extension feature for the pagewise access. The functions and defines are renamed to reflect this. The API is also extended with a dedicated function for erasing a sector.

The features in flashpage_raw are now default in flashpage and do not have to be included as a separate dependency

bergzand · 2020-11-12T21:35:51Z

Thanks for reviewing!

benpicco · 2025-05-26T17:17:49Z

So can we drop all the !CONFIG_RIOTBOOT_FLASHWRITE_RAW code now that it's unused?

bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: drivers Area: Device drivers labels Nov 9, 2020

bergzand requested review from aabadie, benpicco and fjmolinas November 9, 2020 16:02

bergzand requested review from DipSwitch, MrKevinWeiss, basilfx, cgundogan, dylad, gschorcht, haukepetersen, jia200x, kaspar030, keestux, leandrolanzieri, smlng and vincent-d as code owners November 9, 2020 16:02

benpicco reviewed Nov 9, 2020

View reviewed changes

cpu/cc2538/periph/flashpage.c Outdated Show resolved Hide resolved

fjmolinas reviewed Nov 9, 2020

View reviewed changes

cpu/msp430_common/periph/flashpage.c Outdated Show resolved Hide resolved

bergzand force-pushed the pr/flashpage/merge_raw branch from 5d25f04 to 63c39fd Compare November 10, 2020 09:13

benpicco reviewed Nov 10, 2020

View reviewed changes

benpicco reviewed Nov 11, 2020

View reviewed changes

cpu/cc2538/include/cpu_conf.h Outdated Show resolved Hide resolved

bergzand force-pushed the pr/flashpage/merge_raw branch from 257a4f2 to f4dda1a Compare November 11, 2020 16:48

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 11, 2020

benpicco approved these changes Nov 11, 2020

View reviewed changes

bergzand force-pushed the pr/flashpage/merge_raw branch 3 times, most recently from 395198d to 411c690 Compare November 11, 2020 21:42

bergzand requested a review from MichelRottleuthner as a code owner November 11, 2020 21:42

bergzand added 10 commits November 11, 2020 23:16

tests/periph_flashpage: Adapt to optional flashpage_pagewise

5683f30

cc2538: Adapt to flashpage/flashpage_pagewise API

3c10425

efm32: Adapt to flashpage/flashpage_pagewise API

9a79124

kinetis: Adapt to flashpage/flashpage_pagewise API

f85594e

nrf5x: Adapt to flashpage/flashpage_pagewise API

e176649

msp430: Adapt to flashpage/flashpage_pagewise API

61052db

sam0: Adapt to flashpage/flashpage_pagewise API

72d7a90

stm32: Adapt to flashpage/flashpage_pagewise API

1c063a7

mtd_flashpage: only depend on periph_flashpage

3aa5203

The features in flashpage_raw are now default in flashpage and do not have to be included as a separate dependency

riotboot: only depend on periph_flashpage

7222239

bergzand force-pushed the pr/flashpage/merge_raw branch from 411c690 to 7222239 Compare November 11, 2020 22:16

benpicco merged commit d9598a0 into RIOT-OS:master Nov 12, 2020

bergzand deleted the pr/flashpage/merge_raw branch November 12, 2020 21:35

benpicco mentioned this pull request Nov 16, 2020

cpu/sam0_common: flashpage: rename to sam0_flashpage_aux_write() #15453

Merged

Ollrogge mentioned this pull request Mar 1, 2022

cpu: add flash_writable section to linker script #17436

Merged

Conversation

bergzand commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

aabadie commented Nov 9, 2020

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Nov 9, 2020

Uh oh!

fjmolinas commented Nov 9, 2020

Uh oh!

bergzand commented Nov 9, 2020

Uh oh!

aabadie commented Nov 9, 2020

Uh oh!

bergzand commented Nov 9, 2020

Uh oh!

bergzand commented Nov 10, 2020

Uh oh!

bergzand commented Nov 10, 2020

Uh oh!

bergzand commented Nov 10, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

bergzand Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

bergzand Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

bergzand Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benpicco commented Nov 11, 2020

Uh oh!

bergzand commented Nov 11, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand commented Nov 12, 2020

Uh oh!

benpicco commented May 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bergzand commented Nov 9, 2020 •

edited

Loading