stm32_common/flashpage: cleanup#11715
Conversation
fjmolinas
added
Platform: ARM
aabadie
added
CI: ready for build
aabadie
left a comment
aabadie
left a comment
There was a problem hiding this comment.
I have some cosmetic comments, mainly on commit content which are for 2 of them unrelated to the commit message.
| uint16_t *dst = page_addr; | ||
|
|
||
| #endif | ||
| #if defined(CPU_FAM_STM32F0) || defined(CPU_FAM_STM32F1) |
There was a problem hiding this comment.
This change should be in it's own commit and not in b72a6ed where it's unrelated.
There was a problem hiding this comment.
Why? Only STM32F0 STM32F1 need to initialize HSI clock (at least for the implemented boards). My goal was to separate it from the default case which only said /* Default is to support half-word sizes */, But I guess you are right, the commit message is not explicit enough. Would you be Ok with just re-wording?
There was a problem hiding this comment.
No, this changes the initial behaviour as you say, so it needs to be in its own commit.
| defined(CPU_FAM_STM32L4) | ||
| CNTRL_REG &= ~(FLASH_CR_PG); | ||
| DEBUG("[flashpage_raw] write: done writing data\n"); | ||
| #endif |
There was a problem hiding this comment.
The DEBUG message above is now only displayed with F0/F1/L4 but not for L0/L1. It should be put after.
I would also not put this change in 5e0c867 because it seems unrelated.
There was a problem hiding this comment.
You are right about the DEBUG message. But it is related since I'm removing the definition of FLASH_CR_PG Therefore I need to excluded it from compilation and that is the reason for the #ifdefined. Should I split guarding and removing FLASH_CR_PG into two commits?
There was a problem hiding this comment.
I see, let's keep the changes in the same commit then. You still have to fix the commit message: it's full of typos :)
There was a problem hiding this comment.
The DEBUG line must still move out of the define (preferably after).
There was a problem hiding this comment.
damn, I was concentrating on the typos sorry
There was a problem hiding this comment.
Ok I think now it should be ok .. sorry for missing this.
|
There is also a typo in 9416ca8 message: repited => repeated |
|
I tested this on nucleo-l432kc, nucleo-l073rz, nucleo-l433rc and iotlab-m3. It worked. Just rework a bit the commits and we are good to go. |
- Before, HSI was enabled as the default case when it is only used for stm32f0 and stm32f1. It is now implemented explicitly for those platforms, and only those.
fjmolinas
force-pushed
the
pr_stm32_flashpage_cleanup
branch
2 times, most recently
from
0f98ad9 to
72ffaf5
Compare
|
@aabadie Changes addressed, I split the commit you where addressing into two. One separating the hsi handling case from the default case. And the second one making the remaining implicit defines explicit, what do you think? |
- Since writes are performed per word no actions need to be performed for flash access, and the FPRG doesn't need to be cleared.
fjmolinas
force-pushed
the
pr_stm32_flashpage_cleanup
branch
from
72ffaf5 to
46b9013
Compare
2 participants
Contribution description
This PR cleans up
flashpage.c. Following discussion in #11681 it changes defines to makes specific behaviors explicit. Some unused functions and definitions where removed, specifically for stm32l1/0 and stm32l4.NOTE: to verify the stm32l1/l0 change you can refer to the RM p71 for L1 and p89 for L0x3 and p81 for L0x1.
Testing procedure
Run
make -C tests/periph_flashpage/ BOARD=nucleo-l476rg flash testfor anstm32l1,stm32l0andstm32l4,stm32f0andstm32f1board.The test should still pass in all cases.
Tested on:
nucleo-l152re,b-l072rz-lrwan1,nucleo--l476rg,nucleo-f072rb,iotlab-m3.Issues/PRs references