Hi,

On 8/20/20 11:15 AM, Andrei Warkentin wrote:
Hi Jeremy,

Overall, great idea. An improvement on your implementation would be to use 
LocateAndInstallAcpiFromFvConditional (which will call your call back on every 
table, so you can allow/disallow/patch) instead of calling 
LocateAndInstallAcpiFromFv and the locating the SSDT manually.

Yes, I think a few people have heard me complaining about the lack of dyanmic PCD values being easily accessable from AML. It should be easier so we can provide more customization (the actual thermal limits for one) without a lot of additional effort.

I switched to doing this NameOp binary modification from the multiple pass SSDT compiles when I noticed another platform doing binary edits (which I had assumed was a litle to "hacky" for upstream). But, I didn't go to far down that path because the obvious next couple steps are providing a generic PCD->Name association structure and doing it for all DSDT/SSDT's being loaded. But I suspect that it would be a lot more useful to the edk2 project as a whole if we can encode the association directly in the AML with a edk2 specific macro and so i've assumed the time would be better spent coding up a deterministic way to do that.

(which at a minimum is probably a better binary parser than memcmp, or at least a edk2 specific custom nameobj assigment we can key off to upgrade to a standards compliant aml blob)

So, at the moment this is mostly a ok, let me look into it more response... :)



A
________________________________
From: Jeremy Linton <jeremy.lin...@arm.com>
Sent: Wednesday, August 19, 2020 11:42 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Jeremy Linton <jeremy.lin...@arm.com>; Leif Lindholm <l...@nuviainc.com>; Pete Batard 
<p...@akeo.ie>; Andrei Warkentin <awarken...@vmware.com>; Ard Biesheuvel 
<ard.biesheu...@arm.com>; Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>
Subject: [PATCH v2 3/4] Platform/RaspberryPi: Add entry for user fan control

Add a menu item that allows the user to enable GPIO based
fan control via SSDT. This should only be seen/enabled on RPI4
because that is what its been tested with. As of this commit
its currently limited to only operating on a single GPIO pin (19).

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

It appears there are a couple boards (fan SHIM) which operate this
way, and probably should have custom menu items/SSDT edits as
people acquire the boards and test them.

Cc: Leif Lindholm <l...@nuviainc.com>
Cc: Pete Batard <p...@akeo.ie>
Cc: Andrei Warkentin <awarken...@vmware.com>
Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>
Signed-off-by: Jeremy Linton <jeremy.lin...@arm.com>
Reviewed-by: Pete Batard <@pbatard>
---
  Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 78 ++++++++++++++++++++++
  .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  3 +
  .../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, 123 insertions(+)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c 
b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index af54136ade..24e65eeb5e 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,8 +576,71 @@ 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);

+  }

  }



+#define SSDT_PATTERN_LEN 6

+

+EFI_STATUS

+FindInstallSsdt (UINT64 OemTableId, CHAR8 *Name, UINT8 Value)

+{

+  EFI_ACPI_TABLE_PROTOCOL         *AcpiTable;

+  UINTN                           Index;

+  EFI_ACPI_DESCRIPTION_HEADER     *Ssdt;

+  UINTN                           SsdtSize;

+  EFI_STATUS                      Status;

+  UINTN                           TableKey;

+  CHAR8                           Pattern[SSDT_PATTERN_LEN];

+

+

+  Status = gBS->LocateProtocol (&gEfiAcpiTableProtocolGuid, NULL,

+                                (VOID **)&AcpiTable);

+  if (EFI_ERROR (Status)) {

+    return Status;

+  }

+

+  for (Index = 0; !EFI_ERROR (Status); Index++) {

+    Status = GetSectionFromFv (&gEfiCallerIdGuid, EFI_SECTION_RAW, Index,

+                               (VOID **)&Ssdt, &SsdtSize);

+    if (Ssdt->OemTableId == OemTableId)

+      break;

+    SsdtSize = 0;

+  }

+

+  if (SsdtSize > 0) {

+    /*

+     * Do a single 8 bit 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;    //NameOp

+    Pattern[1] = Name[0]; //Name

+    Pattern[2] = Name[1];

+    Pattern[3] = Name[2];

+    Pattern[4] = Name[3];

+    Pattern[5] = 0x0A;    //Size

+

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

+      if (CompareMem ((UINT8*)Ssdt + Index, Pattern, SSDT_PATTERN_LEN) == 0) {

+        *((UINT8 *)Ssdt + Index + SSDT_PATTERN_LEN) = Value;

+        break;

+      }

+    }

+

+    Status = AcpiTable->InstallAcpiTable (AcpiTable, Ssdt, SsdtSize,

+                                          &TableKey);

+    if (EFI_ERROR (Status)) {

+      DEBUG ((DEBUG_WARN, "%a: failed to install SSDT table %r\n",

+              __FUNCTION__, Status));

+    }

+  }

+

+  return Status;

+}



  EFI_STATUS

  EFIAPI

@@ -620,6 +694,10 @@ ConfigInitialize (
        PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_BOTH) {

       Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);

       ASSERT_EFI_ERROR (Status);

+     if (PcdGet32 (PcdFanOnGpio)) {

+       FindInstallSsdt (SIGNATURE_64 ('R', 'P', 'I', 'T', 'H', 'F', 'A', 'N'),

+                        "GIOP", (UINT8)PcdGet32 (PcdFanOnGpio));

+     }

    }



    Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_NOTIFY, RegisterDevices,

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf 
b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
index cdce35bc74..fe3a01a570 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
@@ -28,6 +28,7 @@
    ConfigDxeFormSetGuid.h

    ConfigDxeHii.vfr

    ConfigDxeHii.uni

+  SsdtThermal.asl

    XhciQuirk.c



  [Packages]

@@ -46,6 +47,7 @@
    AcpiLib

    BaseLib

    DebugLib

+  DxeServicesLib

    DxeServicesTableLib

    GpioLib

    HiiLib

@@ -89,6 +91,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




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#64522): https://edk2.groups.io/g/devel/message/64522
Mute This Topic: https://groups.io/mt/76302320/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to