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

ArmPkg: Update Generic Watchdog PCD Type #6562

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

ashishsingha
Copy link
Contributor

@ashishsingha ashishsingha commented Dec 19, 2024

Description

Do not use the PCDs as only fixed when there is an option to use them as dynamic that can be set per SKU.

  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

SOCs with different addresses and INTR numbers governed via SKU were booted with this change.

Integration Instructions

N/A

@ashishsingha
Copy link
Contributor Author

@leiflindholm @ardbiesheuvel @samimujawar This PR can also act as a superset on top of the pending PR #6396. Please let me know if you have any concerns.

Copy link
Contributor

@samimujawar samimujawar left a comment

Choose a reason for hiding this comment

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

@ashishsingha this patch looks good to me.

I would be curious to know how you intend to use the PCDs as dynamic PCDs. Is there a library/module that will initialise the PCDs?

@ashishsingha
Copy link
Contributor Author

@samimujawar, the DSC file can have SKUs defined, and then you can have Dynamic and HII PCDs per SKU. Very early during boot, the SKU can be set, and PCDs for that SKU will be returned upon query. I see you have approved the change. Can you please see what is needed next to get it merged?

@samimujawar
Copy link
Contributor

@ashishsingha thanks for the explanation.
I will wait for other maintainers to respond as well.

@leiflindholm
Copy link
Member

@ashishsingha thanks for the explanation. I will wait for other maintainers to respond as well.

I'm all for it.
Ultimately, I would prefer a higher-level standardised discovery interface across all vendors rather than the platform-specific SKU swizzling, but both require this to move to non-fixed.

@ashishsingha
Copy link
Contributor Author

Thanks @leiflindholm. Who should I be requesting to merge the change now that we’ve received 2 approvals?

@samimujawar samimujawar added the push Auto push patch series in PR if all checks pass label Dec 23, 2024
Copy link

mergify bot commented Dec 23, 2024

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Do not use the PCDs as only fixed when there is an option
to use them as dynamic that can be set per SKU.

Signed-off-by: Ashish Singhal <[email protected]>
@mergify mergify bot merged commit c52dddf into tianocore:master Dec 23, 2024
126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants