Re: [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Fri, 6 Dec 2019 at 12:12, Laszlo Ersek <lersek@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.biesheuvel@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 <lersek@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]
> >
>