-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
nrf54l15dk: add TF-M (/ns) support #17463
base: main
Are you sure you want to change the base?
nrf54l15dk: add TF-M (/ns) support #17463
Conversation
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:trusted-firmware-m: PR head: 8d75b83bf84f86feeb3bf02f4681c5d8400d9058 more detailstrusted-firmware-m:
sdk-nrf:
matter:
zephyr:
Github labels
List of changed files detected by CI (97)
Outputs:ToolchainVersion: 9583beca34 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
12c5853
to
a134757
Compare
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 Publishing GitHub Action. |
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.
Approving the Fast Pair tests
a134757
to
e3eff22
Compare
@@ -39,6 +39,7 @@ IDE, and tool support | |||
Board support | |||
============= | |||
|
|||
* Added TF-M support to the :ref:`nRF54L15 DK <zephyr:nrf54l15dk_nrf54l15>` (board target ``nrf54l15dk/nrf54l15/cpuapp/ns``), replacing the nRF54L15 PDK (board target ``nrf54l15pdk/nrf54l15/cpuapp/ns``). |
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.
Not sure if this is the correct place... Would it make more sense to have it under Security section?
Replacing the Moonlight PDK with DK on samples might be a good idea to mention as well, under the respective samples sections.
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.
So it would instead be listed on every affected sample category? (for now TF-M, crypto and Matter)
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.
That was the thought.
By the way, it looks like this is already done for Matter: https://github.com/nrfconnect/sdk-nrf/pull/17298/files (see release-notes-changelog.rst)
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.
Did that.
e3eff22
to
a1fda80
Compare
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.
Adding @annwoj
| | | | | | ||
| | | | ``nrf54l15pdk/nrf54l15/cpuapp/ns`` | | ||
+-------------------+------------+-------------------------------------------------------------------+---------------------------------------+ |
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.
Shouldn't PDK be removed at this point?
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're right! At least for the NS target of the PDK. (The whole PDK will be removed separately.)
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.
Just an FYI - If there still will be some occurrences left in common articles for pdk targets I'll be doing a cleanup closer to the release date
a1fda80
to
2f455d8
Compare
@@ -207,7 +207,7 @@ file(COPY ${CMAKE_CURRENT_LIST_DIR}/common | |||
DESTINATION ${INSTALL_PLATFORM_NS_DIR}) | |||
|
|||
|
|||
if((NRF_SOC_VARIANT STREQUAL nrf54l15) OR (target STREQUAL nrf54l15)) |
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.
Why are you not using CONFIG_SOC_NRF54L15
?
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.
Good question. I was not the one to introduce this. Here I just cleaned up the use of target
that seemed useless. I guess NRF_SOC_VARIANT
was used because it's defined in the TF-M CMake logic. There is likely no good reason for CONFIG_SOC_*
to not be used instead.
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.
Okay so actually there is. CONFIG_SOC_NRF54L15
is not defined in there, so it doesn't work. I'm reverting to using NRF_SOC_VARIANT
.
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.
If introducing a common app.overlay
then such a file should be generic enough to generally be able to cover majority of boards.
Having the existing (duplicated) nrf5340dk_nrf5340_cpuapp_ns.overlay
and `nrf9160dk_nrf9160_ns.overlay makes it clear for which boards those overlays are intended.
The nRF54l15 requires a different overlay, and probably also the nRF54h20 (btw why is the no nrf54h20 overlay ?)
Dedicated overlays makes it clearer to downstream users creating custom boards that they need a similar overlay for their custom board compared to a generic app.overlay
which doesn't indicates for which boards it is intended.
afaict the app.overlay
doesn't appear to be so common, so I would like a good reason why this change is valuable, especially seen from our user's perspective.
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.
The reason why this change was just to reduce duplication. You have a fair point, I reverted this and added some DT overlay files that were missing in tfm_regression_test
.
(btw why is the no nrf54h20 overlay ?)
Apparently because the 54H20 is not supported. I don't know exactly.
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
Bring in changes needed in Zephyr and TF-M, and make needed adjustments. Support for TF-M in `nrf54l15pdk` is removed at the same time. Signed-off-by: Tomi Fontanilles <[email protected]>
Put common stuff in the `common` section. Also add the `nrf54l15dk/nrf54l15/cpuapp/ns` platform to the `tfm.psa_test_crypto_lvl2` test. Signed-off-by: Tomi Fontanilles <[email protected]>
Return an error instead of triggering an assert when `output` is NULL. This is to prevent PSA arch tests from hanging because of this. Signed-off-by: Tomi Fontanilles <[email protected]>
Remove the remaining `app.overlay` as it doesn't fit all the boards. In addition, make sure that there are DT overlay files for every supported board. Signed-off-by: Tomi Fontanilles <[email protected]>
2f455d8
to
92d20b5
Compare
Addressed the latest comments and rebased. |
Bring in changes needed in Zephyr and TF-M, and make needed adjustments.
Support for TF-M in
nrf54l15pdk
is removed at the same time.test_crypto: PR-667
test_tfm: PR-175