From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by mx.groups.io with SMTP id smtpd.web10.2789.1575634571158215989 for ; Fri, 06 Dec 2019 04:16:11 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=yyHblWWF; spf=pass (domain: linaro.org, ip: 209.85.221.66, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f66.google.com with SMTP id g17so7560380wro.2 for ; Fri, 06 Dec 2019 04:16:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mNGXzqUX+271U4FwMgdgXUZ6hLccztC1LFBWJ7TgF38=; b=yyHblWWF7LeZ3ehsiAzajHh9lFdsNtE64F75FxW2CEB0uNF1uSIUpTlKrcuv3Z9pOY vP8vYScBjJJ6+/gvIa3QyMGeoSBkFQe5blabdgYbemrPoCsa58vxv+cE18CETAkaMZJ+ 78QX4C1eMxRTkUVVnt5zqBwjBDkvpxfzWSaLSjbD45qPsIyMGC5t8nzMk0+otmC8i86i LOXDypC41rwOYJRdFAM+sYLYT3KjyYhvJuYcfqPLBMoJnM51JxIttisfgPFXuyw1UImp U336lOEdnXzHJeUFlEXSeyUsNDsx8sBE/wQz/rYFCpN9tcbZ0GPPUcYQMMtcS2jMqZ2P KiMA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mNGXzqUX+271U4FwMgdgXUZ6hLccztC1LFBWJ7TgF38=; b=OXh+xTiIvrKsdmNemYEEfizue0VMngSV4cZIlk729Fez7qyTlHWNrBNxit1PkK6Txm 4pZfDLO28bmt1Go6q01DoocnrpuyFekW5P6SmQc50wq6ss0ODroL+3lW2p9mi4NdWVXI nN2cr1ovXItGyLfjm3ZiByAyOQ1y8l00PpmsZoXbJREzsfFUkXVi4TXnMbCLVr8vfCQj HM7wFonHhm22Z2E1gc8EKDAR69OhHG0TJoDdF0QUx/g9ROv+TRQvZTROegBI1ocK+PeF YJe8wOYowCKQPpNzdtFhLhO/jLmMPDWF8lGVKVeImrGIRUe8SbLoY9FqteXKr7FWDDV5 Ge1Q== X-Gm-Message-State: APjAAAX58sPudBkc38IV+tY6iLFKbaK6zH25XrNJITaBoViAZiM1Nsjd aR5EjailHCEmwN2dh2oZMi/TPbfPMz0rYXVH+ACINuxvJU2PdQ== X-Google-Smtp-Source: APXvYqwzIdKDkLufaGsp+3tPqzvjo/lveqqe2+hKmf+QLoL0FF9kgAVJKs8rwJTi27364xSqiJWBRNwKEw7Lv6+3Vrs= X-Received: by 2002:adf:cf0a:: with SMTP id o10mr14796849wrj.325.1575634569496; Fri, 06 Dec 2019 04:16:09 -0800 (PST) MIME-Version: 1.0 References: <20191206110219.32190-1-ard.biesheuvel@linaro.org> <20191206110219.32190-3-ard.biesheuvel@linaro.org> <91a5eae8-b646-2673-8eb2-4c0ab9980fb2@redhat.com> In-Reply-To: <91a5eae8-b646-2673-8eb2-4c0ab9980fb2@redhat.com> From: "Ard Biesheuvel" Date: Fri, 6 Dec 2019 12:16:06 +0000 Message-ID: Subject: Re: [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC To: Laszlo Ersek Cc: edk2-devel-groups-io , Sami Mujawar , Leif Lindholm Content-Type: text/plain; charset="UTF-8" On Fri, 6 Dec 2019 at 12:12, Laszlo Ersek 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 > > --- > > 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 > 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] > > >