On Fri, 6 Dec 2019 at 12:12, Laszlo Ersek <lersek@...> wrote:

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.biesheuvel@...>
---
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().

OK, thanks for spotting that.

/**
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.

I'd prefer to keep this as TPL_NOTIFY, given that we are attaching to
a PCI device, enabling it, poking some values into a BAR window and
disabling it again.

With (1) updated, and (2) optionally updated (if you agree):

Reviewed-by: Laszlo Ersek <lersek@...>

Thanks,

+ 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]