* [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration
@ 2019-12-06 11:02 Ard Biesheuvel
2019-12-06 11:02 ` [PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC Ard Biesheuvel
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2019-12-06 11:02 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Laszlo Ersek, Leif Lindholm
The platform DXE driver of the Juno platform enumerates the PCIe explicitly
by calling ConnectController on the PCIe root bridge, in order to ensure
that all PCI I/O protocols have been instantiated when we try to locate the
one describing the Marvel Yukon NIC, so that we can program its MAC address.
Taking control of core firmware behavior like this has already caused
confusion when reasoning about why and when PCIe option ROMs get dispatched,
and is likely to cause more confusion down the road.
So let's fix this, by triggering the MAC programming via a protocol notify
on the PCI I/O protocol.
This has been build tested only at the moment.
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Ard Biesheuvel (2):
Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC
Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC
.../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 163 ++++--------------
.../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf | 1 -
2 files changed, 31 insertions(+), 133 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC 2019-12-06 11:02 [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration Ard Biesheuvel @ 2019-12-06 11:02 ` Ard Biesheuvel 2019-12-06 12:16 ` Laszlo Ersek 2019-12-06 11:02 ` [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC Ard Biesheuvel 2019-12-06 11:12 ` [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration Leif Lindholm 2 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-12-06 11:02 UTC (permalink / raw) To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Laszlo Ersek, Leif Lindholm Generally, variables should only have external linkage if needed, so make mAcpiRegistration STATIC, given that static linkage suffices for it. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c index ea7591d70443..c0ad7ced2959 100644 --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c @@ -64,7 +64,7 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = { } }; -EFI_EVENT mAcpiRegistration = NULL; +STATIC EFI_EVENT mAcpiRegistration = NULL; /** This function reads PCI ID of the controller. -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC 2019-12-06 11:02 ` [PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC Ard Biesheuvel @ 2019-12-06 12:16 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2019-12-06 12:16 UTC (permalink / raw) To: Ard Biesheuvel, devel; +Cc: Sami Mujawar, Leif Lindholm On 12/06/19 12:02, Ard Biesheuvel wrote: > Generally, variables should only have external linkage if needed, > so make mAcpiRegistration STATIC, given that static linkage suffices > for it. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c > index ea7591d70443..c0ad7ced2959 100644 > --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c > +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c > @@ -64,7 +64,7 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = { > } > }; > > -EFI_EVENT mAcpiRegistration = NULL; > +STATIC EFI_EVENT mAcpiRegistration = NULL; > > /** > This function reads PCI ID of the controller. > The patch does what it says on the tin, so: Reviewed-by: Laszlo Ersek <lersek@redhat.com> But, this too should have type "VOID *"; so if you have capacity for an extra patch (or an extra change in this same patch), I'd recommend fixing up the type. Related commits (from edk2): - 10eec5aa9297 ("MdeModulePkg: stop abusing EFI_EVENT for protocol notify registration", 2019-10-09) - fcf8bdcd5313 ("SecurityPkg: stop abusing EFI_EVENT for protocol notify registration", 2019-10-09) Thanks! Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC 2019-12-06 11:02 [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration Ard Biesheuvel 2019-12-06 11:02 ` [PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC Ard Biesheuvel @ 2019-12-06 11:02 ` Ard Biesheuvel 2019-12-06 12:12 ` Laszlo Ersek 2019-12-06 11:12 ` [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration Leif Lindholm 2 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-12-06 11:02 UTC (permalink / raw) To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Laszlo Ersek, Leif Lindholm 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; /** 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, + 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] -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC 2019-12-06 11:02 ` [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC Ard Biesheuvel @ 2019-12-06 12:12 ` Laszlo Ersek 2019-12-06 12:16 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2019-12-06 12:12 UTC (permalink / raw) To: Ard Biesheuvel, devel; +Cc: Sami Mujawar, Leif Lindholm 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(). > > /** > 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 <lersek@redhat.com> 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] > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC 2019-12-06 12:12 ` Laszlo Ersek @ 2019-12-06 12:16 ` Ard Biesheuvel 0 siblings, 0 replies; 9+ messages in thread From: Ard Biesheuvel @ 2019-12-06 12:16 UTC (permalink / raw) To: Laszlo Ersek; +Cc: edk2-devel-groups-io, Sami Mujawar, Leif Lindholm 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] > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration 2019-12-06 11:02 [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration Ard Biesheuvel 2019-12-06 11:02 ` [PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC Ard Biesheuvel 2019-12-06 11:02 ` [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC Ard Biesheuvel @ 2019-12-06 11:12 ` Leif Lindholm 2019-12-06 11:41 ` Sami Mujawar 2 siblings, 1 reply; 9+ messages in thread From: Leif Lindholm @ 2019-12-06 11:12 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: devel, Sami Mujawar, Laszlo Ersek On Fri, Dec 06, 2019 at 11:02:17 +0000, Ard Biesheuvel wrote: > The platform DXE driver of the Juno platform enumerates the PCIe explicitly > by calling ConnectController on the PCIe root bridge, in order to ensure > that all PCI I/O protocols have been instantiated when we try to locate the > one describing the Marvel Yukon NIC, so that we can program its MAC address. > > Taking control of core firmware behavior like this has already caused > confusion when reasoning about why and when PCIe option ROMs get dispatched, > and is likely to cause more confusion down the road. > > So let's fix this, by triggering the MAC programming via a protocol notify > on the PCI I/O protocol. > > This has been build tested only at the moment. > > Cc: Sami Mujawar <sami.mujawar@arm.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif.lindholm@linaro.org> This looks sensible to me (and way more can-of-wormish than I would have expected) - so: Acked-by: Leif Lindholm <leif.lindholm@linaro.org> *but* we need tested-by from somewhere before merging 2/2. 1/2 could go in whenever. / Leif > Ard Biesheuvel (2): > Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC > Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC > > .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 163 ++++-------------- > .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf | 1 - > 2 files changed, 31 insertions(+), 133 deletions(-) > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration 2019-12-06 11:12 ` [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration Leif Lindholm @ 2019-12-06 11:41 ` Sami Mujawar 2019-12-06 14:01 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Sami Mujawar @ 2019-12-06 11:41 UTC (permalink / raw) To: Leif Lindholm, Ard Biesheuvel; +Cc: devel@edk2.groups.io, Laszlo Ersek Tested on Juno R2, the MAC address for Marvel Yukon NIC is programmed correctly. Tested-by: Sami Mujawar <sami.mujawar@arm.com> Regards, Sami Mujawar -----Original Message----- From: Leif Lindholm <leif.lindholm@linaro.org> Sent: 06 December 2019 11:13 AM To: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com>; Laszlo Ersek <lersek@redhat.com> Subject: Re: [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration On Fri, Dec 06, 2019 at 11:02:17 +0000, Ard Biesheuvel wrote: > The platform DXE driver of the Juno platform enumerates the PCIe > explicitly by calling ConnectController on the PCIe root bridge, in > order to ensure that all PCI I/O protocols have been instantiated when > we try to locate the one describing the Marvel Yukon NIC, so that we can program its MAC address. > > Taking control of core firmware behavior like this has already caused > confusion when reasoning about why and when PCIe option ROMs get > dispatched, and is likely to cause more confusion down the road. > > So let's fix this, by triggering the MAC programming via a protocol > notify on the PCI I/O protocol. > > This has been build tested only at the moment. > > Cc: Sami Mujawar <sami.mujawar@arm.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif.lindholm@linaro.org> This looks sensible to me (and way more can-of-wormish than I would have expected) - so: Acked-by: Leif Lindholm <leif.lindholm@linaro.org> *but* we need tested-by from somewhere before merging 2/2. 1/2 could go in whenever. / Leif > Ard Biesheuvel (2): > Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC > Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the > MAC > > .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 163 ++++-------------- > .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf | 1 - > 2 files changed, 31 insertions(+), 133 deletions(-) > > -- > 2.17.1 > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration 2019-12-06 11:41 ` Sami Mujawar @ 2019-12-06 14:01 ` Ard Biesheuvel 0 siblings, 0 replies; 9+ messages in thread From: Ard Biesheuvel @ 2019-12-06 14:01 UTC (permalink / raw) To: Sami Mujawar; +Cc: Leif Lindholm, devel@edk2.groups.io, Laszlo Ersek On Fri, 6 Dec 2019 at 11:41, Sami Mujawar <Sami.Mujawar@arm.com> wrote: > > Tested on Juno R2, the MAC address for Marvel Yukon NIC is programmed correctly. > > Tested-by: Sami Mujawar <sami.mujawar@arm.com> > Thanks all Pushed as 8b72f720d53e..1e4952d63b63 (with two instances of EFI_EVENT converted to VOID*) ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-12-06 14:01 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-06 11:02 [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration Ard Biesheuvel 2019-12-06 11:02 ` [PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC Ard Biesheuvel 2019-12-06 12:16 ` Laszlo Ersek 2019-12-06 11:02 ` [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC Ard Biesheuvel 2019-12-06 12:12 ` Laszlo Ersek 2019-12-06 12:16 ` Ard Biesheuvel 2019-12-06 11:12 ` [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration Leif Lindholm 2019-12-06 11:41 ` Sami Mujawar 2019-12-06 14:01 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox