[PATCH v4 0/6] Platform/RasberryPi: Thermal zone

[PATCH v4 0/6] Platform/RasberryPi: Thermal zone

[Jeremy Linton]

This set creates a basic thermal zone, which reads the
SOC temp via a direct register read in AML. It also
adds an active cooling policy using a GPIO pin for fan
control that can optionally be enabled/disabled on either
GPIO18 (commercial fan shim board) or GPIO19 by the
user from the BDS.

it should now be possible when booted in ACPI mode to:

# sensors
acpitz-acpi-0
Adapter: ACPI interface
temp1:        +57.6C  (crit = +90.0C)

and the fan state, if enabled may be read with:

/sys/bus/acpi/devices/PNP0C06:00/PNP0C0B:00/physical_node/thermal_cooling/cur_state

while the kernel will automatically cycle the fan if the SoC
temp exceeds 60C.

Included as a byproduct of this set is a generic method
for updating values in ACPI DSDT/SSDT tables as they
are installed.

v3->v4:
  Allow the user to set the fan trip point
  Extend ACPI device names to 4 characters

v2->v3:
  Make DSDT/SSDT PCD->Nameop update code generic
  Move the fan SSDT code back into the AcpiTable directory

v1->v2:
  Add choice of GPIO pins to the BDS menu
  Correct whitespace/etc issues from v1 review
  Add an additional cleanup patch for contextual whitespace issues

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>

Jeremy Linton (6):
  Platform/RaspberryPi4: Add a basic thermal zone
  Platform/RaspberryPi: Monitor ACPI Table installs
  Platform/RaspberryPi: Add entry for user fan control
  Platform/RaspberryPi4: Create ACPI fan object
  Platform/RaspberryPi4: Allow the user to set Temp
  Platform/RaspberryPi: Trivial whitespace cleanup

 Platform/RaspberryPi/AcpiTables/AcpiTables.inf     |   1 +
 Platform/RaspberryPi/AcpiTables/Dsdt.asl           |  31 +++
 Platform/RaspberryPi/AcpiTables/SsdtThermal.asl    |  77 ++++++++
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 214 +++++++++++++++++++--
 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |   3 +
 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |   9 +
 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr |  34 ++++
 Platform/RaspberryPi/Include/ConfigVars.h          |   8 +
 Platform/RaspberryPi/RPi3/RPi3.dsc                 |   6 +
 Platform/RaspberryPi/RPi4/RPi4.dsc                 |   9 +
 Platform/RaspberryPi/RaspberryPi.dec               |   2 +
 .../Bcm27xx/Include/IndustryStandard/Bcm2711.h     |   2 +
 12 files changed, 382 insertions(+), 14 deletions(-)
 create mode 100644 Platform/RaspberryPi/AcpiTables/SsdtThermal.asl

-- 
2.13.7

[PATCH v4 1/6] Platform/RaspberryPi4: Add a basic thermal zone

[Jeremy Linton]

Rather than exporting the temp sensor or mailbox
in ACPI land we can wrap them in AML and use the default
ACPI drivers provided by the OS. This enables the use of
"sensors" in linux to report the SOC temp.

As a first pass add a basic passive cooling ACPI thermalzone
with trip points for passive cooling (throttling) handled
by the vc firmware, hibernate and critical shutdown. The
vc apparently kicks in at ~80C, so the hibernate and critical
set points are set at +5 and +10 of that. In the future
CPPC should be able to monitor the thermal throttling.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Pete Batard <@pbatard>
---
 Platform/RaspberryPi/AcpiTables/Dsdt.asl           | 31 ++++++++++++++++++++++
 .../Bcm27xx/Include/IndustryStandard/Bcm2711.h     |  2 ++
 2 files changed, 33 insertions(+)

diff --git a/Platform/RaspberryPi/AcpiTables/Dsdt.asl b/Platform/RaspberryPi/AcpiTables/Dsdt.asl
index 353af2d876..2b9e8211cf 100644
--- a/Platform/RaspberryPi/AcpiTables/Dsdt.asl
+++ b/Platform/RaspberryPi/AcpiTables/Dsdt.asl
@@ -252,6 +252,37 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "RPIFDN", "RPI", 2)
         }
       })
     }
+
+    // Define a simple thermal zone. The idea here is we compute the SOC temp
+    // via a register we can read, and give it to the OS. This enables basic
+    // reports from the "sensors" utility, and the OS can then poll and take
+    // actions if that temp exceeds any of the given thresholds.
+    Device (EC00)
+    {
+      Name (_HID, EISAID ("PNP0C06"))
+      Name (_CCA, 0x0)
+
+      // all temps in are tenths of K (aka 2732 is the min temps in Linux (aka 0C))
+      ThermalZone (TZ00) {
+        Method (_TMP, 0, Serialized) {
+          OperationRegion (TEMS, SystemMemory, THERM_SENSOR, 0x8)
+          Field (TEMS, DWordAcc, NoLock, Preserve) {
+            TMPS, 32
+          }
+          return (((419949 - ((TMPS & 0x3ff) * 487)) / 100) + 2732);
+        }
+        Method (_SCP, 3) { }               // receive cooling policy from OS
+
+        Method (_CRT) { Return (3632) }    // (90C) Critical temp point (immediate power-off)
+        Method (_HOT) { Return (3582) }    // (85C) HOT state where OS should hibernate
+        Method (_PSV) { Return (3532) }    // (80C) Passive cooling (CPU throttling) trip point
+
+        // SSDT inserts _AC0/_AL0 @60C here, if a FAN is configured
+
+        Name (_TZP, 10)                   //The OSPM must poll this device every 1 seconds
+        Name (_PSL, Package () { \_SB_.CPU0, \_SB_.CPU1, \_SB_.CPU2, \_SB_.CPU3 })
+      }
+    }
 #endif
 
   }
diff --git a/Silicon/Broadcom/Bcm27xx/Include/IndustryStandard/Bcm2711.h b/Silicon/Broadcom/Bcm27xx/Include/IndustryStandard/Bcm2711.h
index e9c81cafa1..86906b2438 100644
--- a/Silicon/Broadcom/Bcm27xx/Include/IndustryStandard/Bcm2711.h
+++ b/Silicon/Broadcom/Bcm27xx/Include/IndustryStandard/Bcm2711.h
@@ -86,4 +86,6 @@
 #define GENET_BASE_ADDRESS         FixedPcdGet64 (PcdBcmGenetRegistersAddress)
 #define GENET_LENGTH               0x00010000
 
+#define THERM_SENSOR               0xfd5d2200
+
 #endif /* BCM2711_H__ */
-- 
2.13.7

[PATCH v4 2/6] Platform/RaspberryPi: Monitor ACPI Table installs

[Jeremy Linton]

Hook the ACPI table install sequence and add some
basic conditional and AML NameOp update logic. If
a table has a non-zero PCD declared that pcd is
checked for a non-zero value before allowing the table
to be installed. We also add a table of NameOp to
PCD's which will be written into a DSDT/SSDT table
as part of its install process.

With this change we can declare something in
ASL like:

Name (VARN, 0x1234)

and then add a table entry like:

{"VARN", PcdToken(PcdVarn)}

and the value of PcdVarn will replace the
0x1234 declared in the ASL above.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Pete Batard <@pbatard>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 153 ++++++++++++++++++++-
 1 file changed, 152 insertions(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index af54136ade..9e5d9734ca 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -568,6 +568,156 @@ ApplyVariables (
 }
 
 
+typedef struct {
+  CHAR8 Name[4];
+  UINTN PcdToken;
+} AML_NAME_OP_REPLACE;
+
+typedef struct {
+  UINT64              OemTableId;
+  UINTN               PcdToken;
+  AML_NAME_OP_REPLACE *SdtNameOpReplace;
+} NAMESPACE_TABLES;
+
+#define SSDT_PATTERN_LEN 5
+#define AML_NAMEOP_8   0x0A
+#define AML_NAMEOP_16  0x0B
+#define AML_NAMEOP_32  0x0C
+#define AML_NAMEOP_STR 0x0D
+/*
+ * Scan the given namespace table (DSDT/SSDT) for AML NameOps
+ * listed in the NameOpReplace structure. If one is found then
+ * update the value in the table from the specified Pcd
+ *
+ * This allows us to have conditionals in AML controlled
+ * by options in the BDS or detected during firmware bootstrap.
+ * We could extend this concept for strings/etc but due to len
+ * variations its probably easier to encode the strings
+ * in the ASL and pick the correct one based off a variable.
+ */
+STATIC VOID
+UpdateSdtNameOps(
+  EFI_ACPI_DESCRIPTION_HEADER  *AcpiTable,
+  AML_NAME_OP_REPLACE          *NameOpReplace
+  )
+{
+  UINTN  Idx;
+  UINTN  Index;
+  CHAR8  Pattern[SSDT_PATTERN_LEN];
+  UINTN  PcdVal;
+  UINT8  *SdtPtr;
+  UINT32 SdtSize;
+
+  SdtSize = AcpiTable->Length;
+
+  if (SdtSize > 0) {
+    SdtPtr = (UINT8*)AcpiTable;
+
+    for (Idx = 0; NameOpReplace && NameOpReplace[Idx].PcdToken; Idx++) {
+      /*
+       * Do a single NameOp variable replacement these are of the
+       * form 08 XXXX SIZE VAL, where SIZE is 0A=byte, 0B=word, 0C=dword
+       * and XXXX is the name and VAL is the value
+      */
+      Pattern[0] = 0x08;
+      Pattern[1] = NameOpReplace[Idx].Name[0];
+      Pattern[2] = NameOpReplace[Idx].Name[1];
+      Pattern[3] = NameOpReplace[Idx].Name[2];
+      Pattern[4] = NameOpReplace[Idx].Name[3];
+
+      for (Index = 0; Index < (SdtSize - SSDT_PATTERN_LEN); Index++) {
+        if (CompareMem (SdtPtr + Index, Pattern, SSDT_PATTERN_LEN) == 0) {
+          PcdVal = LibPcdGet32 (NameOpReplace[Idx].PcdToken);
+          switch (SdtPtr[Index + SSDT_PATTERN_LEN]) {
+          case AML_NAMEOP_32:
+            SdtPtr[Index + SSDT_PATTERN_LEN + 4] = (PcdVal >> 24) & 0xFF;
+            SdtPtr[Index + SSDT_PATTERN_LEN + 3] = (PcdVal >> 16) & 0xFF;
+            // Fallthrough
+          case AML_NAMEOP_16:
+            SdtPtr[Index + SSDT_PATTERN_LEN + 2] = (PcdVal >> 8) & 0xFF;
+            // Fallthrough
+          case AML_NAMEOP_8:
+            SdtPtr[Index + SSDT_PATTERN_LEN + 1] = PcdVal & 0xFF;
+            break;
+          case 0:
+          case 1:
+            SdtPtr[Index + SSDT_PATTERN_LEN + 1] = !!PcdVal;
+            break;
+          case AML_NAMEOP_STR:
+            /*
+             * If the string val is added to the NameOpReplace, we can
+             * dynamically update things like _HID too as long as the
+             * string length matches.
+             */
+            break;
+          }
+          break;
+        }
+      }
+    }
+  }
+}
+
+
+STATIC BOOLEAN
+VerifyUpdateTable(
+  IN  EFI_ACPI_DESCRIPTION_HEADER *AcpiHeader,
+  IN  NAMESPACE_TABLES *SdtTable
+  )
+{
+  BOOLEAN ret;
+
+  ret = TRUE;
+  if (SdtTable->PcdToken && !LibPcdGet32 (SdtTable->PcdToken)) {
+    ret = FALSE;
+  }
+  if (ret && SdtTable->SdtNameOpReplace) {
+    UpdateSdtNameOps (AcpiHeader, SdtTable->SdtNameOpReplace);
+  }
+
+  return ret;
+}
+
+
+STATIC NAMESPACE_TABLES SdtTables[] = {
+  {
+    SIGNATURE_64 ('R', 'P', 'I', 0, 0, 0, 0, 0),
+    0,
+    NULL
+  },
+  { }
+};
+
+/*
+ * Monitor the ACPI tables being installed and when
+ * a DSDT/SSDT is detected validate that we want to
+ * install it, and if so update any "NameOp" defined
+ * variables contained in the table from PCD values
+ */
+STATIC BOOLEAN
+HandleDynamicNamespace (
+  IN  EFI_ACPI_DESCRIPTION_HEADER *AcpiHeader
+  )
+{
+  UINTN Tables;
+
+  switch (AcpiHeader->Signature) {
+  case SIGNATURE_32 ('D', 'S', 'D', 'T'):
+  case SIGNATURE_32 ('S', 'S', 'D', 'T'):
+    for (Tables = 0; SdtTables[Tables].OemTableId; Tables++) {
+      if (AcpiHeader->OemTableId == SdtTables[Tables].OemTableId) {
+        return VerifyUpdateTable (AcpiHeader, &SdtTables[Tables]);
+      }
+    }
+    DEBUG ((DEBUG_ERROR, "Found namespace table not in table list.\n"));
+
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+
 EFI_STATUS
 EFIAPI
 ConfigInitialize (
@@ -618,7 +768,8 @@ ConfigInitialize (
 
   if (PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_ACPI ||
       PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_BOTH) {
-     Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
+     Status = LocateAndInstallAcpiFromFvConditional (&mAcpiTableFile,
+                                                     &HandleDynamicNamespace);
      ASSERT_EFI_ERROR (Status);
   }
 
-- 
2.13.7

[PATCH v4 3/6] Platform/RaspberryPi: Add entry for user fan control

[Jeremy Linton]

Add a menu item that allows the user to enable GPIO based
fan control via SSDT and the previous NameObj replacement
commit. This should only be seen/enabled on RPI4
because that is what its been tested with.

Given GPIO pin current limitations its likely that a bit of
additional circuitry is required to drive a fan, and the GPIO
high/low signal can only be used as a enable/disable signal. A
search for "rpi npn gpio fan" or similar should turn up some
hits for how to do this. Alternatively there are some commercial
boards (FAN SHIM) which operate via simple GPIO control.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Pete Batard <@pbatard>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 26 ++++++++++++++++++++++
 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  2 ++
 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  6 +++++
 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 18 +++++++++++++++
 Platform/RaspberryPi/Include/ConfigVars.h          |  4 ++++
 Platform/RaspberryPi/RPi3/RPi3.dsc                 |  5 +++++
 Platform/RaspberryPi/RPi4/RPi4.dsc                 |  8 +++++++
 Platform/RaspberryPi/RaspberryPi.dec               |  1 +
 8 files changed, 70 insertions(+)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index 9e5d9734ca..d58cbbdfe7 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -15,6 +15,7 @@
 #include <Library/AcpiLib.h>
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
+#include <Library/DxeServicesLib.h>
 #include <Library/DxeServicesTableLib.h>
 #include <Library/GpioLib.h>
 #include <Library/HiiLib.h>
@@ -22,6 +23,7 @@
 #include <Library/NetLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiRuntimeServicesTableLib.h>
+#include <Protocol/AcpiTable.h>
 #include <Protocol/BcmGenetPlatformDevice.h>
 #include <Protocol/RpiFirmware.h>
 #include <ConfigVars.h>
@@ -246,6 +248,14 @@ SetupVariables (
     ASSERT_EFI_ERROR (Status);
   }
 
+  Size = sizeof (UINT32);
+  Status = gRT->GetVariable (L"FanOnGpio",
+                  &gConfigDxeFormSetGuid,
+                  NULL, &Size, &Var32);
+  if (EFI_ERROR (Status)) {
+    PcdSet32 (PcdFanOnGpio, PcdGet32 (PcdFanOnGpio));
+  }
+
   Size = sizeof(AssetTagVar);
 
   Status = gRT->GetVariable(L"AssetTag",
@@ -368,6 +378,7 @@ ApplyVariables (
   UINT32 CpuClock = PcdGet32 (PcdCpuClock);
   UINT32 CustomCpuClock = PcdGet32 (PcdCustomCpuClock);
   UINT32 Rate = 0;
+  UINT32 FanOnGpio = PcdGet32 (PcdFanOnGpio);
 
   switch (CpuClock) {
   case CHIPSET_CPU_CLOCK_LOW:
@@ -565,6 +576,11 @@ ApplyVariables (
     GpioPinFuncSet (23, GPIO_FSEL_INPUT);
     GpioPinFuncSet (24, GPIO_FSEL_INPUT);
   }
+
+  if (FanOnGpio) {
+    DEBUG ((DEBUG_INFO, "Fan enabled on GPIO %d\n", FanOnGpio));
+    GpioPinFuncSet (FanOnGpio, GPIO_FSEL_OUTPUT);
+  }
 }
 
 
@@ -679,8 +695,18 @@ VerifyUpdateTable(
 }
 
 
+STATIC AML_NAME_OP_REPLACE SsdtNameOpReplace[] = {
+  {"GIOP", PcdToken(PcdFanOnGpio)},
+  {}
+};
+
 STATIC NAMESPACE_TABLES SdtTables[] = {
   {
+    SIGNATURE_64 ('R', 'P', 'I', 'T', 'H', 'F', 'A', 'N'),
+    PcdToken(PcdFanOnGpio),
+    SsdtNameOpReplace
+  },
+  {
     SIGNATURE_64 ('R', 'P', 'I', 0, 0, 0, 0, 0),
     0,
     NULL
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
index cdce35bc74..321e402e65 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
@@ -46,6 +46,7 @@
   AcpiLib
   BaseLib
   DebugLib
+  DxeServicesLib
   DxeServicesTableLib
   GpioLib
   HiiLib
@@ -89,6 +90,7 @@
   gRaspberryPiTokenSpaceGuid.PcdSystemTableMode
   gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
   gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
+  gRaspberryPiTokenSpaceGuid.PcdFanOnGpio
 
 [Depex]
   gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
index 03763710a1..e2d1bb4b39 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
@@ -48,6 +48,12 @@
 #string STR_ADVANCED_SYSTAB_BOTH     #language en-US "ACPI + Devicetree"
 #string STR_ADVANCED_SYSTAB_DT       #language en-US "Devicetree"
 
+#string STR_ADVANCED_FANONGPIO_PROMPT #language en-US "ACPI fan control"
+#string STR_ADVANCED_FANONGPIO_HELP   #language en-US "Cycle a fan via GPIO if temp exceeds 60C"
+#string STR_ADVANCED_FANONGPIO_OFF    #language en-US "Disabled"
+#string STR_ADVANCED_FANONGPIO_18     #language en-US "Fan Shim/GPIO-18"
+#string STR_ADVANCED_FANONGPIO_19     #language en-US "GPIO-19"
+
 #string STR_ADVANCED_ASSET_TAG_PROMPT #language en-US "Asset Tag"
 #string STR_ADVANCED_ASSET_TAG_HELP   #language en-US "Set the system Asset Tag"
 
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
index d5615d7af0..94332caab3 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
@@ -10,6 +10,7 @@
 #include <Guid/HiiPlatformSetupFormset.h>
 #include "ConfigDxeFormSetGuid.h"
 #include <ConfigVars.h>
+#include <IndustryStandard/Bcm2711.h>
 
 //
 // EFI Variable attributes
@@ -45,6 +46,11 @@ formset
       name  = RamLimitTo3GB,
       guid  = CONFIGDXE_FORM_SET_GUID;
 
+    efivarstore ADVANCED_FAN_ON_GPIO_VARSTORE_DATA,
+      attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
+      name  = FanOnGpio,
+      guid  = CONFIGDXE_FORM_SET_GUID;
+
     efivarstore SYSTEM_TABLE_MODE_VARSTORE_DATA,
       attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
       name  = SystemTableMode,
@@ -174,6 +180,18 @@ formset
             option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags = DEFAULT;
         endoneof;
 
+#if (RPI_MODEL == 4)
+        grayoutif NOT ideqval SystemTableMode.Mode == SYSTEM_TABLE_MODE_ACPI;
+          oneof varid = FanOnGpio.Enabled,
+              prompt      = STRING_TOKEN(STR_ADVANCED_FANONGPIO_PROMPT),
+              help        = STRING_TOKEN(STR_ADVANCED_FANONGPIO_HELP),
+              flags       = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
+              option text = STRING_TOKEN(STR_ADVANCED_FANONGPIO_OFF), value = 0, flags = DEFAULT;
+              option text = STRING_TOKEN(STR_ADVANCED_FANONGPIO_18), value = 18, flags = 0;
+              option text = STRING_TOKEN(STR_ADVANCED_FANONGPIO_19), value = 19, flags = 0;
+          endoneof;
+        endif;
+#endif
         string varid = AssetTag.AssetTag,
             prompt  = STRING_TOKEN(STR_ADVANCED_ASSET_TAG_PROMPT),
             help    = STRING_TOKEN(STR_ADVANCED_ASSET_TAG_HELP),
diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
index b1689b004d..1a40469bfa 100644
--- a/Platform/RaspberryPi/Include/ConfigVars.h
+++ b/Platform/RaspberryPi/Include/ConfigVars.h
@@ -69,6 +69,10 @@ typedef struct {
 } ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA;
 
 typedef struct {
+  UINT32 Enabled;
+} ADVANCED_FAN_ON_GPIO_VARSTORE_DATA;
+
+typedef struct {
 #define SYSTEM_TABLE_MODE_ACPI 0
 #define SYSTEM_TABLE_MODE_BOTH 1
 #define SYSTEM_TABLE_MODE_DT   2
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index 0998d8366c..cef8932ca2 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -499,6 +499,11 @@
   gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|L"SystemTableMode"|gConfigDxeFormSetGuid|0x0|1
 
   #
+  # Enable a fan in the ACPI thermal zone on GPIO pin #
+  #
+  gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
+
+  #
   # Common UEFI ones.
   #
 
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index baa7e63483..9d0eaf10a1 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -510,6 +510,14 @@
   gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|L"SystemTableMode"|gConfigDxeFormSetGuid|0x0|0
 
   #
+  # Enable a fan in the ACPI thermal zone on GPIO pin #
+  #
+  # 0  - DISABLED
+  # 19 - Enabled on pin 19
+  #
+  gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
+
+  #
   # Common UEFI ones.
   #
 
diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
index c71177a2f7..a73650f2c3 100644
--- a/Platform/RaspberryPi/RaspberryPi.dec
+++ b/Platform/RaspberryPi/RaspberryPi.dec
@@ -66,3 +66,4 @@
   gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|1|UINT32|0x0000001B
   gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|0|UINT32|0x00000019
   gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
+  gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|0|UINT32|0x0000001C
-- 
2.13.7

[PATCH v4 4/6] Platform/RaspberryPi4: Create ACPI fan object

[Jeremy Linton]

Now that we have a thermal zone we can add active cooling
by specifying active cooling points (_ACx) which can
be tied to fan objects that turn fans on/off using GPIO
pins.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Pete Batard <@pbatard>
---
 Platform/RaspberryPi/AcpiTables/AcpiTables.inf  |  1 +
 Platform/RaspberryPi/AcpiTables/SsdtThermal.asl | 76 +++++++++++++++++++++++++
 2 files changed, 77 insertions(+)
 create mode 100644 Platform/RaspberryPi/AcpiTables/SsdtThermal.asl

diff --git a/Platform/RaspberryPi/AcpiTables/AcpiTables.inf b/Platform/RaspberryPi/AcpiTables/AcpiTables.inf
index 28d2afe197..c40c32e16f 100644
--- a/Platform/RaspberryPi/AcpiTables/AcpiTables.inf
+++ b/Platform/RaspberryPi/AcpiTables/AcpiTables.inf
@@ -33,6 +33,7 @@
   Csrt.aslc
   Spcr.aslc
   Pptt.aslc
+  SsdtThermal.asl
 
 [Packages]
   ArmPkg/ArmPkg.dec
diff --git a/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl b/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
new file mode 100644
index 0000000000..ee028173e1
--- /dev/null
+++ b/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
@@ -0,0 +1,76 @@
+/** @file
+ *
+ *  Secondary System Description Table (SSDT) for active (fan) cooling
+ *
+ *  Copyright (c) 2020, Arm Ltd. All rights reserved.
+ *
+ *  SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ **/
+
+#include <IndustryStandard/Bcm2711.h>
+#include <IndustryStandard/Bcm2836.h>
+#include <IndustryStandard/Bcm2836Gpio.h>
+
+#include <IndustryStandard/Acpi.h>
+
+DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPITHFAN", 2)
+{
+  External (\_SB_.EC00, DeviceObj)
+  External (\_SB_.EC00.TZ00, DeviceObj)
+
+  Scope (\_SB_.EC00)
+  {
+    // Define a NameOp we will modify during InstallTable
+    Name (GIOP, 0x2) //08 47 49 4f 50 0a 02 (value must be >1)
+    // Describe a fan
+    PowerResource (PFAN, 0, 0) {
+      OperationRegion (GPIO, SystemMemory, GPIO_BASE_ADDRESS, 0x1000)
+      Field (GPIO, DWordAcc, NoLock, Preserve) {
+        Offset (0x1C),
+        GPS0, 32,
+        GPS1, 32,
+        RES1, 32,
+        GPC0, 32,
+        GPC1, 32,
+        RES2, 32,
+        GPL1, 32,
+        GPL2, 32
+      }
+      // We are hitting a GPIO pin to on/off a fan.
+      // This assumes that UEFI has programmed the
+      // direction as OUT. Given the current limitations
+      // on the GPIO pins, its recommended to use
+      // the GPIO to switch a larger voltage/current
+      // for the fan rather than driving it directly.
+      Method (_STA) {
+        if (GPL1 & (1 << GIOP)) {
+          Return (1)                 // present and enabled
+        }
+        Return (0)
+      }
+      Method (_ON)  {                // turn fan on
+        Store (1 << GIOP, GPS0)
+      }
+      Method (_OFF) {                // turn fan off
+        Store (1 << GIOP, GPC0)
+      }
+    }
+    Device (FAN0) {
+      // Note, not currently an ACPIv4 fan
+      // the latter adds speed control/detection
+      // but in the case of linux needs FIF, FPS, FSL, and FST
+      Name (_HID, EISAID ("PNP0C0B"))
+      Name (_PR0, Package () { PFAN })
+    }
+  }
+
+  // merge in an active cooling point.
+  Scope (\_SB_.EC00.TZ00)
+  {
+    Method (_AC0) { Return (3332) }        // (60C) active cooling trip point,
+                                           // if this is lower than PSV then we
+                                           // prefer active cooling
+    Name (_AL0, Package () { \_SB_.EC00.FAN0 }) // the fan used for AC0 above
+  }
+}
-- 
2.13.7

[PATCH v4 5/6] Platform/RaspberryPi4: Allow the user to set Temp

[Jeremy Linton]

Now that we have the ability to enable an AML fan object,
allow the user to select the temperature at which the
fan cycles on.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 Platform/RaspberryPi/AcpiTables/SsdtThermal.asl         |  9 +++++----
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      | 11 ++++++++++-
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  1 +
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  5 ++++-
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 16 ++++++++++++++++
 Platform/RaspberryPi/Include/ConfigVars.h               |  4 ++++
 Platform/RaspberryPi/RPi3/RPi3.dsc                      |  1 +
 Platform/RaspberryPi/RPi4/RPi4.dsc                      |  1 +
 Platform/RaspberryPi/RaspberryPi.dec                    |  1 +
 9 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl b/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
index ee028173e1..acfa4699bb 100644
--- a/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
+++ b/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
@@ -23,6 +23,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPITHFAN", 2)
   {
     // Define a NameOp we will modify during InstallTable
     Name (GIOP, 0x2) //08 47 49 4f 50 0a 02 (value must be >1)
+    Name (FTMP, 0x2)
     // Describe a fan
     PowerResource (PFAN, 0, 0) {
       OperationRegion (GPIO, SystemMemory, GPIO_BASE_ADDRESS, 0x1000)
@@ -68,9 +69,9 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPITHFAN", 2)
   // merge in an active cooling point.
   Scope (\_SB_.EC00.TZ00)
   {
-    Method (_AC0) { Return (3332) }        // (60C) active cooling trip point,
-                                           // if this is lower than PSV then we
-                                           // prefer active cooling
-    Name (_AL0, Package () { \_SB_.EC00.FAN0 }) // the fan used for AC0 above
+    Method (_AC0) { Return ( (FTMP * 10) + 2732) } // (60C) active cooling trip point,
+                                                   // if this is lower than PSV then we
+                                                   // prefer active cooling
+    Name (_AL0, Package () { \_SB_.EC00.FAN0 })    // the fan used for AC0 above
   }
 }
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index d58cbbdfe7..e8f964a329 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -256,8 +256,16 @@ SetupVariables (
     PcdSet32 (PcdFanOnGpio, PcdGet32 (PcdFanOnGpio));
   }
 
-  Size = sizeof(AssetTagVar);
+  Size = sizeof (UINT32);
+  Status = gRT->GetVariable (L"FanTemp",
+                  &gConfigDxeFormSetGuid,
+                  NULL, &Size, &Var32);
+  if (EFI_ERROR (Status)) {
+    PcdSet32 (PcdFanTemp, PcdGet32 (PcdFanTemp));
+  }
+
 
+  Size = sizeof (AssetTagVar);
   Status = gRT->GetVariable(L"AssetTag",
                   &gConfigDxeFormSetGuid,
                   NULL, &Size, AssetTagVar);
@@ -697,6 +705,7 @@ VerifyUpdateTable(
 
 STATIC AML_NAME_OP_REPLACE SsdtNameOpReplace[] = {
   {"GIOP", PcdToken(PcdFanOnGpio)},
+  {"FTMP", PcdToken(PcdFanTemp)},
   {}
 };
 
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
index 321e402e65..544e3b3e10 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
@@ -91,6 +91,7 @@
   gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
   gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
   gRaspberryPiTokenSpaceGuid.PcdFanOnGpio
+  gRaspberryPiTokenSpaceGuid.PcdFanTemp
 
 [Depex]
   gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
index e2d1bb4b39..2afe8f32ae 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
@@ -49,11 +49,14 @@
 #string STR_ADVANCED_SYSTAB_DT       #language en-US "Devicetree"
 
 #string STR_ADVANCED_FANONGPIO_PROMPT #language en-US "ACPI fan control"
-#string STR_ADVANCED_FANONGPIO_HELP   #language en-US "Cycle a fan via GPIO if temp exceeds 60C"
+#string STR_ADVANCED_FANONGPIO_HELP   #language en-US "Cycle a fan via GPIO at given temperature"
 #string STR_ADVANCED_FANONGPIO_OFF    #language en-US "Disabled"
 #string STR_ADVANCED_FANONGPIO_18     #language en-US "Fan Shim/GPIO-18"
 #string STR_ADVANCED_FANONGPIO_19     #language en-US "GPIO-19"
 
+#string STR_ADVANCED_FANTEMP_PROMPT   #language en-US "ACPI fan temperature"
+#string STR_ADVANCED_FANTEMP_HELP     #language en-US "Cycle a fan at C"
+
 #string STR_ADVANCED_ASSET_TAG_PROMPT #language en-US "Asset Tag"
 #string STR_ADVANCED_ASSET_TAG_HELP   #language en-US "Set the system Asset Tag"
 
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
index 94332caab3..de5e43471a 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
@@ -51,6 +51,11 @@ formset
       name  = FanOnGpio,
       guid  = CONFIGDXE_FORM_SET_GUID;
 
+    efivarstore ADVANCED_FANTEMP_VARSTORE_DATA,
+      attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
+      name  = FanTemp,
+      guid  = CONFIGDXE_FORM_SET_GUID;
+
     efivarstore SYSTEM_TABLE_MODE_VARSTORE_DATA,
       attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
       name  = SystemTableMode,
@@ -191,6 +196,17 @@ formset
               option text = STRING_TOKEN(STR_ADVANCED_FANONGPIO_19), value = 19, flags = 0;
           endoneof;
         endif;
+
+        grayoutif ideqval FanOnGpio.Enabled == 0;
+          numeric varid = FanTemp.Value,
+              prompt      = STRING_TOKEN(STR_ADVANCED_FANTEMP_PROMPT),
+              help        = STRING_TOKEN(STR_ADVANCED_FANTEMP_HELP),
+              flags       = DISPLAY_UINT_DEC | NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
+	      minimum = 50,
+              maximum = 80,
+              default = 60,
+          endnumeric;
+        endif;
 #endif
         string varid = AssetTag.AssetTag,
             prompt  = STRING_TOKEN(STR_ADVANCED_ASSET_TAG_PROMPT),
diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
index 1a40469bfa..8094d4ef9a 100644
--- a/Platform/RaspberryPi/Include/ConfigVars.h
+++ b/Platform/RaspberryPi/Include/ConfigVars.h
@@ -73,6 +73,10 @@ typedef struct {
 } ADVANCED_FAN_ON_GPIO_VARSTORE_DATA;
 
 typedef struct {
+  UINT32 Value;
+} ADVANCED_FANTEMP_VARSTORE_DATA;
+
+typedef struct {
 #define SYSTEM_TABLE_MODE_ACPI 0
 #define SYSTEM_TABLE_MODE_BOTH 1
 #define SYSTEM_TABLE_MODE_DT   2
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index cef8932ca2..484a46ffba 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -502,6 +502,7 @@
   # Enable a fan in the ACPI thermal zone on GPIO pin #
   #
   gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
+  gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0
 
   #
   # Common UEFI ones.
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 9d0eaf10a1..823c9fc007 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -516,6 +516,7 @@
   # 19 - Enabled on pin 19
   #
   gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
+  gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|60
 
   #
   # Common UEFI ones.
diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
index a73650f2c3..c64c61930e 100644
--- a/Platform/RaspberryPi/RaspberryPi.dec
+++ b/Platform/RaspberryPi/RaspberryPi.dec
@@ -67,3 +67,4 @@
   gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|0|UINT32|0x00000019
   gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
   gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|0|UINT32|0x0000001C
+  gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D
-- 
2.13.7

[PATCH v4 6/6] Platform/RaspberryPi: Trivial whitespace cleanup

[Jeremy Linton]

Pete's review pointed out some whitespace issues in the
context of a previous patch. Since there are a number of
similar errors in the file lets fix them separately.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Pete Batard <@pbatard>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 24 +++++++++++-----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index e8f964a329..4ed294cdfe 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -209,9 +209,9 @@ SetupVariables (
   }
 
   Size = sizeof (UINT32);
-  Status = gRT->GetVariable(L"CustomCpuClock",
-                            &gConfigDxeFormSetGuid,
-                            NULL, &Size, &Var32);
+  Status = gRT->GetVariable (L"CustomCpuClock",
+                             &gConfigDxeFormSetGuid,
+                             NULL, &Size, &Var32);
   if (EFI_ERROR (Status)) {
     Status = PcdSet32S (PcdCustomCpuClock, PcdGet32 (PcdCustomCpuClock));
     ASSERT_EFI_ERROR (Status);
@@ -266,7 +266,7 @@ SetupVariables (
 
 
   Size = sizeof (AssetTagVar);
-  Status = gRT->GetVariable(L"AssetTag",
+  Status = gRT->GetVariable (L"AssetTag",
                   &gConfigDxeFormSetGuid,
                   NULL, &Size, AssetTagVar);
 
@@ -275,7 +275,7 @@ SetupVariables (
                     L"AssetTag",
                     &gConfigDxeFormSetGuid,
                     EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
-                    sizeof(AssetTagVar),
+                    sizeof (AssetTagVar),
                     AssetTagVar
                     );
   }
@@ -441,9 +441,9 @@ ApplyVariables (
      * spaces. SystemMemorySizeBelow4GB tracks the maximum memory below 4GB
      * line, factoring in the limit imposed by the SoC register range.
      */
-    SystemMemorySizeBelow4GB = MIN(SystemMemorySize, 4UL * SIZE_1GB);
-    SystemMemorySizeBelow4GB = MIN(SystemMemorySizeBelow4GB, BCM2836_SOC_REGISTERS);
-    SystemMemorySizeBelow4GB = MIN(SystemMemorySizeBelow4GB, BCM2711_SOC_REGISTERS);
+    SystemMemorySizeBelow4GB = MIN (SystemMemorySize, 4UL * SIZE_1GB);
+    SystemMemorySizeBelow4GB = MIN (SystemMemorySizeBelow4GB, BCM2836_SOC_REGISTERS);
+    SystemMemorySizeBelow4GB = MIN (SystemMemorySizeBelow4GB, BCM2711_SOC_REGISTERS);
 
     ASSERT (SystemMemorySizeBelow4GB > 3UL * SIZE_1GB);
 
@@ -536,14 +536,14 @@ ApplyVariables (
       /*
        * SD card pins go to Arasan.
        */
-      MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
-                  MmioRead32(GPIO_BASE_ADDRESS + 0xD0) | 0x2);
+      MmioWrite32 ((GPIO_BASE_ADDRESS + 0xD0),
+                  MmioRead32 (GPIO_BASE_ADDRESS + 0xD0) | 0x2);
     } else {
       /*
        * SD card pins back to eMMC2.
        */
-      MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
-                  MmioRead32(GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
+      MmioWrite32 ((GPIO_BASE_ADDRESS + 0xD0),
+                  MmioRead32 (GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
       /*
        * WiFi back to Arasan.
        */
-- 
2.13.7

Re: [PATCH v4 2/6] Platform/RaspberryPi: Monitor ACPI Table installs

[Andrei Warkentin]

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

This is really cool. Can be useful in a number of other places down the line (e.g. MAC address via device properties).

Reviewed-by: Andrei Warkentin <andrey.warkentin@gmail.com>
________________________________
From: Jeremy Linton <jeremy.linton@arm.com>
Sent: Monday, August 31, 2020 12:25 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Jeremy Linton <jeremy.linton@arm.com>; Leif Lindholm <leif@nuviainc.com>; Pete Batard <pete@akeo.ie>; Andrei Warkentin <awarkentin@vmware.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Subject: [PATCH v4 2/6] Platform/RaspberryPi: Monitor ACPI Table installs

Hook the ACPI table install sequence and add some
basic conditional and AML NameOp update logic. If
a table has a non-zero PCD declared that pcd is
checked for a non-zero value before allowing the table
to be installed. We also add a table of NameOp to
PCD's which will be written into a DSDT/SSDT table
as part of its install process.

With this change we can declare something in
ASL like:

Name (VARN, 0x1234)

and then add a table entry like:

{"VARN", PcdToken(PcdVarn)}

and the value of PcdVarn will replace the
0x1234 declared in the ASL above.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <awarkentin@vmware.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Pete Batard <@pbatard>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 153 ++++++++++++++++++++-
 1 file changed, 152 insertions(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index af54136ade..9e5d9734ca 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -568,6 +568,156 @@ ApplyVariables (
 }





+typedef struct {

+  CHAR8 Name[4];

+  UINTN PcdToken;

+} AML_NAME_OP_REPLACE;

+

+typedef struct {

+  UINT64              OemTableId;

+  UINTN               PcdToken;

+  AML_NAME_OP_REPLACE *SdtNameOpReplace;

+} NAMESPACE_TABLES;

+

+#define SSDT_PATTERN_LEN 5

+#define AML_NAMEOP_8   0x0A

+#define AML_NAMEOP_16  0x0B

+#define AML_NAMEOP_32  0x0C

+#define AML_NAMEOP_STR 0x0D

+/*

+ * Scan the given namespace table (DSDT/SSDT) for AML NameOps

+ * listed in the NameOpReplace structure. If one is found then

+ * update the value in the table from the specified Pcd

+ *

+ * This allows us to have conditionals in AML controlled

+ * by options in the BDS or detected during firmware bootstrap.

+ * We could extend this concept for strings/etc but due to len

+ * variations its probably easier to encode the strings

+ * in the ASL and pick the correct one based off a variable.

+ */

+STATIC VOID

+UpdateSdtNameOps(

+  EFI_ACPI_DESCRIPTION_HEADER  *AcpiTable,

+  AML_NAME_OP_REPLACE          *NameOpReplace

+  )

+{

+  UINTN  Idx;

+  UINTN  Index;

+  CHAR8  Pattern[SSDT_PATTERN_LEN];

+  UINTN  PcdVal;

+  UINT8  *SdtPtr;

+  UINT32 SdtSize;

+

+  SdtSize = AcpiTable->Length;

+

+  if (SdtSize > 0) {

+    SdtPtr = (UINT8*)AcpiTable;

+

+    for (Idx = 0; NameOpReplace && NameOpReplace[Idx].PcdToken; Idx++) {

+      /*

+       * Do a single NameOp variable replacement these are of the

+       * form 08 XXXX SIZE VAL, where SIZE is 0A=byte, 0B=word, 0C=dword

+       * and XXXX is the name and VAL is the value

+      */

+      Pattern[0] = 0x08;

+      Pattern[1] = NameOpReplace[Idx].Name[0];

+      Pattern[2] = NameOpReplace[Idx].Name[1];

+      Pattern[3] = NameOpReplace[Idx].Name[2];

+      Pattern[4] = NameOpReplace[Idx].Name[3];

+

+      for (Index = 0; Index < (SdtSize - SSDT_PATTERN_LEN); Index++) {

+        if (CompareMem (SdtPtr + Index, Pattern, SSDT_PATTERN_LEN) == 0) {

+          PcdVal = LibPcdGet32 (NameOpReplace[Idx].PcdToken);

+          switch (SdtPtr[Index + SSDT_PATTERN_LEN]) {

+          case AML_NAMEOP_32:

+            SdtPtr[Index + SSDT_PATTERN_LEN + 4] = (PcdVal >> 24) & 0xFF;

+            SdtPtr[Index + SSDT_PATTERN_LEN + 3] = (PcdVal >> 16) & 0xFF;

+            // Fallthrough

+          case AML_NAMEOP_16:

+            SdtPtr[Index + SSDT_PATTERN_LEN + 2] = (PcdVal >> 8) & 0xFF;

+            // Fallthrough

+          case AML_NAMEOP_8:

+            SdtPtr[Index + SSDT_PATTERN_LEN + 1] = PcdVal & 0xFF;

+            break;

+          case 0:

+          case 1:

+            SdtPtr[Index + SSDT_PATTERN_LEN + 1] = !!PcdVal;

+            break;

+          case AML_NAMEOP_STR:

+            /*

+             * If the string val is added to the NameOpReplace, we can

+             * dynamically update things like _HID too as long as the

+             * string length matches.

+             */

+            break;

+          }

+          break;

+        }

+      }

+    }

+  }

+}

+

+

+STATIC BOOLEAN

+VerifyUpdateTable(

+  IN  EFI_ACPI_DESCRIPTION_HEADER *AcpiHeader,

+  IN  NAMESPACE_TABLES *SdtTable

+  )

+{

+  BOOLEAN ret;

+

+  ret = TRUE;

+  if (SdtTable->PcdToken && !LibPcdGet32 (SdtTable->PcdToken)) {

+    ret = FALSE;

+  }

+  if (ret && SdtTable->SdtNameOpReplace) {

+    UpdateSdtNameOps (AcpiHeader, SdtTable->SdtNameOpReplace);

+  }

+

+  return ret;

+}

+

+

+STATIC NAMESPACE_TABLES SdtTables[] = {

+  {

+    SIGNATURE_64 ('R', 'P', 'I', 0, 0, 0, 0, 0),

+    0,

+    NULL

+  },

+  { }

+};

+

+/*

+ * Monitor the ACPI tables being installed and when

+ * a DSDT/SSDT is detected validate that we want to

+ * install it, and if so update any "NameOp" defined

+ * variables contained in the table from PCD values

+ */

+STATIC BOOLEAN

+HandleDynamicNamespace (

+  IN  EFI_ACPI_DESCRIPTION_HEADER *AcpiHeader

+  )

+{

+  UINTN Tables;

+

+  switch (AcpiHeader->Signature) {

+  case SIGNATURE_32 ('D', 'S', 'D', 'T'):

+  case SIGNATURE_32 ('S', 'S', 'D', 'T'):

+    for (Tables = 0; SdtTables[Tables].OemTableId; Tables++) {

+      if (AcpiHeader->OemTableId == SdtTables[Tables].OemTableId) {

+        return VerifyUpdateTable (AcpiHeader, &SdtTables[Tables]);

+      }

+    }

+    DEBUG ((DEBUG_ERROR, "Found namespace table not in table list.\n"));

+

+    return FALSE;

+  }

+

+  return TRUE;

+}

+

+

 EFI_STATUS

 EFIAPI

 ConfigInitialize (

@@ -618,7 +768,8 @@ ConfigInitialize (


   if (PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_ACPI ||

       PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_BOTH) {

-     Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);

+     Status = LocateAndInstallAcpiFromFvConditional (&mAcpiTableFile,

+                                                     &HandleDynamicNamespace);

      ASSERT_EFI_ERROR (Status);

   }



--
2.13.7


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

Re: [PATCH v4 5/6] Platform/RaspberryPi4: Allow the user to set Temp

[Pete Batard]

One remark about using PcdSet32S ()  as opposed to PcdSet32 (), since we 
just went through an exercise making sure that we switched to the secure 
version of these calls.

On 2020.08.31 18:25, Jeremy Linton wrote:
> Now that we have the ability to enable an AML fan object,
> allow the user to select the temperature at which the
> fan cycles on.
> 
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Pete Batard <pete@akeo.ie>
> Cc: Andrei Warkentin <awarkentin@vmware.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>   Platform/RaspberryPi/AcpiTables/SsdtThermal.asl         |  9 +++++----
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      | 11 ++++++++++-
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  1 +
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  5 ++++-
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 16 ++++++++++++++++
>   Platform/RaspberryPi/Include/ConfigVars.h               |  4 ++++
>   Platform/RaspberryPi/RPi3/RPi3.dsc                      |  1 +
>   Platform/RaspberryPi/RPi4/RPi4.dsc                      |  1 +
>   Platform/RaspberryPi/RaspberryPi.dec                    |  1 +
>   9 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl b/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
> index ee028173e1..acfa4699bb 100644
> --- a/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
> +++ b/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
> @@ -23,6 +23,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPITHFAN", 2)
>     {
>       // Define a NameOp we will modify during InstallTable
>       Name (GIOP, 0x2) //08 47 49 4f 50 0a 02 (value must be >1)
> +    Name (FTMP, 0x2)
>       // Describe a fan
>       PowerResource (PFAN, 0, 0) {
>         OperationRegion (GPIO, SystemMemory, GPIO_BASE_ADDRESS, 0x1000)
> @@ -68,9 +69,9 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPITHFAN", 2)
>     // merge in an active cooling point.
>     Scope (\_SB_.EC00.TZ00)
>     {
> -    Method (_AC0) { Return (3332) }        // (60C) active cooling trip point,
> -                                           // if this is lower than PSV then we
> -                                           // prefer active cooling
> -    Name (_AL0, Package () { \_SB_.EC00.FAN0 }) // the fan used for AC0 above
> +    Method (_AC0) { Return ( (FTMP * 10) + 2732) } // (60C) active cooling trip point,
> +                                                   // if this is lower than PSV then we
> +                                                   // prefer active cooling
> +    Name (_AL0, Package () { \_SB_.EC00.FAN0 })    // the fan used for AC0 above
>     }
>   }
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> index d58cbbdfe7..e8f964a329 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> @@ -256,8 +256,16 @@ SetupVariables (
>       PcdSet32 (PcdFanOnGpio, PcdGet32 (PcdFanOnGpio));
> 
>     }
> 
>   
> 
> -  Size = sizeof(AssetTagVar);
> 
> +  Size = sizeof (UINT32);
> 
> +  Status = gRT->GetVariable (L"FanTemp",
> 
> +                  &gConfigDxeFormSetGuid,
> 
> +                  NULL, &Size, &Var32);
> 
> +  if (EFI_ERROR (Status)) {
> 
> +    PcdSet32 (PcdFanTemp, PcdGet32 (PcdFanTemp));


This should be replaced with:

+    Status = PcdSet32S (PcdFanTemp, PcdGet32 (PcdFanTemp));
+    ASSERT_EFI_ERROR (Status);

Rather than re-spin this patch set yet again, and since the rest of the 
series looks good, I'm hoping this change can be applied during integration.

> 
> +  }
> 
> +
> 
>   
> 
> +  Size = sizeof (AssetTagVar);
> 
>     Status = gRT->GetVariable(L"AssetTag",
> 
>                     &gConfigDxeFormSetGuid,
> 
>                     NULL, &Size, AssetTagVar);
> 
> @@ -697,6 +705,7 @@ VerifyUpdateTable(
>   
> 
>   STATIC AML_NAME_OP_REPLACE SsdtNameOpReplace[] = {
> 
>     {"GIOP", PcdToken(PcdFanOnGpio)},
> 
> +  {"FTMP", PcdToken(PcdFanTemp)},
> 
>     {}
> 
>   };
> 
>   
> 
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> index 321e402e65..544e3b3e10 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> @@ -91,6 +91,7 @@
>     gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
> 
>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
> 
>     gRaspberryPiTokenSpaceGuid.PcdFanOnGpio
> 
> +  gRaspberryPiTokenSpaceGuid.PcdFanTemp
> 
>   
> 
>   [Depex]
> 
>     gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid
> 
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> index e2d1bb4b39..2afe8f32ae 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> @@ -49,11 +49,14 @@
>   #string STR_ADVANCED_SYSTAB_DT       #language en-US "Devicetree"
> 
>   
> 
>   #string STR_ADVANCED_FANONGPIO_PROMPT #language en-US "ACPI fan control"
> 
> -#string STR_ADVANCED_FANONGPIO_HELP   #language en-US "Cycle a fan via GPIO if temp exceeds 60C"
> 
> +#string STR_ADVANCED_FANONGPIO_HELP   #language en-US "Cycle a fan via GPIO at given temperature"
> 
>   #string STR_ADVANCED_FANONGPIO_OFF    #language en-US "Disabled"
> 
>   #string STR_ADVANCED_FANONGPIO_18     #language en-US "Fan Shim/GPIO-18"
> 
>   #string STR_ADVANCED_FANONGPIO_19     #language en-US "GPIO-19"
> 
>   
> 
> +#string STR_ADVANCED_FANTEMP_PROMPT   #language en-US "ACPI fan temperature"
> 
> +#string STR_ADVANCED_FANTEMP_HELP     #language en-US "Cycle a fan at C"
> 
> +
> 
>   #string STR_ADVANCED_ASSET_TAG_PROMPT #language en-US "Asset Tag"
> 
>   #string STR_ADVANCED_ASSET_TAG_HELP   #language en-US "Set the system Asset Tag"
> 
>   
> 
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> index 94332caab3..de5e43471a 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> @@ -51,6 +51,11 @@ formset
>         name  = FanOnGpio,
> 
>         guid  = CONFIGDXE_FORM_SET_GUID;
> 
>   
> 
> +    efivarstore ADVANCED_FANTEMP_VARSTORE_DATA,
> 
> +      attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
> 
> +      name  = FanTemp,
> 
> +      guid  = CONFIGDXE_FORM_SET_GUID;
> 
> +
> 
>       efivarstore SYSTEM_TABLE_MODE_VARSTORE_DATA,
> 
>         attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
> 
>         name  = SystemTableMode,
> 
> @@ -191,6 +196,17 @@ formset
>                 option text = STRING_TOKEN(STR_ADVANCED_FANONGPIO_19), value = 19, flags = 0;
> 
>             endoneof;
> 
>           endif;
> 
> +
> 
> +        grayoutif ideqval FanOnGpio.Enabled == 0;
> 
> +          numeric varid = FanTemp.Value,
> 
> +              prompt      = STRING_TOKEN(STR_ADVANCED_FANTEMP_PROMPT),
> 
> +              help        = STRING_TOKEN(STR_ADVANCED_FANTEMP_HELP),
> 
> +              flags       = DISPLAY_UINT_DEC | NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
> 
> +	      minimum = 50,
> 
> +              maximum = 80,
> 
> +              default = 60,
> 
> +          endnumeric;
> 
> +        endif;
> 
>   #endif
> 
>           string varid = AssetTag.AssetTag,
> 
>               prompt  = STRING_TOKEN(STR_ADVANCED_ASSET_TAG_PROMPT),
> 
> diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
> index 1a40469bfa..8094d4ef9a 100644
> --- a/Platform/RaspberryPi/Include/ConfigVars.h
> +++ b/Platform/RaspberryPi/Include/ConfigVars.h
> @@ -73,6 +73,10 @@ typedef struct {
>   } ADVANCED_FAN_ON_GPIO_VARSTORE_DATA;
> 
>   
> 
>   typedef struct {
> 
> +  UINT32 Value;
> 
> +} ADVANCED_FANTEMP_VARSTORE_DATA;
> 
> +
> 
> +typedef struct {
> 
>   #define SYSTEM_TABLE_MODE_ACPI 0
> 
>   #define SYSTEM_TABLE_MODE_BOTH 1
> 
>   #define SYSTEM_TABLE_MODE_DT   2
> 
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> index cef8932ca2..484a46ffba 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> @@ -502,6 +502,7 @@
>     # Enable a fan in the ACPI thermal zone on GPIO pin #
> 
>     #
> 
>     gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
> 
> +  gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0
> 
>   
> 
>     #
> 
>     # Common UEFI ones.
> 
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index 9d0eaf10a1..823c9fc007 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -516,6 +516,7 @@
>     # 19 - Enabled on pin 19
> 
>     #
> 
>     gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0
> 
> +  gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|60
> 
>   
> 
>     #
> 
>     # Common UEFI ones.
> 
> diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
> index a73650f2c3..c64c61930e 100644
> --- a/Platform/RaspberryPi/RaspberryPi.dec
> +++ b/Platform/RaspberryPi/RaspberryPi.dec
> @@ -67,3 +67,4 @@
>     gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|0|UINT32|0x00000019
> 
>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
> 
>     gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|0|UINT32|0x0000001C
> 
> +  gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D
> 

With the suggested change applied:
Reviewed-by: Pete Batard <pete@akeo.ie>

Re: [PATCH v4 5/6] Platform/RaspberryPi4: Allow the user to set Temp

[Ard Biesheuvel]

On 8/31/20 8:57 PM, Pete Batard wrote:
> One remark about using PcdSet32S ()  as opposed to PcdSet32 (), since we 
> just went through an exercise making sure that we switched to the secure 
> version of these calls.
> 
> On 2020.08.31 18:25, Jeremy Linton wrote:
>> Now that we have the ability to enable an AML fan object,
>> allow the user to select the temperature at which the
>> fan cycles on.
>>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Pete Batard <pete@akeo.ie>
>> Cc: Andrei Warkentin <awarkentin@vmware.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   Platform/RaspberryPi/AcpiTables/SsdtThermal.asl         |  9 +++++----
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      | 11 
>> ++++++++++-
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  1 +
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  5 ++++-
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 16 
>> ++++++++++++++++
>>   Platform/RaspberryPi/Include/ConfigVars.h               |  4 ++++
>>   Platform/RaspberryPi/RPi3/RPi3.dsc                      |  1 +
>>   Platform/RaspberryPi/RPi4/RPi4.dsc                      |  1 +
>>   Platform/RaspberryPi/RaspberryPi.dec                    |  1 +
>>   9 files changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl 
>> b/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
>> index ee028173e1..acfa4699bb 100644
>> --- a/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
>> +++ b/Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
>> @@ -23,6 +23,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", 
>> "RPITHFAN", 2)
>>     {
>>       // Define a NameOp we will modify during InstallTable
>>       Name (GIOP, 0x2) //08 47 49 4f 50 0a 02 (value must be >1)
>> +    Name (FTMP, 0x2)
>>       // Describe a fan
>>       PowerResource (PFAN, 0, 0) {
>>         OperationRegion (GPIO, SystemMemory, GPIO_BASE_ADDRESS, 0x1000)
>> @@ -68,9 +69,9 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", 
>> "RPITHFAN", 2)
>>     // merge in an active cooling point.
>>     Scope (\_SB_.EC00.TZ00)
>>     {
>> -    Method (_AC0) { Return (3332) }        // (60C) active cooling 
>> trip point,
>> -                                           // if this is lower than 
>> PSV then we
>> -                                           // prefer active cooling
>> -    Name (_AL0, Package () { \_SB_.EC00.FAN0 }) // the fan used for 
>> AC0 above
>> +    Method (_AC0) { Return ( (FTMP * 10) + 2732) } // (60C) active 
>> cooling trip point,
>> +                                                   // if this is 
>> lower than PSV then we
>> +                                                   // prefer active 
>> cooling
>> +    Name (_AL0, Package () { \_SB_.EC00.FAN0 })    // the fan used 
>> for AC0 above
>>     }
>>   }
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> index d58cbbdfe7..e8f964a329 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> @@ -256,8 +256,16 @@ SetupVariables (
>>       PcdSet32 (PcdFanOnGpio, PcdGet32 (PcdFanOnGpio));
>>
>>     }
>>
>>
>> -  Size = sizeof(AssetTagVar);
>>
>> +  Size = sizeof (UINT32);
>>
>> +  Status = gRT->GetVariable (L"FanTemp",
>>
>> +                  &gConfigDxeFormSetGuid,
>>
>> +                  NULL, &Size, &Var32);
>>
>> +  if (EFI_ERROR (Status)) {
>>
>> +    PcdSet32 (PcdFanTemp, PcdGet32 (PcdFanTemp));
> 
> 
> This should be replaced with:
> 
> +    Status = PcdSet32S (PcdFanTemp, PcdGet32 (PcdFanTemp));
> +    ASSERT_EFI_ERROR (Status);
> 

Agreed.

> Rather than re-spin this patch set yet again, and since the rest of the 
> series looks good, I'm hoping this change can be applied during 
> integration.
> 

That should not be a problem. If nothing else comes up, I will merge 
these tomorrow.


>>
>> +  }
>>
>> +
>>
>>
>> +  Size = sizeof (AssetTagVar);
>>
>>     Status = gRT->GetVariable(L"AssetTag",
>>
>>                     &gConfigDxeFormSetGuid,
>>
>>                     NULL, &Size, AssetTagVar);
>>
>> @@ -697,6 +705,7 @@ VerifyUpdateTable(
>>
>>   STATIC AML_NAME_OP_REPLACE SsdtNameOpReplace[] = {
>>
>>     {"GIOP", PcdToken(PcdFanOnGpio)},
>>
>> +  {"FTMP", PcdToken(PcdFanTemp)},
>>
>>     {}
>>
>>   };
>>
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> index 321e402e65..544e3b3e10 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> @@ -91,6 +91,7 @@
>>     gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
>>
>>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
>>
>>     gRaspberryPiTokenSpaceGuid.PcdFanOnGpio
>>
>> +  gRaspberryPiTokenSpaceGuid.PcdFanTemp
>>
>>
>>   [Depex]
>>
>>     gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
>> index e2d1bb4b39..2afe8f32ae 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
>> @@ -49,11 +49,14 @@
>>   #string STR_ADVANCED_SYSTAB_DT       #language en-US "Devicetree"
>>
>>
>>   #string STR_ADVANCED_FANONGPIO_PROMPT #language en-US "ACPI fan 
>> control"
>>
>> -#string STR_ADVANCED_FANONGPIO_HELP   #language en-US "Cycle a fan 
>> via GPIO if temp exceeds 60C"
>>
>> +#string STR_ADVANCED_FANONGPIO_HELP   #language en-US "Cycle a fan 
>> via GPIO at given temperature"
>>
>>   #string STR_ADVANCED_FANONGPIO_OFF    #language en-US "Disabled"
>>
>>   #string STR_ADVANCED_FANONGPIO_18     #language en-US "Fan 
>> Shim/GPIO-18"
>>
>>   #string STR_ADVANCED_FANONGPIO_19     #language en-US "GPIO-19"
>>
>>
>> +#string STR_ADVANCED_FANTEMP_PROMPT   #language en-US "ACPI fan 
>> temperature"
>>
>> +#string STR_ADVANCED_FANTEMP_HELP     #language en-US "Cycle a fan at C"
>>
>> +
>>
>>   #string STR_ADVANCED_ASSET_TAG_PROMPT #language en-US "Asset Tag"
>>
>>   #string STR_ADVANCED_ASSET_TAG_HELP   #language en-US "Set the 
>> system Asset Tag"
>>
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
>> index 94332caab3..de5e43471a 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
>> @@ -51,6 +51,11 @@ formset
>>         name  = FanOnGpio,
>>
>>         guid  = CONFIGDXE_FORM_SET_GUID;
>>
>>
>> +    efivarstore ADVANCED_FANTEMP_VARSTORE_DATA,
>>
>> +      attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | 
>> EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
>>
>> +      name  = FanTemp,
>>
>> +      guid  = CONFIGDXE_FORM_SET_GUID;
>>
>> +
>>
>>       efivarstore SYSTEM_TABLE_MODE_VARSTORE_DATA,
>>
>>         attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | 
>> EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
>>
>>         name  = SystemTableMode,
>>
>> @@ -191,6 +196,17 @@ formset
>>                 option text = STRING_TOKEN(STR_ADVANCED_FANONGPIO_19), 
>> value = 19, flags = 0;
>>
>>             endoneof;
>>
>>           endif;
>>
>> +
>>
>> +        grayoutif ideqval FanOnGpio.Enabled == 0;
>>
>> +          numeric varid = FanTemp.Value,
>>
>> +              prompt      = STRING_TOKEN(STR_ADVANCED_FANTEMP_PROMPT),
>>
>> +              help        = STRING_TOKEN(STR_ADVANCED_FANTEMP_HELP),
>>
>> +              flags       = DISPLAY_UINT_DEC | NUMERIC_SIZE_4 | 
>> INTERACTIVE | RESET_REQUIRED,
>>
>> +          minimum = 50,
>>
>> +              maximum = 80,
>>
>> +              default = 60,
>>
>> +          endnumeric;
>>
>> +        endif;
>>
>>   #endif
>>
>>           string varid = AssetTag.AssetTag,
>>
>>               prompt  = STRING_TOKEN(STR_ADVANCED_ASSET_TAG_PROMPT),
>>
>> diff --git a/Platform/RaspberryPi/Include/ConfigVars.h 
>> b/Platform/RaspberryPi/Include/ConfigVars.h
>> index 1a40469bfa..8094d4ef9a 100644
>> --- a/Platform/RaspberryPi/Include/ConfigVars.h
>> +++ b/Platform/RaspberryPi/Include/ConfigVars.h
>> @@ -73,6 +73,10 @@ typedef struct {
>>   } ADVANCED_FAN_ON_GPIO_VARSTORE_DATA;
>>
>>
>>   typedef struct {
>>
>> +  UINT32 Value;
>>
>> +} ADVANCED_FANTEMP_VARSTORE_DATA;
>>
>> +
>>
>> +typedef struct {
>>
>>   #define SYSTEM_TABLE_MODE_ACPI 0
>>
>>   #define SYSTEM_TABLE_MODE_BOTH 1
>>
>>   #define SYSTEM_TABLE_MODE_DT   2
>>
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc 
>> b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> index cef8932ca2..484a46ffba 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> @@ -502,6 +502,7 @@
>>     # Enable a fan in the ACPI thermal zone on GPIO pin #
>>
>>     #
>>
>>     
>> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0 
>>
>>
>> +  
>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|0 
>>
>>
>>
>>     #
>>
>>     # Common UEFI ones.
>>
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc 
>> b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> index 9d0eaf10a1..823c9fc007 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> @@ -516,6 +516,7 @@
>>     # 19 - Enabled on pin 19
>>
>>     #
>>
>>     
>> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFormSetGuid|0x0|0 
>>
>>
>> +  
>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormSetGuid|0x0|60 
>>
>>
>>
>>     #
>>
>>     # Common UEFI ones.
>>
>> diff --git a/Platform/RaspberryPi/RaspberryPi.dec 
>> b/Platform/RaspberryPi/RaspberryPi.dec
>> index a73650f2c3..c64c61930e 100644
>> --- a/Platform/RaspberryPi/RaspberryPi.dec
>> +++ b/Platform/RaspberryPi/RaspberryPi.dec
>> @@ -67,3 +67,4 @@
>>     gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|0|UINT32|0x00000019
>>
>>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
>>
>>     gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|0|UINT32|0x0000001C
>>
>> +  gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D
>>
> 
> With the suggested change applied:
> Reviewed-by: Pete Batard <pete@akeo.ie>

Re: [PATCH v4 6/6] Platform/RaspberryPi: Trivial whitespace cleanup

[Ard Biesheuvel]

On 8/31/20 7:25 PM, Jeremy Linton wrote:
> Pete's review pointed out some whitespace issues in the
> context of a previous patch. Since there are a number of
> similar errors in the file lets fix them separately.
> 
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Pete Batard <pete@akeo.ie>
> Cc: Andrei Warkentin <awarkentin@vmware.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Reviewed-by: Pete Batard <@pbatard>
> ---
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 24 +++++++++++-----------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> index e8f964a329..4ed294cdfe 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> @@ -209,9 +209,9 @@ SetupVariables (
>     }
>   
>     Size = sizeof (UINT32);
> -  Status = gRT->GetVariable(L"CustomCpuClock",
> -                            &gConfigDxeFormSetGuid,
> -                            NULL, &Size, &Var32);
> +  Status = gRT->GetVariable (L"CustomCpuClock",
> +                             &gConfigDxeFormSetGuid,
> +                             NULL, &Size, &Var32);
>     if (EFI_ERROR (Status)) {
>       Status = PcdSet32S (PcdCustomCpuClock, PcdGet32 (PcdCustomCpuClock));
>       ASSERT_EFI_ERROR (Status);
> @@ -266,7 +266,7 @@ SetupVariables (
>   
>   
>     Size = sizeof (AssetTagVar);
> -  Status = gRT->GetVariable(L"AssetTag",
> +  Status = gRT->GetVariable (L"AssetTag",
>                     &gConfigDxeFormSetGuid,
>                     NULL, &Size, AssetTagVar);
>   
> @@ -275,7 +275,7 @@ SetupVariables (
>                       L"AssetTag",
>                       &gConfigDxeFormSetGuid,
>                       EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> -                    sizeof(AssetTagVar),
> +                    sizeof (AssetTagVar),
>                       AssetTagVar
>                       );
>     }
> @@ -441,9 +441,9 @@ ApplyVariables (
>        * spaces. SystemMemorySizeBelow4GB tracks the maximum memory below 4GB
>        * line, factoring in the limit imposed by the SoC register range.
>        */
> -    SystemMemorySizeBelow4GB = MIN(SystemMemorySize, 4UL * SIZE_1GB);
> -    SystemMemorySizeBelow4GB = MIN(SystemMemorySizeBelow4GB, BCM2836_SOC_REGISTERS);
> -    SystemMemorySizeBelow4GB = MIN(SystemMemorySizeBelow4GB, BCM2711_SOC_REGISTERS);
> +    SystemMemorySizeBelow4GB = MIN (SystemMemorySize, 4UL * SIZE_1GB);
> +    SystemMemorySizeBelow4GB = MIN (SystemMemorySizeBelow4GB, BCM2836_SOC_REGISTERS);
> +    SystemMemorySizeBelow4GB = MIN (SystemMemorySizeBelow4GB, BCM2711_SOC_REGISTERS);
>   
>       ASSERT (SystemMemorySizeBelow4GB > 3UL * SIZE_1GB);
>   
> @@ -536,14 +536,14 @@ ApplyVariables (
>         /*
>          * SD card pins go to Arasan.
>          */
> -      MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
> -                  MmioRead32(GPIO_BASE_ADDRESS + 0xD0) | 0x2);
> +      MmioWrite32 ((GPIO_BASE_ADDRESS + 0xD0),
> +                  MmioRead32 (GPIO_BASE_ADDRESS + 0xD0) | 0x2);
>       } else {
>         /*
>          * SD card pins back to eMMC2.
>          */
> -      MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
> -                  MmioRead32(GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
> +      MmioWrite32 ((GPIO_BASE_ADDRESS + 0xD0),
> +                  MmioRead32 (GPIO_BASE_ADDRESS + 0xD0) & ~0x2);

Anyone mind if I replace these with MmioOr32 / MmioAnd32 ?


>         /*
>          * WiFi back to Arasan.
>          */
> 

Re: [PATCH v4 6/6] Platform/RaspberryPi: Trivial whitespace cleanup

[Pete Batard]

Hi Ard,

On 2020.09.01 12:39, Ard Biesheuvel wrote:
> On 8/31/20 7:25 PM, Jeremy Linton wrote:
>> Pete's review pointed out some whitespace issues in the
>> context of a previous patch. Since there are a number of
>> similar errors in the file lets fix them separately.
>>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Pete Batard <pete@akeo.ie>
>> Cc: Andrei Warkentin <awarkentin@vmware.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> Reviewed-by: Pete Batard <@pbatard>
>> ---
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 24 
>> +++++++++++-----------
>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> index e8f964a329..4ed294cdfe 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> @@ -209,9 +209,9 @@ SetupVariables (
>>     }
>>     Size = sizeof (UINT32);
>> -  Status = gRT->GetVariable(L"CustomCpuClock",
>> -                            &gConfigDxeFormSetGuid,
>> -                            NULL, &Size, &Var32);
>> +  Status = gRT->GetVariable (L"CustomCpuClock",
>> +                             &gConfigDxeFormSetGuid,
>> +                             NULL, &Size, &Var32);
>>     if (EFI_ERROR (Status)) {
>>       Status = PcdSet32S (PcdCustomCpuClock, PcdGet32 
>> (PcdCustomCpuClock));
>>       ASSERT_EFI_ERROR (Status);
>> @@ -266,7 +266,7 @@ SetupVariables (
>>     Size = sizeof (AssetTagVar);
>> -  Status = gRT->GetVariable(L"AssetTag",
>> +  Status = gRT->GetVariable (L"AssetTag",
>>                     &gConfigDxeFormSetGuid,
>>                     NULL, &Size, AssetTagVar);
>> @@ -275,7 +275,7 @@ SetupVariables (
>>                       L"AssetTag",
>>                       &gConfigDxeFormSetGuid,
>>                       EFI_VARIABLE_NON_VOLATILE | 
>> EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
>> -                    sizeof(AssetTagVar),
>> +                    sizeof (AssetTagVar),
>>                       AssetTagVar
>>                       );
>>     }
>> @@ -441,9 +441,9 @@ ApplyVariables (
>>        * spaces. SystemMemorySizeBelow4GB tracks the maximum memory 
>> below 4GB
>>        * line, factoring in the limit imposed by the SoC register range.
>>        */
>> -    SystemMemorySizeBelow4GB = MIN(SystemMemorySize, 4UL * SIZE_1GB);
>> -    SystemMemorySizeBelow4GB = MIN(SystemMemorySizeBelow4GB, 
>> BCM2836_SOC_REGISTERS);
>> -    SystemMemorySizeBelow4GB = MIN(SystemMemorySizeBelow4GB, 
>> BCM2711_SOC_REGISTERS);
>> +    SystemMemorySizeBelow4GB = MIN (SystemMemorySize, 4UL * SIZE_1GB);
>> +    SystemMemorySizeBelow4GB = MIN (SystemMemorySizeBelow4GB, 
>> BCM2836_SOC_REGISTERS);
>> +    SystemMemorySizeBelow4GB = MIN (SystemMemorySizeBelow4GB, 
>> BCM2711_SOC_REGISTERS);
>>       ASSERT (SystemMemorySizeBelow4GB > 3UL * SIZE_1GB);
>> @@ -536,14 +536,14 @@ ApplyVariables (
>>         /*
>>          * SD card pins go to Arasan.
>>          */
>> -      MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
>> -                  MmioRead32(GPIO_BASE_ADDRESS + 0xD0) | 0x2);
>> +      MmioWrite32 ((GPIO_BASE_ADDRESS + 0xD0),
>> +                  MmioRead32 (GPIO_BASE_ADDRESS + 0xD0) | 0x2);
>>       } else {
>>         /*
>>          * SD card pins back to eMMC2.
>>          */
>> -      MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
>> -                  MmioRead32(GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
>> +      MmioWrite32 ((GPIO_BASE_ADDRESS + 0xD0),
>> +                  MmioRead32 (GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
> 
> Anyone mind if I replace these with MmioOr32 / MmioAnd32 ?

No objection from me. This should indeed make the code cleaner.

Thanks,

/Pete

> 
> 
>>         /*
>>          * WiFi back to Arasan.
>>          */
>>
> 

Re: [PATCH v4 0/6] Platform/RasberryPi: Thermal zone

[Ard Biesheuvel]

On 8/31/20 7:25 PM, Jeremy Linton wrote:
> This set creates a basic thermal zone, which reads the
> SOC temp via a direct register read in AML. It also
> adds an active cooling policy using a GPIO pin for fan
> control that can optionally be enabled/disabled on either
> GPIO18 (commercial fan shim board) or GPIO19 by the
> user from the BDS.
> 
> it should now be possible when booted in ACPI mode to:
> 
> # sensors
> acpitz-acpi-0
> Adapter: ACPI interface
> temp1:        +57.6C  (crit = +90.0C)
> 
> and the fan state, if enabled may be read with:
> 
> /sys/bus/acpi/devices/PNP0C06:00/PNP0C0B:00/physical_node/thermal_cooling/cur_state
> 
> while the kernel will automatically cycle the fan if the SoC
> temp exceeds 60C.
> 
> Included as a byproduct of this set is a generic method
> for updating values in ACPI DSDT/SSDT tables as they
> are installed.
> 
> v3->v4:
>    Allow the user to set the fan trip point
>    Extend ACPI device names to 4 characters
> 
> v2->v3:
>    Make DSDT/SSDT PCD->Nameop update code generic
>    Move the fan SSDT code back into the AcpiTable directory
> 
> v1->v2:
>    Add choice of GPIO pins to the BDS menu
>    Correct whitespace/etc issues from v1 review
>    Add an additional cleanup patch for contextual whitespace issues
> 
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Pete Batard <pete@akeo.ie>
> Cc: Andrei Warkentin <awarkentin@vmware.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> 
> Jeremy Linton (6):
>    Platform/RaspberryPi4: Add a basic thermal zone
>    Platform/RaspberryPi: Monitor ACPI Table installs
>    Platform/RaspberryPi: Add entry for user fan control
>    Platform/RaspberryPi4: Create ACPI fan object
>    Platform/RaspberryPi4: Allow the user to set Temp
>    Platform/RaspberryPi: Trivial whitespace cleanup
> 

Series pushed as 50639477fc0f..e08fccd8bed5

Note that I incorporated the changes suggested by Pete, as well as the 
Mmio Or/And tweak I mentioned in response to patch #6. I also applied 
some touch ups to patch #2, to make the coding style more idiomatic for 
EDK2, and to use CONST structures where possible.

Thanks all. This is a really nice feature to have, especially as it can 
serve as an example for other platforms as well.



>   Platform/RaspberryPi/AcpiTables/AcpiTables.inf     |   1 +
>   Platform/RaspberryPi/AcpiTables/Dsdt.asl           |  31 +++
>   Platform/RaspberryPi/AcpiTables/SsdtThermal.asl    |  77 ++++++++
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 214 +++++++++++++++++++--
>   .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |   3 +
>   .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |   9 +
>   .../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr |  34 ++++
>   Platform/RaspberryPi/Include/ConfigVars.h          |   8 +
>   Platform/RaspberryPi/RPi3/RPi3.dsc                 |   6 +
>   Platform/RaspberryPi/RPi4/RPi4.dsc                 |   9 +
>   Platform/RaspberryPi/RaspberryPi.dec               |   2 +
>   .../Bcm27xx/Include/IndustryStandard/Bcm2711.h     |   2 +
>   12 files changed, 382 insertions(+), 14 deletions(-)
>   create mode 100644 Platform/RaspberryPi/AcpiTables/SsdtThermal.asl
>