cpu/stm32: unify support for flashpage/flashpage_raw with L0/L1/F0/F1 families by aabadie · Pull Request #8768 · RIOT-OS/RIOT

aabadie · 2018-03-12T07:57:56Z

Contribution description

This PR adapts the STM32 flashpage feature to make it work with the STM32L0 family. It has been tested with success on nucleo-l0xx and b-l072z-lrwan1 boards.

Issues/PRs references

None

kYc0o

Besides the comments on this PR, the flashpage_write_raw implementation is missing and I think it can bee added at the same time, since it's trivial and adds flexibility.

On the other hand, it looks a bit like a duplicate of #7712. I'd say we could merge both implementations to give a complete flashpage driver for the stm32l CPUs.

kYc0o · 2018-03-12T15:04:35Z

cpu/stm32_common/periph/flashpage.c

+#define CNTRL_REG              (FLASH->CR)
+#define CNTRL_REG_LOCK         (FLASH_CR_LOCK)
+#define KEY_REG                (FLASH->KEYR)
+#define FLASHPAGE_LIMIT        (FLASHPAGE_SIZE / 2)


This FLASHPAGE_LIMIT define is not documented and thus I don't see what is really about.

kYc0o · 2018-03-12T15:05:42Z

cpu/stm32_common/periph/flashpage.c

-        FLASH->KEYR = FLASH_KEY2;
+    if (CNTRL_REG & CNTRL_REG_LOCK) {
+        KEY_REG = FLASH_KEY1;
+        KEY_REG = FLASH_KEY2;


You're redefining this values, but I don't see why you don't use them below, which makes me wonder what's the use of it after all.

I'm sorry @kYc0o, but I'd like that you read this PR carefully before commenting (and also the datasheet). They are not reused below because they are not the same (FLASH->PEKEYR != FLASH->PRGKEYR).

I know the datasheet and I know what is this about (my PR was there for about 6 months), I was referring to CTRL_REG and actually since you're defining KEY_REG you could also define KEY_PREG to be reused in other parts of the code, especially if your intention is to add the EEPROM driver.

kYc0o · 2018-03-12T15:16:29Z

cpu/stm32_common/periph/flashpage.c

+#if defined(CPU_FAM_STM32L0)
+    DEBUG("[flashpage] unlocking the flash program memory\n");
+    if (!(CNTRL_REG & CNTRL_REG_LOCK)) {
+        if (FLASH->PECR & FLASH_PECR_PRGLOCK) {


This nested if's seem a bit weird to me... especially because both CTRL_REG and FLASH->PECR are defining the same registers.

vincent-d · 2018-03-12T17:37:47Z

I'd tend to prefer this approach over #7712 but I also think flashpage_write_raw could be added easily, but it could also come in a follow up as it could be implemented for every supported stm32 cpu.

kYc0o · 2018-03-12T17:52:19Z

I'd tend to prefer this approach over #7712 but I also think flashpage_write_raw could be added easily, but it could also come in a follow up as it could be implemented for every supported stm32 cpu.

I still think we can merge both approaches, no need to add more PRs or wait for them. As I said it's trivial and adds a lot of flexibility while writing into flash.

What I propose is to add my changes to this PR, if @vincent-d likes it more.

aabadie · 2018-03-13T10:19:15Z

What I propose is to add my changes to this PR

Indeed there are good ideas on both sides. I updated this PR to integrate them and also added the flashpage_raw feature to stm32f0 and stm32f1.

Tested in nucleo-f030, nucleo-l152 and b-l072z-lrwan1 boards and works. I'm not sure of the values for alignment and block size, so it may be worth checking that.

vincent-d

Didn't test it but I have some comments. Looks already good in general.

vincent-d · 2018-03-13T10:57:15Z

cpu/stm32_common/periph/flashpage.c


 #include "periph/flashpage.h"

+#if defined(CPU_FAM_STM32L0) | defined(CPU_FAM_STM32L1)


vincent-d · 2018-03-13T11:00:15Z

cpu/stm32_common/periph/flashpage.c

+
+void flashpage_write_raw(void *target_addr, void *data, size_t len)
+{
+    /* The actual minimal block size for writing is 16B, thus we


is it really 16B ?

During my tests, it was actually possible to write even 4B, but I left 16B as the smallest.

vincent-d · 2018-03-13T11:03:37Z

cpu/stm32_common/periph/flashpage.c

+#endif
    uint32_t hsi_state = (RCC->CR & RCC_CR_HSION);

    /* the internal RC oscillator (HSI) must be enabled */


isn't it needed in flashpage_write_raw as well?

vincent-d · 2018-03-13T11:04:14Z

cpu/stm32_common/periph/flashpage.c


    /* the internal RC oscillator (HSI) must be enabled */
    RCC->CR |= (RCC_CR_HSION);
    while (!(RCC->CR & RCC_CR_HSIRDY)) {}


you could use stmclk_enable_hsi and stmclk_disable_hsi below

vincent-d · 2018-03-13T12:07:40Z

cpu/stm32_common/periph/flashpage.c

+    /* set PG bit and program page to flash */
+    CNTRL_REG |= FLASH_CR_PG;
+#endif
+    for (size_t i = 0; i < (FLASHPAGE_SIZE / FLASHPAGE_DIV); i++) {


len / FLASHPAGE_DIV ?

vincent-d · 2018-03-13T12:12:22Z

cpu/stm32_common/periph/flashpage.c

    /* finally, lock the flash module again */
-    DEBUG("flashpage] now locking the flash module again\n");
-    FLASH->CR |= FLASH_CR_LOCK;
+    _lock();


I'm wondering if it wouldn't be cleaner to lock right after erasing, as unlocking/locking are done again in flashpage_write_raw in case of writing

vincent-d · 2018-03-13T12:16:55Z

cpu/stm32_common/periph/flashpage.c

+    uint32_t *data_addr = (uint32_t *)data;
+#else
+    uint16_t *dst = target_addr;
+    uint16_t *data_addr = (uint16_t *)data;


There is something I'm not sure to understand here: this seems to allow half-word (i.e. 2 bytes) alignment, as FLASHPAGE_DIV seems to suggest too. But FLASHPAGE_RAW_ALIGNMENT and FLASHPAGE_RAW_BLOCKSIZE are 4 and then it's not allowed to write only 2 bytes.

Am I missing something?

aabadie · 2018-03-13T12:47:23Z

@vincent-d @kYc0o, I tested more in depth the flashpage_raw feature: I think it works (not really sure how the test application is supposed to be used) with L0 and L1 but it crashes with F0 (I have no F1, except iotlab-m3 but have problem flashing this one).
Do you have an idea what could be problem ? I seems to be an alignment issue but I don't understand why.

vincent-d

Works on b-l072z-lrwan1 but crashes on nucleo-f091 if HSI is not enabled before writing

vincent-d · 2018-03-13T14:32:36Z

cpu/stm32_common/periph/flashpage.c

+    uint16_t *dst = (uint16_t *)target_addr;
+    uint16_t *data_addr = (uint16_t *)data;
+#endif
+


HSI needs to be enabled here and disabled after locking, otherwise it crashes on stm32f091

HSI needs to be enabled here and disabled after locking

Thanks, I'll try that

Thanks a lot @vincent-d, that was it! I refactored a bit the code again and tested on nucleo-l073 and nucleo-f070, it works on both.

aabadie · 2018-03-13T15:37:27Z

@vincent-d @kYc0o, I think we are nearly good here.

vincent-d

ACK

aabadie · 2018-03-13T16:05:51Z

@kYc0o just missing your ACK, and we are good !

kYc0o · 2018-03-13T17:03:18Z

It looks ok. I'll test and give my ACK.

kYc0o · 2018-03-13T18:19:40Z

Tested ACK.

aabadie · 2018-03-14T17:22:08Z

cpu/stm32f1/include/cpu_conf.h

+ */
+#define FLASHPAGE_RAW_BLOCKSIZE    (2U)
+/* Writing should be always 4 bytes aligned */
+#define FLASHPAGE_RAW_ALIGNMENT    (4U)


@cladmi, It could be one of those 2 lines, can you test with a value of 2 ?

With ENABLE_DEBUG (1) in stm32_common/periph/flashpage.c it works so it must be something else.

cladmi · 2018-03-14T18:04:00Z

Broken on iotlab-m3. Test procedure on IoT-LAB.

dump 255
1521050271.397776;m3-380;> dump 255
# 0xff everywhere
test 255
1521050275.361341;m3-380;> test 255
     # need to type enter to see the error
1521050278.902577;m3-380;error verifying the content of page 255: >  
dump 255
1521050290.161453;m3-380;> dump 255
# 0xff everywhere

With ENABLE_DEBUG (1) its working properly

test 255
...
1521049756.371371;m3-380;[flashpage_raw] writing s to 0x807fffe
1521049756.372367;m3-380;[flashpage] write: done writing data
1521049756.373414;m3-380;flashpage_raw] now locking the flash module again
1521049756.373651;m3-380;[flashpage] locking the flash module
1521049756.375369;m3-380;wrote local page to flash page 255 at addr 0x807f800
dump 255
1521050605.346219;m3-380;> dump 255
# a b c d... as expected

aabadie 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 Mar 12, 2018

aabadie requested a review from kYc0o March 12, 2018 07:57

aabadie mentioned this pull request Mar 12, 2018

cpu: stm32l1: add flashpage writing support #7712

Closed

kYc0o requested changes Mar 12, 2018

View reviewed changes

aabadie mentioned this pull request Mar 13, 2018

tests/periph_flashpage: small cleanup #8770

Merged

aabadie force-pushed the pr/cpu/flashpage branch 3 times, most recently from d5d3e07 to 0003aae Compare March 13, 2018 10:14

aabadie changed the title ~~cpu/stm32l0: add support for flashpage feature~~ cpu/stm32: unify support for flashpage/flashpage_raw with L0/L1/F0/F1 families Mar 13, 2018

aabadie force-pushed the pr/cpu/flashpage branch from 0003aae to df326ea Compare March 13, 2018 10:16

vincent-d requested changes Mar 13, 2018

View reviewed changes

vincent-d reviewed Mar 13, 2018

View reviewed changes

vincent-d requested changes Mar 13, 2018

View reviewed changes

aabadie added 5 commits March 13, 2018 16:30

cpu/stm32_common: unify flashpage support

67190a3

cpu/stm32l0: add flashpage support

4972f95

cpu/stm32l1: add flashpage support

4e1f2b5

cpu/stm32f0: add flashpage_raw support

4f7dc1a

cpu/stm32f1: add flashpage_raw support

06392bb

aabadie force-pushed the pr/cpu/flashpage branch from 3dc2728 to 06392bb Compare March 13, 2018 15:30

vincent-d approved these changes Mar 13, 2018

View reviewed changes

vincent-d mentioned this pull request Mar 13, 2018

drivers/flashpage: use const pointers for write and verify #8773

Merged

kYc0o approved these changes Mar 13, 2018

View reviewed changes

kYc0o merged commit 5a05f1b into RIOT-OS:master Mar 13, 2018

aabadie commented Mar 14, 2018

View reviewed changes

cladmi mentioned this pull request May 3, 2018

flashpage_write broken on iotlab-m3 #9065

Closed

aabadie mentioned this pull request May 3, 2018

cpu/stm32-common: slightly rework flashpage driver and fix iotlab-m3 #9068

Merged

aabadie deleted the pr/cpu/flashpage branch June 8, 2018 20:49


		#include "periph/flashpage.h"

		#if defined(CPU_FAM_STM32L0) \| defined(CPU_FAM_STM32L1)

Conversation

aabadie commented Mar 12, 2018

Contribution description

Issues/PRs references

Uh oh!

kYc0o 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Mar 12, 2018

Uh oh!

kYc0o commented Mar 12, 2018

Uh oh!

aabadie commented Mar 13, 2018

Uh oh!

vincent-d 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!

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!

aabadie commented Mar 13, 2018

Uh oh!

vincent-d 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!

aabadie commented Mar 13, 2018

Uh oh!

vincent-d left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Mar 13, 2018

Uh oh!

kYc0o commented Mar 13, 2018

Uh oh!

kYc0o commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants