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

Bring in Xilinx-AMD commits #64

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bentheredonethat
Copy link
Contributor

Bring in commits from Xilinx-AMD fork of OpenAMP System Reference Repo.

@bentheredonethat bentheredonethat changed the title Amd main jan9 2025 Bring in Xilinx-AMD commits Jan 9, 2025
@bentheredonethat
Copy link
Contributor Author

CC @arnopo @tnmysh

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

I started to review this PR , but it is too big. Please split in several commits.
Please also pay attention that examples should be as generic as possible, so put AMD specificity in machine when possible

examples/legacy_apps/examples/echo/CMakeLists.txt Outdated Show resolved Hide resolved
examples/legacy_apps/examples/echo/freertos/main.c Outdated Show resolved Hide resolved
examples/legacy_apps/examples/echo/freertos/main.c Outdated Show resolved Hide resolved
examples/legacy_apps/examples/echo/freertos/main.c Outdated Show resolved Hide resolved
examples/legacy_apps/examples/echo/freertos/main.c Outdated Show resolved Hide resolved
examples/legacy_apps/examples/echo/freertos/main.c Outdated Show resolved Hide resolved
examples/legacy_apps/examples/echo/rpmsg-echo.c Outdated Show resolved Hide resolved
Move all app configuration variables to platform_info.h so that all the
relevant information that can be configured is in one file.

This makes the application easier to configure and debug.

Signed-off-by: Ben Levinsky <[email protected]>
@bentheredonethat
Copy link
Contributor Author

@arnopo i have updated as follows

  • removed all but AMD-Xilinx System-Device Tree commits
  • review-specific fixes

hopefully this will make it easier for you to review!

@tnmysh
Copy link
Collaborator

tnmysh commented Jan 16, 2025

when remote processor is detached or stopped by host, it
sets vdev reset flag to 0 via virtio config ops in resource
table. Introduce API that will poll on this flag to destroy
endpoints and virtio devices.

Application should re-create rpmsg devices once coming out
of loop from this function, so host can re-attach to this

Also use new API platform_poll_on_vdev_reset to poll
interrupts from host instead of platform_poll

Signed-off-by: Tanmay Shah <[email protected]>
Signed-off-by: Ben Levinsky <[email protected]>
1. Enable CMake build to pass in linker flags. This will enable Xilinx-AMD
   System Device Tree Flow BSP-based tooling to pass required linker
      flags
2. Enable System Device Flow compliant linker script where many sections
   are placed in DDR for space and entry point provided by BSP is
      _vector_table instead of _boot
3. Ensure symbols are present for both classic and System Device Flow flow
   for each of the above SOC's.
4. Remove symbols that are also present in BSP
5. Add Versal NET SDT Linker script

Signed-off-by: Ben Levinsky <[email protected]>
…lugin

As System Device Tree flow supports Lopper generated OpenAMP Linker scripts
enable the configuration of these for ZynqMP, Versal and Versal-NET SOC's.

Signed-off-by: Ben Levinsky <[email protected]>
@bentheredonethat
Copy link
Contributor Author

@@ -115,23 +106,43 @@ int main(int argc, char *argv[])
ret = platform_init(argc, argv, &platform);
if (ret) {
LPERROR("Failed to initialize platform.\r\n");
ret = -1;
} else {
ML_ERR("Server reboot is required to recover\r\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This commit does not build du to ML_ERR
this messgae seems related to the test environment not the example. I would remove it and

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like this comment cut out. what are you asking for here?

i can move the ML_* commit to first if that helps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the criteria for a PR is that we are able to build the openamp lib, regardless of the commit we point to in the PR.

the build fails pointing on 70598c4

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bentheredonethat just replace all the ML_* macros with metal_* macros from first PR. If it's already resolved, ignore this comment and resolve this conversation.

* Server behavior is undefined. It's better to wait in
* an infinite loop instead
*/
while (1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you will break the CI that should migrate on these examples. This should be addressed in your test environment

Copy link
Contributor Author

@bentheredonethat bentheredonethat Jan 17, 2025

Choose a reason for hiding this comment

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

see tanmay's comment

* Server behavior is undefined. It's better to wait in
* an infinite loop instead
*/
while (1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same issue here

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arnopo is that okay, if we can define return of main based on PROJECT_SYSTEM variable ?

#ifdef PROJECT_SYSTEM == LINUX
return ;
#else
while (1);
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems to me not enough generic.
Behavior is platform dependent so I would be treated in platform_info.c , what about implementing in cleanup_system or a system_shutdown that would be empty by default?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that's a good idea. It will be re-factored accordingly.

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