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

MdePkg, MdeModulePkg, ShellPkg: Drop efi 1.10 usb hc protocol #10761

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

leiflindholm
Copy link
Member

@leiflindholm leiflindholm commented Feb 13, 2025

Description

This PR deletes the EFI_USB_HC_PROTOCOL, as discussed in https://edk2.groups.io/g/devel/message/121115.

As stated there, EFI_USB_HC_PROTOCOL existed in the pre-public EFI 1.10 specification,
but never made it into a UEFI release.

This is the sledgehammer approach - if it turns out there are real-life users of the
protocol, we may need to do something with a bit more finesse. Or this may be an
opportunity to nudge them to rewrite code to the protocol covered by a specification.

  • Breaking change?
    • This change deletes the definitions and implementations of EFI_USB_HC_PROTOCOL,
      and deletes code accommodating it in UsbBusDxe, UhciDxe, and ShellPkg.
    • The only user of this protocol I am aware of is the Ohci Dxe driver in QuarkSocPkg
      in edk2-platforms.

How This Was Tested

Build tested only.

This needs to be tested on out-of-tree platforms, so if the approach is deemed acceptable, I would request merging shortly after stable tag is made. As always, reverts are cheap, but I
think getting data on whether the interface is used at all would be valuable.

Integration Instructions

N/A

EFI_USB_HC_PROTOCOL was never defined in a released UEFI specification,
so drop use of it in UsbBusDxe in preparation of deleting the definitions
based on something allegedly supported in confidential EFI 1.10.

Signed-off-by: Leif Lindholm <leif.lindholm@oss.qualcomm.com>
Protocol/UsbHostController.h described an ancient protocol never part of
a public specification ... and was in fact not needed. So drop the
reference.

Also drop some text from a function documentation header incorrectly
implying use of the UsbHcProtocol.

Signed-off-by: Leif Lindholm <leif.lindholm@oss.qualcomm.com>
EFI_USB_HC_PROTOCOL was never defined in a released UEFI specification,
so drop use of it in ShellPkg in preparation of deleting the definitions
based on something allegedly supported in confidential EFI 1.10.

Signed-off-by: Leif Lindholm <leif.lindholm@oss.qualcomm.com>
Signed-off-by: Leif Lindholm <leif.lindholm@oss.qualcomm.com>
@github-actions github-actions bot added the impact:breaking-change This change breaks existing APIs impacting build or boot. label Feb 13, 2025
@pierregondois
Copy link
Contributor

It seems the platform Silicon/Intel/QuarkSocPkg/QuarkSocPkg.dsc uses this protocol in Silicon/Intel/QuarkSocPkg/QuarkSouthCluster/Usb/Ohci/Dxe/Ohci.c. It also seems that the protocol could be replaced by the gEfiUsb2HcProtocolGuid protocol without too many modifications.
However this platform's build is failing currently.

@leiflindholm
Copy link
Member Author

leiflindholm commented Feb 18, 2025

It seems the platform Silicon/Intel/QuarkSocPkg/QuarkSocPkg.dsc uses this protocol in Silicon/Intel/QuarkSocPkg/QuarkSouthCluster/Usb/Ohci/Dxe/Ohci.c. It also seems that the protocol could be replaced by the gEfiUsb2HcProtocolGuid protocol without too many modifications. However this platform's build is failing currently.

I did point this out in the PR description.
This is a fairly old platform, so I'd say it's worth considering whether it should be archived from edk2-platforms. Obviously updating the driver would be fine too, but I'm not going to :)

What I'm more interested in is whether there are tons of non-public platforms using this not-even-legacy protocol.

@leiflindholm
Copy link
Member Author

On that note, do the other QuarkPlatformPkg maintainers have any comments?
@nate-desimone @SaiChaganty.

@nate-desimone
Copy link
Member

To my knowledge, most x86 SoCs expose their USB host controllers as PCIe Root Complex Integrated Endpoints that implement the XHCI standard. For that reason, the standard XhciDxe from MdeModulePkg works out-of-box without any SoC specific programming.

As far as SoCs that need a bespoke USB host controller driver, that seems to be more common in the embedded space to me. Quark is an interesting exception as it is an x86 SoC designed for the embedded space. I think you will find more of these type of drivers in the ARM ecosystem.

@ardbiesheuvel
Copy link
Member

As far as SoCs that need a bespoke USB host controller driver, that seems to be more common in the embedded space to me. Quark is an interesting exception as it is an x86 SoC designed for the embedded space. I think you will find more of these type of drivers in the ARM ecosystem.

AFAIK, the first ever ARM prototype to run EDK2 was the BeagleBoard, which implements a EHCI high speed controller that relies on transaction translation to support FS/LS support, and so it does not rely on UhciDxe, only on EhciDxe.

I think it is safe to say that we can drop support for EFI_USB_HC_PROTOCOL once we're willing to retire QuarkSoc from edk2-platforms.

@mdkinney
Copy link
Member

The Quark platform can be dropped from edk2-platforms. Please move forward with these changes.

*MaxSpeed = EFI_USB_SPEED_FULL;
*Is64BitCapable = (UINT8)FALSE;
if (UsbBus->Usb2Hc == NULL) {
return EFI_INVALID_PARAMETER;
Copy link
Member

Choose a reason for hiding this comment

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

Can we drop these NULL checks? This code should never be reached if the OpenProtocol() call that populated the Usb2Hc field failed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, good point. It wasn't a "has this been initialized" test, it was a "is this protocol version 1 or 2" test. Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

(will do this when back from holiday on Monday)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:breaking-change This change breaks existing APIs impacting build or boot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants