-
Notifications
You must be signed in to change notification settings - Fork 1.4k
samples: crypto: add support for nrf7120 samples #25357
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
base: main
Are you sure you want to change the base?
samples: crypto: add support for nrf7120 samples #25357
Conversation
| .. table-from-rows:: /includes/sample_board_rows.txt | ||
| :header: heading | ||
| :rows: nrf54l15dk_nrf54l05_cpuapp, nrf54l15dk_nrf54l10_cpuapp, nrf54l15dk_nrf54l15_cpuapp, nrf54l15dk_nrf54l15_cpuapp_ns, nrf54lm20dk_nrf54lm20a_cpuapp, nrf54lv10dk_nrf54lv10a_cpuapp | ||
| :rows: nrf54l15dk_nrf54l05_cpuapp, nrf54l15dk_nrf54l10_cpuapp, nrf54l15dk_nrf54l15_cpuapp, nrf54l15dk_nrf54l15_cpuapp_ns, nrf54lm20dk_nrf54lm20a_cpuapp, nrf54lv10dk_nrf54lv10a_cpuapp, nrf7120pdk_nrf7120p_cpuapp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the naming of the sample was correct for two weeks at least. That is something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't get what that means. Do you mean this sample name is kmu_sage_nrf54l but included nrf7120 should be some new name or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Please rename the sample and any reference to it being 54L only. cc @AntonZma
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: f532d63e92ca85ca9f7b3dc3926cb9060be53a75 more detailssdk-nrf:
Github labels
List of changed files detected by CI (38)Outputs:ToolchainVersion: cfa6b06338 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
435b87b to
6f04b65
Compare
|
You can find the documentation preview for this PR here. Preview links for modified nRF Connect SDK documents: https://ncsdoc.z6.web.core.windows.net/PR-25357/nrf/samples/crypto/kmu_usage_nrf54l/README.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release notes entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add a changelog entry for the sample mentioning added support for a new device.
samples/crypto/persistent_key_usage/boards/nrf7120pdk_nrf7120_cpuapp.conf
Outdated
Show resolved
Hide resolved
| :rows: nrf54l15dk_nrf54l05_cpuapp, nrf54l15dk_nrf54l10_cpuapp, nrf54l15dk_nrf54l15_cpuapp, nrf54l15dk_nrf54l15_cpuapp_ns, nrf54lm20dk_nrf54lm20a_cpuapp, nrf54lv10dk_nrf54lv10a_cpuapp, nrf7120pdk_nrf7120_cpuapp | ||
|
|
||
| .. include:: /includes/tfm.txt | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to edits to this README, you also need to update the following docs in time for the launch of nRF7120:
- https://github.com/nrfconnect/sdk-nrf/blob/main/doc/nrf/app_dev/device_guides/nrf54l/cryptography.rst with information about 7120; the page needs to be moved out of nRF54L section as part of this task
- https://github.com/nrfconnect/sdk-nrf/blob/main/doc/nrf/app_dev/device_guides/nrf54l/kmu_provision.rst with information about 7120; the page needs to be moved out of nRF54L section as part of this task
Can you please sync with your team's tech writer and create a task for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WZN-7049 has been created for this.
Add support for nrf7120pdk_nrf7120_cpuapp to all samples support nrf7120 Signed-off-by: Travis Lam <travis.lam@nordicsemi.no>
6f04b65 to
f532d63
Compare
| - nrf54l15dk/nrf54l10/cpuapp | ||
| - nrf54l15dk/nrf54l10/cpuapp/ns | ||
| - nrf54l15dk/nrf54l05/cpuapp | ||
| - nrf7120pdk/nrf7120/cpuapp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name in line 6 of this file needs to be updated, given your added support for nRF7120.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need to update the name of the folder? to kmu_usage remove nrf54l only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't use kmu_usage, as 5340 is not supported. You will need a generic name for 54l + 71
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
54l + 71 == 125l
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jokes apart, if we need a name to distinguish from 5340 let's maybe go with lumos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is what we use for the upstream Kconfig so it would make sense. I also think kmu_usage_everything_except_5340 rolls off the tongue well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or kmu_usage_for_the_newer_kmu_peripheral?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kmu_usage_^nrf(54|71)[a-z]*\d+[a-z]*$ simple and easy to understand
| nrfutil device erase --all | ||
| #. :ref:`Program <programming>` the sample to your nRF54L Series device. | ||
| #. :ref:`Program <programming>` the sample to your nRF54L Series and nRF7120 device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #. :ref:`Program <programming>` the sample to your nRF54L Series and nRF7120 device. | |
| #. :ref:`Program <programming>` the sample to your nRF54L Series or nRF7120 device. |
Sorry, my mistake from first suggestion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need to specify the device here? Can't we just say "to your device"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you also planning to add 7120 NS?
| :depth: 2 | ||
|
|
||
| The KMU usage sample demonstrates how to generate cryptographic keys for nRF54L Series devices and securely store them in the Key Management Unit (KMU). | ||
| The KMU usage sample demonstrates how to generate cryptographic keys for nRF54L Series devices and nRF7120 and securely store them in the Key Management Unit (KMU). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phrasing seems a bit awkward, maybe rework the structure?
| The KMU usage sample demonstrates how to generate cryptographic keys for nRF54L Series devices and nRF7120 and securely store them in the Key Management Unit (KMU). | |
| The KMU usage sample demonstrates how to generate cryptographic keys and securely store them in the Key Management Unit (KMU) on nRF54L Series and nRF7120 devices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7120 NS maybe in the next PR, to reduce the complexity of this PR?
| nrfutil device erase --all | ||
| #. :ref:`Program <programming>` the sample to your nRF54L Series device. | ||
| #. :ref:`Program <programming>` the sample to your nRF54L Series and nRF7120 device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need to specify the device here? Can't we just say "to your device"?
| - ".*Example finished successfully!.*" | ||
|
|
||
| tests: | ||
| sample.kmu_usage_nrf54l: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too needs renaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls ignore my comments :)
| CONFIG_NRF_GRTC_START_SYSCOUNTER=y | ||
|
|
||
| # Temporarily disable whilst resolving issue with CI | ||
| CONFIG_CRACEN_LIB_KMU=n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wait for https://jenkins-ncs.nordicsemi.no/job/latest/job/test-zephyr-sanitycheck-self-verification/view/change-requests/job/PR-2990/ to be merged before making this change
| :rows: nrf54l15dk_nrf54l05_cpuapp, nrf54l15dk_nrf54l10_cpuapp, nrf54l15dk_nrf54l15_cpuapp, nrf54l15dk_nrf54l15_cpuapp_ns, nrf54lm20dk_nrf54lm20a_cpuapp, nrf54lv10dk_nrf54lv10a_cpuapp, nrf7120pdk_nrf7120_cpuapp | ||
|
|
||
| .. include:: /includes/tfm.txt | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WZN-7049 has been created for this.
Add support for nrf7120pdk_nrf7120_cpuapp to all samples support nrf7120