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

cpu: stm32l1: add flashpage writing support #7712

Closed
wants to merge 7 commits into from

Conversation

kYc0o
Copy link
Contributor

@kYc0o kYc0o commented Oct 10, 2017

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

@kYc0o 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 kYc0o added this to the Release 2017.10 milestone Oct 10, 2017
@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 13, 2017

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

@kaspar030 ping!

@cladmi
Copy link
Contributor

cladmi commented Oct 13, 2017

This will also be affected by #7667

@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 13, 2017

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

* @{
*/
#define FLASHPAGE_SIZE (256U)
#define FLASHPAGE_NUMOF (2048U)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, will fix.


/* 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, a leftover of a test, will remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@kYc0o kYc0o removed this from the Release 2017.10 milestone Oct 16, 2017
@kaspar030 kaspar030 changed the title Add flashpage writing support for STM32L1 cpu: stm32l1: add flashpage writing support Oct 23, 2017
@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 23, 2017

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

@kYc0o
Copy link
Contributor Author

kYc0o commented Nov 8, 2017

Ping.

@kYc0o
Copy link
Contributor Author

kYc0o commented Feb 9, 2018

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

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Minor nits regarding doxygen

@@ -72,6 +72,20 @@ extern "C" {
#define CPU_FLASH_BASE FLASH_BASE
/** @} */

/**
* @brief Flash page configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* @{
*/
#if defined(CPU_MODEL_STM32L152RE)
#define FLASHPAGE_SIZE (256U)
Copy link
Contributor

Choose a reason for hiding this comment

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

these define are not self doxygen documented

Copy link
Contributor Author

@kYc0o kYc0o Feb 9, 2018

Choose a reason for hiding this comment

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

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

@kYc0o kYc0o force-pushed the add_stm32l1_flashpage branch from a4eb2a6 to 7c4df29 Compare February 21, 2018 11:11
@kYc0o kYc0o added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 28, 2018
@kYc0o kYc0o force-pushed the add_stm32l1_flashpage branch from 7c4df29 to 4031cff Compare March 2, 2018 11:01
@kYc0o kYc0o removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 2, 2018
@kYc0o
Copy link
Contributor Author

kYc0o commented Mar 2, 2018

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

@kYc0o
Copy link
Contributor Author

kYc0o commented Mar 12, 2018

ping @aabadie

@aabadie
Copy link
Contributor

aabadie commented Mar 12, 2018

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
Copy link
Contributor Author

kYc0o commented Mar 12, 2018

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
Copy link
Contributor

aabadie commented Mar 12, 2018

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
Copy link
Contributor Author

kYc0o commented Mar 12, 2018

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
Copy link
Contributor

aabadie commented Mar 13, 2018

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

@aabadie aabadie closed this Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants