[edk2-devel][PATCH v2 0/1] Platform/RPi/ConfigDxe: Improve CPU Frequency configuration

[edk2-devel][PATCH v2 0/1] Platform/RPi/ConfigDxe: Improve CPU Frequency configuration

[Pete Batard]

This patch supersedes https://edk2.groups.io/g/devel/topic/71809869#55659.

It's more or less a complete rewrite of the previous proposal, as it
introduces new "Default" and "Low" CPU frequency settings and does away
with the need for platform specific strings altogether.

More importantly, it ensures that the default CPU frequency for the
each platform will be set to its expected defaults, as documented on:
https://www.raspberrypi.org/documentation/configuration/config-txt/overclocking.md
instead of a very low frequency.


Pete Batard (1):
  Platform/RPi/ConfigDxe: Improve CPU Frequency configuration

 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      | 41 ++++++++++++--------
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  5 +++
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  6 +--
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 12 +++---
 Platform/RaspberryPi/RPi3/RPi3.dsc                      | 11 +++++-
 Platform/RaspberryPi/RPi4/RPi4.dsc                      | 11 +++++-
 Platform/RaspberryPi/RaspberryPi.dec                    |  3 ++
 7 files changed, 60 insertions(+), 29 deletions(-)

-- 
2.21.0.windows.1

[edk2-devel][PATCH v2 1/1] Platform/RPi/ConfigDxe: Improve CPU Frequency configuration

[Pete Batard]

Improve the CPU frequency settings of the platforms by:
- Adding a "Default" option that sets the frequency to the
  official default for each model/submodel.
- Adding a "Low" option, that sets the frequency to a fixed
  PCD, custom to each platform.
- Using fixed PCDs to set the maximum and default values for
  the custom frequency range, according to the overclocking
  capabilities of the platform.
- Ensuring that the firmware defaults to using the platform's
  default frequency, instead of a low arbitrary value.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      | 41 ++++++++++++--------
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  5 +++
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  6 +--
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 12 +++---
 Platform/RaspberryPi/RPi3/RPi3.dsc                      | 11 +++++-
 Platform/RaspberryPi/RPi4/RPi4.dsc                      | 11 +++++-
 Platform/RaspberryPi/RaspberryPi.dec                    |  3 ++
 7 files changed, 60 insertions(+), 29 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index b4b13b6798f9..1091e680ff85 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -24,6 +24,8 @@
 #include <Protocol/RpiFirmware.h>
 #include "ConfigDxeFormSetGuid.h"
 
+#define FREQ_1_MHZ 1000000
+
 extern UINT8 ConfigDxeHiiBin[];
 extern UINT8 ConfigDxeStrings[];
 
@@ -256,28 +258,35 @@ ApplyVariables (
   UINT32 Rate = 0;
   UINT64 SystemMemorySize;
 
-  if (CpuClock != 0) {
-    if (CpuClock == 2) {
-      /*
-       * Maximum: 1.2GHz on RPi 3, 1.4GHz on RPi 3B+, unless
-       * overridden with arm_freq=xxx in config.txt.
-       */
-      Status = mFwProtocol->GetMaxClockRate (RPI_MBOX_CLOCK_RATE_ARM, &Rate);
-      if (Status != EFI_SUCCESS) {
-        DEBUG ((DEBUG_ERROR, "Couldn't get the max CPU speed, leaving as is: %r\n", Status));
-      }
-    } else if (CpuClock == 3) {
-      Rate = CustomCpuClock * 1000000;
-    } else {
-      Rate = 600 * 1000000;
+  switch (CpuClock) {
+  case 0: // Low
+    Rate = FixedPcdGet32 (PcdCpuLowSpeedMHz) * FREQ_1_MHZ;
+    break;
+  case 1: // Default
+    /*
+     * What the Raspberry Pi Foundation calls "max clock rate" is really the default value
+     * from: https://www.raspberrypi.org/documentation/configuration/config-txt/overclocking.md
+     */
+    Status = mFwProtocol->GetMaxClockRate (RPI_MBOX_CLOCK_RATE_ARM, &Rate);
+    if (Status != EFI_SUCCESS) {
+      DEBUG ((DEBUG_ERROR, "Couldn't read default CPU speed %r\n", Status));
     }
+    break;
+  case 2: // Max
+    Rate = FixedPcdGet32 (PcdCpuMaxSpeedMHz) * FREQ_1_MHZ;
+    break;
+  case 3: // Custom
+    Rate = CustomCpuClock * FREQ_1_MHZ;
+    break;
   }
 
   if (Rate != 0) {
-    DEBUG ((DEBUG_INFO, "Setting CPU speed to %uHz\n", Rate));
+    DEBUG ((DEBUG_INFO, "Setting CPU speed to %u MHz\n", Rate / FREQ_1_MHZ));
     Status = mFwProtocol->SetClockRate (RPI_MBOX_CLOCK_RATE_ARM, Rate, 1);
     if (Status != EFI_SUCCESS) {
       DEBUG ((DEBUG_ERROR, "Couldn't set the CPU speed: %r\n", Status));
+    } else {
+      PcdSet32 (PcdCustomCpuClock, Rate / FREQ_1_MHZ);
     }
   }
 
@@ -285,7 +294,7 @@ ApplyVariables (
   if (Status != EFI_SUCCESS) {
     DEBUG ((DEBUG_ERROR, "Couldn't get the CPU speed: %r\n", Status));
   } else {
-    DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
+    DEBUG ((DEBUG_INFO, "Current CPU speed is %u MHz\n", Rate / FREQ_1_MHZ));
   }
 
   if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) != 0 &&
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
index 736d49df562b..57963baf9c90 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
@@ -57,6 +57,11 @@ [Protocols]
   gRaspberryPiFirmwareProtocolGuid      ## CONSUMES
   gRaspberryPiConfigAppliedProtocolGuid ## PRODUCES
 
+[FixedPcd]
+  gRaspberryPiTokenSpaceGuid.PcdCpuLowSpeedMHz
+  gRaspberryPiTokenSpaceGuid.PcdCpuDefSpeedMHz
+  gRaspberryPiTokenSpaceGuid.PcdCpuMaxSpeedMHz
+
 [Pcd]
   gBcm27xxTokenSpaceGuid.PcdBcm27xxRegistersAddress
   gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
index 77eda96d8136..046c75f9bfab 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
@@ -22,13 +22,13 @@
 
 #string STR_CHIPSET_CLOCK_CPU_PROMPT #language en-US "CPU Clock"
 #string STR_CHIPSET_CLOCK_CPU_HELP   #language en-US "CPU Speed"
-#string STR_CHIPSET_CLOCK_CPU_NA     #language en-US "Don't Override"
-#string STR_CHIPSET_CLOCK_CPU_600MHZ #language en-US "Min (600MHz)"
+#string STR_CHIPSET_CLOCK_CPU_LOW    #language en-US "Low"
+#string STR_CHIPSET_CLOCK_CPU_DEF    #language en-US "Default"
 #string STR_CHIPSET_CLOCK_CPU_MAX    #language en-US "Max"
 #string STR_CHIPSET_CLOCK_CPU_CUSTOM #language en-US "Custom"
 
 #string STR_CHIPSET_CUSTOM_CPU_CLOCK_PROMPT #language en-US "CPU Clock Rate (MHz)"
-#string STR_CHIPSET_CUSTOM_CPU_CLOCK_HELP   #language en-US "Adjust the CPU speed.\nMin value: 100 MHz\nMax value: 1600 MHz\n\nWarning: Overclocking can make the system unbootable!"
+#string STR_CHIPSET_CUSTOM_CPU_CLOCK_HELP   #language en-US "Manually set the CPU Speed\n\nWarning: Overclocking can make the system unbootable!"
 
 /*
  * Advanced configuration.
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
index 9c2fd64a8e27..bab96885b78c 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
@@ -53,8 +53,8 @@ typedef struct {
 
 typedef struct {
   /*
-   * 0 - don't change the clock rate.
-   * 1 - 600MHz.
+   * 0 - low.
+   * 1 - default.
    * 2 - maximum.
    * 3 - custom.
    */
@@ -253,8 +253,8 @@ formset
             prompt      = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_PROMPT),
             help        = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_HELP),
             flags       = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
-            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_NA), value = 0, flags = DEFAULT;
-            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_600MHZ), value = 1, flags = 0;
+            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_LOW), value = 0, flags = 0;
+            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_DEF), value = 1, flags = DEFAULT;
             option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_MAX), value = 2, flags = 0;
             option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_CUSTOM), value = 3, flags = 0;
         endoneof;
@@ -265,8 +265,8 @@ formset
               help    = STRING_TOKEN(STR_CHIPSET_CUSTOM_CPU_CLOCK_HELP),
               flags   = DISPLAY_UINT_DEC | NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
               minimum = 100,
-              maximum = 1600,
-              default = 600,
+              maximum = FixedPcdGet32 (PcdCpuMaxSpeedMHz),
+              default = FixedPcdGet32 (PcdCpuDefSpeedMHz),
           endnumeric;
         endif;
     endform;
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index 91d5738afbc6..d92f5cc1863d 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -396,6 +396,13 @@ [PcdsFixedAtBuild.common]
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultReceiveFifoDepth|0
 
+  #
+  # Fixed CPU settings.
+  #
+  gRaspberryPiTokenSpaceGuid.PcdCpuLowSpeedMHz|600
+  gRaspberryPiTokenSpaceGuid.PcdCpuDefSpeedMHz|1200
+  gRaspberryPiTokenSpaceGuid.PcdCpuMaxSpeedMHz|1500
+
   ## Default Terminal Type
   ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
   gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
@@ -412,8 +419,8 @@ [PcdsDynamicHii.common.DEFAULT]
   # Clock overrides.
   #
 
-  gRaspberryPiTokenSpaceGuid.PcdCpuClock|L"CpuClock"|gConfigDxeFormSetGuid|0x0|0
-  gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock|L"CustomCpuClock"|gConfigDxeFormSetGuid|0x0|600
+  gRaspberryPiTokenSpaceGuid.PcdCpuClock|L"CpuClock"|gConfigDxeFormSetGuid|0x0|1
+  gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock|L"CustomCpuClock"|gConfigDxeFormSetGuid|0x0|gRaspberryPiTokenSpaceGuid.PcdCpuDefSpeedMHz
 
   #
   # SD-related.
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 2e98c3e16b91..aabf3566360f 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -435,6 +435,13 @@ [PcdsFixedAtBuild.common]
   gArmTokenSpaceGuid.PcdGicDistributorBase|0xFF841000
   gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xFF842000
 
+  #
+  # Fixed CPU settings.
+  #
+  gRaspberryPiTokenSpaceGuid.PcdCpuLowSpeedMHz|800
+  gRaspberryPiTokenSpaceGuid.PcdCpuDefSpeedMHz|1500
+  gRaspberryPiTokenSpaceGuid.PcdCpuMaxSpeedMHz|2200
+
   ## Default Terminal Type
   ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
   gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
@@ -451,8 +458,8 @@ [PcdsDynamicHii.common.DEFAULT]
   # Clock overrides.
   #
 
-  gRaspberryPiTokenSpaceGuid.PcdCpuClock|L"CpuClock"|gConfigDxeFormSetGuid|0x0|0
-  gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock|L"CustomCpuClock"|gConfigDxeFormSetGuid|0x0|600
+  gRaspberryPiTokenSpaceGuid.PcdCpuClock|L"CpuClock"|gConfigDxeFormSetGuid|0x0|1
+  gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock|L"CustomCpuClock"|gConfigDxeFormSetGuid|0x0|gRaspberryPiTokenSpaceGuid.PcdCpuDefSpeedMHz
 
   #
   # SD-related.
diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
index 1355cdee0534..b51eefeb09fb 100644
--- a/Platform/RaspberryPi/RaspberryPi.dec
+++ b/Platform/RaspberryPi/RaspberryPi.dec
@@ -44,6 +44,9 @@ [PcdsFixedAtBuild.common]
   gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwWorkingBase|0x0|UINT32|0x00000007
   gRaspberryPiTokenSpaceGuid.PcdExtendedMemoryBase|0x0|UINT32|0x00000008
   gRaspberryPiTokenSpaceGuid.PcdFdtSize|0x10000|UINT32|0x00000009
+  gRaspberryPiTokenSpaceGuid.PcdCpuLowSpeedMHz|600|UINT32|0x0000000a
+  gRaspberryPiTokenSpaceGuid.PcdCpuDefSpeedMHz|800|UINT32|0x0000000b
+  gRaspberryPiTokenSpaceGuid.PcdCpuMaxSpeedMHz|1000|UINT32|0x0000000c
 
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   gRaspberryPiTokenSpaceGuid.PcdCpuClock|0|UINT32|0x0000000d
-- 
2.21.0.windows.1

Re: [edk2-devel][PATCH v2 1/1] Platform/RPi/ConfigDxe: Improve CPU Frequency configuration

[Ard Biesheuvel]

On Wed, 11 Mar 2020 at 00:31, Pete Batard <pete@akeo.ie> wrote:
>
> Improve the CPU frequency settings of the platforms by:
> - Adding a "Default" option that sets the frequency to the
>   official default for each model/submodel.
> - Adding a "Low" option, that sets the frequency to a fixed
>   PCD, custom to each platform.
> - Using fixed PCDs to set the maximum and default values for
>   the custom frequency range, according to the overclocking
>   capabilities of the platform.
> - Ensuring that the firmware defaults to using the platform's
>   default frequency, instead of a low arbitrary value.
>
> Signed-off-by: Pete Batard <pete@akeo.ie>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed as 79c06cfaeaee..c9f3f689baa6

Thanks Pete.