-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Move GIC PCDs from ArmPkg to MedPkg #10635
base: master
Are you sure you want to change the base?
Conversation
MdePkg/MdePkg.dec
Outdated
|
||
[PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.ARM] | ||
# Define if the GICv3 controller should use the GICv2 legacy | ||
gEfiMdePkgTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000059 |
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.
Can we just get rid of this one?
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.
ArmGicLib and ArmGicDxe are still using this PCDs:
https://github.com/tianocore/edk2/blob/master/ArmPkg/Drivers/ArmGic/ArmGicLib.c#L196
https://github.com/tianocore/edk2-archive/blob/master/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c#L257
Not sure whether any user is still using this GiC V2 Legacy.
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.
Can we just get rid of this one?
I kept it around because it was in use in ArmGic. If we're happy to drop the support from the driver, sure. Don't know how likely we are to have non-open users of it.
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.
This is 10 year old cruft that was added at the time because the GIC version detection is based on the presence of the CPU system register interface rather than the architecture the GIC itself claims to implement.
At the time, this was needed because TF-A was lagging behind, and non-secure can only use the GIC in v3 mode if the secure world does so too.
I'd actually like to some early spring cleaning here, and not only rip this out, but separate the v2 and v3 GixDxes as well - platforms rarely need both, and the V2Legacy behavior can be retained (if needed) by only including the v2 DXE.
I'll try to whip something up asap - in the mean time, can we just drop this V2Legacy for now?
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.
Since I'm not a repo admin, I can't push to the PR branch, so I've updated my branch
https://github.com/leiflindholm/edk2/tree/rework/GicPcd-to-MdePkg
The diff of ArmGicV3Dxe.c looks messy, but I did only delete the if-side, de-indent the else-side, and move a variable declaration to the top of the function.
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.
Thanks @leiflindholm, let me update this PR according to your latest changes.
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.
Done.
91567fb
to
0a3c65b
Compare
MdePkg/MdePkg.dec
Outdated
@@ -2483,6 +2483,15 @@ | |||
# @Prompt Time-out for a request, internal | |||
gEfiMdePkgTokenSpaceGuid.PcdIpmiSerialRequestRetryInterval|60000|UINT32|0x00000055 | |||
|
|||
[PcdsFixedAtBuild.common.AARCH64, PcdsFixedAtBuild.common.ARM, PcdsDynamic.common.AARCH64, PcdsDynamic.common.ARM] |
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.
Syntax should be PcdsFixedAtBuild.AARCH64
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.
Whoops, right you are.
Travelling today with shaky internet connection - if someone else could fix this (drop all of the .common
from this line), that'd be great. If not, I'll do it when practical.
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.
".common" removed in MdePkg.dec.
0a3c65b
to
0b63da3
Compare
I think maybe in days of yore this Pcd was used for the parking protocol? There are no longer any users in edk2 nor edk2-platforms, so instead of moving this one to MdePkg with its companions, let's just drop it. Signed-off-by: Leif Lindholm <[email protected]>
The GICv2 compatibility mode in GICv3 was a migration feature, and we're we're well and truly migrated - so drop the support. Signed-off-by: Leif Lindholm <[email protected]>
In order to keep clean dependencies for UefiPayloadPkg, move the gic Pcds to MdePkg. Since this is very much a breaking change, update all the in-tree users with the new TokenSpaceId. Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Leif Lindholm <[email protected]>
0b63da3
to
1fe07a2
Compare
Description
As discussed in this topic: https://edk2.groups.io/g/devel/topic/patch_v5_2_6/102725178, move GIC PCDs from ArmPkg to MdePkg.
Update usage of GIC PCDs usage to adapt changes.
Cherry-pick from:
leiflindholm@ed2e0db
leiflindholm@87fe6ff
leiflindholm@91567fb