Re: [PATCH v2 3/4] Platform/RaspberryPi: Add entry for user fan control

["Jeremy Linton" <jeremy.linton@arm.com>]

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.linton@arm.com>
> Sent: Wednesday, August 19, 2020 11:42 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 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 <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 | 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
> 
>