From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by mx.groups.io with SMTP id smtpd.web12.60705.1598900266551107007 for ; Mon, 31 Aug 2020 11:57:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=s1BH3EJ9; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.68, mailfrom: pete@akeo.ie) Received: by mail-wm1-f68.google.com with SMTP id s13so488554wmh.4 for ; Mon, 31 Aug 2020 11:57:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=8OFAWMj3K8XnLkcrdBDEJqXBKbKXrFNjuokXa9BnF+U=; b=s1BH3EJ9fr+JSk2euRUdiJZkRn5TEQL9haAY310JcfxOoqwQRkx7fkadYXxrJtST4z A+ETA7DcpP18sIQXJqTQWvwXNSJ+YI6Es1Y6ppsfB9t1S4f2wy0dWbeiJtqN7OLE7PiI jGEa0p4UpJiXFD7FBJbDDejiMGvR1B7j9hBv9CPfw9Ulp1rjyt/BAoYgjPjJjugdH0+x NNCxaLZvEQsaWDDKz4+NiAx7+LxdTdojD3mCidSUQ11hJPk7njz9LdBMXyy9lHSwKrKz YWsE5r0kYsfp/Mko1j+ZD5XLXxKec+zZZiFjV4jXuTvyO9Z0hPrjRxHaX2QUxU0roXq7 eoNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=8OFAWMj3K8XnLkcrdBDEJqXBKbKXrFNjuokXa9BnF+U=; b=tT5FwsbuzURNwe/vn+pPxs3Xd82YCkxTDI01k5ab1c5uTlDQxQ+GEpyhPNmcE6jTcy wdDQBXfm8zXa3a0iITnTpnuxtONaT1DR2wIaQw/JXKSBebteLiqM8XPy16d6xI7PW05V heQEM0SvClk3ilQjN4PD/VQw/+XCrlcM3jeDcnI1mXAP0goNcQUZTyZ5rtSUIZXOn4Ng kVJO+hrttHt1cqkCD5m+AwKT9yBxv3B+XEy1UPHfxA/VPpBya4QRmHs6Ko04dVm/LC2x TyP0bIBhJ31+0uLXqh3w8ly+aQNIYDV95LUQGlt+W/2FhOklpS1MG5VjJocrGqKpXNzg 1H3g== X-Gm-Message-State: AOAM530CQPEv/LREoMfOZGb4fu+W8CxPToN1wqEWUg+JVofgKB64m3Dw ndSx0kIDi0Cg5yXsroQD4Hr/5A== X-Google-Smtp-Source: ABdhPJxFYE091AUGlrM7LNbnSLr+1JBYSYEMbzNw6CkERUtRIukwkx+fIK/IBSjyTImrhtiehyDkbQ== X-Received: by 2002:a7b:c8da:: with SMTP id f26mr707808wml.126.1598900264989; Mon, 31 Aug 2020 11:57:44 -0700 (PDT) Return-Path: Received: from [10.0.0.130] ([84.203.85.10]) by smtp.googlemail.com with ESMTPSA id 188sm1725356wmz.2.2020.08.31.11.57.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 31 Aug 2020 11:57:44 -0700 (PDT) Subject: Re: [PATCH v4 5/6] Platform/RaspberryPi4: Allow the user to set Temp To: Jeremy Linton , devel@edk2.groups.io Cc: Leif Lindholm , Andrei Warkentin , Ard Biesheuvel , Samer El-Haj-Mahmoud References: <20200831172549.24079-1-jeremy.linton@arm.com> <20200831172549.24079-6-jeremy.linton@arm.com> From: "Pete Batard" Message-ID: Date: Mon, 31 Aug 2020 19:57:43 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20200831172549.24079-6-jeremy.linton@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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 > Cc: Pete Batard > Cc: Andrei Warkentin > Cc: Ard Biesheuvel > Cc: Samer El-Haj-Mahmoud > Signed-off-by: Jeremy Linton > --- > 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