-
Notifications
You must be signed in to change notification settings - Fork 1.4k
boards: nordic: add support for TF-m to nrf7120 #24475
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?
boards: nordic: add support for TF-m to nrf7120 #24475
Conversation
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:trusted-firmware-m: PR head: 902f58b94409fa12a57e6b821592ea53d8dfef96 more detailstrusted-firmware-m:
sdk-nrf:
Github labels
List of changed files detected by CI (49)
Outputs:ToolchainVersion: f66cf421f3 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
You can find the documentation preview for this PR here. |
5a3c9d9
to
c224bbc
Compare
Memory footprint analysis revealed the following potential issuessample.matter.template.release[nrf5340dk/nrf5340/cpuapp]: ROM size increased by 2340[B] in comparison to the main[c434614] branch. - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-24475/12) |
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 check line lengths.
I've pointed out some, but not all.
c224bbc
to
297178e
Compare
297178e
to
ed054e1
Compare
soc/nordic/nrf71/CMakeLists.txt
Outdated
endif() | ||
|
||
# WZN-6148: port this to zephyr/modules/hal_nordic/nrfx/CMakeLists.txt in future | ||
zephyr_compile_definitions_ifdef(CONFIG_SOC_NRF7120_SKIP_CLOCK_CONFIG NRF_SKIP_CLOCK_CONFIGURATION) |
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.
we should not pollute compiler invocation line for all Zephyr sources with such configs.
Those should be provided to MDK either for only the sources which needs it or through a form of a single config header, like this: https://github.com/zephyrproject-rtos/zephyr/blob/main/modules/hal_nordic/nrfx/nrfx_kconfig.h
Also providing a Kconfig -> MDK defines this way is not scalable.
The Kconfig already have dependencies, but adding the translation in CMake adds extra level of dependencies (namely the inclusion tree of the CMake file itself), which thus results in constructs like this having to be duplicated.
A single config header, as linked, allows to:
- Have all Kconfig -> MDK defines at a single locations
- Avoids duplicated CMake constructs when different CMake code is included in different trees.
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.
This is what I see nrf54lx is doing it the same: https://github.com/nrfconnect/sdk-zephyr/blob/main/modules/hal_nordic/nrfx/CMakeLists.txt#L193 , and I think it is better to align for same workflow?
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.
@tejlmand What should I do with 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.
I think it is better to align for same workflow?
it's better to clean up the mess instead of building on top of it.
As your example show, then we now have 2 specific Kconfigs mapping to same MDK symbol, and two CMake codelines doing the same thing but in different places.
Not to mention that the trees including nrf71/CMakeLists.txt
and nrfx/CMakeLists.txt
may be included based on additional different (Kconfig) conditions.
The most correct thing is having a single Kconfig, which then has proper dependencies like this:
config NRF_SKIP_CLOCK_CONFIG
bool
prompt "Skip clock frequency configuration" if TRUSTED_EXECUTION_SECURE
depends on SOC_SERIES_NRF54LX || SOC_SERIES_NRF71X
default y if TRUSTED_EXECUTION_NONSECURE
and then in a Kconfig to MDK mapping header have (such as this https://github.com/zephyrproject-rtos/zephyr/blob/main/modules/hal_nordic/nrfx/nrfx_kconfig.h):
#ifdef CONFIG_NRF_SKIP_CLOCK_CONFIG
#define NRF_SKIP_CLOCK_CONFIGURATION
#endif
That will give you:
- A single Kconfig setting instead of 2
- Proper dependencies on the Kconfig which:
- becomes part of the documentation
- Can be easily seen in menuconfig
- Kconfig check support, such as dependency loop checking
- No line duplication in CMake, instead of two lines like:
there's a single location where all mapping between Kconfig and MDK can be found.
zephyr_compile_definitions_ifdef(CONFIG_SOC_NRF7120_SKIP_CLOCK_CONFIG NRF_SKIP_CLOCK_CONFIGURATION) zephyr_compile_definitions_ifdef(CONFIG_SOC_NRF54LX_SKIP_CLOCK_CONFIG NRF_SKIP_CLOCK_CONFIGURATION)
When looking at compile invocation and see gcc ..... -DNRF_SKIP_CLOCK_CONFIGURATION ...
then it's hard to know where this flag originates from and why, because this flag might be present multiple places in the CMake code, and which one was the line causing it to be set ?
But if the flag exist in a Kconfig to MDK mapping header as presented above, then it's much easier to see it's directly related to a Kconfig symbol, and I can check that Kconfig symbol for dependencies / selects.
No extra dependencies are added directly or indirectly, whereas the symbol mapping in CMake automatically adds indirect dependencies
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.
Have a look with this PR: https://github.com/zephyrproject-rtos/zephyr/pull/97474/files
ed054e1
to
72cbbac
Compare
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
ed8a898
to
68c2387
Compare
@nrfconnect/ncs-co-boards @nrfconnect/ncs-code-owners @nrfconnect/ncs-merge @ncs-co-drivers @ncs-aegir |
modules/trusted-firmware-m/tfm_boards/nrf7120_cpuapp/cpuarch.cmake
Outdated
Show resolved
Hide resolved
modules/trusted-firmware-m/tfm_boards/nrf7120_cpuapp/CMakeLists.txt
Outdated
Show resolved
Hide resolved
modules/trusted-firmware-m/tfm_boards/nrf7120_cpuapp/CMakeLists.txt
Outdated
Show resolved
Hide resolved
68c2387
to
984c971
Compare
modules/trusted-firmware-m/tfm_boards/nrf7120_cpuapp/CMakeLists.txt
Outdated
Show resolved
Hide resolved
984c971
to
c599a9b
Compare
I have triggered a more thorough TF-M CI run: https://jenkins-ncs.nordicsemi.no/job/latest/job/sub/job/test-fw-nrfconnect-tfm/job/master/15696/ |
looks good so I merged nrfconnect/sdk-trusted-firmware-m#210 |
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.
Approved with the expectation that # WZN-6148: port this to zephyr/modules/hal_nordic/nrfx/CMakeLists.txt in future
is addressed in a future PR
Add nrf7120 to tf-m Kconfig to support ns build Add tfm-boards for nrf7120_cpuapp to point to trusted-firmware-m build Update soc/nordic/nrf71 for tf-m build Signed-off-by: Travis Lam <[email protected]>
c599a9b
to
acdea85
Compare
acdea85
to
56e1dfe
Compare
Point sdk-trusted-firmware-m in west.yaml new tfm support for nrf7120. Signed-off-by: Travis Lam <[email protected]>
56e1dfe
to
7950319
Compare
select HAS_SEGGER_RTT if ZEPHYR_SEGGER_MODULE | ||
select SOC_EARLY_INIT_HOOK | ||
select SOC_RESET_HOOK | ||
select NRF_PLATFORM_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.
@tejlmand I have created this new Kconfig in zephyr, so that all Kconfigs between nrf71x and nrf54lx can share the common properties, also i.e. many Kconfig that has depend on (SOC_SERIES_NRF54LX || SOC_SERIES_NRF71X)
can change to depend on NRF_PLATFORM_LUMOS
Add nrf7120 to tf-m Kconfig to support ns build
Add tfm-boards for nrf7120_cpuapp to point to trusted-firmware-m build Update soc/nordic/nrf71 for tf-m build