From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.2762.1575634369446042563 for ; Fri, 06 Dec 2019 04:12:49 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RSVV/RrY; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575634368; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=H1ZAIobPK5iNc1aHVtdJCPwLaHng612/Qfu5pCyi/Iw=; b=RSVV/RrY+sVEAWTKwE4VRIwP3Rz/hb4RcwO5eV6IiDnyIRMv4RTIq02kHxe7+f3lA695RF qjGGeybc5C2kSPj6ZAfs5BjQLCp7lNERSbducwTB7iMp+VijcbHPHNlJqpFHMrCEkrG4WP xJ2mm9FxNnECXXIL+fXvoVo5Ld4rv4c= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-99-cEc7cyrCPsS7uv-jxBRCCQ-1; Fri, 06 Dec 2019 07:12:45 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D880D8024C2; Fri, 6 Dec 2019 12:12:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-235.ams2.redhat.com [10.36.116.235]) by smtp.corp.redhat.com (Postfix) with ESMTP id 89CA45D9C9; Fri, 6 Dec 2019 12:12:42 +0000 (UTC) Subject: Re: [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC To: Ard Biesheuvel , devel@edk2.groups.io Cc: Sami Mujawar , Leif Lindholm References: <20191206110219.32190-1-ard.biesheuvel@linaro.org> <20191206110219.32190-3-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <91a5eae8-b646-2673-8eb2-4c0ab9980fb2@redhat.com> Date: Fri, 6 Dec 2019 13:12:41 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191206110219.32190-3-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-MC-Unique: cEc7cyrCPsS7uv-jxBRCCQ-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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(). > > /** > 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 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] >