From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
To: Pete Batard <pete@akeo.ie>, Jeremy Linton <jeremy.linton@arm.com>,
devel@edk2.groups.io
Cc: Leif Lindholm <leif@nuviainc.com>,
Andrei Warkentin <awarkentin@vmware.com>,
Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Subject: Re: [PATCH v4 5/6] Platform/RaspberryPi4: Allow the user to set Temp
Date: Mon, 31 Aug 2020 22:05:32 +0300 [thread overview]
Message-ID: <9d8404ea-cf5e-bd65-f1b8-96eefbbd3ece@arm.com> (raw)
In-Reply-To: <f06fb1de-aa36-4101-025f-8a4d294ccd72@akeo.ie>
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>
next prev parent reply other threads:[~2020-08-31 19:05 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-31 17:25 [PATCH v4 0/6] Platform/RasberryPi: Thermal zone Jeremy Linton
2020-08-31 17:25 ` [PATCH v4 1/6] Platform/RaspberryPi4: Add a basic thermal zone Jeremy Linton
2020-08-31 17:25 ` [PATCH v4 2/6] Platform/RaspberryPi: Monitor ACPI Table installs Jeremy Linton
2020-08-31 17:32 ` Andrei Warkentin
2020-08-31 17:25 ` [PATCH v4 3/6] Platform/RaspberryPi: Add entry for user fan control Jeremy Linton
2020-08-31 17:25 ` [PATCH v4 4/6] Platform/RaspberryPi4: Create ACPI fan object Jeremy Linton
2020-08-31 17:25 ` [PATCH v4 5/6] Platform/RaspberryPi4: Allow the user to set Temp Jeremy Linton
2020-08-31 18:57 ` Pete Batard
2020-08-31 19:05 ` Ard Biesheuvel [this message]
2020-08-31 17:25 ` [PATCH v4 6/6] Platform/RaspberryPi: Trivial whitespace cleanup Jeremy Linton
2020-09-01 11:39 ` Ard Biesheuvel
2020-09-01 11:46 ` Pete Batard
2020-09-01 12:23 ` [PATCH v4 0/6] Platform/RasberryPi: Thermal zone Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9d8404ea-cf5e-bd65-f1b8-96eefbbd3ece@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox