-
Notifications
You must be signed in to change notification settings - Fork 194
Build machine-less libmetal #350
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
base: main
Are you sure you want to change the base?
Conversation
|
I got report that 9f749f0 this change from 1.9.0 release, is causing build failures for xlnx platform. This PR is fixing that build issue. What's best way to integrate changes in this PR? Should we make 1.9.1 or 2025.10.1 release? As the demos are not released yet, we can still upgrade minor version of the library with this fix. |
cd9e5a3 to
64f1c7a
Compare
|
The CI commands might need to be improved for this PR to pass: cd /github/workspace/build-generic/lib && /usr/bin/arm-none-eabi-gcc -DDEFAULT_LOGGER_ON -DMETAL_INTERNAL -I/github/workspace/build-generic/lib/include -Wall -Werror -Wextra -MD -MT lib/CMakeFiles/metal-static.dir/dma.c.obj -MF CMakeFiles/metal-static.dir/dma.c.obj.d -o CMakeFiles/metal-static.dir/dma.c.obj -c /github/workspace/lib/dma.c Since MACHINE is not mandatory anymore, may be we should explicitly pass it to build the library if needed. |
Hi @tnmysh : sorry for the late reply. From my perspective the CI is good we specify the Investigating the issue, it seems that it is coming from a syntax error in the PR |
For xlnx platform, we don't use template machine. In that case, how can we make sure not to enforce anything related to template machine? Tanmay |
At least for build the CI does it for you: https://github.com/OpenAMP/libmetal/blob/main/.github/actions/build_ci/entrypoint.sh#L43-L51 |
43c30b3 to
db68068
Compare
|
Something is not Crystal clear in AMD implementation |
HI @arnopo , sorry for delayed reply. https://github.com/OpenAMP/libmetal/blob/main/lib/sys.h#L85 here we are passing Here libmetal/lib/system/generic/sys.h Line 27 in 1c3410b
we don't have any PROJECT_MACHINE so, no header will be included.
Instead, we will include metal/sys.h into our vendor specific extension module, and implement those interfaces directly there and link them during building demos. |
|
Hi @tnmysh, It is still not clear to me.
what is the value of
You don't include the Perhaps a link to the AMD extension module you mention, would help me to understand how you split the libmetal |
PRJOECT_SYSTEM can be one of the four: "generic", "freertos", "linux", or "zephyr".
Yes, metal/sys.h is included in the extension as header.
Here is the README: https://gitenterprise.xilinx.com/embeddedsw/embeddedsw/blob/xlnx_rel_v2025.2/ThirdParty/sw_services/libmetal_xlnx_extension/src/lib/README.md |
Unfortunately, this seems an AMD internal server. I don't have access to it. |
Ah! @arnopo I am so sorry. I completely missed it. Here is the open source link: https://github.com/Xilinx/embeddedsw/blob/master/ThirdParty/sw_services/libmetal_xlnx_extension/src/lib/metal_xlnx_extension.c Thanks, |
|
I had a look at your repo. Please tell me if my analysis is wrong.
At this point I was expecting to see libmetal/zynqmp_r5/sys.h in AMD extension
This indicates to me that you don't use the include files generated by CMake but rather those defined in the AMD extension. If my analysis is correct, this highlights a limitation in libmetal that forces users to update libmetal itself. I wonder if we should break the direct link between the system and machine include files. Is a fix is mandatory for do a fix for the v2025.10 release? |
This is different project than libmetal. So whatever MACHINE is defined here its not same as libmetal.
No, libmetal has different toolchain.cmake, it is not same as above extension library.
Above is partially correct. Actually, I don't pass template-toolchain.cmake, instead I have defined my own toolchain file for
Correct. That is exactly why I introduced this patch, so extensions can be built out of libmetal.
That is what this patch is doing. It removes requirement of @PROJECT_MACHINE/sys.h file.
This fix is needed because we are forcing template-toolchain.cmake to build the libmetal. So yes it is needed for v2025.10. I think the last patch re-introduced template directory, and when I tested, I didn't have the last patch of v2025.10. That is why I missed it. |
from my test you don't ignore The result is Could you confirm it on your side?
For legacy support, we need to at least use deprecation mechanism for next release
Ok that means that we have to find a short term solution and probably rework it for next release For this release, what about having in libmetal/lib/system/xxxx/sys.h and in cmake |
Correct, and that sys.h won't be included again because, it's guarded with #ifndef. So it's like as if that line is not taking effect.
Confirmed.
Legacy support on AMD-xlnx platform ?
Do you mean, introducing xlnx/sys.h and xlnx/sys.c ?
Which cmake? option.cmake ? or CMakeLists.txt in system directory? Also, If above #include becomes no-op as explained above, then do we need extra definition?
|
Yes but the
Not only AMD platform. The generic template machine can be used as default machine for platforms that only needs metal for open-amp porting. This is the case for instance for ST platforms.
I mean by
apologogize the code should be But writing this I realize that it is not clean, the generated code would be in AMD case So forget this approach What about following the approach done for the cache ?
In such case we could remove Could you try this with your AMD code?
|
Okay, yeah that is reasonable. Then we can remove template/sys.h completely.
|
|
@tnmysh : I pushed a commit on your branch. I tested it and the template machine is functional If Ok for you could you create a new PR for the the |
libmetal can be build without any machine support. It is possible that vendors implement machine specific interfaces outside of libmetal and link it with demos during build time. Hence, remove requirement to have MACHINE and PROJECT_MACHINE variables from the build system. If vendor prefer to choose 'template' machine, they can pass such option during cmake configuration. Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
This directory was removed and re-inroduced by mistake, and should not exists in the libmetal. Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
I tested this on AMD-xilinx platform, and it's working.
Okay, I will create a new PR. |
Declare sys_irq_enable() and sys_irq_enable() directly in lib/system/@PROJECT_SYSTEM@/sys.h. This allows to declare the machine out of tree of the libmetal but also to support the in-tree template. [minor style fix by tanmay] Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
87028b2 to
51e5ecf
Compare
|
@arnopo pushed minor fix in your commit related to checkpatch.pl. Other than that this PR looks good. |
arnopo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same to #354 but for the main branch
|
@tnmysh : Please update this PR to resolve conflicts |
It is not required to have MACHINE variable defined for the libmetal project. As Vendors can choose to implement machine specific interfaces outside of the libmetal and link it build time with the demos.
Hence make MACHINE variable optional in the cmake build system.