EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format abstraction. #18332

purdeaandrei · 2022-09-11T05:42:08Z

Description

Make eeprom driver behaviour more uniform. Adding eeprom_driver_format abstraction.

Before this change, we used to call epprom_driver_erase(), if we detected that the eeprom
content did not have the necessary magic number, but only of it was implemented as an
"eeprom_driver". This is sometimes necessary, and sometimes not. We used to make the decision
based on whether the eeprom_driver_erase() function has been implemented or not, but that is
not really the best way to decide.

Erase in necessary for example when eeprom emulation/wear leveling is in use. This is because
the actual backing store also contains metadata that needs to start out in a clean state.
It is not necessary to erase I2C and SPI EEPROMs before use, however we used to do it.

The new eeprom_driver_format(erase=false) call will allow each eeprom driver to decide what is the
minimum amount of initialization/erasure that must be done to make the eeprom usable.
This also leaves open the door for future optimizations for eeprom emulation/wear leveling
drivers to only erase a subset of the backing store.

eeprom_driver_format(erase=true) will also make sure that the content of the eeprom is erased.

This change replaces calls from eeconfig to eeprom_driver_erase() to eeprom_driver_format().
As a result the following behavioral changes will happen:

Platform	Behavioral Change (if no magic number is found in EEPROM)
EEPROM_DRIVER=custom	No Change (Always Erase, in case eeprom_driver_format is not implemented by the custom driver)
EEPROM_DRIVER=wear_leveling	No Change (Always Erase)
EEPROM_DRIVER=i2c	Erase -> Don't Erase
EEPROM_DRIVER=spi	Erase -> Don't Erase
EEPROM_DRIVER=legacy_stm32_flash	Doesn't work, references inexistent file. If corrected, then No Change -> (Always Erase)
EEPROM_DRIVER=transient	Erase -> Don't Erase (Not necessary to erase because the backing store is in the .bss section)
EEPROM_DRIVER=vendor (AVR)	No Change (Don't erase)
EEPROM_DRIVER=vendor (STM32 072xB 042x6 - legacy)	No Change (Always Erase)
EEPROM_DRIVER=vendor (STM32 using wear_leveling)	No Change (Always Erase) (same as EEPROM_DRIVER=wear_leveling)
EEPROM_DRIVER=vendor (STM32_L0_L1 true eeprom)	Erase -> Don't Erase
EEPROM_DRIVER=vendor (RP2040)	No Change (Always Erase) (same as EEPROM_DRIVER=wear_leveling)
EEPROM_DRIVER=vendor (Kinetis Flexram (KL2x, K20x))	No Change (EEPROM_DRIVER macro is not defined)
EEPROM_DRIVER=vendor (other chibios=transient)	Erase -> Don't Erase (Not necessary to erase because the backing store is in the .bss section)
EEPROM_DRIVER=vendor (ARM_ATSAM)	No Change (EEPROM_DRIVER macro is not defined)

In particular this results in speed improvements when using STM32L0, i2c, and spi EEPROMs

Types of Changes

Issues Fixed or Closed by This PR

Why erase eeprom driver-only on eeconfig_init_quantum? #12618

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).

purdeaandrei · 2022-09-11T05:50:33Z

Optionally, I could remove the (bool erase) argument, and make eeprom_driver_format() always do the absolute minimum necessary. If the user wants to erase the content, then they can always call eeprom_driver_erase(), which doesn't (currently) require _format() to be called before it.

github-actions · 2022-12-06T02:34:02Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

github-actions · 2023-01-06T02:03:44Z

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

purdeaandrei · 2023-04-15T14:32:35Z

I have rebased, I fixed the conflicts, and I updated the commit message to better reflect new names.
I also removed the call to eeprom_driver_erase(); inside eeprom_driver_init of the transient driver. It's not needed since the backing store is in .bss

Please remove the stale label, and please review.

…t abstraction. Before this change, we used to call epprom_driver_erase(), if we detected that the eeprom content did not have the necessary magic number, but only of it was implemented as an "eeprom_driver". This is sometimes necessary, and sometimes not. We used to make the decision based on whether the eeprom_driver_erase() function has been implemented or not, but that is not really the best way to decide. Erase in necessary for example when eeprom emulation/wear leveling is in use. This is because the actual backing store also contains metadata that needs to start out in a clean state. It is not necessary to erase I2C and SPI EEPROMs before use, however we used to do it. The new eeprom_driver_format(erase=false) call will allow each eeprom driver to decide what is the minimum amount of initialization/erasure that must be done to make the eeprom usable. This also leaves open the door for future optimizations for eeprom emulation/wear leveling drivers to only erase a subset of the backing store. eeprom_driver_format(erase=true) will also make sure that the content of the eeprom is erased. This change replaces calls from eeconfig to eeprom_driver_erase() to eeprom_driver_format(). As a result the following behavioral changes will happen: |Platform | Behavioral Change (if no magic number is found in EEPROM) | |----------------------------------------------------|------------------------------------------------------------------------------------------------| |EEPROM_DRIVER=custom | No Change (Always Erase, in case eeprom_driver_format is not implemented by the custom driver) | |EEPROM_DRIVER=wear_leveling | No Change (Always Erase) | |EEPROM_DRIVER=i2c | Erase -> Don't Erase | |EEPROM_DRIVER=spi | Erase -> Don't Erase | |EEPROM_DRIVER=legacy_stm32_flash | Doesn't work, references inexistent file. If corrected, then No Change -> (Always Erase) | |EEPROM_DRIVER=transient | Erase -> Don't Erase (Not necessary to erase because the backing store is in the .bss section) | |EEPROM_DRIVER=vendor (AVR) | No Change (Don't erase) | |EEPROM_DRIVER=vendor (STM32 072xB 042x6 - legacy) | No Change (Always Erase) | |EEPROM_DRIVER=vendor (STM32 using wear_leveling) | No Change (Always Erase) (same as EEPROM_DRIVER=wear_leveling) | |EEPROM_DRIVER=vendor (STM32_L0_L1 true eeprom) | Erase -> Don't Erase | |EEPROM_DRIVER=vendor (RP2040) | No Change (Always Erase) (same as EEPROM_DRIVER=wear_leveling) | |EEPROM_DRIVER=vendor (Kinetis Flexram (KL2x, K20x)) | No Change (EEPROM_DRIVER macro is not defined) | |EEPROM_DRIVER=vendor (other chibios=transient) | Erase -> Don't Erase (Not necessary to erase because the backing store is in the .bss section) | |EEPROM_DRIVER=vendor (ARM_ATSAM) | No Change (EEPROM_DRIVER macro is not defined) | In particular this results in speed improvements when using STM32L0, i2c, and spi EEPROMs

tzarc · 2024-02-16T14:28:35Z

After some internal discussion, we're moving this to a 2024q2 merge candidate, with an intention to merge into develop once the merge window opens. There are some doubts that skipping erasure in certain cases is enough, and we'd like to see it land in develop first to be proven before it hits master.

github-actions · 2024-04-02T01:49:10Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

…abstraction. (qmk#18332)

github-actions bot added the core label Sep 11, 2022

KarlK90 requested a review from tzarc September 12, 2022 12:22

drashna requested a review from a team September 12, 2022 15:50

tzarc mentioned this pull request Oct 3, 2022

Only call eeprom_driver_erase in eeconfig_init_quantum #18567

Closed

14 tasks

github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Dec 6, 2022

github-actions bot closed this Jan 6, 2023

tzarc reopened this Jan 6, 2023

tzarc added awaiting review stale Issues or pull requests that have become inactive without resolution. and removed stale Issues or pull requests that have become inactive without resolution. labels Jan 6, 2023

purdeaandrei force-pushed the f_dont_erase_eeprom_when_we_dont_have_to branch from d18f222 to 0ca6208 Compare April 15, 2023 14:12

purdeaandrei force-pushed the f_dont_erase_eeprom_when_we_dont_have_to branch from 0ca6208 to a51882c Compare April 15, 2023 14:38

drashna added the breaking_change_2024q1 label Dec 2, 2023

tzarc changed the title ~~[RFC] EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format abstraction.~~ EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format abstraction. Feb 16, 2024

tzarc removed awaiting review stale Issues or pull requests that have become inactive without resolution. labels Feb 16, 2024

tzarc approved these changes Feb 16, 2024

View reviewed changes

tzarc requested a review from a team February 16, 2024 13:56

tzarc added breaking_change_2024q2 and removed breaking_change_2024q1 labels Feb 16, 2024

tzarc added the develop-fast-track Intended to be merged early in the next develop cycle. label Feb 16, 2024

github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Apr 2, 2024

drashna approved these changes May 2, 2024

View reviewed changes

drashna requested a review from a team May 2, 2024 22:02

drashna removed the stale Issues or pull requests that have become inactive without resolution. label May 2, 2024

tzarc added breaking_change_2024q3 and removed breaking_change_2024q2 labels May 27, 2024

daskygit merged commit 267dffd into qmk:develop May 28, 2024

TheShige pushed a commit to TheShige/qmk_firmware that referenced this pull request Jul 26, 2024

EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format …

7a7e10f

…abstraction. (qmk#18332)

sqrtnull pushed a commit to sqrtnull/qmk_firmware that referenced this pull request Oct 22, 2024

EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format …

283d9a1

…abstraction. (qmk#18332)

acidMyke pushed a commit to acidMyke/qmk_firmware that referenced this pull request Nov 3, 2024

EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format …

370e88c

…abstraction. (qmk#18332)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format abstraction. #18332

EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format abstraction. #18332

purdeaandrei commented Sep 11, 2022 •

edited

Loading

purdeaandrei commented Sep 11, 2022

github-actions bot commented Dec 6, 2022

github-actions bot commented Jan 6, 2023

purdeaandrei commented Apr 15, 2023

tzarc commented Feb 16, 2024

github-actions bot commented Apr 2, 2024

EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format abstraction. #18332

EEPROM: Don't erase if we don't have to. Adding eeprom_driver_format abstraction. #18332

Conversation

purdeaandrei commented Sep 11, 2022 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

purdeaandrei commented Sep 11, 2022

github-actions bot commented Dec 6, 2022

github-actions bot commented Jan 6, 2023

purdeaandrei commented Apr 15, 2023

tzarc commented Feb 16, 2024

github-actions bot commented Apr 2, 2024

purdeaandrei commented Sep 11, 2022 •

edited

Loading