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.

Sign up for GitHub

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

Jump to bottom

Add AMD ZynqMP Support #731

Merged
merged 8 commits into from
Feb 18, 2025
Merged

Add AMD ZynqMP Support #731

merged 8 commits into from
Feb 18, 2025

Conversation

mariam-elshakfy
Copy link
Contributor

@mariam-elshakfy mariam-elshakfy commented Jan 22, 2025 •
edited
Loading

  • Add ZynqMP Silicon package
  • Add a dummy ZynqMP platform as an example

Tested on KV260

@mariam-elshakfy mariam-elshakfy marked this pull request as ready for review January 22, 2025 09:20
@mariam-elshakfy
Copy link
Contributor Author

Hi @michalsimek,
I'm not able to assign you as a reviewer, but could you take a look at these changes?
Thanks!

@leiflindholm
Copy link
Member

Who is going to maintain this new port? Existing AMD maintainers, developers of the port, or both?

@michalsimek
Copy link
Contributor

Who is going to maintain this new port? Existing AMD maintainers, developers of the port, or both?

It is likely me who will take care about this port and also ports related Xilinx SOCs if @mariam-elshakfy-linaro agrees.

michalsimek
michalsimek reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@michalsimek michalsimek left a comment

Choose a reason for hiding this comment

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

In general this is great step in the right direction and hopefully there will be a way how to get this platform to be more DT driven to avoid hardcoding values in different locations. Only one should be used at best but not sure if this is feasible in edk2.

@leiflindholm
Copy link
Member

Who is going to maintain this new port? Existing AMD maintainers, developers of the port, or both?

It is likely me who will take care about this port and also ports related Xilinx SOCs if @mariam-elshakfy-linaro agrees.

No objections from me, but it would be good to add to CODEOWNERS and/or REVIEWERS as appropriate as part of this set.

@michalsimek
Copy link
Contributor

Who is going to maintain this new port? Existing AMD maintainers, developers of the port, or both?

It is likely me who will take care about this port and also ports related Xilinx SOCs if @mariam-elshakfy-linaro agrees.

No objections from me, but it would be good to add to CODEOWNERS and/or REVIEWERS as appropriate as part of this set.

@mariam-elshakfy-linaro can you please also extend these two files based on your guidance?

@leiflindholm
Copy link
Member

The comment from @michalsimek made me realise the directories you're adding are both packages, and hence it's idiomatic to give them a "Pkg" suffix - as well as their corresponding dec/dsc/fdf files. I.e. Silicon/AMD/ZynqMPPkg/ZynqMPPkg.dec.

@mariam-elshakfy
Copy link
Contributor Author

Who is going to maintain this new port? Existing AMD maintainers, developers of the port, or both?

It is likely me who will take care about this port and also ports related Xilinx SOCs if @mariam-elshakfy-linaro agrees.

No objections from me, but it would be good to add to CODEOWNERS and/or REVIEWERS as appropriate as part of this set.

@mariam-elshakfy-linaro can you please also extend these two files based on your guidance?

Okay, I will update the file after the package & board name is decided

@mariam-elshakfy
Copy link
Contributor Author

The comment from @michalsimek made me realise the directories you're adding are both packages, and hence it's idiomatic to give them a "Pkg" suffix - as well as their corresponding dec/dsc/fdf files. I.e. Silicon/AMD/ZynqMPPkg/ZynqMPPkg.dec.

The platform is updated to ZynqmpVirtPkg. I still haven't updated the Silicon package, but is this a hard requirement? I see other Silicon that doesn't have this suffix (AMD Styx for example).

@changab
Copy link
Member

changab commented Jan 28, 2025 •
edited
Loading

OK, that looks completely different from what was posted earlier, and looks entirely plausible, with the exception of the last two lines which I will interpret as typos. (There is no sense in having a package called Pkg.)

If there are no Include, Library, Driver directories - i.e. no interfaces are exposed, then it's not a package. If a folder contains a .dec, it must have Pkg in its name. If a folder has Pkg in its name, no folder below it can have a .dec or have Pkg in its name.

Ah, yes, it is kind of a typo because I use "<" and ">" in the content. What you replied here is matched to the directory structure posted above.

Platform/AMD/XilinxBoard
Platform/AMD/XilinxBoard/CommonPkg/Universal
Platform/AMD/XilinxBoard/CommonPkg/Inclulde
Platform/AMD/XilinxBoard/CommonPkg/Library
Platform/AMD/XilinxBoard/CommonPkg/CommonPkg.dec
Platform/AMD/XilinxBoard/ZynqMp
Platform/AMD/XilinxBoard/ZynqMp/CommonPkg/Library
Platform/AMD/XilinxBoard/ZynqMp/CommonPkg/Include
Platform/AMD/XilinxBoard/ZynqMp/CommonPkg/Universal
Platform/AMD/XilinxBoard/ZynqMp/CommonPkg/CommonPkg.dec
Platform/AMD/XilinxBoard/ZynqMp/VirtualPkg/Univeral
Platform/AMD/XilinxBoard/ZynqMp/VirtualPkg/VitualPkg.dec
Platform/AMD/XilinxBoard/ZynqMp/xxxBoard/ (if one include, library)

mariam-elshakfy and others added 3 commits January 29, 2025 13:55
@mariam-elshakfy @michalsimek
Silicon/AMD/Xilinx/ZynqMpPkg: Add common platform library
3f76b73 
ZynqMP platform helper library is based on ArmPlatformLibNull
from edk2.

This contains early initializations needed in
SEC/PEI phase. It also configures the memory regions
for the address translation needed for DXE stage.

Co-authored-by: Emekcan Aras <emekcan.aras@linaro.org>
Co-authored-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Mariam Elshakfy <mariam.elshakfy@linaro.org>
@mariam-elshakfy
Silicon/AMD/Xilinx/ZynqMpPkg: Add serial port library
aca8e7c 
ZynqMP is using Cadence UART peripheral, which is already
initialized by Xilinx FSBL. Therefore, the implementation
only pushes/reads characters to/from FIFO.

Co-authored-by: Emekcan Aras <emekcan.aras@linaro.org>
Signed-off-by: Mariam Elshakfy <mariam.elshakfy@linaro.org>
@mariam-elshakfy @michalsimek
Silicon/AMD/Xilinx/CommonPkg: Add Mmc Support
e05d30d 
This change introduces Arasan Mmc driver, which is common
to Xilinx platforms. The Mmc driver is based on:
Platform/RaspberryPi/Drivers/ArasanMmcHostDxe

This driver currently doesn't support SD high speed
mode, or eMMC

Co-authored-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Mariam Elshakfy <mariam.elshakfy@linaro.org>
@mariam-elshakfy mariam-elshakfy force-pushed the add-zynqmp-support branch 2 times, most recently from 3ce737e to f2f6b36 Compare January 29, 2025 14:49
@mariam-elshakfy
Copy link
Contributor Author

mariam-elshakfy commented Jan 29, 2025 •
edited
Loading

I have pushed a restructure change now:

https://github.com/tianocore/edk2-platforms/compare/5d365685b9b13f265dd81f3bbf978ce56129bfe6..f2f6b36b35228762de66f74a0ec1777c1d90530c

The latest message from @changab is missing Silicon/, so I wasn't sure if I should move everything under Platform/. Let me know if that was the case.

These changes should comply to the rules stated by @leiflindholm

If there are no Include, Library, Driver directories - i.e. no interfaces are exposed, then it's not a package.
If a folder contains a .dec, it must have Pkg in its name.
If a folder has Pkg in its name, no folder below it can have a .dec or have Pkg in its name.

with the exception that AMD/XilinxBoard/ZynqMp/VirtualPkg is called a package without exposing any interfaces. I can't imagine what kind of libraries/interfaces will be provided through this virtual package in the future since there are:

Silicon/AMD/Xilinx/CommonPkg for common drivers/libraries
Silicon/AMD/Xilinx/ZynqMpPkg for ZynqMp drivers/libraries

So, I'm not convinced it should be a package.

Also, @michalsimek could you check if the current split of PCDs/drivers between CommonPkg and ZynqMpPkg makes sense from hardware perspective, please?

Thanks!

@mariam-elshakfy mariam-elshakfy force-pushed the add-zynqmp-support branch 2 times, most recently from 1d3eb49 to 8b6aa44 Compare January 29, 2025 15:44
leiflindholm
leiflindholm approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member

@leiflindholm leiflindholm left a comment

Choose a reason for hiding this comment

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

For my part, approved.

@mariam-elshakfy mariam-elshakfy force-pushed the add-zynqmp-support branch 2 times, most recently from bcee54d to 1812418 Compare January 30, 2025 10:02
michalsimek
michalsimek reviewed Jan 30, 2025
View reviewed changes
Copy link
Contributor

@michalsimek michalsimek left a comment

Choose a reason for hiding this comment

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

Structure is nice and good for future work. I have pretty much only two generic comment. I don't like too much to rely on setup out of VirtualPkg.dsc about IPs which can be selected. I would prefer more to list all of them directly here that looking at one file will give you good overview about system configuration.

And second comment is about text base which is setup to 0 now and was at 0x8000000 and it is not clear why that change was made.

@mariam-elshakfy mariam-elshakfy force-pushed the add-zynqmp-support branch 2 times, most recently from 8400534 to 19fe3c7 Compare January 30, 2025 15:09
@mariam-elshakfy
Silicon/AMD/Xilinx/CommonPkg: Add initial package files
7b24d2e 
Add .dec file with common Xilinx PCDs. Also, add .dsc.inc
and .fdf.inc files to be included in platform descriptions.

Signed-off-by: Mariam Elshakfy <mariam.elshakfy@linaro.org>
@mariam-elshakfy
Silicon/AMD/Xilinx/ZynqMpPkg: Add initial package files
14ce6d9 
Add .dec file with ZynqMP-specific PCDs. Also, add .dsc.inc
and .fdf.inc files to be included for ZynqMP-based platform
descriptions.

Signed-off-by: Mariam Elshakfy <mariam.elshakfy@linaro.org>
@mariam-elshakfy
Platform/AMD/XilinxBoard/ZynqMp/VirtualPkg: Add virtual ZynqMP board
aea68aa 
Add an empty board based on Silicon/AMD/Xilinx/ZynqMpPkg.
This board contains empty devictree file, and has the
following features:

- Only 2GB DDR with no extra memory
- Uses SDHCI1 and UART1
- TF-A is located by the end of DDR
- Doesn't use OP-TEE

Relevant PCDs are set accordingly in VirtualPkg.dsc

This can be used as a reference for users to port EDK-II to
ZynqMP-based boards.

Signed-off-by: Mariam Elshakfy <mariam.elshakfy@linaro.org>
@mariam-elshakfy
Silicon/AMD/Xilinx/ZynqMpPkg: Add package README
bb7fbad 
Add documentation on how to port and build EDK-II for
ZynqMP-based platforms using this package.

Signed-off-by: Mariam Elshakfy <mariam.elshakfy@linaro.org>
@mariam-elshakfy
CODEOWNERS: Add @michalsimek as Xilinx maintainer
07aa903 
Update CODEOWNERS and CONTRIBUTORS.md to include Michal Šimek
as maintainer to Xilinx platforms.

Signed-off-by: Mariam Elshakfy <mariam.elshakfy@linaro.org>
@mariam-elshakfy
Copy link
Contributor Author

Removed ArmDisassemblerLib since it's going away now

changab
changab approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@changab changab left a comment

Choose a reason for hiding this comment

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

Approval for the folder structure under AMD platform and silicon.

@changab
Copy link
Member

changab commented Jan 31, 2025

@mariam-elshakfy-linaro, just letting me know that we are all set to merge this PR after you address all the comments.
Thanks
Abner

@abdattar
Copy link
Contributor

Code changes and code layout looks good.

@mariam-elshakfy
Copy link
Contributor Author

@mariam-elshakfy-linaro, just letting me know that we are all set to merge this PR after you address all the comments. Thanks Abner

Thanks.
All review comments are addressed. I think only @michalsimek's approval is missing.

@mariam-elshakfy
Copy link
Contributor Author

mariam-elshakfy commented Feb 10, 2025 •
edited
Loading

@changab All comments are addressed and resolved on this PR. Can you help merge it?
Thanks!

@changab changab merged commit 728c8bb into tianocore:master Feb 18, 2025
1 check passed
@mariam-elshakfy mariam-elshakfy deleted the add-zynqmp-support branch February 19, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalsimek michalsimek michalsimek left review comments

@leiflindholm leiflindholm leiflindholm approved these changes

@changab changab changab approved these changes

@abdattar abdattar abdattar approved these changes

@pbgrimes pbgrimes Awaiting requested review from pbgrimes

@mdkinney mdkinney Awaiting requested review from mdkinney mdkinney is a code owner

Assignees

@mariam-elshakfy mariam-elshakfy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mariam-elshakfy @leiflindholm @michalsimek @changab @abdattar