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

stm32: Fix b_u585i_iot02a and regression script #106

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

erwango
Copy link
Member

@erwango erwango commented Jun 24, 2024

There are 2 issues with current TF-M preventing to use on STM32 targets:

  • Use of STSAFE should be disabled. This was fixed upstream with https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/29034/2. Note that this patch updates flash_layout which is only done for test purpose and required a flash partition update on Zephyr target which I prefer to avoid, so this particular fix is discarded
  • "Hard reset" option usage in regression script actually prevent to update option bytes. Fix in upstream is under preparation but we do need to fix it for V3.7.0

@ithinuel
Copy link
Collaborator

Does that mean that the Zephyr 3.7 LTS would need a non-LTS release (or even a "floating git hash") of TF-M?

@erwango
Copy link
Member Author

erwango commented Jun 24, 2024

Does that mean that the Zephyr 3.7 LTS would need a non-LTS release (or even a "floating git hash") of TF-M?

Good point. This should be fixed in TF-M LTS first. This should be done this week.
I'll close this PR and open an issue to track the point.

@erwango
Copy link
Member Author

erwango commented Jun 24, 2024

Closing. Issue will be tracked by zephyrproject-rtos/zephyr#74880

@erwango
Copy link
Member Author

erwango commented Jun 26, 2024

Re-opening based on comment from maintainer

@erwango erwango requested a review from d3zd3z June 26, 2024 15:08
@Vge0rge
Copy link
Collaborator

Vge0rge commented Jun 28, 2024

Please cherry pick the commit "stm : fix error on b_u585i_iot02a with TF-Mv2.1.0" so that it has a ChangeId (and ideally but not that importantly the commit hash) so that it is easier to manage these commits in the next upmerge.

@erwango
Copy link
Member Author

erwango commented Jun 28, 2024

Please cherry pick the commit "stm : fix error on b_u585i_iot02a with TF-Mv2.1.0" so that it has a ChangeId (and ideally but not that importantly the commit hash) so that it is easier to manage these commits in the next upmerge.

See comment in the description. I don't want to cherrypick the complete change since it introduces a flash_layout change which is only there for debug purpose (according to the developer) and would require an update of the flash partition of the board with less room available for f/w as a consequence.

@ithinuel
Copy link
Collaborator

I have seen other PR where the changes are cherry picked and then another commit "reverts" part of it explaining in the commit message the reason for the revert.
It makes tracking the divergence between upstream & this fork easier. (because the "revert commit" can be itself reverted before pulling more commits from upstream on the next bump of TF-M).
I don't have strong opinions on that, maybe @d3zd3z or another maintainer of the repo can give some guidance?

@Vge0rge
Copy link
Collaborator

Vge0rge commented Jun 28, 2024

I have seen other PR where the changes are cherry picked and then another commit "reverts" part of it explaining in the commit message the reason for the revert. It makes tracking the divergence between upstream & this fork easier. (because the "revert commit" can be itself reverted before pulling more commits from upstream on the next bump of TF-M). I don't have strong opinions on that, maybe @d3zd3z or another maintainer of the repo can give some guidance?

@erwango Sure, there are ways to handle your use case though. What @ithinuel makes a lot of sense since it helps tracking what's happening in a separate commit. But at the very least a mention in the commit/change id of the upstream change helps a lot the people who do upmerges in this repo.

@erwango
Copy link
Member Author

erwango commented Jun 28, 2024

I have seen other PR where the changes are cherry picked and then another commit "reverts" part of it explaining in the commit message the reason for the revert.

Ok, I'll proceed this way.

ahmadstm and others added 3 commits June 28, 2024 14:18
Align regressions and TFM_Update script with STM32CubeProgrammer 2.16


Change-Id: I91e9a61acd8881e58e475bcfef4f555456c6a1ef
Signed-off-by: Ahmad EL JOUAID <ahmad.eljouaid@st.com>
Deactivation of STSAFEA, which is used to deactivate the flag
MBEDTLS_PSA_CRYPTO_SE_C and the latter causes a problem of structure
alignment /*psa_key_attributes_s*/ in the crypto_struct.h file
between the 2 service protected storage(PS) and crypto

Change-Id: I4159a124eb4aa4173a03f166875265b6121ca575
Signed-off-by: Ahmad EL JOUAID <ahmad.eljouaid@st.com>
Let this flash_layout unchanged as the increase was just made for
debug purpose and then not really justified while it requires to
updade board's flash partition and reduce room for application f/w.

Signed-off-by: Erwan Gouriou <erwan.gouriou@st.com>
@erwango
Copy link
Member Author

erwango commented Jun 28, 2024

@ithinuel @Vge0rge I've done as suggested. Please let me know if this is fine now.

@erwango
Copy link
Member Author

erwango commented Jul 1, 2024

@Vge0rge PTAL

@Vge0rge
Copy link
Collaborator

Vge0rge commented Jul 2, 2024

Look good now @erwango :)

@Vge0rge
Copy link
Collaborator

Vge0rge commented Jul 2, 2024

By the way, I cannot add myself as reviewer but if someone adds me I can approve as well.

@erwango
Copy link
Member Author

erwango commented Jul 2, 2024

By the way, I cannot add myself as reviewer but if someone adds me I can approve as well.

I tried, but I didn't managed.

@ithinuel ithinuel merged commit f8d3a90 into zephyrproject-rtos:main Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants