[edk2-devel] [PATCH v2 3/4] Platform/RaspberryPi: Add entry for user fan control
Jeremy Linton
jeremy.linton at arm.com
Fri Aug 21 04:15:56 UTC 2020
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 at arm.com>
> Sent: Wednesday, August 19, 2020 11:42 PM
> To: devel at edk2.groups.io <devel at edk2.groups.io>
> Cc: Jeremy Linton <jeremy.linton at arm.com>; Leif Lindholm <leif at nuviainc.com>; Pete Batard <pete at akeo.ie>; Andrei Warkentin <awarkentin at vmware.com>; Ard Biesheuvel <ard.biesheuvel at arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at 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 at nuviainc.com>
> Cc: Pete Batard <pete at akeo.ie>
> Cc: Andrei Warkentin <awarkentin at vmware.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>
> Signed-off-by: Jeremy Linton <jeremy.linton at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list