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

Wear-leveling EEPROM drivers: embedded_flash, spi_flash, legacy #17376

Merged
merged 19 commits into from
Jun 29, 2022

Conversation

tzarc
Copy link
Member

@tzarc tzarc commented Jun 13, 2022

Description

Hooks up the wear-leveling algorithm in #16996 with actual writes to underlying storage.

Adds ChibiOS EFL driver for most implementations -- the glaring exception is STM32F401/F411. These still use the old flash_stm32.c implementation because EFL doesn't yet support those MCUs. ChibiOS 21.11.3+ will support it -- have submitted a patch for them and it's been committed. Need to work out if we go with a non-point-release of ChibiOS.

Otherwise, the EFL driver supports F0, F1, F3, (some) F4, G0, G4, L4, L4+, NUC123, GD32VF103.

So far, have tested the PR with drivers:

  • EFL: STM32F103, STM32L412, STM32G474, GD32VF103
  • Legacy: STM32F401, STM32F411 (both with and without tinyuf2)
  • External flash: W25Q32JVSSIQ (Djinn), W25Q64JVSSIQ (Ghoul), W25X20CL (AnnePro2/C18)

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

Initial configs for testing:

STM32F401, STM32F411, with and without tinyuf2:

rules.mk:

EEPROM_DRIVER = wear_leveling
WEAR_LEVELING_DRIVER = legacy_emulation

STM32F103xB, STM32G474xE, GD32VF103xB, STM32L412xB, STM32L432xC:

rules.mk:

EEPROM_DRIVER = wear_leveling
WEAR_LEVELING_DRIVER = efl

W25Q32, W25Q64:

rules.mk:

EEPROM_DRIVER = wear_leveling
WEAR_LEVELING_DRIVER = flash_spi

@tzarc tzarc added the awaiting_pr Relies on another PR to be merged first label Jun 13, 2022
@tzarc tzarc requested a review from a team June 13, 2022 05:41
@tzarc tzarc self-assigned this Jun 13, 2022
@KarlK90
Copy link
Member

KarlK90 commented Jun 25, 2022

As discussed via Discord it would be good to have some basic code and data protection in place so it is not possible to corrupt QMK it self. In the future we would want designated eeprom areas in flash from linker scripts for this. As a short therm bodge it would probably suffice to check that the erasing / writing address is outside of the __text_base__ + __text_end__, __rodata_base__ + __rodata_end__ and __vectors_base__ + __vectors_end__ range.

@tzarc
Copy link
Member Author

tzarc commented Jun 25, 2022

As discussed via Discord it would be good to have some basic code and data protection in place so it is not possible to corrupt QMK it self. In the future we would want designated eeprom areas in flash from linker scripts for this. As a short therm bodge it would probably suffice to check that the erasing / writing address is outside of the __text_base__ + __text_end__, __rodata_base__ + __rodata_end__ and __vectors_base__ + __vectors_end__ range.

Agreed; I think they can come later, though. Virtually all implementations are currently correctly sized, and only people like myself or @drashna are anywhere near the flash limits on the MCUs.

For the scope of this PR I'd prefer to leave it out, so that we can get some relief on the L4xx complaints by Keychron users.
We can work out a proper solution later, especially if it factors in things like #8179 in a cohesive manner.

@tzarc tzarc removed the awaiting_pr Relies on another PR to be merged first label Jun 26, 2022
@Jpe230
Copy link
Contributor

Jpe230 commented Jun 28, 2022

I been using this for my AnnePro2 C18 with a Winbond W25X20CL, worth mentioning that it is required to enable SPI and configure the SPI Driver according to the docs.

rules.mk:

EEPROM_DRIVER = wear_leveling
WEAR_LEVELING_DRIVER = flash_spi

config.h:

#define EXTERNAL_FLASH_SPI_SLAVE_SELECT_PIN A3
#define EXTERNAL_FLASH_SPI_CLOCK_DIVISOR 16
#define EXTERNAL_FLASH_PAGE_SIZE 256
#define EXTERNAL_FLASH_SECTOR_SIZE 4096
#define EXTERNAL_FLASH_BLOCK_SIZE 4096
#define EXTERNAL_FLASH_SIZE (256 * 1024) // 2M-bit flash size
#define WEAR_LEVELING_LOGICAL_SIZE ((WEAR_LEVELING_BACKING_SIZE) / 4) // Decrease Logical size bc RAM constrains

(Also describing the AnnePro2 C18 SPI configuration)

#define SPI_DRIVER SPID1
#define SPI_SCK_PIN A0
#define SPI_SCK_PAL_MODE 5
#define SPI_MOSI_PIN A1
#define SPI_MOSI_PAL_MODE 5
#define SPI_MISO_PIN A2
#define SPI_MISO_PAL_MODE 5

@tzarc tzarc changed the title Add first batch of wear-leveling EEPROM drivers Wear-leveling EEPROM drivers: efl, flash_spi, legacy_emulation Jun 29, 2022
@tzarc tzarc marked this pull request as ready for review June 29, 2022 12:47
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

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

LGTM, I'll nuke the GD32VF103 flash mangling once this is merged.

docs/eeprom_driver.md Show resolved Hide resolved
docs/eeprom_driver.md Outdated Show resolved Hide resolved
docs/eeprom_driver.md Show resolved Hide resolved
builddefs/common_features.mk Show resolved Hide resolved
@KarlK90 KarlK90 requested a review from a team June 29, 2022 20:21
@tzarc tzarc changed the title Wear-leveling EEPROM drivers: efl, flash_spi, legacy_emulation Wear-leveling EEPROM drivers: embedded_flash, spi_flash, legacy Jun 29, 2022
@KarlK90
Copy link
Member

KarlK90 commented Jun 29, 2022

Thanks for the quick changes, even better now 😉

@tzarc tzarc merged commit 34e244c into qmk:develop Jun 29, 2022
@tzarc tzarc deleted the wl_stm32f4 branch June 29, 2022 21:42
0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022
Comment on lines +30 to +31
# if defined(QMK_MCU_SERIES_STM32F0XX) || defined(QMK_MCU_SERIES_STM32F1XX) || defined(QMK_MCU_SERIES_STM32F3XX) || defined(QMK_MCU_SERIES_STM32F4XX) || defined(QMK_MCU_SERIES_STM32G4XX) || defined(QMK_MCU_SERIES_STM32L0XX) || defined(QMK_MCU_SERIES_STM32L4XX) || defined(QMK_MCU_SERIES_GD32VF103)
return ((*(uint32_t *)FLASHSIZE_BASE) & 0xFFFFU) << 10U; // this register has the flash size in kB, so we convert it to bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

If the information at https://mecrisp-stellaris-folkdoc.sourceforge.io/bluepill-diagnostics-v1.6.html#passed-genuine-stm32f103c8t6 is accurate, and genuine STM32F103x8 chips actually report the 64K flash size in this register, then switching to this driver could potentially break some bluepill boards that are currently working with the default vendor driver config (although currently the default LD script for F103+stm32duino is for STM32F103x8, it does not reserve the space for EEPROM emulation, therefore it is possible that some firmwares have the size which does not leave enough space for EEPROM if using only the documented 64K). Apparently some people also rely on that feature when actually using the extra space for firmware too: #12537 (comment).

I cannot verify this on any actual hardware, because the “STM32F103C8T6” chip that I have reports 128K in the flash size register for some reason (therefore Bluepill Diagnostics decides that it's a fake).

Not really sure what to do about this though. It could be possible to hardcode the expected address at the compile time, so that by default a firmware compiled for STM32F103x8 would always place the EEPROM backing store below the 64K limit (for reliability this requires implementing the flash size check which takes the backing store into account), and a firmware compiled for STM32F103xB would always place the EEPROM backing store just below the 128K limit (instead of trying to check the flash size and potentially placing the backing store below 64K instead of 128K). But then there is not much point in doing this autodetection at all (if the chip ended up smaller than expected, the backing store might not fit; if the chip ended up larger than expected, moving the EEPROM backing store to the end of the real flash does not accomplish much).

BTW, another potential problem with STM32F103 is that the ChibiOS EFL driver for that series apparently does not have proper support for the “XL density” chips (> 512K flash) — those chips have 2 flash banks with separate SR/CR/AR registers, but the EFL driver code in ChibiOS does not have support for that second set of registers. But I don't have any such chips for testing.

@KeychronMacro KeychronMacro mentioned this pull request Aug 16, 2022
14 tasks
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants