On 12/06/19 12:02, Ard Biesheuvel wrote:
> Instead of connecting and thus enumerating the PCIe topology in a
> platform driver, just so that we can grab the PciIo protocol that
> belongs to the Marvell Yukon NIC and program its MAC address, rely
> on a protocol notification handler to do this whenever the core code
> decides to enumerate the PCIe.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
> Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 161
> ++++----------------
> Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf | 1 -
> 2 files changed, 30 insertions(+), 132 deletions(-)
>
> diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> index c0ad7ced2959..ebaf2aa134da 100644
> --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> @@ -32,39 +32,8 @@
> STATIC CONST EFI_GUID mJunoAcpiTableFile = { 0xa1dd808e, 0x1e95, 0x4399, {
> 0xab, 0xc0, 0x65, 0x3c, 0x82, 0xe8, 0x53, 0x0c } };
> #endif
>
> -typedef struct {
> - ACPI_HID_DEVICE_PATH AcpiDevicePath;
> - PCI_DEVICE_PATH PciDevicePath;
> - EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
> -} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
> -
> -STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
> - {
> - { ACPI_DEVICE_PATH,
> - ACPI_DP,
> - { (UINT8) (sizeof (ACPI_HID_DEVICE_PATH)),
> - (UINT8) ((sizeof (ACPI_HID_DEVICE_PATH)) >> 8) }
> - },
> - EISA_PNP_ID (0x0A03),
> - 0
> - },
> - {
> - { HARDWARE_DEVICE_PATH,
> - HW_PCI_DP,
> - { (UINT8) (sizeof (PCI_DEVICE_PATH)),
> - (UINT8) ((sizeof (PCI_DEVICE_PATH)) >> 8) }
> - },
> - 0,
> - 0
> - },
> - {
> - END_DEVICE_PATH_TYPE,
> - END_ENTIRE_DEVICE_PATH_SUBTYPE,
> - { END_DEVICE_PATH_LENGTH, 0 }
> - }
> -};
> -
> STATIC EFI_EVENT mAcpiRegistration = NULL;
> +STATIC EFI_EVENT mPciIoNotificationRegistration = NULL;
(1) Please use "VOID *" as the type of "mPciIoNotificationRegistration".
EFI_EVENT happens to work (because it is, regrettably, a typedef, per
spec, to "VOID *").
But semantically we need "VOID *" here -- please see the "Registration"
parameter of EFI_BOOT_SERVICES.RegisterProtocolNotify() and
EFI_BOOT_SERVICES.LocateProtocol().
>
> /**
> This function reads PCI ID of the controller.
> @@ -99,59 +68,6 @@ ReadMarvellYoukonPciId (
> return EFI_SUCCESS;
> }
>
> -/**
> - This function searches for Marvell Yukon NIC on the Juno
> - platform and returns PCI IO protocol handle for the controller.
> -
> - @param[out] PciIo PCI IO protocol handle
> -**/
> -STATIC
> -EFI_STATUS
> -GetMarvellYukonPciIoProtocol (
> - OUT EFI_PCI_IO_PROTOCOL **PciIo
> - )
> -{
> - UINTN HandleCount;
> - EFI_HANDLE *HandleBuffer;
> - UINTN HIndex;
> - EFI_STATUS Status;
> -
> - Status = gBS->LocateHandleBuffer (
> - ByProtocol,
> - &gEfiPciIoProtocolGuid,
> - NULL,
> - &HandleCount,
> - &HandleBuffer);
> - if (EFI_ERROR (Status)) {
> - return (Status);
> - }
> -
> - for (HIndex = 0; HIndex < HandleCount; ++HIndex) {
> - // If PciIo opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL, the
> CloseProtocol() is not required
> - Status = gBS->OpenProtocol (
> - HandleBuffer[HIndex],
> - &gEfiPciIoProtocolGuid,
> - (VOID **) PciIo,
> - NULL,
> - NULL,
> - EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> - if (EFI_ERROR (Status)) {
> - continue;
> - }
> -
> - Status = ReadMarvellYoukonPciId (*PciIo, JUNO_MARVELL_YUKON_ID);
> - if (EFI_ERROR (Status)) {
> - continue;
> - } else {
> - break;
> - }
> - }
> -
> - gBS->FreePool (HandleBuffer);
> -
> - return Status;
> -}
> -
> /**
> This function restore the original controller attributes
>
> @@ -326,18 +242,14 @@ WriteMacAddress (
> **/
> STATIC
> EFI_STATUS
> -ArmJunoSetNicMacAddress ()
> +ArmJunoSetNicMacAddress (
> + IN EFI_PCI_IO_PROTOCOL *PciIo
> + )
> {
> UINT64 OldPciAttr;
> - EFI_PCI_IO_PROTOCOL* PciIo;
> UINT32 PciRegBase;
> EFI_STATUS Status;
>
> - Status = GetMarvellYukonPciIoProtocol (&PciIo);
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> PciRegBase = 0;
> Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
> if (EFI_ERROR (Status)) {
> @@ -352,14 +264,8 @@ ArmJunoSetNicMacAddress ()
> }
>
> /**
> - Notification function of the event defined as belonging to the
> - EFI_END_OF_DXE_EVENT_GROUP_GUID event group that was created in
> - the entry point of the driver.
> -
> - This function is called when an event belonging to the
> - EFI_END_OF_DXE_EVENT_GROUP_GUID event group is signalled. Such an
> - event is signalled once at the end of the dispatching of all
> - drivers (end of the so called DXE phase).
> + This function is called when a gEfiPciIoProtocolGuid protocol instance is
> + registered in the protocol database.
>
> @param[in] Event Event declared in the entry point of the driver whose
> notification function is being invoked.
> @@ -367,33 +273,30 @@ ArmJunoSetNicMacAddress ()
> **/
> STATIC
> VOID
> -OnEndOfDxe (
> +PciIoNotificationEvent (
> IN EFI_EVENT Event,
> IN VOID *Context
> )
> {
> - EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath;
> - EFI_HANDLE Handle;
> - EFI_STATUS Status;
> + EFI_STATUS Status;
> + EFI_PCI_IO_PROTOCOL *PciIo;
>
> - //
> - // PCI Root Complex initialization
> - // At the end of the DXE phase, we should get all the driver dispatched.
> - // Force the PCI Root Complex to be initialized. It allows the OS to skip
> - // this step.
> - //
> - PciRootComplexDevicePath = (EFI_DEVICE_PATH_PROTOCOL*)
> &mPciRootComplexDevicePath;
> - Status = gBS->LocateDevicePath (&gEfiPciRootBridgeIoProtocolGuid,
> - &PciRootComplexDevicePath,
> - &Handle);
> + Status = gBS->LocateProtocol (&gEfiPciIoProtocolGuid,
> + mPciIoNotificationRegistration, (VOID **)&PciIo);
> + if (EFI_ERROR (Status)) {
> + return;
> + }
>
> - Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath,
> FALSE);
> - ASSERT_EFI_ERROR (Status);
> + Status = ReadMarvellYoukonPciId (PciIo, JUNO_MARVELL_YUKON_ID);
> + if (EFI_ERROR (Status)) {
> + return;
> + }
>
> - Status = ArmJunoSetNicMacAddress ();
> + Status = ArmJunoSetNicMacAddress (PciIo);
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_ERROR, "ArmJunoDxe: Failed to set Marvell Yukon NIC MAC
> address\n"));
> }
> + gBS->CloseEvent (Event);
> }
>
> EFI_STATUS
> @@ -408,7 +311,6 @@ ArmJunoEntryPoint (
> CHAR16 *TextDevicePath;
> UINTN TextDevicePathSize;
> UINT32 JunoRevision;
> - EFI_EVENT EndOfDxeEvent;
>
> //
> // Register the OHCI and EHCI controllers as non-coherent
> @@ -497,20 +399,17 @@ ArmJunoEntryPoint (
> PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
>
> //
> - // Create an event belonging to the "gEfiEndOfDxeEventGroupGuid" group.
> - // The "OnEndOfDxe()" function is declared as the call back function.
> - // It will be called at the end of the DXE phase when an event of the
> - // same group is signalled to inform about the end of the DXE phase.
> - // Install the INSTALL_FDT_PROTOCOL protocol.
> + // Create a protocol notification event handler on the PciIo protocol
> + // so we can set the MAC address on the Marvell Yukon as soon as it
> + // appears.
> //
> - Status = gBS->CreateEventEx (
> - EVT_NOTIFY_SIGNAL,
> - TPL_CALLBACK,
> - OnEndOfDxe,
> - NULL,
> - &gEfiEndOfDxeEventGroupGuid,
> - &EndOfDxeEvent
> - );
> + EfiCreateProtocolNotifyEvent (
> + &gEfiPciIoProtocolGuid,
> + TPL_NOTIFY,
(2) I generally prefer the lowest TPL that works, so I'd suggest
TPL_CALLBACK here.
With (1) updated, and (2) optionally updated (if you agree):
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks!
Laszlo
> + PciIoNotificationEvent,
> + NULL,
> + &mPciIoNotificationRegistration
> + );
>
> #ifndef DYNAMIC_TABLES_FRAMEWORK
> // Declare the related ACPI Tables
> diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> index 7c118d9c9c6b..d016967c3c37 100644
> --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> @@ -44,7 +44,6 @@ [LibraryClasses]
> UefiDriverEntryPoint
>
> [Guids]
> - gEfiEndOfDxeEventGroupGuid
> gEfiFileInfoGuid
>
> [Protocols]
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51902): https://edk2.groups.io/g/devel/message/51902
Mute This Topic: https://groups.io/mt/67467393/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
