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

Mon, 31 Aug 2020 12:05:54 -0700 

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 <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>
---
  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 <p...@akeo.ie>



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

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























					Reply via email to