From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.31565.1590994797764127209 for ; Sun, 31 May 2020 23:59:58 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 692D755D; Sun, 31 May 2020 23:59:56 -0700 (PDT) Received: from [192.168.1.69] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 69AC53F52E; Sun, 31 May 2020 23:59:55 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds To: Andrei Warkentin , devel@edk2.groups.io Cc: leif@nuviainc.com, pete@akeo.ie, philmd@redhat.com References: <20200601032130.95634-1-andrey.warkentin@gmail.com> From: "Ard Biesheuvel" Message-ID: <3121e7a6-afb7-038e-4fcd-5a317d3194e9@arm.com> Date: Mon, 1 Jun 2020 08:59:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: <20200601032130.95634-1-andrey.warkentin@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Andrei, On 6/1/20 5:21 AM, Andrei Warkentin wrote: > Newer Pi 4 boards (such as 8GiB variant) no longer carry a SPI > EEPROM with the VLI805 (USB) controller firmware. So, ask the > VPU firmare to load the image it has into the controller > specified by BDF. > > This is benign on non-8GiB boards (reloading that firmware if > VPU fw is new enough or doing nothing on old VPU firmware). > > Tested on 4GB. Also have a positive test report for an 8GB board > from a forum member. > > Signed-off-by: Andrei Warkentin > --- > Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 12 ++- > Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.h | 22 +++++ > Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf | 5 +- > Platform/RaspberryPi/Drivers/ConfigDxe/XhciQuirk.c | 97 ++++++++++++++++++++ > Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 58 +++++++++++- > Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h | 3 + > Platform/RaspberryPi/Include/Protocol/RpiFirmware.h | 10 ++ > 7 files changed, 200 insertions(+), 7 deletions(-) > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > index ac1004fe..ad14eb3d 100644 > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > @@ -1,7 +1,7 @@ > /** @file > * > * Copyright (c) 2019 - 2020, ARM Limited. All rights reserved. > - * Copyright (c) 2018 - 2019, Andrei Warkentin > + * Copyright (c) 2018 - 2020, Andrei Warkentin > * > * SPDX-License-Identifier: BSD-2-Clause-Patent > * > @@ -11,21 +11,18 @@ > #include > #include > #include > -#include > #include > -#include > #include > #include > #include > #include > #include > #include > -#include > #include > #include > -#include > #include > #include "ConfigDxeFormSetGuid.h" > +#include "ConfigDxe.h" > > #define FREQ_1_MHZ 1000000 > > @@ -584,5 +581,10 @@ ConfigInitialize ( > NULL, &gEfiEndOfDxeEventGroupGuid, &EndOfDxeEvent); > ASSERT_EFI_ERROR (Status); > > + > + if (mModelFamily == 4) { > + RegisterXhciQuirkHandler (mFwProtocol); > + } > + > return EFI_SUCCESS; > } > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.h b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.h > new file mode 100644 > index 00000000..6e35fd95 > --- /dev/null > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.h > @@ -0,0 +1,22 @@ > +/** @file > + * > + * Copyright (c) 2020, Andrei Warkentin > + * > + * SPDX-License-Identifier: BSD-2-Clause-Patent > + * > + **/ > + > +#ifndef _CONFIG_DXE_H_ > +#define _CONFIG_DXE_H_ > + > +#include > +#include > +#include > +#include > +#include > + Please include only the headers you need to declare the prototypes below. > +VOID RegisterXhciQuirkHandler ( > + IN RASPBERRY_PI_FIRMWARE_PROTOCOL *FwProtocol > + ); > + > +#endif /* _CONFIG_DXE_H_ */ > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf > index f20f3bcc..b3ef2624 100644 > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf > @@ -3,7 +3,7 @@ > # Component description file for the RasbperryPi DXE platform config driver. > # > # Copyright (c) 2019 - 2020, ARM Limited. All rights reserved. > -# Copyright (c) 2018, Andrei Warkentin > +# Copyright (c) 2018 - 2020, Andrei Warkentin > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -25,6 +25,7 @@ > # > [Sources] > ConfigDxe.c > + XhciQuirk.c Keep this ordered > ConfigDxeFormSetGuid.h > ConfigDxeHii.vfr > ConfigDxeHii.uni > @@ -50,6 +51,7 @@ > HiiLib > NetLib > PcdLib > + UefiLib Keep ordered > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiRuntimeServicesTableLib > @@ -62,6 +64,7 @@ > gBcmGenetPlatformDeviceProtocolGuid ## PRODUCES > gRaspberryPiFirmwareProtocolGuid ## CONSUMES > gRaspberryPiConfigAppliedProtocolGuid ## PRODUCES > + gEfiPciIoProtocolGuid ## CONSUMES > Keep ordered > [FixedPcd] > gRaspberryPiTokenSpaceGuid.PcdCpuLowSpeedMHz > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/XhciQuirk.c b/Platform/RaspberryPi/Drivers/ConfigDxe/XhciQuirk.c > new file mode 100644 > index 00000000..93ab5a35 > --- /dev/null > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/XhciQuirk.c > @@ -0,0 +1,97 @@ > +/** @file > + * > + * Copyright (c) 2020, Andrei Warkentin > + * > + * SPDX-License-Identifier: BSD-2-Clause-Patent > + * > + **/ > + > +#include "ConfigDxe.h" > +#include > +#include > +#include > + > +#pragma pack(1) > +typedef struct { > + UINT8 ProgInterface; > + UINT8 SubClassCode; > + UINT8 BaseCode; > +} USB_CLASSC; > +#pragma pack() > + > +STATIC VOID *mPciIoNotificationRegistration = NULL; > + > +STATIC > +VOID This should have EFIAPI > +PciIoNotificationEvent ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ) > +{ > + EFI_STATUS Status; > + EFI_PCI_IO_PROTOCOL *PciIo; > + USB_CLASSC UsbClassCReg; > + UINTN SegmentNumber; > + UINTN BusNumber; > + UINTN DeviceNumber; > + UINTN FunctionNumber; > + RASPBERRY_PI_FIRMWARE_PROTOCOL *FwProtocol = Context; > + > + Status = gBS->LocateProtocol (&gEfiPciIoProtocolGuid, > + mPciIoNotificationRegistration, (VOID **)&PciIo); Indentation > + if (EFI_ERROR (Status)) { > + return; > + } > + > + Status = PciIo->Pci.Read ( > + PciIo, > + EfiPciIoWidthUint8, > + PCI_CLASSCODE_OFFSET, > + sizeof (USB_CLASSC) / sizeof (UINT8), > + &UsbClassCReg > + ); > + Indentation > + if (EFI_ERROR (Status)) { > + return; > + } > + > + // > + // Test whether the controller belongs to Xhci type > + // > + if ((UsbClassCReg.BaseCode != PCI_CLASS_SERIAL) || > + (UsbClassCReg.SubClassCode != PCI_CLASS_SERIAL_USB) || > + (UsbClassCReg.ProgInterface != PCI_IF_XHCI)) { > + return; > + } > + > + Status = PciIo->GetLocation (PciIo, &SegmentNumber, &BusNumber, > + &DeviceNumber, &FunctionNumber); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "%a: failed to get SBDF for xHCI controller: %r\n", > + __FUNCTION__, Status)); Need to return here. > + } > + > + DEBUG ((DEBUG_INFO, "xHCI found at %u:%u:%u:%u\n", > + SegmentNumber, BusNumber, DeviceNumber, FunctionNumber)); > + > + ASSERT (SegmentNumber == 0); > + Status = FwProtocol->NotifyXhciReset(BusNumber, DeviceNumber, FunctionNumber); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "%a: couldn't signal xHCI firmware load: %r\n", > + __FUNCTION__, Status)); > + } > +} > + > +VOID > +RegisterXhciQuirkHandler ( > + IN RASPBERRY_PI_FIRMWARE_PROTOCOL *FwProtocol > + ) > +{ > + EfiCreateProtocolNotifyEvent ( > + &gEfiPciIoProtocolGuid, > + TPL_NOTIFY, > + PciIoNotificationEvent, > + FwProtocol, > + &mPciIoNotificationRegistration > + ); > +} > diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > index 6c45cf47..22f75a52 100644 > --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > @@ -2,7 +2,7 @@ > * > * Copyright (c) 2020, Pete Batard > * Copyright (c) 2019, ARM Limited. All rights reserved. > - * Copyright (c) 2017-2018, Andrei Warkentin > + * Copyright (c) 2017-2020, Andrei Warkentin > * Copyright (c) 2016, Linaro, Ltd. All rights reserved. > * > * SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -1237,6 +1237,61 @@ RpiFirmwareSetLed ( > } > } > > +#pragma pack() > +typedef struct { > + UINT32 DeviceAddress; > +} RPI_FW_NOTIFY_XHCI_RESET_TAG; > + > +typedef struct { > + RPI_FW_BUFFER_HEAD BufferHead; > + RPI_FW_TAG_HEAD TagHead; > + RPI_FW_NOTIFY_XHCI_RESET_TAG TagBody; > + UINT32 EndTag; > +} RPI_FW_NOTIFY_XHCI_RESET_CMD; > +#pragma pack() > + > +STATIC > +EFI_STATUS EFIAPI > +RpiFirmwareNotifyXhciReset ( > + IN UINTN BusNumber, > + IN UINTN DeviceNumber, > + IN UINTN FunctionNumber > + ) > +{ > + RPI_FW_NOTIFY_XHCI_RESET_CMD *Cmd; > + EFI_STATUS Status; > + UINT32 Result; > + > + if (!AcquireSpinLockOrFail (&mMailboxLock)) { > + DEBUG ((DEBUG_ERROR, "%a: failed to acquire spinlock\n", __FUNCTION__)); > + return EFI_DEVICE_ERROR; > + } > + > + Cmd = mDmaBuffer; > + ZeroMem (Cmd, sizeof (*Cmd)); > + > + Cmd->BufferHead.BufferSize = sizeof (*Cmd); > + Cmd->BufferHead.Response = 0; > + Cmd->TagHead.TagId = RPI_MBOX_NOTIFY_XHCI_RESET; > + Cmd->TagHead.TagSize = sizeof (Cmd->TagBody); > + Cmd->TagHead.TagValueSize = 0; > + Cmd->TagBody.DeviceAddress = BusNumber << 20 | DeviceNumber << 15 | FunctionNumber << 12; > + Cmd->EndTag = 0; > + > + Status = MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_CHANNEL, &Result); > + So this pokes the XHCI controller via its config space? Is it safe to do that without raising the TPL? What happens if another config space access occurs concurrently? > + ReleaseSpinLock (&mMailboxLock); > + > + if (EFI_ERROR (Status) || > + Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) { > + DEBUG ((DEBUG_ERROR, > + "%a: mailbox transaction error: Status == %r, Response == 0x%x\n", > + __FUNCTION__, Status, Cmd->BufferHead.Response)); > + } > + > + return Status; > +} > + > STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwareProtocol = { > RpiFirmwareSetPowerState, > RpiFirmwareGetMacAddress, > @@ -1259,6 +1314,7 @@ STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwareProtocol = { > RpiFirmwareGetCpuName, > RpiFirmwareGetArmMemory, > RPiFirmwareGetModelInstalledMB, > + RpiFirmwareNotifyXhciReset > }; > > /** > diff --git a/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h b/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h > index 3328be58..71040689 100644 > --- a/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h > +++ b/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h > @@ -1,6 +1,7 @@ > /** @file > * > * Copyright (c) 2019-2020, Pete Batard > + * Copyright (c) 2017-2020, Andrei Warkentin > * Copyright (c) 2016, Linaro Limited. All rights reserved. > * > * SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -61,6 +62,8 @@ > #define RPI_MBOX_GET_MAX_CLOCK_RATE 0x00030004 > #define RPI_MBOX_GET_MIN_CLOCK_RATE 0x00030007 > > +#define RPI_MBOX_NOTIFY_XHCI_RESET 0x00030058 > + > #define RPI_MBOX_SET_CLOCK_RATE 0x00038002 > #define RPI_MBOX_SET_GPIO 0x00038041 > > diff --git a/Platform/RaspberryPi/Include/Protocol/RpiFirmware.h b/Platform/RaspberryPi/Include/Protocol/RpiFirmware.h > index 108becbd..56a8d15a 100644 > --- a/Platform/RaspberryPi/Include/Protocol/RpiFirmware.h > +++ b/Platform/RaspberryPi/Include/Protocol/RpiFirmware.h > @@ -1,6 +1,7 @@ > /** @file > * > * Copyright (c) 2019, ARM Limited. All rights reserved. > + * Copyright (c) 2017 - 2020, Andrei Warkentin > * Copyright (c) 2016, Linaro Limited. All rights reserved. > * > * SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -140,6 +141,14 @@ EFI_STATUS > UINT32 *Size > ); > > +typedef > +EFI_STATUS > +(EFIAPI *NOTIFY_XHCI_RESET) ( > + UINTN BusNumber, > + UINTN DeviceNumber, > + UINTN FunctionNumber > + ); > + > typedef struct { > SET_POWER_STATE SetPowerState; > GET_MAC_ADDRESS GetMacAddress; > @@ -162,6 +171,7 @@ typedef struct { > GET_CPU_NAME GetCpuName; > GET_ARM_MEM GetArmMem; > GET_MODEL_INSTALLED_MB GetModelInstalledMB; > + NOTIFY_XHCI_RESET NotifyXhciReset; > } RASPBERRY_PI_FIRMWARE_PROTOCOL; > > extern EFI_GUID gRaspberryPiFirmwareProtocolGuid; >