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

stm32u5 hal dma_ex function not inlined with GCC 11 or 12 #115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FRASTM
Copy link

@FRASTM FRASTM commented Nov 7, 2024

Fix the Issue linked to compilation of file HAL_DMAEx_List_ReplaceNode_Head.c
using a GCC version comprised between v11.x and v12.x.

and blocking the zephyr CI

Issue is seen only with TF-M build config RelWithDebInfo (With MinSizeRel config, this file is not built).
Issue could be fixed by "adding attribute ((no_inline)) to DMA_List_CheckNodesBaseAddresses allows the build to pass"

Zephyr issue: zephyrproject-rtos/zephyr#80932

@FRASTM FRASTM closed this Nov 7, 2024
@FRASTM FRASTM deleted the noinline branch November 7, 2024 15:16
@FRASTM FRASTM restored the noinline branch November 7, 2024 15:17
@FRASTM FRASTM deleted the noinline branch November 7, 2024 15:19
@FRASTM FRASTM restored the noinline branch November 7, 2024 16:52
@FRASTM FRASTM reopened this Nov 7, 2024
Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

  • Did you submit this fix upstream? I could not find anything with the Change-Id mentioned.
  • When this is done, make sure to cherry pick the upstream commit into the Zephyr fork by using the -x option of git cherry-pick to keep track of the cherry-picked commit.
  • This PR is not quite solving the issue in Zephyr by itself. The issue will be solved in Zephyr once this PR is merged and that commit is integrated into Zephyr. For that you will need to create a manifest PR in the Zephyr repository (see PRs with the manifest-trusted-firmware-m label for examples), and there you can have the Fixes X.

@FRASTM
Copy link
Author

FRASTM commented Nov 12, 2024

  • Did you submit this fix upstream? I could not find anything with the Change-Id mentioned.
  • When this is done, make sure to cherry pick the upstream commit into the Zephyr fork by using the -x option of git cherry-pick to keep track of the cherry-picked commit.
  • This PR is not quite solving the issue in Zephyr by itself. The issue will be solved in Zephyr once this PR is merged and that commit is integrated into Zephyr. For that you will need to create a manifest PR in the Zephyr repository (see PRs with the manifest-trusted-firmware-m label for examples), and there you can have the Fixes X.

@tomi-font
fix upstream is https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/33420

Even If I push this change on ~/zephyrproject/modules/tee/tf-m/trusted-firmware-m
the upstream main branch is on 8134106
and what I see in the west.yaml is a11cd27

Where Should my change be ? on top of remotes/upstream/main or on a11cd27 or ?

@tomi-font
Copy link
Collaborator

First, where in Zephyr would you like this fix to be included? v3.7-branch or main? (Or both?)

And I'm not sure I get what you're saying, but it's normal that the end commit hashes differ even when cherry picking the commit (because the two branches are not identical.)

Anyway, we'll want to wait for your patch to be approved upstream before merging it here.

@erwango
Copy link
Member

erwango commented Nov 22, 2024

Anyway, we'll want to wait for your patch to be approved upstream before merging it here.

@tomi-font TF-M upstream team won't integrate the change until PR is merged and delivered in official HAL package (they don't merge HAL fixes). See https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/33420.
I don't think we can afford to wait that long for current PR. Can we consider merging it faster ?

@tomi-font
Copy link
Collaborator

Can't you somehow speed up the process on the HAL side?
(after re-having a look at the upstream PR) I just saw the comment in the review which states 3-4 months. Sounds a bit crazy.

Anyway, sure we can integrate that faster into Zephyr. But can you rework the commit to cherry pick the one you submitted upstream with the -x option (so we have Change-Id and cherry picked from lines)?

@erwango
Copy link
Member

erwango commented Nov 22, 2024

Can't you somehow speed up the process on the HAL side?

That's not only about merging this fix. It's about integrating it in a newSTM32Cube package version, perform validation and deliver. Then this Cube package should be integrated into TF-M, validated, then upstreamed. That's a long process.
Then of course this is to be done by teams that have their own priorities which don't fit exactly ours, so you should put additional delay on top of that to get some of their free slots.

Anyway, sure we can integrate that faster into Zephyr.

Ok, great. Thanks

But can you rework the commit to cherry pick the one you submitted upstream with the -x option (so we have Change-Id and cherry picked from lines)?

Sure, we'll do.

@FRASTM
Copy link
Author

FRASTM commented Dec 2, 2024

commit msg now mentions the reference for the cherry picked

@FRASTM FRASTM requested a review from tomi-font December 2, 2024 14:41
Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

Please make the changes identical between this PR and your upstream one.
Also, you will need a PR in Zephyr to update the reference to this repo in order to pull those changes in. While this PR is not merged, the revision should be pull/115/head.

FRASTM added a commit to FRASTM/zephyr that referenced this pull request Dec 4, 2024
Takes the PR
zephyrproject-rtos/trusted-firmware-m#115
to fix the build error on stm32u5 due to gcc revision

Signed-off-by: Francois Ramu <francois.ramu@st.com>
@FRASTM
Copy link
Author

FRASTM commented Dec 4, 2024

Please make the changes identical between this PR and your upstream one. Also, you will need a PR in Zephyr to update the reference to this repo in order to pull those changes in. While this PR is not merged, the revision should be pull/115/head.

done in zephyrproject-rtos/zephyr#82534

FRASTM added a commit to FRASTM/zephyr that referenced this pull request Dec 4, 2024
Takes the PR
zephyrproject-rtos/trusted-firmware-m#115
to fix the build error on stm32u5 due to gcc revision

Signed-off-by: Francois Ramu <francois.ramu@st.com>
FRASTM added a commit to FRASTM/zephyr that referenced this pull request Dec 9, 2024
Takes the PR
zephyrproject-rtos/trusted-firmware-m#115
to fix the build error on stm32u5 due to gcc revision

Signed-off-by: Francois Ramu <francois.ramu@st.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants