[PATCH v2 0/5] Platform/Rpi: Various cleanups + DT booting

[PATCH v2 0/5] Platform/Rpi: Various cleanups + DT booting

[Jeremy Linton]

From: Jeremy Linton <lintonrjeremy@gmail.com>

This set is a few patches I've been collecting to fix minor issues I've seen
while debugging other problems, or just various things I think should probably
be changed. It also includes the patch to adjust the PCIe/XHCI dma-attributes
which fixes DT booting on recent Linux kernels as well as OpenBSD.

I've been running all of them in some form or another for a while and
generally nothing has broken because of them AFAIK. So its probably time to
start getting a few of them out of my private tree.

The first patch syncs the DT/PCIe translation to the way its being used in
UEFI, and removes the XHCI reset node. The second is just a compiler
warning. The third is mostly expanding the mailbox lock to cover the return
data.  Number 4 is an odd one because it just looks wrong, and I'm worried its
causing random bugs. The final is a corrected shutdown sequence that was
discussed months ago. It looks right. but didn't actually fix the data
persistence problems that resulted in the couple second reboot delay that is
currently in place.

v1->v2:
    Include the DT patch to sync the PCIe/XHCI values
        Now that it also fixes a problem with OpenBSD.
    Rework the error handling in the DisconnectAll code
        borrowed from the UEFI spec.
    Adjust some DEBUG_XXX values so they make more sense.
    Some comment/changelog grammar.	

Jeremy Linton (5):
  Platform/RaspberryPi: Always use non translating DMA in DT mode
  Platform/RaspberryPi: Fix vfr warning caused by two defaults
  Platform/RaspberryPi: Expand locking to cover return data
  Platform/RaspberryPi: Normal memory should not be marked as uncached
  Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot

 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c |   4 +-
 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr |   2 +-
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c       |  75 +++++++++++++++
 .../Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c        | 102 ++++++++++++---------
 Platform/RaspberryPi/Library/ResetLib/ResetLib.c   |  42 +++++++++
 5 files changed, 179 insertions(+), 46 deletions(-)

-- 
2.13.7

[PATCH v2 1/5] Platform/RaspberryPi: Always use non translating DMA in DT mode

[Jeremy Linton]

One of the many issues with the PCIe on this platform is its inbound
DMA is either constrained to the lower 3G, or on later SOC's a
translation may be used. That translation is problematic with some of
the OS's expected to boot on this platform. So, across the board a 3G
DMA limit is enforced during boot when in ACPI mode. This itself
causes problems because the later boards removed the SPI EEPROM used
by the onboard XHCI controller, instead favoring using a block of RAM
to load its firmware. Hence it is the lower level firmware's
responsibility via a mailbox call, to read the bridge
translation/configuration before telling the XHCI controller where it
can find its firmware.

Everything is great in ACPI land. Now it appears that Linux after
reprogramming the bridge to match the DT (when using a translation)
can't actually get the XHCI/quirk/reset to function. Apparently,
because the firmware only reads the bridge configuration the first
time its called(?), or the kernel reset sequence isn't correct. Worse,
with just the DMA ranges corrected, the XHCI/QUIRK itself then causes
the controller to start having what appear to be DMA issues.

Lets simplify the situation and make all DT's provided by this
firmware have a 3G DMA limit on the PCIe bus. Then remove the ability
for Linux/etc to trigger the quirk by remove the DT node attaching the
reset controller to the XHCI. The latter seems somewhat questionable,
since the DT/PCIe host bridge driver is doing what appears to be a
PERST which might then require a firmware reload, but at the
moment seems to work without.

The first part of this patch also appears to fix a problem with
OpenBSD which interprets the DT as describing how the firmware
has configured the device, and makes no attempt to reconfigure it.
Hence the newer SOC's implementing a translation fail to boot
since the DT being passed to the OS doesn't match the translation
the firmware has setup.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 75 ++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
index 0472d8ecf6..a4816d4295 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
@@ -334,6 +334,76 @@ CleanSimpleFramebuffer (
   return EFI_SUCCESS;
 }
 
+STATIC
+EFI_STATUS
+SyncPcie (
+  VOID
+  )
+{
+#if (RPI_MODEL == 4)
+  INTN          Node;
+  INTN          Retval;
+  UINT32        DmaRanges[7];
+
+  Node = fdt_path_offset (mFdtImage, "pcie0");
+  if (Node < 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'pcie0' alias\n", __FUNCTION__));
+    return EFI_NOT_FOUND;
+  }
+
+  // non translated 32-bit DMA window with a limit of 0xc0000000
+  DmaRanges[0] = cpu_to_fdt32 (0x02000000); 
+  DmaRanges[1] = cpu_to_fdt32 (0x00000000); 
+  DmaRanges[2] = cpu_to_fdt32 (0x00000000);
+  DmaRanges[3] = cpu_to_fdt32 (0x00000000);
+  DmaRanges[4] = cpu_to_fdt32 (0x00000000);
+  DmaRanges[5] = cpu_to_fdt32 (0x00000000);
+  DmaRanges[6] = cpu_to_fdt32 (0xc0000000);
+
+  DEBUG ((DEBUG_INFO, "%a: Updating PCIe dma-ranges\n",  __FUNCTION__));
+
+  /* 
+   * Match dma-ranges with the EDK2+ACPI setup we are using.  This works
+   * around a failure in Linux and OpenBSD to reset the PCIe/XHCI correctly
+   * when in DT mode.
+   */
+  Retval = fdt_setprop (mFdtImage, Node, "dma-ranges",
+                        DmaRanges,  sizeof DmaRanges);
+  if (Retval != 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to locate PCIe 'dma-ranges' property (%d)\n",
+      __FUNCTION__, Retval));
+    return EFI_NOT_FOUND;
+  }
+
+  /*
+   * Now that we are always running without DMA translation, and with a 3G
+   * limit, there shouldn't be a need to reset/reload the XHCI. The
+   * possible problem is that the PCIe root port is itself being reset (by
+   * Linux+DT). The RPi foundation claims this is needed as a pre-req to
+   * reloading the XHCI firmware, which also implies that a PCI fundamental
+   * reset should cause the XHCI itself to reset.  This isn't happening
+   * fully, otherwise reloading the firmware would be mandatory. As it is,
+   * recent kernels actually start to have problems following the XHCI
+   * reset notification mailbox!  Instead lets stop the kernel from
+   * triggering the mailbox by removing the node.
+   */
+
+  Node = fdt_path_offset (mFdtImage, "/scb/pcie@7d500000/pci@1,0");
+  if (Node < 0) {
+    // This can happen on CM4/etc which doesn't have an onboard XHCI
+    DEBUG ((DEBUG_INFO, "%a: failed to locate /scb/pcie@7d500000/pci@1/usb@1\n", __FUNCTION__));
+  } else {
+    Retval = fdt_del_node (mFdtImage, Node);
+    if (Retval != 0) {
+      DEBUG ((DEBUG_ERROR, "Failed to remove /scb/pcie@7d500000/pci@1/usb@1\n"));
+      return EFI_NOT_FOUND;
+    }
+  }
+
+#endif
+  return EFI_SUCCESS;
+}
+
 /**
   @param  ImageHandle   of the loaded driver
   @param  SystemTable   Pointer to the System Table
@@ -431,6 +501,11 @@ FdtDxeInitialize (
     Print (L"Failed to update USB compatible properties: %r\n", Status);
   }
 
+  SyncPcie ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to update PCIe address ranges: %r\n", Status);
+  }
+
   DEBUG ((DEBUG_INFO, "Installed devicetree at address %p\n", mFdtImage));
   Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
   if (EFI_ERROR (Status)) {
-- 
2.13.7

[PATCH v2 2/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults

[Jeremy Linton]

The build has been tossing a warning about having two defaults
for a while now, lets fix it.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
index 18b3ec726e..e8bf16312d 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
@@ -192,7 +192,7 @@ formset
             flags       = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
             option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_ACPI), value = SYSTEM_TABLE_MODE_ACPI, flags = 0;
             option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_BOTH), value = SYSTEM_TABLE_MODE_BOTH, flags = DEFAULT;
-            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags = DEFAULT;
+            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags =0;
         endoneof;
 
 #if (RPI_MODEL == 4)
-- 
2.13.7

[PATCH v2 3/5] Platform/RaspberryPi: Expand locking to cover return data

[Jeremy Linton]

It appears that the locking for many of the mailbox commands is
incorrect. All UEFI firmware calls to the RPi mailbox share a single
mDmaBuffer. That buffer is used to fill out the command passed to the
vc firmware, and record its response. The buffer is protected by
mMailboxLock, yet in many cases the mailbox response is copied from
the buffer after the lock has been released. This doesn't currently
appear to be causing any problems, but should be fixed anyway.

There are a couple other minor tweaks in this patch that are hard to
justify on their own, one is a bit of whitespace cleanup, and the
other is the addition of a debug message to print the returned clock
rate for the requested clock. This latter print would have immediatly
shown that the vc firmware was returning 0 as the emmc clock rate
rather than something reasonable.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
---
 .../Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c        | 102 ++++++++++++---------
 1 file changed, 59 insertions(+), 43 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index bf74148bbb..d38ffbc7d3 100644
--- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -203,7 +203,6 @@ RpiFirmwareSetPowerState (
 
   Status = MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
 
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) {
@@ -219,6 +218,7 @@ RpiFirmwareSetPowerState (
       __FUNCTION__, PowerState ? "en" : "dis", DeviceId));
     Status = EFI_DEVICE_ERROR;
   }
+  ReleaseSpinLock (&mMailboxLock);
 
   return Status;
 }
@@ -266,18 +266,20 @@ RpiFirmwareGetArmMemory (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Base = Cmd->TagBody.Base;
   *Size = Cmd->TagBody.Size;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -323,17 +325,18 @@ RpiFirmwareGetMacAddress (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   CopyMem (MacAddress, Cmd->TagBody.MacAddress, sizeof (Cmd->TagBody.MacAddress));
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -378,17 +381,17 @@ RpiFirmwareGetSerial (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Serial = Cmd->TagBody.Serial;
+  ReleaseSpinLock (&mMailboxLock);
   // Some platforms return 0 or 0x0000000010000000 for serial.
   // For those, try to use the MAC address.
   if ((*Serial == 0) || ((*Serial & 0xFFFFFFFF0FFFFFFFULL) == 0)) {
@@ -441,17 +444,18 @@ RpiFirmwareGetModel (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Model = Cmd->TagBody.Model;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -496,17 +500,18 @@ RpiFirmwareGetModelRevision (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Revision = Cmd->TagBody.Revision;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -538,17 +543,18 @@ RpiFirmwareGetFirmwareRevision (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Revision = Cmd->TagBody.Revision;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -831,18 +837,19 @@ RpiFirmwareGetFbSize (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Width = Cmd->TagBody.Width;
   *Height = Cmd->TagBody.Height;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -872,16 +879,18 @@ RpiFirmwareFreeFb (VOID)
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -935,19 +944,20 @@ RpiFirmwareAllocFb (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Pitch = Cmd->Pitch.Pitch;
   *FbBase = Cmd->AllocFb.AlignmentBase - BCM2836_DMA_DEVICE_OFFSET;
   *FbSize = Cmd->AllocFb.Size;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -999,13 +1009,12 @@ RpiFirmwareGetCommmandLine (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
@@ -1013,6 +1022,7 @@ RpiFirmwareGetCommmandLine (
   if (Cmd->TagHead.TagValueSize >= BufferSize &&
       Cmd->CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') {
     DEBUG ((DEBUG_ERROR, "%a: insufficient buffer size\n", __FUNCTION__));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_OUT_OF_RESOURCES;
   }
 
@@ -1026,6 +1036,7 @@ RpiFirmwareGetCommmandLine (
     CommandLine[Cmd->TagHead.TagValueSize] = '\0';
   }
 
+  ReleaseSpinLock (&mMailboxLock);
   return EFI_SUCCESS;
 }
 
@@ -1075,18 +1086,20 @@ RpiFirmwareSetClockRate (
   Cmd->TagBody.SkipTurbo      = SkipTurbo ? 1 : 0;
   Cmd->EndTag                 = 0;
 
+  DEBUG ((DEBUG_INFO, "%a: Request clock rate %X = %d\n", __FUNCTION__, ClockId, ClockRate));
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -1131,20 +1144,23 @@ RpiFirmwareGetClockRate (
   Cmd->TagHead.TagValueSize   = 0;
   Cmd->TagBody.ClockId        = ClockId;
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *ClockRate = Cmd->TagBody.ClockRate;
+  ReleaseSpinLock (&mMailboxLock);
+
+  DEBUG ((DEBUG_INFO, "%a: Get Clock Rate return: ClockRate=%d ClockId=%X\n", __FUNCTION__, *ClockRate, ClockId));
+
   return EFI_SUCCESS;
 }
 
@@ -1191,7 +1207,7 @@ RpiFirmwareGetMinClockRate (
 {
   return RpiFirmwareGetClockRate (ClockId, RPI_MBOX_GET_MIN_CLOCK_RATE, ClockRate);
 }
-
+
 #pragma pack()
 typedef struct {
   UINT32                    ClockId;
@@ -1236,16 +1252,17 @@ RpiFirmwareSetClockState (
 
   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));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -1297,16 +1314,15 @@ RpiFirmwareSetGpio (
 
   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));
   }
+  ReleaseSpinLock (&mMailboxLock);
 }
-
+
 STATIC
 VOID
 EFIAPI
@@ -1361,8 +1377,6 @@ RpiFirmwareNotifyXhciReset (
 
   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,
@@ -1370,6 +1384,8 @@ RpiFirmwareNotifyXhciReset (
       __FUNCTION__, Status, Cmd->BufferHead.Response));
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return Status;
 }
 
@@ -1424,8 +1440,6 @@ RpiFirmwareNotifyGpioGetCfg (
 
   *Polarity = Cmd->TagBody.Polarity;
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
@@ -1433,6 +1447,8 @@ RpiFirmwareNotifyGpioGetCfg (
       __FUNCTION__, Status, Cmd->BufferHead.Response));
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return Status;
 }
 
@@ -1471,8 +1487,8 @@ RpiFirmwareNotifyGpioSetCfg (
 
   Status = RpiFirmwareNotifyGpioGetCfg (Gpio, &Result);
   if (EFI_ERROR (Status)) {
-	  DEBUG ((DEBUG_ERROR, "%a: Failed to get GPIO polarity\n", __FUNCTION__));
-	  Result = 0; //default polarity
+      DEBUG ((DEBUG_ERROR, "%a: Failed to get GPIO polarity\n", __FUNCTION__));
+      Result = 0; //default polarity
   }
 
 
@@ -1488,7 +1504,7 @@ RpiFirmwareNotifyGpioSetCfg (
   Cmd->BufferHead.Response    = 0;
   Cmd->TagHead.TagId          = RPI_MBOX_SET_GPIO_CONFIG;
   Cmd->TagHead.TagSize        = sizeof (Cmd->TagBody);
-
+
   Cmd->TagBody.Gpio = 128 + Gpio;
   Cmd->TagBody.Direction = Direction;
   Cmd->TagBody.Polarity = Result;
@@ -1501,17 +1517,17 @@ RpiFirmwareNotifyGpioSetCfg (
 
   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));
   }
-
-  RpiFirmwareSetGpio (Gpio,!State);
-
+
+  ReleaseSpinLock (&mMailboxLock);
+
+  RpiFirmwareSetGpio (Gpio,!State);
+
 
   return Status;
 }
@@ -1540,7 +1556,7 @@ STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwareProtocol = {
   RPiFirmwareGetModelInstalledMB,
   RpiFirmwareNotifyXhciReset,
   RpiFirmwareGetCurrentClockState,
-  RpiFirmwareSetClockState,
+  RpiFirmwareSetClockState,
   RpiFirmwareNotifyGpioSetCfg
 };
 
-- 
2.13.7

[PATCH v2 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached

[Jeremy Linton]

The efi spec seems to indicate that the efi uncacheable attribute
should be mapped to device memory rather than normal-nc. This means
that the uefi mem attribute for the >3G ram doesn't match the remainder
of the RAM in the machine.

So, lets remove the uncacheable attribute to make it more consistent.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index 2ef7da67bd..415d99fadb 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -499,7 +499,7 @@ ApplyVariables (
 
     Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 3UL * BASE_1GB,
                     SystemMemorySizeBelow4GB - (3UL * SIZE_1GB),
-                    EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB);
+                    EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB);
     ASSERT_EFI_ERROR (Status);
     Status = gDS->SetMemorySpaceAttributes (3UL * BASE_1GB,
                     SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), EFI_MEMORY_WB);
@@ -511,7 +511,7 @@ ApplyVariables (
       //
       Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 4UL * BASE_1GB,
                       SystemMemorySize - (4UL * SIZE_1GB),
-                      EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB);
+                      EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB);
       ASSERT_EFI_ERROR (Status);
       Status = gDS->SetMemorySpaceAttributes (4UL * BASE_1GB,
                       SystemMemorySize - (4UL * SIZE_1GB), EFI_MEMORY_WB);
-- 
2.13.7

[PATCH v2 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot

[Jeremy Linton]

In theory we should be properly cleaning up all the device drivers before
hitting the big reset. The partition manager will issue flush commands to
attached disks as it goes down. This assures that devices running in WB mode,
which correctly handle flush/sync/etc commands, are persisted to physical
media before reset.

Without this, there are definitely cases where the relevant specifications
don't guarantee persistence of data in their buffers in the face of reset
conditions. We can't really do anything about the many devices that don't
honor persistence requests, but we can start here.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 42 ++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
index a70eee485d..2bcef8d4db 100644
--- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
+++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
@@ -19,11 +19,51 @@
 #include <Library/TimerLib.h>
 #include <Library/EfiResetSystemLib.h>
 #include <Library/ArmSmcLib.h>
+#include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
 #include <Library/UefiRuntimeLib.h>
 
 #include <IndustryStandard/ArmStdSmc.h>
 
+
+/**
+  Disconnect everything.
+  Modified from the UEFI 2.3 spec (May 2009 version)
+
+**/
+STATIC
+VOID
+DisconnectAll (
+  VOID
+  )
+{
+  EFI_STATUS Status;
+  UINTN HandleCount;
+  EFI_HANDLE *HandleBuffer;
+  UINTN HandleIndex;
+
+  /*
+   * Retrieve the list of all handles from the handle database
+   */
+  Status = gBS->LocateHandleBuffer (
+    AllHandles,
+    NULL,
+    NULL,
+    &HandleCount,
+    &HandleBuffer
+   );
+  if (EFI_ERROR (Status)) {
+      return;
+  }
+
+  for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) {
+    gBS->DisconnectController (HandleBuffer[HandleIndex], NULL, NULL);
+  }
+
+  gBS->FreePool(HandleBuffer);
+}
+
+
 /**
   Resets the entire platform.
 
@@ -53,6 +93,8 @@ LibResetSystem (
      */
     EfiEventGroupSignal (&gRaspberryPiEventResetGuid);
 
+    DisconnectAll ();
+
     Delay = PcdGet32 (PcdPlatformResetDelay);
     if (Delay != 0) {
       DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
-- 
2.13.7

Re: [PATCH v2 0/5] Platform/Rpi: Various cleanups + DT booting

[Ard Biesheuvel]

On Mon, 18 Oct 2021 at 22:51, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> From: Jeremy Linton <lintonrjeremy@gmail.com>
>
> This set is a few patches I've been collecting to fix minor issues I've seen
> while debugging other problems, or just various things I think should probably
> be changed. It also includes the patch to adjust the PCIe/XHCI dma-attributes
> which fixes DT booting on recent Linux kernels as well as OpenBSD.
>
> I've been running all of them in some form or another for a while and
> generally nothing has broken because of them AFAIK. So its probably time to
> start getting a few of them out of my private tree.
>
> The first patch syncs the DT/PCIe translation to the way its being used in
> UEFI, and removes the XHCI reset node. The second is just a compiler
> warning. The third is mostly expanding the mailbox lock to cover the return
> data.  Number 4 is an odd one because it just looks wrong, and I'm worried its
> causing random bugs. The final is a corrected shutdown sequence that was
> discussed months ago. It looks right. but didn't actually fix the data
> persistence problems that resulted in the couple second reboot delay that is
> currently in place.
>
> v1->v2:
>     Include the DT patch to sync the PCIe/XHCI values
>         Now that it also fixes a problem with OpenBSD.
>     Rework the error handling in the DisconnectAll code
>         borrowed from the UEFI spec.
>     Adjust some DEBUG_XXX values so they make more sense.
>     Some comment/changelog grammar.
>
> Jeremy Linton (5):
>   Platform/RaspberryPi: Always use non translating DMA in DT mode
>   Platform/RaspberryPi: Fix vfr warning caused by two defaults
>   Platform/RaspberryPi: Expand locking to cover return data
>   Platform/RaspberryPi: Normal memory should not be marked as uncached
>   Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot
>

Pushed as dc7fe2ac2b74..63d520f9431a

Thanks!



>  Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c |   4 +-
>  .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr |   2 +-
>  Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c       |  75 +++++++++++++++
>  .../Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c        | 102 ++++++++++++---------
>  Platform/RaspberryPi/Library/ResetLib/ResetLib.c   |  42 +++++++++
>  5 files changed, 179 insertions(+), 46 deletions(-)
>
> --
> 2.13.7
>