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

Add TFM support for stm32l562e_dk board #31118

Merged
merged 5 commits into from
Mar 18, 2021

Conversation

yestin
Copy link
Contributor

@yestin yestin commented Jan 6, 2021

This PR is to add the TF-M support for the stm32l562e_dk board.

* - TFM_PSA_API=ON (IPC)
* - ISOLATION_LEVEL 2
* - TEST_S=ON (REGRESSION)
* - TEST_NS=ON (REGRESSION)
Copy link
Member

@erwango erwango Jan 6, 2021

Choose a reason for hiding this comment

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

After some more search, it seems that this is not the case anymore (#30210), at least this sample doesn't require TEST_NS to be enabled.

It might be worth checking potential impacts on partitions sizes.

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 see. And yes, the non secure image in this sample is pretty small, like 19KB. I can try to decrease the non secure image size, but may still not enough for the secondary partitions.

Copy link
Member

Choose a reason for hiding this comment

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

I was just looking at this PR, and indeed with 512 KB flash it will be a bit of a juggling-act to try to make this work with the dual-bank bootloader. It's possible, but more complex projects will be a challenge without using a different approach for the secure bootloader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the new patch, I leave the partition layout as is except updated the comments. As the secure image is almost 224 KB, it could not fit a secondary partition in 512 KB flash.

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

porting looks fine to me, and along the lines of the other tf-m capable platforms.
needs formal approval from @erwango

@galak
Copy link
Collaborator

galak commented Feb 16, 2021

@yestin do you mind rebasing this to address the merge conflict.

@yestin yestin force-pushed the stm32l562e-tfm-support branch 2 times, most recently from 0362171 to 687f0b5 Compare February 22, 2021 02:33
@ioannisg
Copy link
Member

@yestin may i kindly ask you to do one more rebase of this PR? I recently merged #32044 which centralizes the signing for TFM builds, so you do not need to duplicate this any more in the board definition. Please, take a look at the other TFM-capable boards, for guidance.

After this rebase, assuming I get a +1 from @microbuilder , I'll merge this PR.

@yestin
Copy link
Contributor Author

yestin commented Feb 24, 2021

@yestin may i kindly ask you to do one more rebase of this PR? I recently merged #32044 which centralizes the signing for TFM builds, so you do not need to duplicate this any more in the board definition. Please, take a look at the other TFM-capable boards, for guidance.

After this rebase, assuming I get a +1 from @microbuilder , I'll merge this PR.

Yes, I will look into rebasing on top of the new changes. I am running into issue that the signing generates .hex file instead .bin file with the new merge. The TFM_UPDATE.sh script calls STM32_Programmer_CLI to flash, and when the download file is hex file, it does not honor the downloading address that passed in.

The .hex file for NS image still links against the start address at 0x08000000, which is not a correct start address in secure state. For stm32l552xx and stm32l562xx, the flash memory space is aliased to 0x0C000000 in secure state (TZEN=1). I still need some time to debug this.

This commit allows to append an optional --hex-addr argument to
the wrapper script if speficied. This can adjust the base address
of the output hex file when signing the non-secure or secure
firmware images.

Signed-off-by: Yestin Sun <[email protected]>
This commit adds a second target for the stm32l562e_dk board.
The non secure target can be configured for TFM IPC application.

Signed-off-by: Yestin Sun <[email protected]>
After TZ is enabled, the GPIO peripherals are secured and are
not accessible from non secure world.
This commit prevents the GPIO pinctrl from the non secure target
for stm32l562e_dk board.

Signed-off-by: Yestin Sun <[email protected]>
This commit enables the TF-M IPC sample application on stm32l562e_dk
board.
It provides device tree flash partition as an overlay in order to
configure and flash the bl2, secure/non secure firmwares.

Signed-off-by: Yestin Sun <[email protected]>
This commit removes the signing commands from the post build steps,
in order to leverage the consolidated TFM signing code.

Also with the support to adjust the hex base address when signing,
there is no need to run the TFM_UPDATE.sh script. We can use west
flash to flash the merged hex file on the board.

Signed-off-by: Yestin Sun <[email protected]>
@yestin
Copy link
Contributor Author

yestin commented Feb 27, 2021

@ioannisg @microbuilder the rebased changes are ready for review. I add an option to the signing script to adjust the hex base address. This can allow the output hex file to be flashed to a different address. In the case for nucleo_l552ze_q and stm32l562e_dk boards, they have a different flash memory space (0x0C000000) when TZ is enabled.

I also update the nucleo_l552ze_q board to leverage the centralized signing code. cc: @erwango

@ioannisg ioannisg added the DNM This PR should not be merged (Do Not Merge) label Mar 5, 2021
@yestin
Copy link
Contributor Author

yestin commented Mar 18, 2021

@ioannisg do you get a chance to review the PR? It has been pending for some time and I am worried I would have to rebase it again. Please let me know if you have concerns.

@ioannisg
Copy link
Member

@ioannisg do you get a chance to review the PR? It has been pending for some time and I am worried I would have to rebase it again. Please let me know if you have concerns.

@yestin apologies for forgetting this PR, for some time. To clarify, the reason for adding DNM flag is that i was not able to run the tfm_ipc test properly, on the STM32L562 board, using the PR, so needed to investigate this more.

@yestin are you able to successfully compile, flash and run the tfm_ipc sample on this board and on nucleo_l552ze_q?

using the ``nucleo_l552ze_q_ns`` target. When building a ``*_ns`` image with TF-M,
a ``build/tfm/install/postbuild.sh`` bash script will be run as a post-build step
to make some required flash layout changes. The ``build/tfm/regression.sh`` script
will need to be run to perform device initialization, and then run ``west flash --hex-file build/tfm_merged.hex``
Copy link
Member

Choose a reason for hiding this comment

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

Can you review the command here?
I had to go in samples/tfm_integration/tfm_ipc/build and then launch west flash --hex-file tfm_merged.hex

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this needs an improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the build output is generated to the sample project directory instead of the root directory. I did a clean run and can see the merged hex file is generated at build/tfm_merged.hex. Do you have any local environments that may impact?

Copy link
Member

Choose a reason for hiding this comment

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

@yestin the file is present here indeed, though, when running the command, I'm getting the following west error:

$ west flash --hex-file build/stm32l562e_dk_ns/tfm_merged.hex
FATAL ERROR: --build-dir was not given, /local/mcu/zephyrproject/zephyr/samples/tfm_integration/tfm_ipc is not a build directory and the default build directory cannot be determined. Check your build.dir-fmt configuration option

@ioannisg is that what you're seeing as well ?

Copy link
Member

Choose a reason for hiding this comment

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

@yestin I confirm this is only due to my local build.dir-fmt configuration.
I've raised #33498 to discuss it

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Approved, I'll review the doc part once merged.

@ioannisg ioannisg removed the DNM This PR should not be merged (Do Not Merge) label Mar 18, 2021
CONFIG_UART_CONSOLE=y

# Enable MPU
CONFIG_ARM_MPU=y
Copy link
Member

Choose a reason for hiding this comment

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

We should probably enable HW_STACK_PROTECTION=y here as well. Or is there a problem with this @yestin @erwango ?

@ioannisg ioannisg merged commit 93ff119 into zephyrproject-rtos:master Mar 18, 2021
@yestin
Copy link
Contributor Author

yestin commented Mar 19, 2021

@ioannisg do you get a chance to review the PR? It has been pending for some time and I am worried I would have to rebase it again. Please let me know if you have concerns.

@yestin apologies for forgetting this PR, for some time. To clarify, the reason for adding DNM flag is that i was not able to run the tfm_ipc test properly, on the STM32L562 board, using the PR, so needed to investigate this more.

@yestin are you able to successfully compile, flash and run the tfm_ipc sample on this board and on nucleo_l552ze_q?

Yes, I verify that I can run the tfm_ipc sample on both boards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants