public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds
@ 2020-06-01  3:21 Andrei Warkentin
  2020-06-01  6:59 ` Ard Biesheuvel
  0 siblings, 1 reply; 5+ messages in thread
From: Andrei Warkentin @ 2020-06-01  3:21 UTC (permalink / raw)
  To: devel; +Cc: ard.biesheuvel, leif, pete, philmd

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 <andrey.warkentin@gmail.com>
---
 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 <andrey.warkentin@gmail.com>
+ *  Copyright (c) 2018 - 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
  *
  *  SPDX-License-Identifier: BSD-2-Clause-Patent
  *
@@ -11,21 +11,18 @@
 #include <IndustryStandard/Bcm2711.h>
 #include <IndustryStandard/Bcm2836.h>
 #include <IndustryStandard/Bcm2836Gpio.h>
-#include <IndustryStandard/RpiMbox.h>
 #include <Library/AcpiLib.h>
-#include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/DxeServicesTableLib.h>
 #include <Library/GpioLib.h>
 #include <Library/HiiLib.h>
 #include <Library/IoLib.h>
 #include <Library/NetLib.h>
-#include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiRuntimeServicesTableLib.h>
 #include <Protocol/BcmGenetPlatformDevice.h>
-#include <Protocol/RpiFirmware.h>
 #include <ConfigVars.h>
 #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 <andrey.warkentin@gmail.com>
+ *
+ *  SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ **/
+
+#ifndef _CONFIG_DXE_H_
+#define _CONFIG_DXE_H_
+
+#include <Uefi.h>
+#include <Protocol/RpiFirmware.h>
+#include <Library/DebugLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <IndustryStandard/RpiMbox.h>
+
+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 <andrey.warkentin@gmail.com>
+#  Copyright (c) 2018 - 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -25,6 +25,7 @@
 #
 [Sources]
   ConfigDxe.c
+  XhciQuirk.c
   ConfigDxeFormSetGuid.h
   ConfigDxeHii.vfr
   ConfigDxeHii.uni
@@ -50,6 +51,7 @@
   HiiLib
   NetLib
   PcdLib
+  UefiLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiRuntimeServicesTableLib
@@ -62,6 +64,7 @@
   gBcmGenetPlatformDeviceProtocolGuid             ## PRODUCES
   gRaspberryPiFirmwareProtocolGuid                ## CONSUMES
   gRaspberryPiConfigAppliedProtocolGuid           ## PRODUCES
+  gEfiPciIoProtocolGuid                           ## CONSUMES
 
 [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 <andrey.warkentin@gmail.com>
+ *
+ *  SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ **/
+
+#include "ConfigDxe.h"
+#include <Protocol/PciIo.h>
+#include <Library/UefiLib.h>
+#include <IndustryStandard/Pci.h>
+
+#pragma pack(1)
+typedef struct {
+  UINT8 ProgInterface;
+  UINT8 SubClassCode;
+  UINT8 BaseCode;
+} USB_CLASSC;
+#pragma pack()
+
+STATIC VOID *mPciIoNotificationRegistration = NULL;
+
+STATIC
+VOID
+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);
+  if (EFI_ERROR (Status)) {
+    return;
+  }
+
+  Status = PciIo->Pci.Read (
+    PciIo,
+    EfiPciIoWidthUint8,
+    PCI_CLASSCODE_OFFSET,
+    sizeof (USB_CLASSC) / sizeof (UINT8),
+    &UsbClassCReg
+    );
+
+  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));
+  }
+
+  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 <pete@akeo.ie>
  *  Copyright (c) 2019, ARM Limited. All rights reserved.
- *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
+ *  Copyright (c) 2017-2020, Andrei Warkentin <andrey.warkentin@gmail.com>
  *  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
+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);
+
+  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 <pete@akeo.ie>
+ * Copyright (c) 2017-2020, Andrei Warkentin <andrey.warkentin@gmail.com>
  * 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 <andrey.warkentin@gmail.com>
  *  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;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds
  2020-06-01  3:21 [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds Andrei Warkentin
@ 2020-06-01  6:59 ` Ard Biesheuvel
  2020-06-01  9:37   ` [edk2-devel] " Andrei Warkentin
  0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2020-06-01  6:59 UTC (permalink / raw)
  To: Andrei Warkentin, devel; +Cc: leif, pete, philmd

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 <andrey.warkentin@gmail.com>
> ---
>   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 <andrey.warkentin@gmail.com>
> + *  Copyright (c) 2018 - 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
>    *
>    *  SPDX-License-Identifier: BSD-2-Clause-Patent
>    *
> @@ -11,21 +11,18 @@
>   #include <IndustryStandard/Bcm2711.h>
>   #include <IndustryStandard/Bcm2836.h>
>   #include <IndustryStandard/Bcm2836Gpio.h>
> -#include <IndustryStandard/RpiMbox.h>
>   #include <Library/AcpiLib.h>
> -#include <Library/DebugLib.h>
>   #include <Library/DevicePathLib.h>
>   #include <Library/DxeServicesTableLib.h>
>   #include <Library/GpioLib.h>
>   #include <Library/HiiLib.h>
>   #include <Library/IoLib.h>
>   #include <Library/NetLib.h>
> -#include <Library/UefiBootServicesTableLib.h>
>   #include <Library/UefiRuntimeServicesTableLib.h>
>   #include <Protocol/BcmGenetPlatformDevice.h>
> -#include <Protocol/RpiFirmware.h>
>   #include <ConfigVars.h>
>   #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 <andrey.warkentin@gmail.com>
> + *
> + *  SPDX-License-Identifier: BSD-2-Clause-Patent
> + *
> + **/
> +
> +#ifndef _CONFIG_DXE_H_
> +#define _CONFIG_DXE_H_
> +
> +#include <Uefi.h>
> +#include <Protocol/RpiFirmware.h>
> +#include <Library/DebugLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <IndustryStandard/RpiMbox.h>
> +

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 <andrey.warkentin@gmail.com>
> +#  Copyright (c) 2018 - 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
>   #
>   #  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 <andrey.warkentin@gmail.com>
> + *
> + *  SPDX-License-Identifier: BSD-2-Clause-Patent
> + *
> + **/
> +
> +#include "ConfigDxe.h"
> +#include <Protocol/PciIo.h>
> +#include <Library/UefiLib.h>
> +#include <IndustryStandard/Pci.h>
> +
> +#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 <pete@akeo.ie>
>    *  Copyright (c) 2019, ARM Limited. All rights reserved.
> - *  Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> + *  Copyright (c) 2017-2020, Andrei Warkentin <andrey.warkentin@gmail.com>
>    *  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 <pete@akeo.ie>
> + * Copyright (c) 2017-2020, Andrei Warkentin <andrey.warkentin@gmail.com>
>    * 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 <andrey.warkentin@gmail.com>
>    *  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;
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds
  2020-06-01  6:59 ` Ard Biesheuvel
@ 2020-06-01  9:37   ` Andrei Warkentin
  2020-06-01  9:44     ` Ard Biesheuvel
  0 siblings, 1 reply; 5+ messages in thread
From: Andrei Warkentin @ 2020-06-01  9:37 UTC (permalink / raw)
  To: Andrei Warkentin, devel@edk2.groups.io, ard.biesheuvel@arm.com
  Cc: leif@nuviainc.com, pete@akeo.ie, philmd@redhat.com

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

Hi Ard,

The xHCI controller is initialized with its microcode by the VPU firmware, if
I understood correctly, synchronously. When RPI_MBOX_NOTIFY_XHCI_RESET finishes, the XHCI controller is ready to go.

So this is not any more unsafe than any of the other mailbox calls. To be honest, I think RpiFirmwareDxe should be cleaned up to replace the useless spinlocks (there's no multiprocessing component) with a TPL manip (I checked SynchonizationLib and it doesn't touch the TPL)

Applying your other feedback now...

A
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ard Biesheuvel via groups.io <ard.biesheuvel=arm.com@groups.io>

> +
> +  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?

A

[-- Attachment #2: Type: text/html, Size: 2615 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds
  2020-06-01  9:37   ` [edk2-devel] " Andrei Warkentin
@ 2020-06-01  9:44     ` Ard Biesheuvel
  2020-06-01  9:48       ` Andrei Warkentin
  0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2020-06-01  9:44 UTC (permalink / raw)
  To: Andrei Warkentin, Andrei Warkentin, devel@edk2.groups.io
  Cc: leif@nuviainc.com, pete@akeo.ie, philmd@redhat.com

On 6/1/20 11:37 AM, Andrei Warkentin wrote:
> Hi Ard,
> 
> The xHCI controller is initialized with its microcode by the VPU 
> firmware, if
> I understood correctly, synchronously. When RPI_MBOX_NOTIFY_XHCI_RESET 
> finishes, the XHCI controller is ready to go.
> 

I suppose there are no other agents that may consume the config space 
during this time, right?

> So this is not any more unsafe than any of the other mailbox calls. To 
> be honest, I think RpiFirmwareDxe should be cleaned up to replace the 
> useless spinlocks (there's no multiprocessing component) with a TPL 
> manip (I checked SynchonizationLib and it doesn't touch the TPL)
> 

The spinlock protects from re-entrancy: if an event callback routine 
(such as the one you are adding for PCI I/O protocol registration) 
attempts to do a firmware call while one is already in progress, it will 
fail.

But perhaps it is indeed better to run at TPL_NOTIFY level - that way, 
the calls will be ordered one after the other instead of one being shot 
down.


> Applying your other feedback now...
> 
> A
> ------------------------------------------------------------------------
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ard 
> Biesheuvel via groups.io <ard.biesheuvel=arm.com@groups.io>
> 
>> +
>> +  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?
> 
> A


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds
  2020-06-01  9:44     ` Ard Biesheuvel
@ 2020-06-01  9:48       ` Andrei Warkentin
  0 siblings, 0 replies; 5+ messages in thread
From: Andrei Warkentin @ 2020-06-01  9:48 UTC (permalink / raw)
  To: Ard Biesheuvel, Andrei Warkentin, devel@edk2.groups.io
  Cc: leif@nuviainc.com, pete@akeo.ie, philmd@redhat.com

[-- Attachment #1: Type: text/plain, Size: 2460 bytes --]

During the time when the device handle with PciIo is published? Presumably only via the event callbacks (thus no). I see a similar approach done in ArmJunoDxe to populate MAC address into the PCI device.

I'm up for the RpiFirmwareDxe cleanup (as fas as moving to a model where the TPL is raised instead of using spinlock), but could that be done separately to this change?

A
________________________________
From: Ard Biesheuvel <ard.biesheuvel@arm.com>
Sent: Monday, June 1, 2020 4:44 AM
To: Andrei Warkentin <awarkentin@vmware.com>; Andrei Warkentin <andrey.warkentin@gmail.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: leif@nuviainc.com <leif@nuviainc.com>; pete@akeo.ie <pete@akeo.ie>; philmd@redhat.com <philmd@redhat.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds

On 6/1/20 11:37 AM, Andrei Warkentin wrote:
> Hi Ard,
>
> The xHCI controller is initialized with its microcode by the VPU
> firmware, if
> I understood correctly, synchronously. When RPI_MBOX_NOTIFY_XHCI_RESET
> finishes, the XHCI controller is ready to go.
>

I suppose there are no other agents that may consume the config space
during this time, right?

> So this is not any more unsafe than any of the other mailbox calls. To
> be honest, I think RpiFirmwareDxe should be cleaned up to replace the
> useless spinlocks (there's no multiprocessing component) with a TPL
> manip (I checked SynchonizationLib and it doesn't touch the TPL)
>

The spinlock protects from re-entrancy: if an event callback routine
(such as the one you are adding for PCI I/O protocol registration)
attempts to do a firmware call while one is already in progress, it will
fail.

But perhaps it is indeed better to run at TPL_NOTIFY level - that way,
the calls will be ordered one after the other instead of one being shot
down.


> Applying your other feedback now...
>
> A
> ------------------------------------------------------------------------
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ard
> Biesheuvel via groups.io <ard.biesheuvel=arm.com@groups.io>
>
>> +
>> +  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?
>
> A


[-- Attachment #2: Type: text/html, Size: 3951 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-06-01  9:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-01  3:21 [edk2-platforms][PATCH 1/1] Pi4: notify VPU to load xHCI firmware before XhciDxe binds Andrei Warkentin
2020-06-01  6:59 ` Ard Biesheuvel
2020-06-01  9:37   ` [edk2-devel] " Andrei Warkentin
2020-06-01  9:44     ` Ard Biesheuvel
2020-06-01  9:48       ` Andrei Warkentin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox