From: "Pete Batard" <pete@akeo.ie>
To: Jeremy Linton <jeremy.linton@arm.com>, devel@edk2.groups.io
Cc: Leif Lindholm <leif@nuviainc.com>,
Andrei Warkentin <awarkentin@vmware.com>,
Ard Biesheuvel <ard.biesheuvel@arm.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 19:57:43 +0100 [thread overview]
Message-ID: <f06fb1de-aa36-4101-025f-8a4d294ccd72@akeo.ie> (raw)
In-Reply-To: <20200831172549.24079-6-jeremy.linton@arm.com>
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);
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.
>
> + }
>
> +
>
>
>
> + 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 18:57 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 [this message]
2020-08-31 19:05 ` Ard Biesheuvel
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=f06fb1de-aa36-4101-025f-8a4d294ccd72@akeo.ie \
--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