eeprom emulation for stm32f4 #7803

yulei · 2020-01-06T01:42:19Z

Description

Using flash to emulate EEPROM for STM32F4, every byte of eeprom's address and data will be composed to a WORD(4bytes) and then write to the flash. The writing operation will from the start address to the end, and the reading operation will from the end address to the begin. Thus we can write the eeprom byte with the same eeprom address many times without erase the the flash page.
I have tested this implementation on the NOAH keyboard.

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

yanfali · 2020-01-06T01:54:50Z

Can you tell us what the origin of this code is and what license it is under?

yulei · 2020-01-06T01:56:18Z

Can you tell us what the origin of this code is and what license it is under?

It's written by myself, should I adding the license on the file?

yanfali

We're going to need some docs to go with this and a link to where it came from and what license the code is under. Thanks

yanfali · 2020-01-06T01:57:15Z

Can you tell us what the origin of this code is and what license it is under?

It's written by myself, should I adding the license on the file?

Oh, that's awesome. Yeah, you should probably add a GPL compatible license to this. Thanks

tmk_core/common/chibios/eeprom_stm32f4.c

yanfali · 2020-01-06T01:58:13Z

tmk_core/common/chibios/eeprom_stm32f4.c

+#include "hal.h"
+
+#define EE_SECTOR_ID 3
+#define EE_SECTOR_SIZE 0x4000


some pointers to documentation where you got these magic values from would be great too. thanks.

description for this was added

Doesn't this limit the executable flash space (starting at 0x08000000) to 48kB unless each stm32f4 project's ldscripts get modified?
If they're not changed, then this eeprom implementation will potentially overwrite program code, will it not?

Yes, this limits the flash space, currently the qmk need about 30K flash(with all rgb lights enabled). The flash sector was not uniform distributed as the F1 or F3. the sector 0-3 are 16KB each, sector 4 has 64KB, sector 5 to last were 128KB. maybe we can use the last sector and use the DataBuf for caches.

Unsure what you're including in your "normal" builds, but I have an ARM board that uses RGB, OLED, external EEPROM and custom matrix -- it's 52kB of flash used. I'm currently working on one that includes colour images for an LCD -- 30kB would get used very quickly.

Now, I'm not targeting F4 for this build, but I don't feel like we should be artifically placing a restriction on the amount of flash available in general. The assertion that around 30kB is "normal" doesn't really hold true now, and certainly won't do so in the future.

Normally I'll flash an small bootloader for IAP at the start of flash space. And the qmk firmware will sit behind the bootloader and the emulated eeprom area. This can remove the hard limit of flash space for application. Anyway the latest codes have used the last flash sector for eeprom emulation, so the qmk can programmed at start of the flash for common usage.

yanfali · 2020-01-06T02:00:02Z

tmk_core/common/chibios/eeprom_stm32.h

@@ -47,6 +49,9 @@
 #    elif defined(MCU_STM32F103ZE) || defined(MCU_STM32F103RE) || defined(MCU_STM32F103RD) || defined(MCU_STM32F303CC) || defined(MCU_STM32F072CB)
 #        define FEE_PAGE_SIZE (uint16_t)0x800  // Page size = 2KByte
 #        define FEE_DENSITY_PAGES 4            // How many pages are used
+#    elif defined(MCU_STM32F411xE)


Some docs explaining why you had to add a new flash driver rather than re-use the existing one would also be helpful for review.

the explanation was added on the eeprom_stm32f4.c

I would rather use the existing code if possible, and target an "improved" version across supported chips, instead of fragmenting implementations.

We also have #7274 looking close to completion. If there is a hard requirement for another implementation, it would be nice to target the newer structure.

stm32F4 has different register bits for flash programming compared with stm32f1 and f3. It will introduce many hardware specific #define on the existing code. The new structure introduced by #7274 will not conflict the current EEPROM emulation implementation.

I am just putting it on record that:

I'm not keen on having a non-uniform approach to the eeprom emulation strategy

I'm not keen on the code duplication this PR introduces

zvecr · 2020-01-06T03:20:11Z

@tzarc I know you have a eeprom PR in the queue, any chance you could give this one a once over?

tzarc · 2020-01-06T11:09:43Z

tmk_core/common/chibios/eeprom_stm32f4.c

+#include "hal.h"
+
+#define EE_SECTOR_ID 3
+#define EE_SECTOR_SIZE 0x4000


Doesn't this limit the executable flash space (starting at 0x08000000) to 48kB unless each stm32f4 project's ldscripts get modified?
If they're not changed, then this eeprom implementation will potentially overwrite program code, will it not?

tzarc · 2020-01-06T11:20:08Z

tmk_core/common/chibios/eeprom_stm32f4.c

+    }
+    /* Return the Erase Status */
+    return status;
+}


Conventional erased value for an external flash is 0xFF for each byte -- is the erased value after this operation 0x00 or 0xFF?
eeconfig will not function correctly if the default value is non-zero after an erase.

I suppose writing 0xFF for everything means the iteration through the list won't actually match anything when querying values. In hindsight, this should be fine.

After flash was erased, all the bit will be set to "1". We can only program the bit from "1" to "0", and can't from "0" to "1" which need a full page erasing operation. Thus the 0xFF means the empty value in flash.

tzarc · 2020-01-06T11:26:34Z

tmk_core/common/chibios/eeprom_stm32f4.c

+}
+
+uint16_t EEPROM_WriteDataByte(uint16_t Address, uint8_t DataByte) {
+    uint32_t addr = FLASH_FindValidAddress();


Given there's a possibility of using the "shadow" copy in the static DataBuf array, to increase the endurance perhaps use it as a cache and compare against the existing value before attempting a write. Writing the same value to the same address will needlessly add entries to flash with this implementation.
Reads can follow the same principle, consulting the shadow copy, rather than iterating through the flash?

tzarc · 2020-01-06T11:27:10Z

tmk_core/common/chibios/eeprom_stm32f4.c

+static void FLASH_Backup(void) {
+    uint32_t begin = EE_SECTOR_BEGIN;
+    uint32_t end = EE_SECTOR_END;
+    memset(&DataBuf[0], 0xFF, sizeof(DataBuf));


Again, 0x00 vs. 0xFF.

Just confirm that it's writing 0xFF to flash when erasing sectors. Any attempted write with an invalid address will be skipped anyway.

tzarc · 2020-01-06T11:31:33Z

tmk_core/common/chibios/eeprom_stm32f4.c

+}
+
+uint8_t  EEPROM_ReadDataByte(uint16_t Address) {
+    uint32_t begin = EE_SECTOR_BEGIN;


If using DataBuf as a shadow copy, consult that instead here.

The latest implementation uses the DataBuf as cache to avoid same value writing and skip the flash iteration for the reading operation.

tzarc · 2020-01-06T11:32:06Z

tmk_core/common/chibios/eeprom_stm32f4.c

+}
+
+// memory emulate, for verification
+#define EE_MEMORY 0


Remove this test code, it's a duplicate from other places?

this code were used for verification the implementation, they can be removed now.

tzarc · 2020-01-06T11:34:29Z

As far as being related to #7274 -- as @yulei said it won't really collide, it'll just mean that one of the PRs will need Makefile changes to support it.

tzarc · 2020-01-06T11:38:49Z

Also, sort of like @zvecr -- I don't really like the duplication, and I think that the implementation here with the addr+value being written as a list could be a good thing to standardise when writing to flash.

Personally, I'd look at using that functionality for F303 and the like anyway. Suggestion would be to merge that logic, then abstract out the operations for unlocking and the like with #defines selecting which implementation to use.

yulei · 2020-01-06T14:47:56Z

Also, sort of like @zvecr -- I don't really like the duplication, and I think that the implementation here with the addr+value being written as a list could be a good thing to standardise when writing to flash.

Personally, I'd look at using that functionality for F303 and the like anyway. Suggestion would be to merge that logic, then abstract out the operations for unlocking and the like with #defines selecting which implementation to use.

I'll try to use this idea (combine address and value) to implement eeprom emulation for F0,F1&F3, and it will take some time to verify on those MCUs.

…d on candybar(F0), jm60(F1), HSV2(F3). compile and tested on NOAH(F4).

tzarc · 2020-04-08T04:35:56Z

This should probably be moved to drivers/eeprom now.

xyzz · 2020-04-09T23:00:54Z

Design suggestion - store "original" contents of eeprom as the first FEE_MAX_BYTES bytes in the eeprom flash area, followed by "patches" to the data (i.e. address/data pairs same as done in this PR). This should save some space as when you do the backup/restore cycle, only FEE_MAX_BYTES bytes are wasted, as opposed to the current 3*FEE_MAX_BYTES if EEPROM_Restore writes complete eeprom data as 3-byte address/data pairs.

tzarc · 2020-07-25T22:01:48Z

Retargeted develop due to core changes.

LSChyi · 2020-09-06T14:10:24Z

tmk_core/common/chibios/eeprom_stm32.h

+//#define FEE_ADDR_OFFSET(Address) (Address * 2)      // 1 Byte per Word will be saved to preserve Flash
+#define FEE_DATA_ADDRESS(Address) ((Address)+2)     // flash address of the eeprom data
+#define FEE_ADDR_ADDRESS(Address) (Address)         // flash address of the eeprom address
+#define FEE_MAX_BYTES ((FEE_DENSITY_BYTES+1) > 1024 ? 1024 : (FEE_DENSITY_BYTES+1)) // eeprom size


I think this is a bug, it should be

#define FEE_MAX_BYTES ((FEE_DENSITY_BYTES+1) > 1024 ? (FEE_DENSITY_BYTES+1) : 1024)

The Atmega32u4's eeprom size was 1024, so it was limited to keep the compatibility.

Hmm, so you do not want to use the full space? Supposedly there is no problem if you use a larger external EEPROM, so there is no reason to limit the max size to just 1024.

If VIA didn't exist, I would say that is just fine. But ... it does, so a higher max by default is absolutely beneficial. To the point that I think it's more important than compatibility.

In fact, for the emulated eeprom for the F303, isn't it 4kb?

xyzz · 2020-10-18T08:41:27Z

tmk_core/common/chibios/flash_stm32.c

+        FLASH->AR = Page;
        FLASH->CR |= FLASH_CR_PER;
-        FLASH->AR = Page_Address;
+#endif


you swapped these two around, any reason for that? actually this reliably bricks my F072 board by erasing the first page (at 0x08000000)

To reproduce, I'm flashing with:

sudo dfu-util -d 0483:DF11 -a 0 -s 0x08000000:mass-erase:force -D .build/xyz_g60_via.bin

After a reboot, after the first FLASH_ErasePage is executed, the first page is gone, set to all FFs.

reference from datasheet (dm00031936), FLASH_AR must be set after setting FLASH_CR_PER:

When I added the page erase codes for STM32F4, the register setting order was misplaced.

drashna · 2020-12-17T04:02:33Z

There are merge conflicts here. Needs a rebase.

xyzz · 2020-12-21T07:39:02Z

tmk_core/common/chibios/eeprom_stm32.c

@@ -30,15 +31,24 @@
 /* Private variables ---------------------------------------------------------*/
 /* Functions -----------------------------------------------------------------*/

-uint8_t DataBuf[FEE_PAGE_SIZE];
+#define IS_VALID_ADDRESS(x) ((x) < FEE_MAX_BYTES)


suggest adding a cast to ensure unsigned x here

xyzz · 2020-12-21T07:41:28Z

tmk_core/common/chibios/eeprom_stm32.h

+#define FEE_DATA_ADDRESS(Address) ((Address)+2)     // flash address of the eeprom data
+#define FEE_ADDR_ADDRESS(Address) (Address)         // flash address of the eeprom address


these two IMO don't make sense with the new algorithm and should be removed (previously, it contained a mapping from eeprom address to flash address, but currently such mapping is not required)

xyzz · 2020-12-21T07:41:57Z

tmk_core/common/chibios/eeprom_stm32.c

+// restore data from DataBuf
+FLASH_Status EEPROM_Restore(void) {
+    uint32_t cur = FEE_PAGE_BASE_ADDRESS;
+    for (uint8_t i = 0; i < FEE_MAX_BYTES; i++) {


this should not be uint8_t as FEE_MAX_BYTES is 1024, it would loop forever and likely brick the board

xyzz · 2020-12-21T07:45:50Z

tmk_core/common/chibios/eeprom_stm32.c

+            DataBuf[addr] = *(__IO uint16_t*)(FEE_DATA_ADDRESS(begin));
+        } else if( addr == FEE_EMPTY_WORD) {
+            uint16_t data = *(__IO uint16_t*)(FEE_DATA_ADDRESS(begin));
+            if (data == FEE_EMPTY_WORD) {


I don't think this check is needed, you can bail right away

xyzz · 2020-12-21T07:47:44Z

tmk_core/common/chibios/eeprom_stm32.c

+ *  helper functions
+ *******************************************************************************/
+// backup the current data
+void EEPROM_Backup(void)


suggest renaming to EEPROM_Load or similar, Backup doesn't really make sense to me as it's loaded once during initialization

xyzz · 2020-12-21T08:07:13Z

tmk_core/common/chibios/eeprom_stm32.c

+    DataBuf[Address] = DataByte;
+#pragma GCC diagnostic pop
+    if (addr == FEE_EMPTY_VALUE) {
+        // EEPROM is full, need to erase and reprogramming


there's an issue here that when your eeprom is completely full, every single byte write will perform a full erase. I think it would be good to add some "slack" e.g. reserve twice the flash memory compared to emulated eeprom size (so times 8 instead of current times 4)

drashna · 2021-03-14T19:07:32Z

This looks like it needs a rebase, and to fix merge conflicts.

@yulei

based on code by @yulei in qmk/qmk_firmware#7803

@yulei

based on code by @yulei in qmk/qmk_firmware#7803

@yulei

based on code by @yulei in qmk/qmk_firmware#7803

zvecr · 2021-08-29T01:02:25Z

superseded by 14195

add eeprom emulation with flash for stm32f4

25641bd

yanfali added awaiting review needs doc on hold labels Jan 6, 2020

yanfali requested changes Jan 6, 2020

View reviewed changes

yanfali reviewed Jan 6, 2020

View reviewed changes

tmk_core/common/chibios/eeprom_stm32f4.c Outdated Show resolved Hide resolved

yanfali reviewed Jan 6, 2020

View reviewed changes

yulei added 2 commits January 6, 2020 10:52

adding license and doc on the file

9a05aa7

fixed typo

092831a

adjust code format

ad3fb66

tzarc requested changes Jan 6, 2020

View reviewed changes

yulei added 4 commits January 7, 2020 12:40

removed eeprom_stm32f4.c and merge and update eeprom_stm32.c, compile…

e966b7e

…d on candybar(F0), jm60(F1), HSV2(F3). compile and tested on NOAH(F4).

fixed a critical typo

f546bd3

Merge remote-tracking branch 'upstream/master' into eef4

db9e78a

add stm32f4 support

c2c5d09

yulei mentioned this pull request Mar 7, 2020

[Bug] [matrix/noah] Mod-Tap requires 2 taps to initiate #8259

Closed

3 tasks

yulei mentioned this pull request Apr 23, 2020

update chibios os usb for the otg driver #8893

Merged

13 tasks

tzarc changed the base branch from master to develop July 25, 2020 22:01

noroadsleft force-pushed the develop branch from 7d7f5bf to 2302d3b Compare July 26, 2020 07:32

noroadsleft force-pushed the develop branch 2 times, most recently from 1d4c0cd to 65c27a3 Compare August 29, 2020 21:42

noroadsleft force-pushed the develop branch from 3a8e1b8 to a1b96ba Compare September 4, 2020 22:03

LSChyi reviewed Sep 6, 2020

View reviewed changes

noroadsleft force-pushed the develop branch from a1b96ba to 3430907 Compare September 12, 2020 11:47

xyzz reviewed Oct 18, 2020

View reviewed changes

github-actions bot added the core label Oct 18, 2020

skullydazed force-pushed the develop branch from fa8cf59 to cce70f1 Compare October 18, 2020 21:10

fixed page and sector erase codes according to the stm32 hal lib codes

a9aa75e

noroadsleft force-pushed the develop branch from 5720202 to dcefc1a Compare October 23, 2020 22:37

skullydazed force-pushed the develop branch from db745a9 to 68fc044 Compare October 28, 2020 02:16

noroadsleft force-pushed the develop branch from 92ca6b9 to c3221d4 Compare October 30, 2020 20:23

noroadsleft closed this Nov 28, 2020

noroadsleft deleted the branch qmk:develop November 28, 2020 20:02

tzarc reopened this Nov 28, 2020

noroadsleft force-pushed the develop branch from 7d3841e to 3cc7d22 Compare November 30, 2020 11:32

xyzz reviewed Dec 21, 2020

View reviewed changes

xyzz mentioned this pull request Dec 26, 2020

eeprom_stm32: implement wear leveling #11313

Closed

14 tasks

tzarc closed this Feb 27, 2021

tzarc deleted the branch qmk:develop February 27, 2021 20:27

noroadsleft reopened this Feb 27, 2021

xyzz added a commit to vial-kb/vial-qmk that referenced this pull request Jul 11, 2021

eeprom_stm32: add stm32f4 series support

6b74c8b

based on code by @yulei in qmk/qmk_firmware#7803

xyzz added a commit to vial-kb/vial-qmk that referenced this pull request Jul 12, 2021

eeprom_stm32: add stm32f4 series support

a22cb46

based on code by @yulei in qmk/qmk_firmware#7803

xyzz added a commit to vial-kb/vial-qmk that referenced this pull request Jul 18, 2021

eeprom_stm32: add stm32f4 series support

1ba3126

based on code by @yulei in qmk/qmk_firmware#7803

zvecr closed this Aug 29, 2021

		#define FEE_DATA_ADDRESS(Address) ((Address)+2) // flash address of the eeprom data
		#define FEE_ADDR_ADDRESS(Address) (Address) // flash address of the eeprom address

eeprom emulation for stm32f4 #7803

eeprom emulation for stm32f4 #7803

Conversation

yulei commented Jan 6, 2020 • edited by drashna Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

yanfali commented Jan 6, 2020

yulei commented Jan 6, 2020

yanfali left a comment

Choose a reason for hiding this comment

yanfali commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvecr Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvecr commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzarc commented Jan 6, 2020

tzarc commented Jan 6, 2020 • edited Loading

yulei commented Jan 6, 2020

tzarc commented Apr 8, 2020

xyzz commented Apr 9, 2020

tzarc commented Jul 25, 2020

LSChyi Sep 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LSChyi Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drashna commented Dec 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drashna commented Mar 14, 2021 • edited Loading

zvecr commented Aug 29, 2021

yulei commented Jan 6, 2020 •

edited by drashna

Loading

zvecr Jan 6, 2020 •

edited

Loading

tzarc commented Jan 6, 2020 •

edited

Loading

LSChyi Sep 6, 2020 •

edited

Loading

LSChyi Sep 7, 2020 •

edited

Loading

drashna commented Mar 14, 2021 •

edited

Loading