Xlnx cmake machine name change #302

bentheredonethat · 2024-07-16T22:20:34Z

Note that the second commit is just for consistency and can be removed -- It is known that the apps are moving to separate repo.

The intent here is to make the xlnx section of libmetal lib to be SOC agnostic.
CC @tnmysh @arnopo @wmamills

arnopo

should zynqmp_amp_demo become amp_demo?

bentheredonethat · 2024-07-19T14:11:35Z

@arnopo yes that makes sense. The patch of more interest from my perspective is the 1/2 patch.

bentheredonethat · 2024-07-22T15:40:35Z

@arnopo updated.

tnmysh · 2024-07-22T21:01:17Z

lib/system/generic/CMakeLists.txt

+if ("${PROJECT_MACHINE}" STREQUAL "xlnx_a53" OR
+    "${PROJECT_MACHINE}" STREQUAL "xlnx_a72" OR
+    "${PROJECT_MACHINE}" STREQUAL "xlnx_a78" OR
+    "${PROJECT_MACHINE}" STREQUAL "xlnx_r5"  OR


These machine names need to be chosen carefully.
So, Mulit-core platforms like zynqmp, has different configuration in same core. I can imagine three machine names in zynqmp: R5-lockstep-core, R5-split-core0 and R5-split-core1.

If we go that route, we might end up, with long list of MACHINE names, as future plaforms might have more such cores and configurations.

@arnopo , @wmamills any other approach in such case ?

Or, we can introduce other cmake variables such as:

PROJECT_MACHINE=zynqmp, PROJECT_CORE_TYPE=a53/R5, PROJECT_CORE_CONF=lockstep/split, PROJECT_CORE_NUMBER="0, 1 etc... ?

Hi @tnmysh from Xilinx-amd side, the core number and SOC for latest workflows (SDT) is inferred via header files.

That is 'zynqmp' / 'versal' etc and core 0/ core1 are symbols in the BSP. That being said we can update BSP to output those two. lockstep/split is not always visible from tooling in Xilinx-amd so in many cases lockstep==rpu0 for the defaults.

if it is custom tooling then we can add separate handling for lockstep but again, rpu0 defaults should already be ok for lockstep unless you mean for lockstep it should be using all the available TCM banks for that cluster. please clarify what is needed for the lockstep case in default demos.

though separately we would like to see if it is possible to remove the demos (can open separate PR / discussion for that)

That is 'zynqmp' / 'versal' etc and core 0/ core1 are symbols in the BSP.

In such case, I believe we shouldn't need xlnx_a53, xlnx_a72 both.
As zynqmp -> a53, versal -> a72. So, can we just have "xlnx" Instead?
How do we distinguish between cortex-a and cortex-r ? Headers also provide such defines?

Exactly, that is why I was suggesting to have more config variables, and those should be used across various software stacks i.e. apps, open-amp and libmetal.
My suggestion was for more generic case that can handle not xlnx platforms, but any multi-core platform.

May be we end up addressing it in different PR, but I am looking for suggestion that is scalable to other platforms too, not only xlnx platform.

@tnmysh adding adding machine name for SOC/Core combo is fair since we see that combination across multiple vendor/SOC's for remoteproc as well as in open source libs.

what that means is 'versal_a72' is a common-sense convention. it has the drawback of being redundant with headers but scales for other SOC's and is sufficient for many cases. Core or vendor specific configurations (lockstep for rpu) can be captured in header symbols.

PROJECT_MACHINE is not a generic variable and seems only used by AMD.

The objective here is to include xlnx subdirectory, right ?

What about a directory hierarchy vendor/platform/core or vendor/core?

a PROJECT_VENDOR ( or another variable) could define the vendor sub-directory and then in vendor directory the core can be differentiated, if needed, by PROJECT_MACHINE or PROJECT_TARGET

Hi @arnopo thanks for the comment here.
so if I understand your suggestion then the libmetal usage would be as follows:

add new cmake var PROJECT_VENDOR

in libmetal lib ports we can just keep the vendor (each vendor can then organize that as needed, xilinx-amd would just keep the xlnx subdir)

in cmake lib simply validate PROJECT_VENDOR and then in the vendor-specific CMake there can be additional validation of the PROJECT_MACHINE/PROJECT_TARGET as needed

@tnmysh @arnopo are you OK with that proposal?

This triggers a discussion on how to harmonize the folders in the different repositories to add more platform support. Let's add this to the agenda for the next RP call.

wmamills · 2024-08-08T17:08:25Z

lib/system/freertos/CMakeLists.txt

@@ -17,14 +17,8 @@ collect (PROJECT_LIB_SOURCES irq.c)
 collect (PROJECT_LIB_SOURCES shmem.c)
 collect (PROJECT_LIB_SOURCES time.c)

-if (EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${PROJECT_MACHINE})
+if ("${PROJECT_VENDOR}" STREQUAL "xlnx")


I was expecting something that looks like the PROJECT_MACHINE stuff below this.

if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${PROJECT_VENDOR})
add_subdirectory(${PROJECT_VENDOR})

but you may need to do something if PROJECT_VENDOR is not set as the ${CMAKE_CURRENT_SOURCE_DIR}/ dir will exist. You can add another test or define a default value of PROJECT_VENDOR as "none" in the top level.

Can you get that done?

bentheredonethat · 2024-08-12T15:19:14Z

@wmamills updated first commit per your review.
ill have to look at the default / non-vendor case still.

bentheredonethat · 2024-08-12T15:21:40Z

the way it is now is keeping the pre-exissting CMake Machine logic check (template machine exists)
if you want to switch from using machine at all in the general/non-vendor case then i can say to use 'template' since we already have that.

please advise.

wmamills · 2024-08-14T12:44:18Z

@bentheredonethat how are you calling cmake for your builds?
I was expecting cmake/platforms/zynqmp-{a53,r5}-{freertos,generic}.cmake and microblaze-generic.cmake to change or be collapsed to one per RTOS.

If it were me I would add xlnx-freertos.cmake and xlnx-generic.cmake and either delete the others or make them symlinks for back compatibility. Don't add symlinks for a72/a73 as the don't exist today. In the xlnx-* templates you should define the PROJECT_VENDOR.

For handling the non-vendor case I would add a default of "none" for PROJECT_VENDOR in cmake/options.cmake. You can look at the handling of MACHINE in that file. I would expect something like: (untested)
if (NOT DEFINED PROJECT_VENDOR)
set (PROJECT_VENDOR "none")
endif (NOT DEFINED PROJECT_VENDOR)
message ("-- Vendor: ${PROJECT_VENDOR}")

Since the lib/freertos/none will never exist, this will prevent your current logic from including lib/freertos twice.

It will also allow the PROJECT_MACHINE test to run and right now I don't think it can.
I believe:
if (EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${PROJECT_VENDOR})
will always be true if PROJECT_VENDOR is undefined as it will expand to:
if (EXISTS lib/system/freertos/ )
correct?

bentheredonethat · 2024-08-14T14:52:47Z

@wmamills the PROJECT_VENDOR part i can add and test.

for the cmake/platforms part -- im taking a look at that and not sure if I understand how that would work. I took a look and the thing is we still want the PROJECT_MACHINE for the microblaze case and for demos.

additionally the CMAKE_C_FLAGS, CMAKE_SYSTEM_PROCESSOR, and CROSS_PREFIX are all different. having multiple differences per machine means it makes sense to keep them as is and (to me at least) just add the PROJECT_VENDOR to each of these.

i have proceeded with the above in latest commits.

wmamills · 2024-08-14T20:13:29Z

@bentheredonethat yes you are right about the templates. I looked at the new commit and the template files look OK.

We still need a default value for PROJECT_VENDOR as I outlined above as the base case fails with out it.
See the CI results for build-generic-arm.
Build Failure is:
CMake Error at lib/system/generic/CMakeLists.txt:21 (add_subdirectory):
add_subdirectory called with incorrect number of arguments
-- Configuring incomplete, errors occurred!

I will try what I suggested here to make sure it works.

You can run the generic build yourself, just look at the build_generic function in:
.github/actions/build_ci/entrypoint.sh
(I would add -DWITH_DOC=OFF to the cmake line so you don't need to worry about the docs stuff. It turns itself off in the needed programs are not found but if you have some and not all it will fail.)

…JECT_VENDOR Simplify logic that coordinates when to pull in Xilinx-AMD BSP and setup code basedd on new CMake variable 'PROJECT_VENDOR'. Add PROJECT_VENDOR check in cmake/options.cmake for case where PROJECT_VENDOR is not defined. Signed-off-by: Ben Levinsky <[email protected]>

Add PROJECT_VENDOR to Xilinx-AMD .cmake platform files. Signed-off-by: Ben Levinsky <[email protected]>

Match examples to lib CMake machine name change Signed-off-by: Ben Levinsky <[email protected]>

bentheredonethat · 2024-08-14T20:32:31Z

tested and built as suggested above. Thanks @wmamills !

newest commits add the null-case (no project_vendor)

wmamills · 2024-08-14T21:24:29Z

I tested as well and the generic-build works OK on my desk and in CI.

CI is getting an error on the Zephyr step but that looks like an upstream change not something in the PR.
I am proving that to myself now.

wmamills · 2024-08-14T21:27:05Z

Background on Zephyr error:
zephyrproject-rtos/zephyr#74682
Zephyr v3.7.0 or main has this PR. I am trying v3.6.0 which is what we used in the v2024.05 release.

wmamills · 2024-08-14T21:34:54Z

With Zephyr locked to v3.6.0 the CI passes.
https://github.com/wmamills/libmetal/actions/runs/10395182873/job/28786640624
Fixing Zephyr v3.7.0 is not an issue for this PR.

diff --git a/.github/actions/build_ci/entrypoint.sh b/.github/actions/build_ci/entrypoint.sh
index 9333c32..99eb283 100755
--- a/.github/actions/build_ci/entrypoint.sh
+++ b/.github/actions/build_ci/entrypoint.sh
@@ -5,6 +5,7 @@ readonly TARGET="$1"
ZEPHYR_TOOLCHAIN_VARIANT=zephyr
ZEPHYR_SDK_API_FOLDER=https://api.github.com/repos/zephyrproject-rtos/sdk-ng/releases/latest
ZEPHYR_SDK_SETUP_TAR=zephyr-sdk-.*linux-x86_64.tar.xz
+ZEPHYR_VER=v3.6.0

FREERTOS_ZIP_URL=https://sourceforge.net/projects/freertos/files/FreeRTOS/V10.0.1/FreeRTOSv10.0.1.zip

@@ -82,7 +83,7 @@ build_zephyr(){
pv $ZEPHYR_SDK_TAR -i 3 -ptebr -f | tar xJ || exit 1
rm -rf $ZEPHYR_SDK_TAR || exit 1
yes | ./$ZEPHYR_SDK_SETUP_DIR/setup.sh || exit 1

```
  west init ./zephyrproject || exit 1
```

  west init --mr $ZEPHYR_VER ./zephyrproject || exit 1
  cd ./zephyrproject || exit 1
  west update --narrow || exit 1
  west zephyr-export || exit 1

wmamills

New version looks good now.

arnopo · 2024-08-26T14:01:46Z

Hi @bentheredonethat
Any reason to not have same folder hiearchy for Linux examples, using PROJECT_VENDOR?

bentheredonethat · 2024-08-26T14:05:00Z

@arnopo thanks for the comment. in that case would it be:
examples -> xlnx -> xlnx_r5
?

arnopo · 2024-08-26T15:07:14Z

@arnopo thanks for the comment. in that case would it be: examples -> xlnx -> xlnx_r5 ?

The LInux example does not run on the R5 but on the A53, right?
what about examples/system/linux/xlnx/zynqmp_amp_demo/ -> examples/system/linux/xlnx_a53/amp_demo/

bentheredonethat · 2024-08-26T15:16:02Z

@arnopo thanks for the comment. in that case would it be: examples -> xlnx -> xlnx_r5 ?

The LInux example does not run on the R5 but on the A53, right? what about examples/system/linux/xlnx/zynqmp_amp_demo/ -> examples/system/linux/xlnx_a53/amp_demo/

Should this not be:

The LInux example does not run on the R5 but on the A53, right? what about > examples/system/linux/xlnx/zynqmp_amp_demo/ -> examples/system/linux/xlnx/xlnx_a53/amp_demo/

this way w are keeping the 'xlnx' / vendor name right?

so that would be:
examples/system/linux/xlnx/xlnx_a53/amp_demo/
and
examples/system/linux/xlnx/xlnx_r5/amp_demo/

arnopo · 2024-08-26T16:18:46Z

@arnopo thanks for the comment. in that case would it be: examples -> xlnx -> xlnx_r5 ?

The LInux example does not run on the R5 but on the A53, right? what about examples/system/linux/xlnx/zynqmp_amp_demo/ -> examples/system/linux/xlnx_a53/amp_demo/

Should this not be:

The LInux example does not run on the R5 but on the A53, right? what about > examples/system/linux/xlnx/zynqmp_amp_demo/ -> examples/system/linux/xlnx/xlnx_a53/amp_demo/

Reading your PR again , I think that my advice is not accurate. What I would like here for the folder hierachy to be coherent for the whole project.
I would expect having the same hierarchy in lib and examples following the template examples/system/${PROJECT_SYSTEM}/${PROJECT_VENDOR}

So
examples/system/linux/xlnx/zynqmp_amp_demo/ -> examples/system/linux/xlnx/xlnx_a53/amp_demo/
examples/system/generic/xlnx_r5/amp_demo/ -> examples/system/generic/xlnx/amp_demo/
examples/system/freertos/xlnx_r5/amp_demo/ -> examples/system/freertos/xlnx/amp_demo/

bentheredonethat · 2024-08-26T17:58:18Z

@arnopo I am good with that. will send patch as suggested above. thanks

arnopo requested review from edmooring, wmamills and tnmysh July 19, 2024 12:53

arnopo reviewed Jul 19, 2024

View reviewed changes

bentheredonethat force-pushed the xlnx-cmake-machine-name-change branch from 2ad15da to 0bba9ed Compare July 22, 2024 15:40

tnmysh reviewed Jul 22, 2024

View reviewed changes

bentheredonethat force-pushed the xlnx-cmake-machine-name-change branch from 0bba9ed to 29f1504 Compare August 8, 2024 16:35

wmamills requested changes Aug 8, 2024

View reviewed changes

bentheredonethat force-pushed the xlnx-cmake-machine-name-change branch from 29f1504 to bbb6ea3 Compare August 12, 2024 15:17

bentheredonethat force-pushed the xlnx-cmake-machine-name-change branch from bbb6ea3 to 39671f3 Compare August 14, 2024 14:52

bentheredonethat added 3 commits August 14, 2024 13:31

cmake: platforms: xlnx: Add PROJECT_VENDOR

a03b649

Add PROJECT_VENDOR to Xilinx-AMD .cmake platform files. Signed-off-by: Ben Levinsky <[email protected]>

examples: xlnx: Change machine name to be SOC agnostic

2078aa0

Match examples to lib CMake machine name change Signed-off-by: Ben Levinsky <[email protected]>

bentheredonethat force-pushed the xlnx-cmake-machine-name-change branch from 39671f3 to 2078aa0 Compare August 14, 2024 20:32

wmamills self-requested a review August 14, 2024 21:35

wmamills approved these changes Aug 14, 2024

View reviewed changes

wmamills mentioned this pull request Aug 14, 2024

CI: Pin to specific version of Zephyr SDK in default PRs #295

Closed

wmamills merged commit 5c36d6b into OpenAMP:main Aug 15, 2024
2 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xlnx cmake machine name change #302

Xlnx cmake machine name change #302

bentheredonethat commented Jul 16, 2024 •

edited

Loading

arnopo left a comment •

edited

Loading

bentheredonethat commented Jul 19, 2024

bentheredonethat commented Jul 22, 2024

tnmysh Jul 22, 2024

tnmysh Jul 22, 2024 •

edited

Loading

bentheredonethat Jul 22, 2024

bentheredonethat Jul 22, 2024

tnmysh Jul 22, 2024

tnmysh Jul 22, 2024

bentheredonethat Jul 22, 2024 •

edited

Loading

arnopo Jul 24, 2024

bentheredonethat Jul 24, 2024

arnopo Jul 25, 2024

wmamills Aug 8, 2024

bentheredonethat commented Aug 12, 2024

bentheredonethat commented Aug 12, 2024

wmamills commented Aug 14, 2024 •

edited

Loading

bentheredonethat commented Aug 14, 2024

wmamills commented Aug 14, 2024

bentheredonethat commented Aug 14, 2024 •

edited

Loading

wmamills commented Aug 14, 2024

wmamills commented Aug 14, 2024

wmamills commented Aug 14, 2024

wmamills left a comment

arnopo commented Aug 26, 2024

bentheredonethat commented Aug 26, 2024

arnopo commented Aug 26, 2024

bentheredonethat commented Aug 26, 2024 •

edited

Loading

arnopo commented Aug 26, 2024

bentheredonethat commented Aug 26, 2024

Xlnx cmake machine name change #302

Xlnx cmake machine name change #302

Conversation

bentheredonethat commented Jul 16, 2024 • edited Loading

arnopo left a comment • edited Loading

Choose a reason for hiding this comment

bentheredonethat commented Jul 19, 2024

bentheredonethat commented Jul 22, 2024

Choose a reason for hiding this comment

tnmysh Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bentheredonethat Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bentheredonethat commented Aug 12, 2024

bentheredonethat commented Aug 12, 2024

wmamills commented Aug 14, 2024 • edited Loading

bentheredonethat commented Aug 14, 2024

wmamills commented Aug 14, 2024

bentheredonethat commented Aug 14, 2024 • edited Loading

wmamills commented Aug 14, 2024

wmamills commented Aug 14, 2024

wmamills commented Aug 14, 2024

wmamills left a comment

Choose a reason for hiding this comment

arnopo commented Aug 26, 2024

bentheredonethat commented Aug 26, 2024

arnopo commented Aug 26, 2024

bentheredonethat commented Aug 26, 2024 • edited Loading

arnopo commented Aug 26, 2024

bentheredonethat commented Aug 26, 2024

bentheredonethat commented Jul 16, 2024 •

edited

Loading

arnopo left a comment •

edited

Loading

tnmysh Jul 22, 2024 •

edited

Loading

bentheredonethat Jul 22, 2024 •

edited

Loading

wmamills commented Aug 14, 2024 •

edited

Loading

bentheredonethat commented Aug 14, 2024 •

edited

Loading

bentheredonethat commented Aug 26, 2024 •

edited

Loading