[PATCH 0/5] Platform/Rpi: Various cleanups

[PATCH 0/5] Platform/Rpi: Various cleanups

[Jeremy Linton]

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. Generally I don't think they fix anything that is currently user
visible, although its quite possible some mysterious failures go away.

I've been running all of them in some form or another for a few months 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 is just a compiler warning. The second is mostly expanding the
mailbox lock to cover the return data. The third is an update to make the
hopefully merged soon CM4 quirk actually work with the patches currently on
LKML. 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.

Jeremy Linton (5):
  Platform/RaspberryPi: Fix vfr warning caused by two defaults
  Platform/RaspberryPi: Expand locking to cover return data
  Platform/RaspberryPi: Update Linux quirk name
  Platform/RaspberryPi: Normal memory should not be marked as uncached
  Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot

 Platform/RaspberryPi/AcpiTables/Pci.asl            |   2 +-
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c |   4 +-
 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr |   2 +-
 .../Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c        | 102 ++++++++++++---------
 Platform/RaspberryPi/Library/ResetLib/ResetLib.c   |  44 +++++++++
 5 files changed, 107 insertions(+), 47 deletions(-)

-- 
2.13.7

[PATCH 1/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>
---
 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 2/5] Platform/RaspberryPi: Expand locking to cover return data

[Jeremy Linton]

While debugging problems with the GET/SET_CLOCK mailbox calls it appeared
that the locking in most of the mailbox commands isn't perfectly
correct. All UEFI firmware calls to the RPi mailbox share a single
mDmaBuffer which 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 result of the request is copied from the buffer after the
lock has been released. This doesn't currently appear to be causing any
problems, but should probably 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>
---
 .../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..29719aa5ec 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_ERROR, "%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_ERROR, "%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 3/5] Platform/RaspberryPi: Update Linux quirk name

[Jeremy Linton]

During review/merge of the linux ecam quirk, some logic
was added to require the quirk name to be exactly 6
characters, matching the MADT field its overriding.

As such, the rpi quirk here needed to be shorted by
a character to avoid confusion.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 Platform/RaspberryPi/AcpiTables/Pci.asl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/AcpiTables/Pci.asl b/Platform/RaspberryPi/AcpiTables/Pci.asl
index ee37b7a21e..e5fe755923 100644
--- a/Platform/RaspberryPi/AcpiTables/Pci.asl
+++ b/Platform/RaspberryPi/AcpiTables/Pci.asl
@@ -65,7 +65,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4PCIE", 2)
       Name (_DSD, Package () {
         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
           Package () {
-            Package () { "linux-ecam-quirk-id", "bcm2711" },
+            Package () { "linux-ecam-quirk-id", "bc2711" },
           }
       })
 
-- 
2.13.7

[PATCH 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 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
pulling the big switch. Particularly the partition mgr will issue
flush commands to attached disks as it goes down. This assures that
devices running in WB mode (that correctly handle flush/sync/etc) commands
are persisted to physical media before we hit reset.

Without this, there are definitly 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 persistance requests but we can start here.

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

diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
index a70eee485d..036f619cb5 100644
--- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
+++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
@@ -19,11 +19,54 @@
 #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)
+
+  @retval EFI_SUCCESS     The operation was successful.
+
+**/
+EFI_STATUS
+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)) {
+    for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) {
+      Status = gBS->DisconnectController (
+        HandleBuffer[HandleIndex],
+        NULL,
+        NULL
+       );
+    }
+    gBS->FreePool(HandleBuffer);
+  }
+  return (EFI_SUCCESS);
+}
+
+
 /**
   Resets the entire platform.
 
@@ -57,6 +100,7 @@ LibResetSystem (
     if (Delay != 0) {
       DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
               Delay / 1000000, (Delay % 1000000) / 100000));
+      DisconnectAll ();
       MicroSecondDelay (Delay);
     }
   }
-- 
2.13.7

Re: [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults

[Andrei Warkentin]

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

Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
________________________________
From: Jeremy Linton <jeremy.linton@arm.com>
Sent: Friday, October 1, 2021 7:52 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
Subject: [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults

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


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

Re: [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name

[Andrei Warkentin]

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

Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
________________________________
From: Jeremy Linton <jeremy.linton@arm.com>
Sent: Friday, October 1, 2021 7:52 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
Subject: [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name

During review/merge of the linux ecam quirk, some logic
was added to require the quirk name to be exactly 6
characters, matching the MADT field its overriding.

As such, the rpi quirk here needed to be shorted by
a character to avoid confusion.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 Platform/RaspberryPi/AcpiTables/Pci.asl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/AcpiTables/Pci.asl b/Platform/RaspberryPi/AcpiTables/Pci.asl
index ee37b7a21e..e5fe755923 100644
--- a/Platform/RaspberryPi/AcpiTables/Pci.asl
+++ b/Platform/RaspberryPi/AcpiTables/Pci.asl
@@ -65,7 +65,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4PCIE", 2)
       Name (_DSD, Package () {
         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
           Package () {
-            Package () { "linux-ecam-quirk-id", "bcm2711" },
+            Package () { "linux-ecam-quirk-id", "bc2711" },
           }
       })

--
2.13.7


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

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

[Andrei Warkentin]

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

I may have misunderstood the flags as being valid ways of mapping the added range. Should we also then take out WC and WT?
________________________________
From: Jeremy Linton <jeremy.linton@arm.com>
Sent: Friday, October 1, 2021 7:52 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
Subject: [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached

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


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

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

[Andrei Warkentin]

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

Seems smart to do.

Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
________________________________
From: Jeremy Linton <jeremy.linton@arm.com>
Sent: Friday, October 1, 2021 7:52 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
Subject: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot

In theory we should be properly cleaning up all the device drivers before
pulling the big switch. Particularly the partition mgr will issue
flush commands to attached disks as it goes down. This assures that
devices running in WB mode (that correctly handle flush/sync/etc) commands
are persisted to physical media before we hit reset.

Without this, there are definitly 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 persistance requests but we can start here.

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

diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
index a70eee485d..036f619cb5 100644
--- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
+++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
@@ -19,11 +19,54 @@
 #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)
+
+  @retval EFI_SUCCESS     The operation was successful.
+
+**/
+EFI_STATUS
+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)) {
+    for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) {
+      Status = gBS->DisconnectController (
+        HandleBuffer[HandleIndex],
+        NULL,
+        NULL
+       );
+    }
+    gBS->FreePool(HandleBuffer);
+  }
+  return (EFI_SUCCESS);
+}
+
+
 /**
   Resets the entire platform.

@@ -57,6 +100,7 @@ LibResetSystem (
     if (Delay != 0) {
       DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
               Delay / 1000000, (Delay % 1000000) / 100000));
+      DisconnectAll ();
       MicroSecondDelay (Delay);
     }
   }
--
2.13.7


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

Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data

[Andrei Warkentin]

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

Yikes - that's embarrassing. Thanks for the fix.

Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
________________________________
From: Jeremy Linton <jeremy.linton@arm.com>
Sent: Friday, October 1, 2021 7:52 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
Subject: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data

While debugging problems with the GET/SET_CLOCK mailbox calls it appeared
that the locking in most of the mailbox commands isn't perfectly
correct. All UEFI firmware calls to the RPi mailbox share a single
mDmaBuffer which 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 result of the request is copied from the buffer after the
lock has been released. This doesn't currently appear to be causing any
problems, but should probably 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>
---
 .../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..29719aa5ec 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_ERROR, "%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_ERROR, "%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


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

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

[Ard Biesheuvel]

On Sat, 2 Oct 2021 at 03:14, Andrei Warkentin <awarkentin@vmware.com> wrote:
>
> I may have misunderstood the flags as being valid ways of mapping the added range. Should we also then take out WC and WT?

No, you understood correctly. The problem is that normal memory ceases
to behave like normal memory when you map it UC (ie., unaligned
accesses and DC ZVA instructions are no longer allowed) so it should
be omitted. We use it in the memory map for things like the runtime
MMIO mappings for NOR flash and RTC.

WC and WT are reasonable for bare metal, but note that we omit those
as well for ArmVirtPkg, as using those breaks coherency, which means
the hypervisor/virtualization host's view of the guest's memory goes
out of sync.


> ________________________________
> From: Jeremy Linton <jeremy.linton@arm.com>
> Sent: Friday, October 1, 2021 7:52 PM
> To: devel@edk2.groups.io <devel@edk2.groups.io>
> Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
> Subject: [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached
>
> 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
>

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

[Ard Biesheuvel]

On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> In theory we should be properly cleaning up all the device drivers before
> pulling the big switch. Particularly the partition mgr will issue
> flush commands to attached disks as it goes down. This assures that
> devices running in WB mode (that correctly handle flush/sync/etc) commands
> are persisted to physical media before we hit reset.
>
> Without this, there are definitly 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 persistance requests but we can start here.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 ++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> index a70eee485d..036f619cb5 100644
> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> @@ -19,11 +19,54 @@
>  #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)
> +
> +  @retval EFI_SUCCESS     The operation was successful.
> +
> +**/

STATIC

> +EFI_STATUS
> +DisconnectAll(

Space before (

> +  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)) {

I understand that this code is copy/pasted but I'd still prefer to
avoid the 'success handling' anti pattern here.

if (EFI_ERROR (Status)) {
  return Status;
}

> +    for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) {
> +      Status = gBS->DisconnectController (
> +        HandleBuffer[HandleIndex],
> +        NULL,
> +        NULL
> +       );
> +    }
> +    gBS->FreePool(HandleBuffer);
> +  }
> +  return (EFI_SUCCESS);

No need for ()

> +}
> +
> +
>  /**
>    Resets the entire platform.
>
> @@ -57,6 +100,7 @@ LibResetSystem (
>      if (Delay != 0) {
>        DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
>                Delay / 1000000, (Delay % 1000000) / 100000));
> +      DisconnectAll ();

Capture Status here and ASSERT_EFI_ERROR() ??

Maybe it is overkill, and maybe DisconnectController() fails
spuriously, so I am not entirely sure, but adding a local function
that returns a value and then ignore it seems slightly sloppy to me.

>        MicroSecondDelay (Delay);
>      }
>    }
> --
> 2.13.7
>

Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data

[Ard Biesheuvel]

On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> While debugging problems with the GET/SET_CLOCK mailbox calls it appeared
> that the locking in most of the mailbox commands isn't perfectly
> correct. All UEFI firmware calls to the RPi mailbox share a single
> mDmaBuffer which 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 result of the request is copied from the buffer after the
> lock has been released. This doesn't currently appear to be causing any
> problems, but should probably 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>

This fails to apply for me - can you rebase onto the last
edk2-platform master please?

> ---
>  .../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..29719aa5ec 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_ERROR, "%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_ERROR, "%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
>

Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data

[Jeremy Linton]

Hi,

On 10/5/21 5:12 AM, Ard Biesheuvel wrote:
> On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote:
>>
>> While debugging problems with the GET/SET_CLOCK mailbox calls it appeared
>> that the locking in most of the mailbox commands isn't perfectly
>> correct. All UEFI firmware calls to the RPi mailbox share a single
>> mDmaBuffer which 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 result of the request is copied from the buffer after the
>> lock has been released. This doesn't currently appear to be causing any
>> problems, but should probably 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>
> 
> This fails to apply for me - can you rebase onto the last
> edk2-platform master please?

Sure. Is it whitespace, or general conflicts? I noticed that groups.io's 
web UI looked like I had MIME linebreaks sprinked through my patch 
despite specifying 8bit ASCII. I flirted with 7-bit ASCII with some of 
the previous patches, but its pretty clear that the arm foss server is 
mangling what it thinks are MIME emails and putting cr/lf's changes its 
behavior a bit.


Let me try and force the 7bit again, which git rejects if it finds utf 
in the files (and a few of them had it in the past <sigh>).


> 
>> ---
>>   .../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..29719aa5ec 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_ERROR, "%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_ERROR, "%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
>>

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

[Jeremy Linton]

Hi,

On 10/5/21 5:11 AM, Ard Biesheuvel wrote:
> On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote:
>>
>> In theory we should be properly cleaning up all the device drivers before
>> pulling the big switch. Particularly the partition mgr will issue
>> flush commands to attached disks as it goes down. This assures that
>> devices running in WB mode (that correctly handle flush/sync/etc) commands
>> are persisted to physical media before we hit reset.
>>
>> Without this, there are definitly 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 persistance requests but we can start here.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 ++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
>> index a70eee485d..036f619cb5 100644
>> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
>> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
>> @@ -19,11 +19,54 @@
>>   #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)
>> +
>> +  @retval EFI_SUCCESS     The operation was successful.
>> +
>> +**/
> 
> STATIC
> 
>> +EFI_STATUS
>> +DisconnectAll(
> 
> Space before (
> 
>> +  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)) {
> 
> I understand that this code is copy/pasted but I'd still prefer to
> avoid the 'success handling' anti pattern here.

Sure.

> 
> if (EFI_ERROR (Status)) {
>    return Status;
> }
> 
>> +    for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) {
>> +      Status = gBS->DisconnectController (
>> +        HandleBuffer[HandleIndex],
>> +        NULL,
>> +        NULL
>> +       );
>> +    }
>> +    gBS->FreePool(HandleBuffer);
>> +  }
>> +  return (EFI_SUCCESS);
> 
> No need for ()

Yup

> 
>> +}
>> +
>> +
>>   /**
>>     Resets the entire platform.
>>
>> @@ -57,6 +100,7 @@ LibResetSystem (
>>       if (Delay != 0) {
>>         DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
>>                 Delay / 1000000, (Delay % 1000000) / 100000));
>> +      DisconnectAll ();
> 
> Capture Status here and ASSERT_EFI_ERROR() ??
> 
> Maybe it is overkill, and maybe DisconnectController() fails
> spuriously, so I am not entirely sure, but adding a local function
> that returns a value and then ignore it seems slightly sloppy to me.

Which makes the above bits about failure returns sorta redundant as I 
should probably just make DisconnectAll() void. There isn't really 
anything to do with a failed return other than print a message and 
ignore it.


> 
>>         MicroSecondDelay (Delay);
>>       }
>>     }
>> --
>> 2.13.7
>>

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

[Ard Biesheuvel]

On Tue, 5 Oct 2021 at 23:25, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
> On 10/5/21 5:11 AM, Ard Biesheuvel wrote:
> > On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote:
> >>
> >> In theory we should be properly cleaning up all the device drivers before
> >> pulling the big switch. Particularly the partition mgr will issue
> >> flush commands to attached disks as it goes down. This assures that
> >> devices running in WB mode (that correctly handle flush/sync/etc) commands
> >> are persisted to physical media before we hit reset.
> >>
> >> Without this, there are definitly 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 persistance requests but we can start here.
> >>
> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >> ---
> >>   Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 ++++++++++++++++++++++++
> >>   1 file changed, 44 insertions(+)
> >>
> >> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> >> index a70eee485d..036f619cb5 100644
> >> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> >> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
> >> @@ -19,11 +19,54 @@
> >>   #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)
> >> +
> >> +  @retval EFI_SUCCESS     The operation was successful.
> >> +
> >> +**/
> >
> > STATIC
> >
> >> +EFI_STATUS
> >> +DisconnectAll(
> >
> > Space before (
> >
> >> +  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)) {
> >
> > I understand that this code is copy/pasted but I'd still prefer to
> > avoid the 'success handling' anti pattern here.
>
> Sure.
>
> >
> > if (EFI_ERROR (Status)) {
> >    return Status;
> > }
> >
> >> +    for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) {
> >> +      Status = gBS->DisconnectController (
> >> +        HandleBuffer[HandleIndex],
> >> +        NULL,
> >> +        NULL
> >> +       );
> >> +    }
> >> +    gBS->FreePool(HandleBuffer);
> >> +  }
> >> +  return (EFI_SUCCESS);
> >
> > No need for ()
>
> Yup
>
> >
> >> +}
> >> +
> >> +
> >>   /**
> >>     Resets the entire platform.
> >>
> >> @@ -57,6 +100,7 @@ LibResetSystem (
> >>       if (Delay != 0) {
> >>         DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
> >>                 Delay / 1000000, (Delay % 1000000) / 100000));
> >> +      DisconnectAll ();
> >
> > Capture Status here and ASSERT_EFI_ERROR() ??
> >
> > Maybe it is overkill, and maybe DisconnectController() fails
> > spuriously, so I am not entirely sure, but adding a local function
> > that returns a value and then ignore it seems slightly sloppy to me.
>
> Which makes the above bits about failure returns sorta redundant as I
> should probably just make DisconnectAll() void. There isn't really
> anything to do with a failed return other than print a message and
> ignore it.
>
>

Works for me.


> >
> >>         MicroSecondDelay (Delay);
> >>       }
> >>     }
> >> --
> >> 2.13.7
> >>
>

Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data

[Ard Biesheuvel]

On Tue, 5 Oct 2021 at 23:20, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
> On 10/5/21 5:12 AM, Ard Biesheuvel wrote:
> > On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote:
> >>
> >> While debugging problems with the GET/SET_CLOCK mailbox calls it appeared
> >> that the locking in most of the mailbox commands isn't perfectly
> >> correct. All UEFI firmware calls to the RPi mailbox share a single
> >> mDmaBuffer which 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 result of the request is copied from the buffer after the
> >> lock has been released. This doesn't currently appear to be causing any
> >> problems, but should probably 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>
> >
> > This fails to apply for me - can you rebase onto the last
> > edk2-platform master please?
>
> Sure. Is it whitespace, or general conflicts? I noticed that groups.io's
> web UI looked like I had MIME linebreaks sprinked through my patch
> despite specifying 8bit ASCII. I flirted with 7-bit ASCII with some of
> the previous patches, but its pretty clear that the arm foss server is
> mangling what it thinks are MIME emails and putting cr/lf's changes its
> behavior a bit.
>
>
> Let me try and force the 7bit again, which git rejects if it finds utf
> in the files (and a few of them had it in the past <sigh>).
>

Yeah, it looked like whitespace, to be honest. The first failing hunk
was a whitespace only change, but when I edited that out, it failed on
the following one. All the other patches applied fine so I'm not sure
what happened here.

>
> >
> >> ---
> >>   .../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..29719aa5ec 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_ERROR, "%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_ERROR, "%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
> >>
>

Re: [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name

[Jeremy Linton]

Hi,


On 10/1/21 7:52 PM, Jeremy Linton wrote:
> During review/merge of the linux ecam quirk, some logic
> was added to require the quirk name to be exactly 6
> characters, matching the MADT field its overriding.
> 
> As such, the rpi quirk here needed to be shorted by
> a character to avoid confusion.

I'm going to drop this from the v2 of this set because while the 
mainline patch has a bunch of Ack's, its stuck because we are still 
discussing whether a DSD here is the right choice.


> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>   Platform/RaspberryPi/AcpiTables/Pci.asl | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Platform/RaspberryPi/AcpiTables/Pci.asl b/Platform/RaspberryPi/AcpiTables/Pci.asl
> index ee37b7a21e..e5fe755923 100644
> --- a/Platform/RaspberryPi/AcpiTables/Pci.asl
> +++ b/Platform/RaspberryPi/AcpiTables/Pci.asl
> @@ -65,7 +65,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4PCIE", 2)
>         Name (_DSD, Package () {
>           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>             Package () {
> -            Package () { "linux-ecam-quirk-id", "bcm2711" },
> +            Package () { "linux-ecam-quirk-id", "bc2711" },
>             }
>         })
>   
>