cpu: stm32l1: add flashpage writing support by kYc0o · Pull Request #7712 · RIOT-OS/RIOT

kYc0o · 2017-10-10T19:29:14Z

Just added flashpage write support for stm32l1 cpu's, which can be tested with tests/periph_flashpage.

kYc0o · 2017-10-13T10:27:04Z

Can maybe @haukepetersen or @vincent-d take a look on this one?

@kaspar030 ping!

cladmi · 2017-10-13T13:28:33Z

This will also be affected by #7667

kYc0o · 2017-10-13T13:41:00Z

Yes indeed, let's reach consensus there for a good solution and I'll implement it here.

haukepetersen · 2017-10-13T14:10:08Z

cpu/stm32l1/include/cpu_conf.h

+ * @{
+ */
+#define FLASHPAGE_SIZE      (256U)
+#define FLASHPAGE_NUMOF     (2048U)


This is off: I am pretty sure the flash has not the same size for all members of the stm32l1 family -> these values have to be guarded with the actual CPU_MODEL_xx values.

True, will fix.

haukepetersen · 2017-10-13T14:11:26Z

cpu/stm32_common/periph/flashpage.c

+
+    /* If the erase operation is completed, disable the ERASE and PROG bits */
+    DEBUG("[flashpage] erase: resetting the page erase and prog bit\n");
+    FLASH->PECR &= (uint32_t)(~FLASH_PECR_PROG);


hm, are you sure about this one? You also disable it in line 168, and shouldn't it only be done there, after you have (potentially) written to flash?

Oops, a leftover of a test, will remove.

It is working like that, since the program bit apparently only affects when erasing the flash.

kYc0o · 2017-10-23T22:37:40Z

Comments addressed. Ping @haukepetersen, maybe @vincent-d or @aabadie can also take a look?

kYc0o · 2017-11-08T20:08:03Z

Ping.

kYc0o · 2018-02-09T17:29:06Z

Finally got to update this PR. It includes now flashpage_write_raw which allows to write blocks of 4B long.

Depends on ~~#8545~~ and ~~#8518~~

aabadie

Minor nits regarding doxygen

aabadie · 2018-02-09T17:37:31Z

cpu/stm32l1/include/cpu_conf.h

 /** @} */

+/**
+ * @brief   Flash page configuration


Should be @name and also add a closing line /** @} */ after the defines.

aabadie · 2018-02-09T17:38:19Z

cpu/stm32l1/include/cpu_conf.h

+ * @{
+ */
+#if defined(CPU_MODEL_STM32L152RE)
+#define FLASHPAGE_SIZE      (256U)


these define are not self doxygen documented

Are these required? I don't see any example in other CPUs...

kYc0o · 2018-03-02T14:24:26Z

No more waiting for PR. @aabadie are your concerns addressed?

kYc0o · 2018-03-12T13:34:08Z

ping @aabadie

aabadie · 2018-03-12T14:23:19Z

I forgot this PR when I worked on #8768. Looking at this PR again and comparing with #8768, I see several issues:

copy pasted code. It's possible to inline implementations depending on stm32 family.
flashpage_raw feature could be added in a follow-up
although interesting, there are other refactorings that could be done in follow-ups as well (_lock/_unlock static functions)
since the unlock keys are shared between stm32l0 and stm32l1 families and are useful for writing on EEPROM (this is what I'm mainly interested in in fact), their definitions could be moved to a common stm32 include file.

kYc0o · 2018-03-12T14:52:11Z

copy pasted code. It's possible to inline implementations depending on stm32 family.

The code for stm32l is actually very different from other stm32 variants. I didn't copy/pasted any code for this.

flashpage_raw feature could be added in a follow-up

flashpage_write_raw avoids to repeat code, thus I think it should be included to avoid double work in the future. Besides, it allows to write smaller blocks which avoids the need of having FLASHPAGE_SIZE arrays to store data.

although interesting, there are other refactorings that could be done in follow-ups as well (_lock/_unlock static functions)

Indeed, these functions were added to avoid code duplication in this implementation while adding flashpage_write_raw, I don't see a reason to remove them.

since the unlock keys are shared between stm32l0 and stm32l1 families and are useful for writing on EEPROM (this is what I'm mainly interested in in fact), their definitions could be moved to a common stm32 include file.

This can be discussed, writing into EEPROM might need another kind of driver since it's not a flash memory (as the name indicates), thus another API for EEPROM is needed IMHO.

aabadie · 2018-03-12T15:16:10Z

The code for stm32l is actually very different from other stm32 variants. I didn't copy/pasted any code for this.

Indeed, but the workflow is the same between variants and can for sure be factorized using preprocessor macros. See #8768, where the code is better factorized IMHO.

flashpage_write_raw avoids to repeat code, thus I think it should be included to avoid double work in the future. Besides, it allows to write smaller blocks which avoids the need of having FLASHPAGE_SIZE arrays to store data.

Yes, but it's an extra feature. The order, before this PR or after, is not very important, since it can be applied to all variants of STMs that already have flashpage (stm32f1).

Now that I'm thinking of it, this addition (and the _lock/_unlock) should have been done in a preliminary PR.

writing into EEPROM might need another kind of driver since it's not a flash memory (as the name indicates)

Not sure about this. My reading on wikipedia states that Flash memory is a kind of EEPROM

thus another API for EEPROM is needed IMHO.

I'm not sure about that. I was more thinking of extending the flashpage API (starting by simply renaming the module flash). But this should be discussed and it's not the purpose of this PR.

kYc0o · 2018-03-12T18:00:46Z

Yes, but it's an extra feature. The order, before this PR or after, is not very important, since it can be applied to all variants of STMs that already have flashpage (stm32f1).

I'm actually working on this extension for stm32f1, which is the only one so far tested.

Now that I'm thinking of it, this addition (and the _lock/_unlock) should have been done in a preliminary PR.

Without flashpage_write_raw, these functions have no use, so I don't see the need to PR them before.

Not sure about this. My reading on wikipedia states that Flash memory is a kind of EEPROM

Which doesn't mean that EEPROM is a kind of flash, data EEPROM in these chips is not divided by pages, which IMHO is out of the scope of this driver.

I'm not sure about that. I was more thinking of extending the flashpage API (starting by simply renaming the module flash). But this should be discussed and it's not the purpose of this PR.

Read and write EEPROM can be abstracted to not only create an API for chips which have it internally, but also for chips which can be attached through I2C or SPI. I see very complicated to merge everything in one flash driver since the purposes for both memories are different.

aabadie · 2018-03-13T19:38:58Z

Now that #8768 is merged, we can merge this one.

kYc0o added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 10, 2017

kYc0o added this to the Release 2017.10 milestone Oct 10, 2017

kYc0o assigned kaspar030 Oct 10, 2017

haukepetersen requested changes Oct 13, 2017

View reviewed changes

kYc0o removed this from the Release 2017.10 milestone Oct 16, 2017

kaspar030 changed the title ~~Add flashpage writing support for STM32L1~~ cpu: stm32l1: add flashpage writing support Oct 23, 2017

kYc0o mentioned this pull request Dec 11, 2017

periph/rtc breaks nucleo-l152 #8240

Closed

kYc0o mentioned this pull request Dec 25, 2017

boards/cpu: nucleo-l152 (stm32l1) broken on master #8024

Closed

kYc0o force-pushed the add_stm32l1_flashpage branch from 6f8052a to 5ee33ee Compare February 9, 2018 17:26

aabadie requested changes Feb 9, 2018

View reviewed changes

kYc0o force-pushed the add_stm32l1_flashpage branch from a4eb2a6 to 7c4df29 Compare February 21, 2018 11:11

kYc0o added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 28, 2018

kYc0o added 7 commits March 2, 2018 11:59

cpu/stm32_common/periph: add flashpage driver for stm32l1 family

c8e9c22

cpu/stm32l1/include: add flashpage defines

623d815

cpu/stm32l1/Makefile.features: add flashpage

11fc2ab

cpu/stm32l1: add flashpage_raw defines

e470a97

cpu/stm32_common/periph: add flashpage_write_raw

cc713c4

cpu/stm32l1: add flashpage_write_raw feature

7f82fc6

fixup! cpu/stm32l1/include: add flashpage defines

4031cff

kYc0o force-pushed the add_stm32l1_flashpage branch from 7c4df29 to 4031cff Compare March 2, 2018 11:01

kYc0o removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 2, 2018

vincent-d mentioned this pull request Mar 12, 2018

cpu/stm32: unify support for flashpage/flashpage_raw with L0/L1/F0/F1 families #8768

Merged

aabadie closed this Mar 13, 2018

Conversation

kYc0o commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kYc0o commented Oct 13, 2017

Uh oh!

cladmi commented Oct 13, 2017

Uh oh!

kYc0o commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Oct 23, 2017

Uh oh!

kYc0o commented Nov 8, 2017

Uh oh!

kYc0o commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Mar 2, 2018

Uh oh!

kYc0o commented Mar 12, 2018

Uh oh!

aabadie commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kYc0o commented Mar 12, 2018

Uh oh!

aabadie commented Mar 12, 2018

Uh oh!

kYc0o commented Mar 12, 2018

Uh oh!

aabadie commented Mar 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kYc0o commented Oct 10, 2017 •

edited

Loading

kYc0o commented Feb 9, 2018 •

edited

Loading

kYc0o Feb 9, 2018 •

edited

Loading

aabadie commented Mar 12, 2018 •

edited

Loading