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

emds: Revise rram related timing in emds #19275

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

Balaklaka
Copy link
Contributor

Updated estimated timing for rram storage using emds. Also updated documentation and test to a more proper indication of values for rram.

@Balaklaka Balaklaka requested review from a team as code owners December 5, 2024 08:12
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Dec 5, 2024
@Balaklaka Balaklaka removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 5, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Dec 5, 2024

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 6

Inputs:

Sources:

sdk-nrf: PR head: 070328ff05fba447099208af1cd3700df1fb978e

more details

sdk-nrf:

PR head: 070328ff05fba447099208af1cd3700df1fb978e
merge base: bb2f537273f8ba6e6028c7496eb8ab6057f939dd
target head (main): f3ac9cfb784d163f4c1af11209976fcd5c403d5a
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (4)
doc
│  ├── nrf
│  │  ├── libraries
│  │  │  ├── others
│  │  │  │  │ emds.rst
subsys
│  ├── emds
│  │  │ Kconfig
tests
│  ├── subsys
│  │  ├── emds
│  │  │  ├── emds_api
│  │  │  │  │ prj.conf
│  │  │  ├── emds_flash
│  │  │  │  ├── src
│  │  │  │  │  │ main.c

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 57
  • ✅ Integration tests
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-chip
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
subsys/emds/Kconfig Show resolved Hide resolved
subsys/emds/Kconfig Outdated Show resolved Hide resolved
doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
tests/subsys/emds/emds_flash/src/main.c Outdated Show resolved Hide resolved
@Balaklaka Balaklaka force-pushed the develop/revise_rram_time_emds branch from 9d09608 to 780cf30 Compare December 5, 2024 19:07
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 5, 2024
@Balaklaka Balaklaka force-pushed the develop/revise_rram_time_emds branch from 780cf30 to 0eeab6e Compare December 5, 2024 21:59
Copy link
Contributor

@ludvigsj ludvigsj left a comment

Choose a reason for hiding this comment

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

Tiny nitpicks left

doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
If partial erase is not enabled or supported, the time of a complete sector erase has to be included in the :kconfig:option:`CONFIG_EMDS_FLASH_TIME_BASE_OVERHEAD_US`.
When partial erase is enabled and supported by the hardware, include the time it takes for the scheduler to trigger, which is depending on the time defined in :kconfig:option:`CONFIG_SOC_FLASH_NRF_PARTIAL_ERASE_MS`.
When changing the :kconfig:option:`CONFIG_EMDS_FLASH_TIME_BASE_OVERHEAD_US` option, it is important that the worst time is considered.
When configuring these values using RRAM there is not a need to consider garbage collection in the same way as for flash, as it can be written to regardless of value. But the ERASEALL functionality
Copy link
Contributor

Choose a reason for hiding this comment

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

But the ERASEALL functionality should be avoided

How can you 'avoid' eraseall functionality? EMDS storage page erase anyway needed upon the next reboot right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RRAMC provides functionality to protect the device against the erase all function.
See https://docs.nordicsemi.com/bundle/ps_nrf54L15/page/rramc.html#ariaid-title7

These can be found by looking at datasheets, driver documentation, and the configuration of the application.
:math:`s_\text{ate}` is the size of the allocation table entry used by the EMDS flash module, which is 8 B.
:math:`s_\text{ate}` is the size of the allocation table entry used by the EMDS flash/RRAM module, which is 8 B.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:math:`s_\text{ate}` is the size of the allocation table entry used by the EMDS flash/RRAM module, which is 8 B.
:math:`s_\text{ate}` is the size of the allocation table entry used by the EMDS flash/RRAM module, which is 8 B if NVS is used, else it is 16 B for ZMS.

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 ate size is 8 regardless of ZMS or NVS. It is defined in emds_flash.c.

@@ -142,7 +144,7 @@ The following values can be inserted into the formula:
This is the worst case overhead when a store is triggered in the middle of an erase on nRF52840 with :kconfig:option:`CONFIG_SOC_FLASH_NRF_PARTIAL_ERASE` enabled in the sample by default, and should be adjusted when using other configurations.
* Set :math:`t_\text{entry}` = 300 µs and :math:`t_\text{word}` = 41 µs. *Note: These values are valid only for this specific chip and configuration, and should be computed for the specific configuration whenever using EMDS.*
* The sample uses two entries, one for the RPL with 255 entries (:math:`s_i` = 2040 B) and one for the lightness state (:math:`s_i` = 3 B).
* The flash write block size :math:`s_\text{block}` in this case is 4 B, and the ATE size :math:`s_\text{ate}` is 8 B.
* The flash/RRAM write block size :math:`s_\text{block}` in this case is 4 B, and the ATE size :math:`s_\text{ate}` is 8 B.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The flash/RRAM write block size :math:`s_\text{block}` in this case is 4 B, and the ATE size :math:`s_\text{ate}` is 8 B.
* The flash/RRAM write block size :math:`s_\text{block}` in this case is 4 B, and for NVS backend the ATE size :math:`s_\text{ate}` is 8 B.

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 ate size is 8 regardless of ZMS or NVS. It is defined in emds_flash.c.

Comment on lines 10 to 11
Overview
********
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Overview
********

Implementation
==============
The :kconfig:option:`CONFIG_EMDS` Kconfig option enables the emergency data storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Implementation
==============
The :kconfig:option:`CONFIG_EMDS` Kconfig option enables the emergency data storage.
Implementation
**************
The :kconfig:option:`CONFIG_EMDS` Kconfig option enables the emergency data storage.

@Balaklaka Balaklaka force-pushed the develop/revise_rram_time_emds branch from 0eeab6e to a646365 Compare December 17, 2024 14:55
@@ -68,8 +73,7 @@ config EMDS_FLASH_TIME_BASE_OVERHEAD_US
worst case scenario, before starting storing entries. This value
is dependent on the chip used, and should be checked against the chip
datasheet.
For RRAM-based flash drivers, the time is defined by the erasing block size.
The default erasing block size is 4096 bytes.
For RRAM-based flash drivers, ERASEPROTECT should be used and we can disregard the SOC_FLASH_NRF_PARTIAL_ERASE_MS parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For RRAM-based flash drivers, ERASEPROTECT should be used and we can disregard the SOC_FLASH_NRF_PARTIAL_ERASE_MS parameter.
For RRAM-based flash drivers, use ERASEPROTECT and disregard the SOC_FLASH_NRF_PARTIAL_ERASE_MS parameter.

@@ -128,9 +128,9 @@ The :c:func:`emds_store_time_get` function estimates the required worst-case tim
t_\text{store} = t_\text{base} + \sum_{i = 1}^n \left(t_\text{entry} + t_\text{word}\left(\left\lceil\frac{s_\text{ate}}{s_\text{block}}\right\rceil + \left\lceil\frac{s_i}{s_\text{block}}\right\rceil \right)\right)

where :math:`t_\text{base}` is the value specified by :kconfig:option:`CONFIG_EMDS_FLASH_TIME_BASE_OVERHEAD_US`, :math:`t_\text{entry}` is the value specified by :kconfig:option:`CONFIG_EMDS_FLASH_TIME_ENTRY_OVERHEAD_US` and :math:`t_\text{word}` is the value specified by :kconfig:option:`CONFIG_EMDS_FLASH_TIME_WRITE_ONE_WORD_US`.
:math:`s_i` is the size of the :math:`i`\ th entry in bytes and :math:`s_\text{block}` is the number of bytes in one word of flash.
:math:`s_i` is the size of the :math:`i`\ th entry in bytes and :math:`s_\text{block}` is the number of bytes in one word of flash, while for RRAM the write-buffer size i number of 128-bit words (16 bytes).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:math:`s_i` is the size of the :math:`i`\ th entry in bytes and :math:`s_\text{block}` is the number of bytes in one word of flash, while for RRAM the write-buffer size i number of 128-bit words (16 bytes).
:math:`s_i` is the size of the :math:`i`\ th entry in bytes and :math:`s_\text{block}` is the number of bytes in one word of flash, while for RRAM the write-buffer size ``i`` number of 128-bit words (16 bytes).

@@ -64,7 +64,7 @@ The :c:func:`emds_is_ready` function can be called to check if EMDS is prepared
Once the data storage has completed, a callback is called if provided in :c:func:`emds_init`.
This callback notifies the application that the data storage has completed, and can be used to reboot the CPU or execute another function that is needed.

After completion of :c:func:`emds_store`, the :c:func:`emds_is_ready` function call will return error, since it can no longer guarantee that the data will fit into the flash area.
After completion of :c:func:`emds_store`, the :c:func:`emds_is_ready` function call will return error, since it can no longer guarantee that the data will fit into the persistent memory area.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
After completion of :c:func:`emds_store`, the :c:func:`emds_is_ready` function call will return error, since it can no longer guarantee that the data will fit into the persistent memory area.
After completion of :c:func:`emds_store`, the :c:func:`emds_is_ready` function call will return an error, because it can no longer guarantee that the data will fit into the persistent memory area.

If partial erase is not enabled or supported, the time of a complete sector erase has to be included in the :kconfig:option:`CONFIG_EMDS_FLASH_TIME_BASE_OVERHEAD_US`.
When partial erase is enabled and supported by the hardware, include the time it takes for the scheduler to trigger, which is depending on the time defined in :kconfig:option:`CONFIG_SOC_FLASH_NRF_PARTIAL_ERASE_MS`.
When changing the :kconfig:option:`CONFIG_EMDS_FLASH_TIME_BASE_OVERHEAD_US` option, it is important that the worst time is considered.
When configuring these values using RRAM there is not a need to consider garbage collection in the same way as for flash, as it can be written to regardless of value. But the ERASEALL functionality
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When configuring these values using RRAM there is not a need to consider garbage collection in the same way as for flash, as it can be written to regardless of value. But the ERASEALL functionality
When configuring these values using RRAM, you do not need to consider garbage collection in the same way as for flash, as it can be written to regardless of the value.
However, avoid the ERASEALL functionality,

If partial erase is not enabled or supported, the time of a complete sector erase has to be included in the :kconfig:option:`CONFIG_EMDS_FLASH_TIME_BASE_OVERHEAD_US`.
When partial erase is enabled and supported by the hardware, include the time it takes for the scheduler to trigger, which is depending on the time defined in :kconfig:option:`CONFIG_SOC_FLASH_NRF_PARTIAL_ERASE_MS`.
When changing the :kconfig:option:`CONFIG_EMDS_FLASH_TIME_BASE_OVERHEAD_US` option, it is important that the worst time is considered.
When configuring these values using RRAM there is not a need to consider garbage collection in the same way as for flash, as it can be written to regardless of value. But the ERASEALL functionality
should be avoided as that can increase the time before the EMDS store functions are called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
should be avoided as that can increase the time before the EMDS store functions are called.
because that can increase the time before the EMDS store functions are called.

@Balaklaka Balaklaka force-pushed the develop/revise_rram_time_emds branch from a646365 to e216aee Compare December 18, 2024 09:34
@Balaklaka Balaklaka requested a review from peknis December 18, 2024 09:35
@peknis
Copy link
Contributor

peknis commented Dec 18, 2024

How about a changelog entry? Is this important enough to add one?

doc/nrf/libraries/others/emds.rst Outdated Show resolved Hide resolved
Updated estimated timing for rram storage using emds.
Also updated documentation and test to a more proper
indication of values for rram.

Signed-off-by: Ingar Kulbrandstad <[email protected]>
@Balaklaka Balaklaka force-pushed the develop/revise_rram_time_emds branch from e216aee to 070328f Compare December 18, 2024 13:24
@Balaklaka
Copy link
Contributor Author

How about a changelog entry? Is this important enough to add one?

This is not something new, so I think it is not important enough to add a changelog entry.

@Balaklaka Balaklaka removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 18, 2024
Copy link
Contributor

@alxelax alxelax left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor question.

@@ -68,8 +73,7 @@ config EMDS_FLASH_TIME_BASE_OVERHEAD_US
worst case scenario, before starting storing entries. This value
is dependent on the chip used, and should be checked against the chip
datasheet.
For RRAM-based flash drivers, the time is defined by the erasing block size.
The default erasing block size is 4096 bytes.
For RRAM-based persistent memory driver, use ERASEPROTECT and disregard the SOC_FLASH_NRF_PARTIAL_ERASE_MS parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

use ERASEPROTECT what does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... it is not clear why is ERASE PROTECT required for emds? Seems this is up to the application whether it uses or doesn't.

@nordicjm nordicjm merged commit cac49ac into nrfconnect:main Jan 6, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants