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.

Sign up for GitHub

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

eeprom emulation for stm32f4 #7803

Closed
wants to merge 9 commits into from
Closed

eeprom emulation for stm32f4 #7803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
25641bd
add eeprom emulation with flash for stm32f4
yulei Jan 6, 2020
9a05aa7
adding license and doc on the file
yulei Jan 6, 2020
092831a
fixed typo
yulei Jan 6, 2020
ad3fb66
adjust code format
yulei Jan 6, 2020
e966b7e
removed eeprom_stm32f4.c and merge and update eeprom_stm32.c, compile…
yulei Jan 7, 2020
f546bd3
fixed a critical typo
yulei Jan 7, 2020
db9e78a
Merge remote-tracking branch 'upstream/master' into eef4
yulei Feb 23, 2020
c2c5d09
add stm32f4 support
yulei Feb 23, 2020
a9aa75e
fixed page and sector erase codes according to the stm32 hal lib codes
yulei Oct 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions common_features.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,11 @@ else
SRC += $(PLATFORM_COMMON_DIR)/flash_stm32.c
OPT_DEFS += -DEEPROM_EMU_STM32F072xB
OPT_DEFS += -DSTM32_EEPROM_ENABLE
else ifeq ($(MCU_SERIES), STM32F4xx)
SRC += $(PLATFORM_COMMON_DIR)/eeprom_stm32.c
SRC += $(PLATFORM_COMMON_DIR)/flash_stm32.c
OPT_DEFS += -DEEPROM_EMU_STM32F411xE
OPT_DEFS += -DSTM32_EEPROM_ENABLE
else
# This will effectively work the same as "transient" if not supported by the chip
SRC += $(PLATFORM_COMMON_DIR)/eeprom_teensy.c
Expand Down
154 changes: 108 additions & 46 deletions tmk_core/common/chibios/eeprom_stm32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* Artur F.
*
* Modifications for QMK and STM32F303 by Yiancar
* update for STM32F4 support and the the flashing algorithm by yulei
*/

#include <stdio.h>
Expand All @@ -30,15 +31,24 @@
/* Private variables ---------------------------------------------------------*/
/* Functions -----------------------------------------------------------------*/

uint8_t DataBuf[FEE_PAGE_SIZE];
#define IS_VALID_ADDRESS(x) ((x) < FEE_MAX_BYTES)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest adding a cast to ensure unsigned x here


uint8_t DataBuf[FEE_MAX_BYTES];
static void EEPROM_Backup(void);
static FLASH_Status EEPROM_Restore(void);
static uint32_t EEPROM_FindValidAddress(void);
static void EEPROM_Clear(void);

/*****************************************************************************
* Delete Flash Space used for user Data, deletes the whole space between
* RW_PAGE_BASE_ADDRESS and the last uC Flash Page
* Unlock the FLASH programming and initialized the DataBuf
******************************************************************************/
uint16_t EEPROM_Init(void) {
// unlock flash
FLASH_Unlock();

// initialize DataBuf
EEPROM_Backup();

// Clear Flags
// FLASH_ClearFlag(FLASH_SR_EOP|FLASH_SR_PGERR|FLASH_SR_WRPERR);

Expand All @@ -48,72 +58,124 @@ uint16_t EEPROM_Init(void) {
* Erase the whole reserved Flash Space used for user Data
******************************************************************************/
void EEPROM_Erase(void) {
int page_num = 0;
// erase all flash pages
EEPROM_Clear();

// delete all pages from specified start page to the last page
do {
FLASH_ErasePage(FEE_PAGE_BASE_ADDRESS + (page_num * FEE_PAGE_SIZE));
page_num++;
} while (page_num < FEE_DENSITY_PAGES);
// reset the content of the buffer
memset(&DataBuf[0], FEE_EMPTY_BYTE, sizeof(DataBuf));
}

/*****************************************************************************
* Writes once data byte to flash on specified address. If a byte is already
* written, the whole page must be copied to a buffer, the byte changed and
* the manipulated buffer written after PageErase.
* Write data with its eeprom address to flash if there has empty words,
* otherwise backup the current valid data, erase all flash pages,
* and finally restore the valid data.
*******************************************************************************/
uint16_t EEPROM_WriteDataByte(uint16_t Address, uint8_t DataByte) {
FLASH_Status FlashStatus = FLASH_COMPLETE;

uint32_t page;
int i;

// exit if desired address is above the limit (e.G. under 2048 Bytes for 4 pages)
if (Address > FEE_DENSITY_BYTES) {
uint32_t addr;
// exit if not a valid address
if (!IS_VALID_ADDRESS(Address)) {
return 0;
}

// calculate which page is affected (Pagenum1/Pagenum2...PagenumN)
page = FEE_ADDR_OFFSET(Address) / FEE_PAGE_SIZE;
// we are sure the address will not be out of bound
#pragma GCC diagnostic ignored "-Warray-bounds"
// same value, do not need program
if (DataBuf[Address] == DataByte) {
return FlashStatus;
}

// if current data is 0xFF, the byte is empty, just overwrite with the new one
if ((*(__IO uint16_t *)(FEE_PAGE_BASE_ADDRESS + FEE_ADDR_OFFSET(Address))) == FEE_EMPTY_WORD) {
FlashStatus = FLASH_ProgramHalfWord(FEE_PAGE_BASE_ADDRESS + FEE_ADDR_OFFSET(Address), (uint16_t)(0x00FF & DataByte));
// find the address can be written
addr = EEPROM_FindValidAddress();
DataBuf[Address] = DataByte;
#pragma GCC diagnostic pop
if (addr == FEE_EMPTY_VALUE) {
// EEPROM is full, need to erase and reprogramming
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

EEPROM_Clear();
EEPROM_Restore();
} else {
// Copy Page to a buffer
memcpy(DataBuf, (uint8_t *)FEE_PAGE_BASE_ADDRESS + (page * FEE_PAGE_SIZE), FEE_PAGE_SIZE); // !!! Calculate base address for the desired page

// check if new data is differ to current data, return if not, proceed if yes
if (DataByte == *(__IO uint8_t *)(FEE_PAGE_BASE_ADDRESS + FEE_ADDR_OFFSET(Address))) {
return 0;
}

// manipulate desired data byte in temp data array if new byte is differ to the current
DataBuf[FEE_ADDR_OFFSET(Address) % FEE_PAGE_SIZE] = DataByte;

// Erase Page
FlashStatus = FLASH_ErasePage(FEE_PAGE_BASE_ADDRESS + (page * FEE_PAGE_SIZE));

// Write new data (whole page) to flash if data has been changed
for (i = 0; i < (FEE_PAGE_SIZE / 2); i++) {
if ((__IO uint16_t)(0xFF00 | DataBuf[FEE_ADDR_OFFSET(i)]) != 0xFFFF) {
FlashStatus = FLASH_ProgramHalfWord((FEE_PAGE_BASE_ADDRESS + (page * FEE_PAGE_SIZE)) + (i * 2), (uint16_t)(0xFF00 | DataBuf[FEE_ADDR_OFFSET(i)]));
}
}
FLASH_ProgramHalfWord(FEE_ADDR_ADDRESS(addr), Address);
FLASH_ProgramHalfWord(FEE_DATA_ADDRESS(addr), DataByte);
}

return FlashStatus;
}
/*****************************************************************************
* Read once data byte from a specified address.
*******************************************************************************/
uint8_t EEPROM_ReadDataByte(uint16_t Address) {
uint8_t DataByte = 0xFF;
if (!IS_VALID_ADDRESS(Address)) {
return FEE_EMPTY_BYTE;
}

// Get Byte from specified address
DataByte = (*(__IO uint8_t *)(FEE_PAGE_BASE_ADDRESS + FEE_ADDR_OFFSET(Address)));
// Get Byte from caches
return DataBuf[Address];
}

return DataByte;
/*****************************************************************************
* helper functions
*******************************************************************************/
// backup the current data
void EEPROM_Backup(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
uint32_t begin = FEE_PAGE_BASE_ADDRESS;
uint32_t end = FEE_PAGE_END_ADDRESS;
memset(&DataBuf[0], FEE_EMPTY_BYTE, sizeof(DataBuf));
while( begin < end) {
uint16_t addr = *(__IO uint16_t*)(FEE_ADDR_ADDRESS(begin));
if (IS_VALID_ADDRESS(addr)) {
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

// we reached the end of valid data
break;
}
}
begin += 4;
}
}
// 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++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (DataBuf[i] != FEE_EMPTY_BYTE) {
FLASH_ProgramHalfWord(FEE_ADDR_ADDRESS(cur), i);
FLASH_ProgramHalfWord(FEE_DATA_ADDRESS(cur), DataBuf[i]);
cur += 4;
}
}
return FLASH_COMPLETE;
}
// find an empty place for programming
uint32_t EEPROM_FindValidAddress(void) {
uint32_t begin = FEE_PAGE_BASE_ADDRESS;
uint32_t end = FEE_PAGE_END_ADDRESS;
while( begin < end) {
uint32_t data = *(__IO uint32_t*)(begin);
if (data == FEE_EMPTY_VALUE) {
return begin;
}
begin += 4;
}
return FEE_EMPTY_VALUE;
}

void EEPROM_Clear(void)
{
#if defined(EEPROM_EMU_STM32F411xE)
FLASH_ErasePage(FEE_SECTOR_ID);
#else
int page_num = 0;

// delete all pages from specified start page to the last page
do {
FLASH_ErasePage(FEE_PAGE_BASE_ADDRESS + (page_num * FEE_PAGE_SIZE));
page_num++;
} while (page_num < FEE_DENSITY_PAGES);
#endif
}
/*****************************************************************************
* Wrap library in AVR style functions.
*******************************************************************************/
Expand Down
24 changes: 19 additions & 5 deletions tmk_core/common/chibios/eeprom_stm32.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,23 @@
# define MCU_STM32F103RB
#elif defined(EEPROM_EMU_STM32F072xB)
# define MCU_STM32F072CB
#elif defined(EEPROM_EMU_STM32F411xE)
# define MCU_STM32F411xE
#else
# error "not implemented."
#endif

#ifndef EEPROM_PAGE_SIZE
# if defined(MCU_STM32F103RB)
# define FEE_PAGE_SIZE (uint16_t)0x400 // Page size = 1KByte
# define FEE_DENSITY_PAGES 2 // How many pages are used
# define FEE_DENSITY_PAGES 4 // atleast needs 4K Bytes to emulated 1024 eeprom on AVR
# 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the explanation was added on the eeprom_stm32f4.c

Copy link
Member

@zvecr zvecr Jan 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I am just putting it on record that:

  1. I'm not keen on having a non-uniform approach to the eeprom emulation strategy
  2. I'm not keen on the code duplication this PR introduces

# define FEE_PAGE_SIZE (uint32_t)0x20000 // Page size = 128KByte
# define FEE_DENSITY_PAGES 1 // How many pages are used
# define FEE_SECTOR_ID 7 // sector id of the flash
# else
# error "No MCU type specified. Add something like -DMCU_STM32F103RB to your compiler arguments (probably in a Makefile)."
# endif
Expand All @@ -61,18 +67,26 @@
# define FEE_MCU_FLASH_SIZE 384 // Size in Kb
# elif defined(MCU_STM32F303CC)
# define FEE_MCU_FLASH_SIZE 256 // Size in Kb
# elif defined(MCU_STM32F411xE)
# define FEE_MCU_FLASH_SIZE 512 // Size in Kb
# else
# error "No MCU type specified. Add something like -DMCU_STM32F103RB to your compiler arguments (probably in a Makefile)."
# endif
#endif

// DONT CHANGE
// Choose location for the first EEPROM Page address on the top of flash
#define FEE_PAGE_BASE_ADDRESS ((uint32_t)(0x8000000 + FEE_MCU_FLASH_SIZE * 1024 - FEE_DENSITY_PAGES * FEE_PAGE_SIZE))
#define FEE_DENSITY_BYTES ((FEE_PAGE_SIZE / 2) * FEE_DENSITY_PAGES - 1)
#define FEE_LAST_PAGE_ADDRESS (FEE_PAGE_BASE_ADDRESS + (FEE_PAGE_SIZE * FEE_DENSITY_PAGES))
#define FEE_PAGE_END_ADDRESS ((uint32_t)(0x08000000 + FEE_MCU_FLASH_SIZE * 1024))
#define FEE_PAGE_BASE_ADDRESS ((uint32_t)((FEE_PAGE_END_ADDRESS) - FEE_DENSITY_PAGES * FEE_PAGE_SIZE))
#define FEE_DENSITY_BYTES ((FEE_PAGE_SIZE / 4) * FEE_DENSITY_PAGES - 1) // 4 Bytes for address, value pair
//#define FEE_LAST_PAGE_ADDRESS (FEE_PAGE_BASE_ADDRESS + (FEE_PAGE_SIZE * FEE_DENSITY_PAGES))
#define FEE_EMPTY_BYTE ((uint8_t)0xFF)
#define FEE_EMPTY_WORD ((uint16_t)0xFFFF)
#define FEE_ADDR_OFFSET(Address) (Address * 2) // 1Byte per Word will be saved to preserve Flash
#define FEE_EMPTY_VALUE ((uint32_t)0xFFFFFFFF)
//#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
Comment on lines +87 to +88
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

#define FEE_MAX_BYTES ((FEE_DENSITY_BYTES+1) > 1024 ? 1024 : (FEE_DENSITY_BYTES+1)) // eeprom size
Copy link
Contributor

@LSChyi LSChyi Sep 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is a bug, it should be

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@LSChyi LSChyi Sep 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?


// Use this function to initialize the functionality
uint16_t EEPROM_Init(void);
Expand Down
Loading