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

Jump to bottom

suit: Rework SDFW and SDFW Recovery sinks #18514

Merged
merged 1 commit into from
Nov 7, 2024
Merged

suit: Rework SDFW and SDFW Recovery sinks #18514

merged 1 commit into from
Nov 7, 2024

Conversation

adsz-nordic
Copy link
Contributor

@adsz-nordic adsz-nordic commented Nov 4, 2024 •
edited
Loading

New implementation of SDFW and SDFW Recovery sinks. Supports fixed installation order (SDFW, then SDFW Recovery). Treats SDFW Recovery installation as optional.
Envelope is saved when SDFW slot is successfully installed.

Changes from https://github.com/nrfconnect/sdk-secdom/pull/939 are also needed to make this PR usable.

Note: Similar changes were already introduced in #17837 but were reverted during secdom release process. This PR additionally fixes an issue with reboot loop in scenario of both SDFW and SDFW Recovery update failures.

Ref: NCSDK-29561

@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 Nov 4, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Nov 4, 2024 •
edited
Loading

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: 80e258746b0234964fac28a2806cc6ec15966081

more details

sdk-nrf:

PR head: 80e258746b0234964fac28a2806cc6ec15966081
merge base: 540f139344b7ea46f82bd9c59ff740dc6cde556c
target head (main): 540f139344b7ea46f82bd9c59ff740dc6cde556c
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 (7) 
subsys
│  ├── suit
│  │  ├── plat_err
│  │  │  ├── include
│  │  │  │  │ suit_plat_err.h
│  │  ├── platform
│  │  │  ├── sdfw
│  │  │  │  ├── src
│  │  │  │  │  │ suit_plat_copy.c
│  │  │  ├── src
│  │  │  │  │ suit_plat_error_convert.c
│  │  ├── stream
│  │  │  ├── stream_sinks
│  │  │  │  ├── src
│  │  │  │  │  ├── suit_sdfw_recovery_sink.c
│  │  │  │  │  │ suit_sdfw_sink.c
│  │  ├── utils
│  │  │  ├── include
│  │  │  │  │ suit_plat_mem_util.h
│  │  │  ├── src
│  │  │  │  │ suit_plat_mem_util.c

Outputs:

Toolchain

Version: b81a7cd864
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b81a7cd864_912848a074

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

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 119
  • ✅ Integration tests
    • ✅ test-sdk-dfu
    • ⚠️ test-sdk-dfu
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_cloud
    • 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_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-proprietary_esb
    • 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-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi

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

@adsz-nordic adsz-nordic marked this pull request as ready for review November 4, 2024 14:53
@adsz-nordic adsz-nordic requested a review from a team as a code owner November 4, 2024 14:53
@adsz-nordic adsz-nordic force-pushed the ncsdk-29561 branch 2 times, most recently from a454115 to 2a07859 Compare November 6, 2024 06:46
SylwesterKonczyk
SylwesterKonczyk reviewed Nov 6, 2024
View reviewed changes
subsys/suit/utils/src/suit_plat_mem_util.c
if (NRF_SICR->UROT.UPDATE.OPERATION != SICR_UROT_UPDATE_OPERATION_OPCODE_Nop ||
NRF_SICR->UROT.UPDATE.STATUS != SICR_UROT_UPDATE_STATUS_CODE_None) {

const nrf_mramc_mode_write_t new_write_mode = NRF_MRAMC_MODE_WRITE_DIRECT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to change MRAM write mode here? Shouldn't direct mode be enabled during SDFW startup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During development I added some logs to show current write mode and as you say, it was already set as you describe.
However, this snippet is based on sdfw_update.c code, where such write mode is being done before changing the register values, so I assumed it is better to ensure it.
Additionally, the mode change is only done when desired mode is different than current one, so I guess it shouldn't be harmful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SylwesterKonczyk If you know for sure that the write mode is always going to be appropriate, I can remove the code that checks and adjusts it. I preferred to be cautious here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Piece of code you are proposing works as intended. At the same time - improper handling of MRAM mode may lead to data incoherency, so it shall be managed in SDFW in a coherent way, and local 'hacks' should be, in my opinion, avoided.

Let me approve it to avoid further blocking of that PR, but please setup a separate thread to deal with MRAM mode in a proper way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, if we can avoid additional checks and mode setting, we should remove this mechanism.
I've created a Jira task to follow it up: https://nordicsemi.atlassian.net/browse/NCSDK-30143.

@adsz-nordic
suit: Rework SDFW and SDFW Recovery sinks
80e2587 
New implementation of SDFW and SDFW Recovery sinks.
Supports fixed installation order (SDFW, then SDFW Recovery).
Treats SDFW Recovery installation as optional.
Envelope is saved when SDFW slot is successfully installed.

Ref: NCSDK-29561

Signed-off-by: Adam Szczygieł <adam.szczygiel@nordicsemi.no>
@rlubos rlubos merged commit c6a3f97 into nrfconnect:main Nov 7, 2024
12 checks passed
@adsz-nordic adsz-nordic deleted the ncsdk-29561 branch November 8, 2024 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomchy tomchy tomchy approved these changes

@ahasztag ahasztag ahasztag approved these changes

@SylwesterKonczyk SylwesterKonczyk SylwesterKonczyk approved these changes

@robertstypa robertstypa Awaiting requested review from robertstypa

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@adsz-nordic @NordicBuilder @tomchy @SylwesterKonczyk @ahasztag @rlubos