SCMI-Server: Add support for multiple source files in build #6190

nicola-mazzucato-arm · 2023-07-24T10:06:46Z

This PR adds support to include c files, other than the "mod_" matching name from the SCP-firmware source tree to build the SCMI Server.
This is required to properly integrate the following change in SCP-firmware:
ARM-software/SCP-firmware#812

nicola-mazzucato-arm · 2023-07-27T11:10:08Z

@jforissier can you please provide some feedback on this when you have time? Thanks in advance.

jforissier · 2023-07-27T11:12:08Z

@nicola-mazzucato-arm yes I will, but I would also like @etienne-lms to check this when he's back from vacations 😉 Thanks!

nicola-mazzucato-arm · 2023-07-28T09:24:28Z

@jforissier Thanks. Can you please help me figuring out where the failures happened?
The multi-platform build seems to fail on stm32, though it builds just fine locally. Am I missing something?
The other two failures, I am not quite sure where to look at..
Thanks

jforissier · 2023-07-28T09:44:25Z

@jforissier Thanks. Can you please help me figuring out where the failures happened? The multi-platform build seems to fail on stm32, though it builds just fine locally. Am I missing something?

The CI jobs pulls the SCMI code from the master branch of https://github.com/ARM-software/SCP-firmware.git (see https://github.com/OP-TEE/optee_os/actions/runs/5643523525/job/15392702318?pr=6190#step:5:193), which indeed doesn't have the file module/scmi_perf/src/scmi_perf_protocol_ops.c.

The other two failures, I am not quite sure where to look at.. Thanks

They are unrelated to this PR. They are about insufficient disk space, and I have submitted a fix (OP-TEE/build#671). Sorry about that :-/

nicola-mazzucato-arm · 2023-07-28T09:52:41Z

The CI jobs pulls the SCMI code from the master branch of https://github.com/ARM-software/SCP-firmware.git (see https://github.com/OP-TEE/optee_os/actions/runs/5643523525/job/15392702318?pr=6190#step:5:193), which indeed doesn't have the file module/scmi_perf/src/scmi_perf_protocol_ops.c.

Right! That seems a bit to create a circular dependency :)
Do you have a way to manually kick off the above tests by fetching the SCP-firmware source from the PR [https://github.com/ARM-software/SCP-firmware/pull/812] to validate the builds? If so, then someone may need to force the merge of this change, but that's up to the maintainers I guess.

They are unrelated to this PR. They are about insufficient disk space, and I have submitted a fix (OP-TEE/build#671). Sorry about that :-/

Thanks! That's a relief :)

jforissier · 2023-07-28T16:32:24Z

Do you have a way to manually kick off the above tests by fetching the SCP-firmware source from the PR [https://github.com/ARM-software/SCP-firmware/pull/812] to validate the builds?

You can do it by adding a commit that changes the CI file (.github/workflows/ci.yml). Replace function download_scp_firmware() { ... } with the following:

function download_scp_firmware() { git clone --single-branch https://github.com/ARM-software/SCP-firmware.git $HOME/scp-firmware && { cd $HOME/scp-firmware && git fetch origin pull/812/head && git checkout FETCH_HEAD; } || echo Nevermind; }

Call this commit "[TESTING] Update CI to test with SCP firmware PR 812" for instance.

jforissier · 2023-08-01T10:05:17Z

@nicola-mazzucato-arm I'm sorry the line should be:

function download_scp_firmware() { git clone --single-branch https://github.com/ARM-software/SCP-firmware.git $HOME/scp-firmware && ( cd $HOME/scp-firmware && git fetch origin pull/812/head && git checkout FETCH_HEAD; ) || echo Nevermind; }

...that is, with parentheses ( ) instead of curly braces { }, so that a sub-shell is forked and the cd command only has local scope.

nicola-mazzucato-arm · 2023-08-01T10:09:58Z

@nicola-mazzucato-arm I'm sorry the line should be:
function download_scp_firmware() { git clone --single-branch https://github.com/ARM-software/SCP-firmware.git $HOME/scp-firmware && ( cd $HOME/scp-firmware && git fetch origin pull/812/head && git checkout FETCH_HEAD; ) || echo Nevermind; }
...that is, with parentheses ( ) instead of curly braces { }, so that a sub-shell is forked and the cd command only has local scope.

Thanks @jforissier , I was just adding the cd ... I'll try your suggestion anyway :)

etienne-lms

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"core: scmi-server: Add support for conditional options for SCMI-Perf" and
"optee-fvp: Enable CFG_SCPFW_SCMI_PERF_PROTOCOL_OPS" with comment addressed or not.

etienne-lms · 2023-08-03T13:15:38Z

core/lib/scmi-server/sub.mk

+srcs-$(CFG_SCPFW_MOD_SCMI_PERF) += $(scpfw-path)/$3/$2/src/scmi_perf_protocol_ops.c
+endif
+
+ifneq (,$(filter y, $(CFG_SCPFW_SCMI_PERF_FAST_CHANNELS)))


Do you foresee added CFG_ dependencies? Unless what, ifneq ($(CFG_SCPFW_SCMI_PERF_xxx),y) is simpler.
Ditto at line 159.

Not really.. I will change this and push an updated PR. Thanks!

nicola-mazzucato-arm · 2023-08-04T11:14:13Z

FYI @jforissier @etienne-lms I have updated the PR. Thanks

nicola-mazzucato-arm · 2023-08-07T09:23:20Z

@jforissier can you kindly kick-off all the tests? Thanks in advance :)

jforissier · 2023-08-07T13:15:34Z

@jforissier can you kindly kick-off all the tests? Thanks in advance :)

Done.

Can we expect the SCP-firmware to be merged before merging this? I do not want to merge something that breaks CI for everyone.

nicola-mazzucato-arm · 2023-08-07T14:10:12Z

@jforissier can you kindly kick-off all the tests? Thanks in advance :)

Done.

Can we expect the SCP-firmware to be merged before merging this? I do not want to merge something that breaks CI for everyone.

Thanks! I was hoping the other way round :). The moment you merge this PR I will merge PR812 in SCP. That should align the two repos and CI should be happy in either sides.

jforissier · 2023-08-07T14:53:04Z

Then the SCP build has to be disabled first in CI. Add a commit (first commit in the series) that explains that the build is going to be broken by subsequent commits and will require 812 to be merged in SCP-firmware and therefore the build is disabled temporarily. Then when 812 is merged you can simply revert that commit.

nicola-mazzucato-arm · 2023-08-08T09:23:55Z

Then the SCP build has to be disabled first in CI. Add a commit (first commit in the series) that explains that the build is going to be broken by subsequent commits and will require 812 to be merged in SCP-firmware and therefore the build is disabled temporarily. Then when 812 is merged you can simply revert that commit.

Thanks @jforissier for the suggestion. PR has been resubmitted with the scmi-server temporarily disabled.

jforissier · 2023-08-08T12:59:45Z

One typo: s/rever/revert
I suspect checkpactch will complain on the long line with the URL, please use this format instead:

Link: https://some-url/ [1]
Signed-off-by: ...

(No need for a blank line between the two)
One last thing, it seems easier to just not download the source code (make the download function do nothing) instead of changing 3 lines to =n, but I am OK with your approach too.
For this commit:

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

nicola-mazzucato-arm · 2023-08-08T13:54:17Z

One typo: s/rever/revert I suspect checkpactch will complain on the long line with the URL, please use this format instead:
Link: https://some-url/ [1]
Signed-off-by: ...
(No need for a blank line between the two) One last thing, it seems easier to just not download the source code (make the download function do nothing) instead of changing 3 lines to =n, but I am OK with your approach too. For this commit:

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Thank you @jforissier

For this new force-push:

fixed typo in commit msg
emptied out download_scp_firmware() instead of setting CFG_SCMI_SCPFW to n as suggested
Thanks!

nicola-mazzucato-arm · 2023-08-09T08:22:16Z

@jforissier any clue why the multi-platform failed? Thanks

jforissier · 2023-08-09T11:34:27Z

.github/workflows/ci.yml

@@ -81,7 +81,7 @@ jobs:

          function _make() { make -j$(nproc) -s O=out $*; }
          function download_plug_and_trust() { mkdir -p $HOME/se050 && git clone --single-branch -b v0.4.2 https://github.com/foundriesio/plug-and-trust $HOME/se050/plug-and-trust || (rm -rf $HOME/se050 ; echo Nervermind); }
-          function download_scp_firmware() { git clone --single-branch https://github.com/ARM-software/SCP-firmware.git $HOME/scp-firmware || echo Nervermind; }
+          function download_scp_firmware() { #Temporarily disabled }


You cannot use a comment here, it comments out the closing brace. Add it on the previous line.

You cannot use a comment here, it comments out the closing brace. Add it on the previous line.

Got it, thanks!
New set submitted.

PR812 in SCP-firmware [1] has added some source files into mod_scmi_perf directory, but it currently fails when building for the optee-os case. This is due to the fact that the current build system in optee, for building the scmi-server fetched from SCP-firmware codebase, works well under the assumption that only one source file is required. To unlock the circular dependency between build systems and source codes, temporarily remove the build for the scmi-server. This will allow the following two patches to be safely merged into the optee_os repo: core: scmi-server: Add support for conditional options for SCMI-Perf optee-fvp: Enable CFG_SCPFW_SCMI_PERF_PROTOCOL_OPS Subsequently, PR812 in SCP-firmware can be merged, and immediately a revert patch for this present one will follow to reintroduce the libscmi-server build. Link: ARM-software/SCP-firmware#812 [1] Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>

SCMI-Server is built upon the SCP-firmware source tree and a recent change [1] being proposed is moving functionalities into side source files. This patch adds support to fetch those additional files based on two options: CFG_SCPFW_SCMI_PERF_FAST_CHANNELS (existing) CFG_SCPFW_SCMI_PERF_PROTOCOL_OPS (new) [1] ARM-software/SCP-firmware#812 Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>

Enable the SCMI-Perf protocol operations for the optee-fvp target. Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>

nicola-mazzucato-arm · 2023-08-11T09:08:03Z

@jforissier anything else required on my end for this to be merged? Thanks

jforissier · 2023-08-11T13:18:37Z

@jforissier anything else required on my end for this to be merged? Thanks

No, we're good, thanks!

A previous patch temporarily removed the libscmi-server build. Now that the related PR in SCP-firmware has been merged, reintroduce the build step. Refs: ARM-software/SCP-firmware#812 OP-TEE#6190 Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>

A previous patch temporarily removed the libscmi-server build. Now that the related PR in SCP-firmware has been merged, reintroduce the build step. Link: ARM-software/SCP-firmware#812 Link: OP-TEE#6190 Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>

A previous patch temporarily removed the libscmi-server build. Now that the related PR in SCP-firmware has been merged, reintroduce the build step. Link: ARM-software/SCP-firmware#812 Link: OP-TEE#6190 Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>

A previous patch temporarily removed the libscmi-server build. Now that the related PR in SCP-firmware has been merged, reintroduce the build step. Link: ARM-software/SCP-firmware#812 Link: #6190 Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>

A previous patch temporarily removed the libscmi-server build. Now that the related PR in SCP-firmware has been merged, reintroduce the build step. Link: ARM-software/SCP-firmware#812 Link: OP-TEE#6190 Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>

nicola-mazzucato-arm force-pushed the perf_ops_mmk branch from 22ce024 to e57570a Compare August 1, 2023 10:10

etienne-lms reviewed Aug 3, 2023

View reviewed changes

nicola-mazzucato-arm force-pushed the perf_ops_mmk branch from e57570a to 4fa5dc0 Compare August 4, 2023 09:17

nicola-mazzucato-arm force-pushed the perf_ops_mmk branch from 4fa5dc0 to 91dd41a Compare August 8, 2023 09:22

nicola-mazzucato-arm force-pushed the perf_ops_mmk branch from 91dd41a to dc26486 Compare August 8, 2023 13:52

jforissier reviewed Aug 9, 2023

View reviewed changes

nicola-mazzucato-arm force-pushed the perf_ops_mmk branch from dc26486 to 0235833 Compare August 10, 2023 09:47

nicola-mazzucato-arm added 3 commits August 10, 2023 12:09

optee-fvp: Enable CFG_SCPFW_SCMI_PERF_PROTOCOL_OPS

cf1af05

Enable the SCMI-Perf protocol operations for the optee-fvp target. Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>

nicola-mazzucato-arm force-pushed the perf_ops_mmk branch from 0235833 to cf1af05 Compare August 10, 2023 11:09

jforissier merged commit d93f6d0 into OP-TEE:master Aug 11, 2023
6 checks passed

nicola-mazzucato-arm mentioned this pull request Aug 18, 2023

ci.yml: Reintroduce build of libscmi-server #6246

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCMI-Server: Add support for multiple source files in build #6190

SCMI-Server: Add support for multiple source files in build #6190

nicola-mazzucato-arm commented Jul 24, 2023

nicola-mazzucato-arm commented Jul 27, 2023

jforissier commented Jul 27, 2023

nicola-mazzucato-arm commented Jul 28, 2023

jforissier commented Jul 28, 2023

nicola-mazzucato-arm commented Jul 28, 2023 •

edited

Loading

jforissier commented Jul 28, 2023

jforissier commented Aug 1, 2023

nicola-mazzucato-arm commented Aug 1, 2023

etienne-lms left a comment

etienne-lms Aug 3, 2023

nicola-mazzucato-arm Aug 4, 2023

nicola-mazzucato-arm commented Aug 4, 2023

nicola-mazzucato-arm commented Aug 7, 2023

jforissier commented Aug 7, 2023

nicola-mazzucato-arm commented Aug 7, 2023

jforissier commented Aug 7, 2023

nicola-mazzucato-arm commented Aug 8, 2023

jforissier commented Aug 8, 2023

nicola-mazzucato-arm commented Aug 8, 2023

nicola-mazzucato-arm commented Aug 9, 2023

jforissier Aug 9, 2023

nicola-mazzucato-arm Aug 10, 2023

nicola-mazzucato-arm commented Aug 11, 2023

jforissier commented Aug 11, 2023

SCMI-Server: Add support for multiple source files in build #6190

SCMI-Server: Add support for multiple source files in build #6190

Conversation

nicola-mazzucato-arm commented Jul 24, 2023

nicola-mazzucato-arm commented Jul 27, 2023

jforissier commented Jul 27, 2023

nicola-mazzucato-arm commented Jul 28, 2023

jforissier commented Jul 28, 2023

nicola-mazzucato-arm commented Jul 28, 2023 • edited Loading

jforissier commented Jul 28, 2023

jforissier commented Aug 1, 2023

nicola-mazzucato-arm commented Aug 1, 2023

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms Aug 3, 2023

Choose a reason for hiding this comment

nicola-mazzucato-arm Aug 4, 2023

Choose a reason for hiding this comment

nicola-mazzucato-arm commented Aug 4, 2023

nicola-mazzucato-arm commented Aug 7, 2023

jforissier commented Aug 7, 2023

nicola-mazzucato-arm commented Aug 7, 2023

jforissier commented Aug 7, 2023

nicola-mazzucato-arm commented Aug 8, 2023

jforissier commented Aug 8, 2023

nicola-mazzucato-arm commented Aug 8, 2023

nicola-mazzucato-arm commented Aug 9, 2023

jforissier Aug 9, 2023

Choose a reason for hiding this comment

nicola-mazzucato-arm Aug 10, 2023

Choose a reason for hiding this comment

nicola-mazzucato-arm commented Aug 11, 2023

jforissier commented Aug 11, 2023

nicola-mazzucato-arm commented Jul 28, 2023 •

edited

Loading