On Fri, 6 Dec 2019 at 12:12, Laszlo Ersek <ler...@redhat.com> 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.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().
>
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 <ler...@redhat.com>
>
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]
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51904): https://edk2.groups.io/g/devel/message/51904
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]
-=-=-=-=-=-=-=-=-=-=-=-
