public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platforms][PATCH 0/2] RPi - add DT-only mode
@ 2020-05-10 21:34 Andrei Warkentin
  2020-05-10 21:34 ` [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h Andrei Warkentin
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Andrei Warkentin @ 2020-05-10 21:34 UTC (permalink / raw)
  To: devel; +Cc: ard.biesheuvel, leif, pete, philmd

Today the Pies can be booted in a way where only ACPI is exposed,
or both ACPI and DT are exposed.

This patch set adds one more mode - DT only, no ACPI. The target
audience is developers. When both are exposed, it's up to the OS
to decide which gets used, and that choice can differ between OSes.

Note: this does _not_ change defaults. Pi 3 still defaults to
ACPI + DT, while Pi 4 still defaults to ACPI only.

We don't really want to remove DT + ACPI mode - it is the default
on Pi 3, and removing it is bound to just annoy users - WoA and
NetBSD (voa UEFI) on Pi 3 only work with ACPI, while everything
else (Linux, FreeBSD) only work with DT. I'd make an analogy of
MPS and ACPI being exposed for the longest time ever together on
PCs.

Andrei Warkentin (2):
  RPi: move varstore structure defs to ConfigVars.h
  RPi: allow selecting DT-only mode

 .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c |  21 ++-
 .../Drivers/ConfigDxe/ConfigDxe.inf           |   2 +-
 .../Drivers/ConfigDxe/ConfigDxeHii.uni        |   9 +-
 .../Drivers/ConfigDxe/ConfigDxeHii.vfr        | 146 ++----------------
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c  |   6 +-
 .../RaspberryPi/Drivers/FdtDxe/FdtDxe.inf     |   2 +-
 Platform/RaspberryPi/Include/ConfigVars.h     | 131 ++++++++++++++++
 Platform/RaspberryPi/RPi3/RPi3.dsc            |   8 +-
 Platform/RaspberryPi/RPi4/RPi4.dsc            |   8 +-
 Platform/RaspberryPi/RaspberryPi.dec          |   2 +-
 10 files changed, 180 insertions(+), 155 deletions(-)
 create mode 100644 Platform/RaspberryPi/Include/ConfigVars.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread
* [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h
  2020-05-10 21:34 [edk2-platforms][PATCH 0/2] RPi - add DT-only mode Andrei Warkentin
@ 2020-05-10 21:34 ` Andrei Warkentin
  2020-05-11 10:57   ` Pete Batard
  2020-05-10 21:34 ` [edk2-platforms][PATCH 2/2] RPi: allow selecting DT-only mode Andrei Warkentin
  2020-05-11 11:28 ` [edk2-platforms][PATCH 0/2] RPi - add " Ard Biesheuvel
  2 siblings, 1 reply; 7+ messages in thread
From: Andrei Warkentin @ 2020-05-10 21:34 UTC (permalink / raw)
  To: devel; +Cc: ard.biesheuvel, leif, pete, philmd

To avoid hardcoding constants for non-obvious fields (e.g. enum
instead of basic enable/disable),  move variable structure and value
definitions into a separate header, ConfigVars.h.

Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      |  10 +-
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 131 +------------------
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c            |   3 +-
 Platform/RaspberryPi/Include/ConfigVars.h               | 132 ++++++++++++++++++++
 4 files changed, 144 insertions(+), 132 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index c90c2530..8c9609f3 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -22,6 +22,7 @@
 #include <IndustryStandard/Bcm2836Gpio.h>
 #include <Library/GpioLib.h>
 #include <Protocol/RpiFirmware.h>
+#include <ConfigVars.h>
 #include "ConfigDxeFormSetGuid.h"
 
 #define FREQ_1_MHZ 1000000
@@ -259,10 +260,10 @@ ApplyVariables (
   UINT64 SystemMemorySize;
 
   switch (CpuClock) {
-  case 0: // Low
+  case CHIPSET_CPU_CLOCK_LOW:
     Rate = FixedPcdGet32 (PcdCpuLowSpeedMHz) * FREQ_1_MHZ;
     break;
-  case 1: // Default
+  case CHIPSET_CPU_CLOCK_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
@@ -272,10 +273,10 @@ ApplyVariables (
       DEBUG ((DEBUG_ERROR, "Couldn't read default CPU speed %r\n", Status));
     }
     break;
-  case 2: // Max
+  case CHIPSET_CPU_CLOCK_MAX:
     Rate = FixedPcdGet32 (PcdCpuMaxSpeedMHz) * FREQ_1_MHZ;
     break;
-  case 3: // Custom
+  case CHIPSET_CPU_CLOCK_CUSTOM:
     Rate = CustomCpuClock * FREQ_1_MHZ;
     break;
   }
@@ -490,5 +491,6 @@ ConfigInitialize (
   Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
   ASSERT_EFI_ERROR (Status);
 
+
   return EFI_SUCCESS;
 }
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
index 0a650a94..576eabe9 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
@@ -8,128 +8,7 @@
 
 #include <Guid/HiiPlatformSetupFormset.h>
 #include "ConfigDxeFormSetGuid.h"
-
-#pragma pack(1)
-typedef struct {
-  /*
-   * One bit for each scaled resolution supported,
-   * these are ordered exactly like mGopModeData
-   * in DisplayDxe.
-   *
-   * 800x600, 640x480, 1024x768, 720p, 1080p, native.
-   */
-   UINT8 v640   : 1;
-   UINT8 v800   : 1;
-   UINT8 v1024  : 1;
-   UINT8 v720p  : 1;
-   UINT8 v1080p : 1;
-   UINT8 Physical : 1;
-} DISPLAY_ENABLE_SCALED_VMODES_VARSTORE_DATA;
-#pragma pack()
-
-typedef struct {
-  /*
-   * 0 - No screenshot support.
-   * 1 - Screenshot support via hotkey.
-   */
-   UINT32 Enable;
-} DISPLAY_ENABLE_SSHOT_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * 0 - No JTAG.
-   * 1 - JTAG mode.
-   */
-   UINT32 Enable;
-} DEBUG_ENABLE_JTAG_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * 0 - Don't show UEFI exit message.
-   * 1 - Show UEFI exit message.
-   */
-   UINT32 Show;
-} DEBUG_SHOW_UEFI_EXIT_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * 0 - low.
-   * 1 - default.
-   * 2 - maximum.
-   * 3 - custom.
-   */
-  UINT32 Clock;
-} CHIPSET_CPU_CLOCK_VARSTORE_DATA;
-
-typedef struct {
-  UINT32 Clock;
-} CHIPSET_CUSTOM_CPU_CLOCK_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * Always set by ConfigDxe prior to HII init to reflect
-   * platform capability.
-   */
-  UINT32 Supported;
-} ADVANCED_RAM_MORE_THAN_3GB_VARSTORE_DATA;
-
-typedef struct {
-  UINT32 Enabled;
-} ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * 0 - Do not provide a Device Tree to the OS
-   * 1 - Provide a Device Tree to the OS
-   */
-  UINT32 Enabled;
-} ADVANCED_DEVICE_TREE_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * 0 - uSD slot routed to Broadcom SDHOST on Pi 3 or eMMC2 on Pi 4.
-   * 1 - uSD slot routed to Arasan SDHCI.
-   */
-  UINT32 Routing;
-} MMC_SD_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * 0 - Don't disable multi-block.
-   * 1 - Disable multi-block commands.
-   */
-  UINT32 DisableMulti;
-} MMC_DISMULTI_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * 0 - Don't force 1 bit mode.
-   * 1 - Force 1 bit mode.
-   */
-  UINT32 Force1Bit;
-} MMC_FORCE1BIT_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * 0 - Don't force default speed.
-   * 1 - Force default speed.
-   */
-  UINT32 ForceDS;
-} MMC_FORCEDS_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * Default Speed MHz override (25MHz default).
-   */
-  UINT32 MHz;
-} MMC_SD_DS_MHZ_VARSTORE_DATA;
-
-typedef struct {
-  /*
-   * High Speed MHz override (50MHz default).
-   */
-  UINT32 MHz;
-} MMC_SD_HS_MHZ_VARSTORE_DATA;
+#include <ConfigVars.h>
 
 //
 // EFI Variable attributes
@@ -253,10 +132,10 @@ 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_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;
+            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_LOW), value = CHIPSET_CPU_CLOCK_LOW, flags = 0;
+            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_DEF), value = CHIPSET_CPU_CLOCK_DEFAULT, flags = DEFAULT;
+            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_MAX), value = CHIPSET_CPU_CLOCK_MAX, flags = 0;
+            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_CUSTOM), value = CHIPSET_CPU_CLOCK_CUSTOM, flags = 0;
         endoneof;
 
         grayoutif NOT ideqval CpuClock.Clock == 3;
diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
index cb11256e..3aaa0a7f 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
@@ -15,10 +15,9 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
 #include <libfdt.h>
-
 #include <Protocol/RpiFirmware.h>
-
 #include <Guid/Fdt.h>
+#include <ConfigVars.h>
 
 STATIC VOID                             *mFdtImage;
 
diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
new file mode 100644
index 00000000..a0959b4b
--- /dev/null
+++ b/Platform/RaspberryPi/Include/ConfigVars.h
@@ -0,0 +1,132 @@
+/** @file
+ *
+ *  Copyright (c) 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
+ *
+ *  SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ **/
+
+#ifndef CONFIG_VARS_H
+#define CONFIG_VARS_H
+
+#pragma pack(1)
+typedef struct {
+  /*
+   * One bit for each scaled resolution supported,
+   * these are ordered exactly like mGopModeData
+   * in DisplayDxe.
+   *
+   * 800x600, 640x480, 1024x768, 720p, 1080p, native.
+   */
+   UINT8 v640   : 1;
+   UINT8 v800   : 1;
+   UINT8 v1024  : 1;
+   UINT8 v720p  : 1;
+   UINT8 v1080p : 1;
+   UINT8 Physical : 1;
+} DISPLAY_ENABLE_SCALED_VMODES_VARSTORE_DATA;
+#pragma pack()
+
+typedef struct {
+  /*
+   * 0 - No screenshot support.
+   * 1 - Screenshot support via hotkey.
+   */
+   UINT32 Enable;
+} DISPLAY_ENABLE_SSHOT_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * 0 - No JTAG.
+   * 1 - JTAG mode.
+   */
+   UINT32 Enable;
+} DEBUG_ENABLE_JTAG_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * 0 - Don't show UEFI exit message.
+   * 1 - Show UEFI exit message.
+   */
+   UINT32 Show;
+} DEBUG_SHOW_UEFI_EXIT_VARSTORE_DATA;
+
+typedef struct {
+#define CHIPSET_CPU_CLOCK_LOW     0
+#define CHIPSET_CPU_CLOCK_DEFAULT 1
+#define CHIPSET_CPU_CLOCK_MAX     2
+#define CHIPSET_CPU_CLOCK_CUSTOM  3
+  UINT32 Clock;
+} CHIPSET_CPU_CLOCK_VARSTORE_DATA;
+
+typedef struct {
+  UINT32 Clock;
+} CHIPSET_CUSTOM_CPU_CLOCK_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * Always set by ConfigDxe prior to HII init to reflect
+   * platform capability.
+   */
+  UINT32 Supported;
+} ADVANCED_RAM_MORE_THAN_3GB_VARSTORE_DATA;
+
+typedef struct {
+  UINT32 Enabled;
+} ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * 0 - Do not provide a Device Tree to the OS
+   * 1 - Provide a Device Tree to the OS
+   */
+  UINT32 Enabled;
+} ADVANCED_DEVICE_TREE_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * 0 - uSD slot routed to Broadcom SDHOST on Pi 3 or eMMC2 on Pi 4.
+   * 1 - uSD slot routed to Arasan SDHCI.
+   */
+  UINT32 Routing;
+} MMC_SD_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * 0 - Don't disable multi-block.
+   * 1 - Disable multi-block commands.
+   */
+  UINT32 DisableMulti;
+} MMC_DISMULTI_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * 0 - Don't force 1 bit mode.
+   * 1 - Force 1 bit mode.
+   */
+  UINT32 Force1Bit;
+} MMC_FORCE1BIT_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * 0 - Don't force default speed.
+   * 1 - Force default speed.
+   */
+  UINT32 ForceDS;
+} MMC_FORCEDS_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * Default Speed MHz override (25MHz default).
+   */
+  UINT32 MHz;
+} MMC_SD_DS_MHZ_VARSTORE_DATA;
+
+typedef struct {
+  /*
+   * High Speed MHz override (50MHz default).
+   */
+  UINT32 MHz;
+} MMC_SD_HS_MHZ_VARSTORE_DATA;
+
+#endif /* CONFIG_VARS_H */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [edk2-platforms][PATCH 2/2] RPi: allow selecting DT-only mode
  2020-05-10 21:34 [edk2-platforms][PATCH 0/2] RPi - add DT-only mode Andrei Warkentin
  2020-05-10 21:34 ` [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h Andrei Warkentin
@ 2020-05-10 21:34 ` Andrei Warkentin
  2020-05-11 10:58   ` Pete Batard
  2020-05-11 11:28 ` [edk2-platforms][PATCH 0/2] RPi - add " Ard Biesheuvel
  2 siblings, 1 reply; 7+ messages in thread
From: Andrei Warkentin @ 2020-05-10 21:34 UTC (permalink / raw)
  To: devel; +Cc: ard.biesheuvel, leif, pete, philmd

Today the Pies can be booted in a way where only ACPI is exposed,
or both ACPI and DT are exposed.

This adds one more mode - DT only, no ACPI. The target audience
is developers. When both are exposed, it's up to the OS to decide
which gets used, and that choice can differ between OSes,

Note: this does _not_ change defaults. Pi 3 still defaults to
ACPI + DT, while Pi 4 still defaults to ACPI only.

We don't really want to remove DT + ACPI mode - it is the default
on Pi 3, and removing it is bound to just annoy users - WoA and
NetBSD (voa UEFI) on Pi 3 only work with ACPI, while everything
else (Linux, FreeBSD) only work with DT. I'd make an analogy of
MPS and ACPI being exposed for the longest time ever together on
PCs.

Testing: OpenBSD on Pi 4 with DT-only and ACPI-only boots.

Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      | 11 +++++++----
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  2 +-
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  9 +++++----
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 15 ++++++++-------
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c            |  3 ++-
 Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf          |  2 +-
 Platform/RaspberryPi/Include/ConfigVars.h               | 11 +++++------
 Platform/RaspberryPi/RPi3/RPi3.dsc                      |  8 ++++++--
 Platform/RaspberryPi/RPi4/RPi4.dsc                      |  8 ++++++--
 Platform/RaspberryPi/RaspberryPi.dec                    |  2 +-
 10 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index 8c9609f3..29701db2 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -155,11 +155,11 @@ SetupVariables (
   }
 
   Size = sizeof (UINT32);
-  Status = gRT->GetVariable (L"OptDeviceTree",
+  Status = gRT->GetVariable (L"SystemTableMode",
                   &gConfigDxeFormSetGuid,
                   NULL, &Size, &Var32);
   if (EFI_ERROR (Status)) {
-    PcdSet32 (PcdOptDeviceTree, PcdGet32 (PcdOptDeviceTree));
+    PcdSet32 (PcdSystemTableMode, PcdGet32 (PcdSystemTableMode));
   }
 
   Size = sizeof (UINT32);
@@ -488,8 +488,11 @@ ConfigInitialize (
     DEBUG ((DEBUG_ERROR, "Couldn't install ConfigDxe configuration pages: %r\n", Status));
   }
 
-  Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
-  ASSERT_EFI_ERROR (Status);
+  if (PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_ACPI ||
+      PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_BOTH) {
+     Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
+     ASSERT_EFI_ERROR (Status);
+  }
 
 
   return EFI_SUCCESS;
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
index 57963baf..e47f3af6 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
@@ -77,7 +77,7 @@
   gRaspberryPiTokenSpaceGuid.PcdDebugShowUEFIExit
   gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes
   gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot
-  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree
+  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode
   gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
   gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
 
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
index 07660072..db36e200 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
@@ -41,10 +41,11 @@
 #string STR_ADVANCED_3GB_OFF         #language en-US "Disabled"
 #string STR_ADVANCED_3GB_ON          #language en-US "Enabled"
 
-#string STR_ADVANCED_DT_PROMPT       #language en-US "Device Tree"
-#string STR_ADVANCED_DT_HELP         #language en-US "Disable this option to force OSes such as GNU/Linux to use ACPI"
-#string STR_ADVANCED_DT_OFF          #language en-US "Disabled (Force ACPI)"
-#string STR_ADVANCED_DT_ON           #language en-US "Enabled"
+#string STR_ADVANCED_SYSTAB_PROMPT   #language en-US "System Table Selection"
+#string STR_ADVANCED_SYSTAB_HELP     #language en-US "ACPI/DT choice for specific OSes"
+#string STR_ADVANCED_SYSTAB_ACPI     #language en-US "ACPI"
+#string STR_ADVANCED_SYSTAB_BOTH     #language en-US "ACPI + Devicetree"
+#string STR_ADVANCED_SYSTAB_DT       #language en-US "Devicetree"
 
 /*
  * MMC/SD configuration.
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
index 576eabe9..91a0ea2d 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
@@ -44,9 +44,9 @@ formset
       name  = RamLimitTo3GB,
       guid  = CONFIGDXE_FORM_SET_GUID;
 
-    efivarstore ADVANCED_DEVICE_TREE_VARSTORE_DATA,
+    efivarstore SYSTEM_TABLE_MODE_VARSTORE_DATA,
       attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
-      name  = OptDeviceTree,
+      name  = SystemTableMode,
       guid  = CONFIGDXE_FORM_SET_GUID;
 
     efivarstore MMC_SD_VARSTORE_DATA,
@@ -164,12 +164,13 @@ formset
           endoneof;
         endif;
 
-        oneof varid = OptDeviceTree.Enabled,
-            prompt      = STRING_TOKEN(STR_ADVANCED_DT_PROMPT),
-            help        = STRING_TOKEN(STR_ADVANCED_DT_HELP),
+        oneof varid = SystemTableMode.Mode,
+            prompt      = STRING_TOKEN(STR_ADVANCED_SYSTAB_PROMPT),
+            help        = STRING_TOKEN(STR_ADVANCED_SYSTAB_HELP),
             flags       = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
-            option text = STRING_TOKEN(STR_ADVANCED_DT_OFF), value = 0, flags = 0;
-            option text = STRING_TOKEN(STR_ADVANCED_DT_ON), value = 1, flags = DEFAULT;
+            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_ACPI), value = SYSTEM_TABLE_MODE_ACPI, flags = 0;
+            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_BOTH), value = SYSTEM_TABLE_MODE_BOTH, flags = DEFAULT;
+            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags = DEFAULT;
         endoneof;
     endform;
 
diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
index 3aaa0a7f..44c6c87c 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
@@ -355,7 +355,8 @@ FdtDxeInitialize (
   UINTN      FdtSize;
   VOID       *FdtImage = NULL;
 
-  if (PcdGet32 (PcdOptDeviceTree) == 0) {
+  if (PcdGet32 (PcdSystemTableMode) != SYSTEM_TABLE_MODE_BOTH &&
+      PcdGet32 (PcdSystemTableMode) != SYSTEM_TABLE_MODE_DT) {
     DEBUG ((DEBUG_INFO, "Device Tree disabled per user configuration\n"));
     return EFI_SUCCESS;
   }
diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
index 49ba5ba1..26f84e59 100644
--- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
+++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
@@ -46,4 +46,4 @@
   gRaspberryPiTokenSpaceGuid.PcdFdtBaseAddress
 
 [Pcd]
-  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree
+  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode
diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
index a0959b4b..28837f98 100644
--- a/Platform/RaspberryPi/Include/ConfigVars.h
+++ b/Platform/RaspberryPi/Include/ConfigVars.h
@@ -76,12 +76,11 @@ typedef struct {
 } ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA;
 
 typedef struct {
-  /*
-   * 0 - Do not provide a Device Tree to the OS
-   * 1 - Provide a Device Tree to the OS
-   */
-  UINT32 Enabled;
-} ADVANCED_DEVICE_TREE_VARSTORE_DATA;
+#define SYSTEM_TABLE_MODE_ACPI 0
+#define SYSTEM_TABLE_MODE_BOTH 1
+#define SYSTEM_TABLE_MODE_DT   2
+  UINT32 Mode;
+} SYSTEM_TABLE_MODE_VARSTORE_DATA;
 
 typedef struct {
   /*
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index 563fb891..a138c874 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -481,9 +481,13 @@
   gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|L"RamLimitTo3GB"|gConfigDxeFormSetGuid|0x0|0
 
   #
-  # Device Tree
+  # Device Tree and ACPI selection.
   #
-  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|L"OptDeviceTree"|gConfigDxeFormSetGuid|0x0|1
+  # 0 - SYSTEM_TABLE_MODE_ACPI
+  # 1 - SYSTEM_TABLE_MODE_BOTH (default)
+  # 2 - SYSTEM_TABLE_MODE_DT
+  #
+  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|L"SystemTableMode"|gConfigDxeFormSetGuid|0x0|1
 
   #
   # Common UEFI ones.
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 4deccd9d..75867f03 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -496,9 +496,13 @@
   gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|L"RamLimitTo3GB"|gConfigDxeFormSetGuid|0x0|1
 
   #
-  # Device Tree
+  # Device Tree and ACPI selection.
   #
-  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|L"OptDeviceTree"|gConfigDxeFormSetGuid|0x0|0
+  # 0 - SYSTEM_TABLE_MODE_ACPI (default)
+  # 1 - SYSTEM_TABLE_MODE_BOTH
+  # 2 - SYSTEM_TABLE_MODE_DT
+  #
+  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|L"SystemTableMode"|gConfigDxeFormSetGuid|0x0|0
 
   #
   # Common UEFI ones.
diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
index 66ef6186..1a3c44e0 100644
--- a/Platform/RaspberryPi/RaspberryPi.dec
+++ b/Platform/RaspberryPi/RaspberryPi.dec
@@ -65,6 +65,6 @@
   gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock|0|UINT32|0x00000016
   gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes|0x3F|UINT8|0x00000017
   gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot|0|UINT32|0x00000018
-  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|1|UINT32|0x0000001B
+  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|1|UINT32|0x0000001B
   gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|0|UINT32|0x00000019
   gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h
  2020-05-10 21:34 ` [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h Andrei Warkentin
@ 2020-05-11 10:57   ` Pete Batard
  0 siblings, 0 replies; 7+ messages in thread
From: Pete Batard @ 2020-05-11 10:57 UTC (permalink / raw)
  To: Andrei Warkentin, devel; +Cc: ard.biesheuvel, leif, philmd

One small whitespace remark inline:

On 2020.05.10 22:34, Andrei Warkentin wrote:
> To avoid hardcoding constants for non-obvious fields (e.g. enum
> instead of basic enable/disable),  move variable structure and value
> definitions into a separate header, ConfigVars.h.
> 
> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> ---
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      |  10 +-
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 131 +------------------
>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c            |   3 +-
>   Platform/RaspberryPi/Include/ConfigVars.h               | 132 ++++++++++++++++++++
>   4 files changed, 144 insertions(+), 132 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> index c90c2530..8c9609f3 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> @@ -22,6 +22,7 @@
>   #include <IndustryStandard/Bcm2836Gpio.h>
>   #include <Library/GpioLib.h>
>   #include <Protocol/RpiFirmware.h>
> +#include <ConfigVars.h>
>   #include "ConfigDxeFormSetGuid.h"
>   
>   #define FREQ_1_MHZ 1000000
> @@ -259,10 +260,10 @@ ApplyVariables (
>     UINT64 SystemMemorySize;
>   
>     switch (CpuClock) {
> -  case 0: // Low
> +  case CHIPSET_CPU_CLOCK_LOW:
>       Rate = FixedPcdGet32 (PcdCpuLowSpeedMHz) * FREQ_1_MHZ;
>       break;
> -  case 1: // Default
> +  case CHIPSET_CPU_CLOCK_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
> @@ -272,10 +273,10 @@ ApplyVariables (
>         DEBUG ((DEBUG_ERROR, "Couldn't read default CPU speed %r\n", Status));
>       }
>       break;
> -  case 2: // Max
> +  case CHIPSET_CPU_CLOCK_MAX:
>       Rate = FixedPcdGet32 (PcdCpuMaxSpeedMHz) * FREQ_1_MHZ;
>       break;
> -  case 3: // Custom
> +  case CHIPSET_CPU_CLOCK_CUSTOM:
>       Rate = CustomCpuClock * FREQ_1_MHZ;
>       break;
>     }
> @@ -490,5 +491,6 @@ ConfigInitialize (
>     Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
>     ASSERT_EFI_ERROR (Status);
>   
> +

An empty line is being added here for no reason.

>     return EFI_SUCCESS;
>   }
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> index 0a650a94..576eabe9 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> @@ -8,128 +8,7 @@
>   
>   #include <Guid/HiiPlatformSetupFormset.h>
>   #include "ConfigDxeFormSetGuid.h"
> -
> -#pragma pack(1)
> -typedef struct {
> -  /*
> -   * One bit for each scaled resolution supported,
> -   * these are ordered exactly like mGopModeData
> -   * in DisplayDxe.
> -   *
> -   * 800x600, 640x480, 1024x768, 720p, 1080p, native.
> -   */
> -   UINT8 v640   : 1;
> -   UINT8 v800   : 1;
> -   UINT8 v1024  : 1;
> -   UINT8 v720p  : 1;
> -   UINT8 v1080p : 1;
> -   UINT8 Physical : 1;
> -} DISPLAY_ENABLE_SCALED_VMODES_VARSTORE_DATA;
> -#pragma pack()
> -
> -typedef struct {
> -  /*
> -   * 0 - No screenshot support.
> -   * 1 - Screenshot support via hotkey.
> -   */
> -   UINT32 Enable;
> -} DISPLAY_ENABLE_SSHOT_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * 0 - No JTAG.
> -   * 1 - JTAG mode.
> -   */
> -   UINT32 Enable;
> -} DEBUG_ENABLE_JTAG_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * 0 - Don't show UEFI exit message.
> -   * 1 - Show UEFI exit message.
> -   */
> -   UINT32 Show;
> -} DEBUG_SHOW_UEFI_EXIT_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * 0 - low.
> -   * 1 - default.
> -   * 2 - maximum.
> -   * 3 - custom.
> -   */
> -  UINT32 Clock;
> -} CHIPSET_CPU_CLOCK_VARSTORE_DATA;
> -
> -typedef struct {
> -  UINT32 Clock;
> -} CHIPSET_CUSTOM_CPU_CLOCK_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * Always set by ConfigDxe prior to HII init to reflect
> -   * platform capability.
> -   */
> -  UINT32 Supported;
> -} ADVANCED_RAM_MORE_THAN_3GB_VARSTORE_DATA;
> -
> -typedef struct {
> -  UINT32 Enabled;
> -} ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * 0 - Do not provide a Device Tree to the OS
> -   * 1 - Provide a Device Tree to the OS
> -   */
> -  UINT32 Enabled;
> -} ADVANCED_DEVICE_TREE_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * 0 - uSD slot routed to Broadcom SDHOST on Pi 3 or eMMC2 on Pi 4.
> -   * 1 - uSD slot routed to Arasan SDHCI.
> -   */
> -  UINT32 Routing;
> -} MMC_SD_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * 0 - Don't disable multi-block.
> -   * 1 - Disable multi-block commands.
> -   */
> -  UINT32 DisableMulti;
> -} MMC_DISMULTI_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * 0 - Don't force 1 bit mode.
> -   * 1 - Force 1 bit mode.
> -   */
> -  UINT32 Force1Bit;
> -} MMC_FORCE1BIT_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * 0 - Don't force default speed.
> -   * 1 - Force default speed.
> -   */
> -  UINT32 ForceDS;
> -} MMC_FORCEDS_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * Default Speed MHz override (25MHz default).
> -   */
> -  UINT32 MHz;
> -} MMC_SD_DS_MHZ_VARSTORE_DATA;
> -
> -typedef struct {
> -  /*
> -   * High Speed MHz override (50MHz default).
> -   */
> -  UINT32 MHz;
> -} MMC_SD_HS_MHZ_VARSTORE_DATA;
> +#include <ConfigVars.h>
>   
>   //
>   // EFI Variable attributes
> @@ -253,10 +132,10 @@ 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_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;
> +            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_LOW), value = CHIPSET_CPU_CLOCK_LOW, flags = 0;
> +            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_DEF), value = CHIPSET_CPU_CLOCK_DEFAULT, flags = DEFAULT;
> +            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_MAX), value = CHIPSET_CPU_CLOCK_MAX, flags = 0;
> +            option text = STRING_TOKEN(STR_CHIPSET_CLOCK_CPU_CUSTOM), value = CHIPSET_CPU_CLOCK_CUSTOM, flags = 0;
>           endoneof;
>   
>           grayoutif NOT ideqval CpuClock.Clock == 3;
> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> index cb11256e..3aaa0a7f 100644
> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> @@ -15,10 +15,9 @@
>   #include <Library/UefiBootServicesTableLib.h>
>   #include <Library/UefiLib.h>
>   #include <libfdt.h>
> -
>   #include <Protocol/RpiFirmware.h>
> -
>   #include <Guid/Fdt.h>
> +#include <ConfigVars.h>
>   
>   STATIC VOID                             *mFdtImage;
>   
> diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
> new file mode 100644
> index 00000000..a0959b4b
> --- /dev/null
> +++ b/Platform/RaspberryPi/Include/ConfigVars.h
> @@ -0,0 +1,132 @@
> +/** @file
> + *
> + *  Copyright (c) 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
> + *
> + *  SPDX-License-Identifier: BSD-2-Clause-Patent
> + *
> + **/
> +
> +#ifndef CONFIG_VARS_H
> +#define CONFIG_VARS_H
> +
> +#pragma pack(1)
> +typedef struct {
> +  /*
> +   * One bit for each scaled resolution supported,
> +   * these are ordered exactly like mGopModeData
> +   * in DisplayDxe.
> +   *
> +   * 800x600, 640x480, 1024x768, 720p, 1080p, native.
> +   */
> +   UINT8 v640   : 1;
> +   UINT8 v800   : 1;
> +   UINT8 v1024  : 1;
> +   UINT8 v720p  : 1;
> +   UINT8 v1080p : 1;
> +   UINT8 Physical : 1;
> +} DISPLAY_ENABLE_SCALED_VMODES_VARSTORE_DATA;
> +#pragma pack()
> +
> +typedef struct {
> +  /*
> +   * 0 - No screenshot support.
> +   * 1 - Screenshot support via hotkey.
> +   */
> +   UINT32 Enable;
> +} DISPLAY_ENABLE_SSHOT_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * 0 - No JTAG.
> +   * 1 - JTAG mode.
> +   */
> +   UINT32 Enable;
> +} DEBUG_ENABLE_JTAG_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * 0 - Don't show UEFI exit message.
> +   * 1 - Show UEFI exit message.
> +   */
> +   UINT32 Show;
> +} DEBUG_SHOW_UEFI_EXIT_VARSTORE_DATA;
> +
> +typedef struct {
> +#define CHIPSET_CPU_CLOCK_LOW     0
> +#define CHIPSET_CPU_CLOCK_DEFAULT 1
> +#define CHIPSET_CPU_CLOCK_MAX     2
> +#define CHIPSET_CPU_CLOCK_CUSTOM  3
> +  UINT32 Clock;
> +} CHIPSET_CPU_CLOCK_VARSTORE_DATA;
> +
> +typedef struct {
> +  UINT32 Clock;
> +} CHIPSET_CUSTOM_CPU_CLOCK_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * Always set by ConfigDxe prior to HII init to reflect
> +   * platform capability.
> +   */
> +  UINT32 Supported;
> +} ADVANCED_RAM_MORE_THAN_3GB_VARSTORE_DATA;
> +
> +typedef struct {
> +  UINT32 Enabled;
> +} ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * 0 - Do not provide a Device Tree to the OS
> +   * 1 - Provide a Device Tree to the OS
> +   */
> +  UINT32 Enabled;
> +} ADVANCED_DEVICE_TREE_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * 0 - uSD slot routed to Broadcom SDHOST on Pi 3 or eMMC2 on Pi 4.
> +   * 1 - uSD slot routed to Arasan SDHCI.
> +   */
> +  UINT32 Routing;
> +} MMC_SD_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * 0 - Don't disable multi-block.
> +   * 1 - Disable multi-block commands.
> +   */
> +  UINT32 DisableMulti;
> +} MMC_DISMULTI_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * 0 - Don't force 1 bit mode.
> +   * 1 - Force 1 bit mode.
> +   */
> +  UINT32 Force1Bit;
> +} MMC_FORCE1BIT_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * 0 - Don't force default speed.
> +   * 1 - Force default speed.
> +   */
> +  UINT32 ForceDS;
> +} MMC_FORCEDS_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * Default Speed MHz override (25MHz default).
> +   */
> +  UINT32 MHz;
> +} MMC_SD_DS_MHZ_VARSTORE_DATA;
> +
> +typedef struct {
> +  /*
> +   * High Speed MHz override (50MHz default).
> +   */
> +  UINT32 MHz;
> +} MMC_SD_HS_MHZ_VARSTORE_DATA;
> +
> +#endif /* CONFIG_VARS_H */
> 

Reviewed-by: Pete Batard <pete@akeo.ie>


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 2/2] RPi: allow selecting DT-only mode
  2020-05-10 21:34 ` [edk2-platforms][PATCH 2/2] RPi: allow selecting DT-only mode Andrei Warkentin
@ 2020-05-11 10:58   ` Pete Batard
  2020-05-11 11:25     ` Ard Biesheuvel
  0 siblings, 1 reply; 7+ messages in thread
From: Pete Batard @ 2020-05-11 10:58 UTC (permalink / raw)
  To: Andrei Warkentin, devel; +Cc: ard.biesheuvel, leif, philmd

On 2020.05.10 22:34, Andrei Warkentin wrote:
> Today the Pies can be booted in a way where only ACPI is exposed,
> or both ACPI and DT are exposed.
> 
> This adds one more mode - DT only, no ACPI. The target audience
> is developers. When both are exposed, it's up to the OS to decide
> which gets used, and that choice can differ between OSes,
> 
> Note: this does _not_ change defaults. Pi 3 still defaults to
> ACPI + DT, while Pi 4 still defaults to ACPI only.
> 
> We don't really want to remove DT + ACPI mode - it is the default
> on Pi 3, and removing it is bound to just annoy users - WoA and
> NetBSD (voa UEFI) on Pi 3 only work with ACPI, while everything
> else (Linux, FreeBSD) only work with DT. I'd make an analogy of
> MPS and ACPI being exposed for the longest time ever together on
> PCs.

I agree with this.

Even if other platforms may avoid this configuration, we did make DT + 
ACPI mode the default for the Pi 3, and folks might already be using 
this setup on the Pi 3 to boot Linux in DT mode and Windows in ACPI mode 
without having to change their settings. So removing that dual mode may 
be seen as a regression.

> 
> Testing: OpenBSD on Pi 4 with DT-only and ACPI-only boots.
> 
> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> ---
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      | 11 +++++++----
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  2 +-
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  9 +++++----
>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 15 ++++++++-------
>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c            |  3 ++-
>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf          |  2 +-
>   Platform/RaspberryPi/Include/ConfigVars.h               | 11 +++++------
>   Platform/RaspberryPi/RPi3/RPi3.dsc                      |  8 ++++++--
>   Platform/RaspberryPi/RPi4/RPi4.dsc                      |  8 ++++++--
>   Platform/RaspberryPi/RaspberryPi.dec                    |  2 +-
>   10 files changed, 42 insertions(+), 29 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> index 8c9609f3..29701db2 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> @@ -155,11 +155,11 @@ SetupVariables (
>     }
>   
>     Size = sizeof (UINT32);
> -  Status = gRT->GetVariable (L"OptDeviceTree",
> +  Status = gRT->GetVariable (L"SystemTableMode",
>                     &gConfigDxeFormSetGuid,
>                     NULL, &Size, &Var32);
>     if (EFI_ERROR (Status)) {
> -    PcdSet32 (PcdOptDeviceTree, PcdGet32 (PcdOptDeviceTree));
> +    PcdSet32 (PcdSystemTableMode, PcdGet32 (PcdSystemTableMode));
>     }
>   
>     Size = sizeof (UINT32);
> @@ -488,8 +488,11 @@ ConfigInitialize (
>       DEBUG ((DEBUG_ERROR, "Couldn't install ConfigDxe configuration pages: %r\n", Status));
>     }
>   
> -  Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
> -  ASSERT_EFI_ERROR (Status);
> +  if (PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_ACPI ||
> +      PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_BOTH) {

Overall, I think code readability might have been improved by using 
bitmasks (e.g. bit 0 for ACPI, bit 1 for DT) rather than a triplet of 
values, but I'm fine with the patch as it stands.

> +     Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
> +     ASSERT_EFI_ERROR (Status);
> +  }
>   
>   
>     return EFI_SUCCESS;
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> index 57963baf..e47f3af6 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> @@ -77,7 +77,7 @@
>     gRaspberryPiTokenSpaceGuid.PcdDebugShowUEFIExit
>     gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes
>     gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot
> -  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree
> +  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode
>     gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
>   
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> index 07660072..db36e200 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> @@ -41,10 +41,11 @@
>   #string STR_ADVANCED_3GB_OFF         #language en-US "Disabled"
>   #string STR_ADVANCED_3GB_ON          #language en-US "Enabled"
>   
> -#string STR_ADVANCED_DT_PROMPT       #language en-US "Device Tree"
> -#string STR_ADVANCED_DT_HELP         #language en-US "Disable this option to force OSes such as GNU/Linux to use ACPI"
> -#string STR_ADVANCED_DT_OFF          #language en-US "Disabled (Force ACPI)"
> -#string STR_ADVANCED_DT_ON           #language en-US "Enabled"
> +#string STR_ADVANCED_SYSTAB_PROMPT   #language en-US "System Table Selection"
> +#string STR_ADVANCED_SYSTAB_HELP     #language en-US "ACPI/DT choice for specific OSes"
> +#string STR_ADVANCED_SYSTAB_ACPI     #language en-US "ACPI"
> +#string STR_ADVANCED_SYSTAB_BOTH     #language en-US "ACPI + Devicetree"
> +#string STR_ADVANCED_SYSTAB_DT       #language en-US "Devicetree"
>   
>   /*
>    * MMC/SD configuration.
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> index 576eabe9..91a0ea2d 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> @@ -44,9 +44,9 @@ formset
>         name  = RamLimitTo3GB,
>         guid  = CONFIGDXE_FORM_SET_GUID;
>   
> -    efivarstore ADVANCED_DEVICE_TREE_VARSTORE_DATA,
> +    efivarstore SYSTEM_TABLE_MODE_VARSTORE_DATA,
>         attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
> -      name  = OptDeviceTree,
> +      name  = SystemTableMode,
>         guid  = CONFIGDXE_FORM_SET_GUID;
>   
>       efivarstore MMC_SD_VARSTORE_DATA,
> @@ -164,12 +164,13 @@ formset
>             endoneof;
>           endif;
>   
> -        oneof varid = OptDeviceTree.Enabled,
> -            prompt      = STRING_TOKEN(STR_ADVANCED_DT_PROMPT),
> -            help        = STRING_TOKEN(STR_ADVANCED_DT_HELP),
> +        oneof varid = SystemTableMode.Mode,
> +            prompt      = STRING_TOKEN(STR_ADVANCED_SYSTAB_PROMPT),
> +            help        = STRING_TOKEN(STR_ADVANCED_SYSTAB_HELP),
>               flags       = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
> -            option text = STRING_TOKEN(STR_ADVANCED_DT_OFF), value = 0, flags = 0;
> -            option text = STRING_TOKEN(STR_ADVANCED_DT_ON), value = 1, flags = DEFAULT;
> +            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_ACPI), value = SYSTEM_TABLE_MODE_ACPI, flags = 0;
> +            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_BOTH), value = SYSTEM_TABLE_MODE_BOTH, flags = DEFAULT;
> +            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags = DEFAULT;
>           endoneof;
>       endform;
>   
> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> index 3aaa0a7f..44c6c87c 100644
> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
> @@ -355,7 +355,8 @@ FdtDxeInitialize (
>     UINTN      FdtSize;
>     VOID       *FdtImage = NULL;
>   
> -  if (PcdGet32 (PcdOptDeviceTree) == 0) {
> +  if (PcdGet32 (PcdSystemTableMode) != SYSTEM_TABLE_MODE_BOTH &&
> +      PcdGet32 (PcdSystemTableMode) != SYSTEM_TABLE_MODE_DT) {
>       DEBUG ((DEBUG_INFO, "Device Tree disabled per user configuration\n"));
>       return EFI_SUCCESS;
>     }
> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
> index 49ba5ba1..26f84e59 100644
> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
> @@ -46,4 +46,4 @@
>     gRaspberryPiTokenSpaceGuid.PcdFdtBaseAddress
>   
>   [Pcd]
> -  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree
> +  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode
> diff --git a/Platform/RaspberryPi/Include/ConfigVars.h b/Platform/RaspberryPi/Include/ConfigVars.h
> index a0959b4b..28837f98 100644
> --- a/Platform/RaspberryPi/Include/ConfigVars.h
> +++ b/Platform/RaspberryPi/Include/ConfigVars.h
> @@ -76,12 +76,11 @@ typedef struct {
>   } ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA;
>   
>   typedef struct {
> -  /*
> -   * 0 - Do not provide a Device Tree to the OS
> -   * 1 - Provide a Device Tree to the OS
> -   */
> -  UINT32 Enabled;
> -} ADVANCED_DEVICE_TREE_VARSTORE_DATA;
> +#define SYSTEM_TABLE_MODE_ACPI 0
> +#define SYSTEM_TABLE_MODE_BOTH 1
> +#define SYSTEM_TABLE_MODE_DT   2
> +  UINT32 Mode;
> +} SYSTEM_TABLE_MODE_VARSTORE_DATA;
>   
>   typedef struct {
>     /*
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> index 563fb891..a138c874 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> @@ -481,9 +481,13 @@
>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|L"RamLimitTo3GB"|gConfigDxeFormSetGuid|0x0|0
>   
>     #
> -  # Device Tree
> +  # Device Tree and ACPI selection.
>     #
> -  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|L"OptDeviceTree"|gConfigDxeFormSetGuid|0x0|1
> +  # 0 - SYSTEM_TABLE_MODE_ACPI
> +  # 1 - SYSTEM_TABLE_MODE_BOTH (default)
> +  # 2 - SYSTEM_TABLE_MODE_DT
> +  #
> +  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|L"SystemTableMode"|gConfigDxeFormSetGuid|0x0|1
>   
>     #
>     # Common UEFI ones.
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index 4deccd9d..75867f03 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -496,9 +496,13 @@
>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|L"RamLimitTo3GB"|gConfigDxeFormSetGuid|0x0|1
>   
>     #
> -  # Device Tree
> +  # Device Tree and ACPI selection.
>     #
> -  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|L"OptDeviceTree"|gConfigDxeFormSetGuid|0x0|0
> +  # 0 - SYSTEM_TABLE_MODE_ACPI (default)
> +  # 1 - SYSTEM_TABLE_MODE_BOTH
> +  # 2 - SYSTEM_TABLE_MODE_DT
> +  #
> +  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|L"SystemTableMode"|gConfigDxeFormSetGuid|0x0|0
>   
>     #
>     # Common UEFI ones.
> diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
> index 66ef6186..1a3c44e0 100644
> --- a/Platform/RaspberryPi/RaspberryPi.dec
> +++ b/Platform/RaspberryPi/RaspberryPi.dec
> @@ -65,6 +65,6 @@
>     gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock|0|UINT32|0x00000016
>     gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes|0x3F|UINT8|0x00000017
>     gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot|0|UINT32|0x00000018
> -  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|1|UINT32|0x0000001B
> +  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|1|UINT32|0x0000001B
>     gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|0|UINT32|0x00000019
>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
> 

Reviewed-by: Pete Batard <pete@akeo.ie>


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 2/2] RPi: allow selecting DT-only mode
  2020-05-11 10:58   ` Pete Batard
@ 2020-05-11 11:25     ` Ard Biesheuvel
  0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2020-05-11 11:25 UTC (permalink / raw)
  To: Pete Batard, Andrei Warkentin, devel; +Cc: leif, philmd

On 5/11/20 12:58 PM, Pete Batard wrote:
> On 2020.05.10 22:34, Andrei Warkentin wrote:
>> Today the Pies can be booted in a way where only ACPI is exposed,
>> or both ACPI and DT are exposed.
>>
>> This adds one more mode - DT only, no ACPI. The target audience
>> is developers. When both are exposed, it's up to the OS to decide
>> which gets used, and that choice can differ between OSes,
>>
>> Note: this does _not_ change defaults. Pi 3 still defaults to
>> ACPI + DT, while Pi 4 still defaults to ACPI only.
>>
>> We don't really want to remove DT + ACPI mode - it is the default
>> on Pi 3, and removing it is bound to just annoy users - WoA and
>> NetBSD (voa UEFI) on Pi 3 only work with ACPI, while everything
>> else (Linux, FreeBSD) only work with DT. I'd make an analogy of
>> MPS and ACPI being exposed for the longest time ever together on
>> PCs.
> 
> I agree with this.
> 
> Even if other platforms may avoid this configuration, we did make DT + 
> ACPI mode the default for the Pi 3, and folks might already be using 
> this setup on the Pi 3 to boot Linux in DT mode and Windows in ACPI mode 
> without having to change their settings. So removing that dual mode may 
> be seen as a regression.
> 

Fair enough.


>>
>> Testing: OpenBSD on Pi 4 with DT-only and ACPI-only boots.
>>
>> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
>> ---
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c      | 11 
>> +++++++----
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf    |  2 +-
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni |  9 +++++----
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 15 
>> ++++++++-------
>>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c            |  3 ++-
>>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf          |  2 +-
>>   Platform/RaspberryPi/Include/ConfigVars.h               | 11 
>> +++++------
>>   Platform/RaspberryPi/RPi3/RPi3.dsc                      |  8 ++++++--
>>   Platform/RaspberryPi/RPi4/RPi4.dsc                      |  8 ++++++--
>>   Platform/RaspberryPi/RaspberryPi.dec                    |  2 +-
>>   10 files changed, 42 insertions(+), 29 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> index 8c9609f3..29701db2 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> @@ -155,11 +155,11 @@ SetupVariables (
>>     }
>>     Size = sizeof (UINT32);
>> -  Status = gRT->GetVariable (L"OptDeviceTree",
>> +  Status = gRT->GetVariable (L"SystemTableMode",
>>                     &gConfigDxeFormSetGuid,
>>                     NULL, &Size, &Var32);
>>     if (EFI_ERROR (Status)) {
>> -    PcdSet32 (PcdOptDeviceTree, PcdGet32 (PcdOptDeviceTree));
>> +    PcdSet32 (PcdSystemTableMode, PcdGet32 (PcdSystemTableMode));
>>     }
>>     Size = sizeof (UINT32);
>> @@ -488,8 +488,11 @@ ConfigInitialize (
>>       DEBUG ((DEBUG_ERROR, "Couldn't install ConfigDxe configuration 
>> pages: %r\n", Status));
>>     }
>> -  Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
>> -  ASSERT_EFI_ERROR (Status);
>> +  if (PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_ACPI ||
>> +      PcdGet32 (PcdSystemTableMode) == SYSTEM_TABLE_MODE_BOTH) {
> 
> Overall, I think code readability might have been improved by using 
> bitmasks (e.g. bit 0 for ACPI, bit 1 for DT) rather than a triplet of 
> values, but I'm fine with the patch as it stands.
> 
>> +     Status = LocateAndInstallAcpiFromFv (&mAcpiTableFile);
>> +     ASSERT_EFI_ERROR (Status);
>> +  }
>>     return EFI_SUCCESS;
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> index 57963baf..e47f3af6 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
>> @@ -77,7 +77,7 @@
>>     gRaspberryPiTokenSpaceGuid.PcdDebugShowUEFIExit
>>     gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes
>>     gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot
>> -  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree
>> +  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode
>>     gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
>>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
>> index 07660072..db36e200 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
>> @@ -41,10 +41,11 @@
>>   #string STR_ADVANCED_3GB_OFF         #language en-US "Disabled"
>>   #string STR_ADVANCED_3GB_ON          #language en-US "Enabled"
>> -#string STR_ADVANCED_DT_PROMPT       #language en-US "Device Tree"
>> -#string STR_ADVANCED_DT_HELP         #language en-US "Disable this 
>> option to force OSes such as GNU/Linux to use ACPI"
>> -#string STR_ADVANCED_DT_OFF          #language en-US "Disabled (Force 
>> ACPI)"
>> -#string STR_ADVANCED_DT_ON           #language en-US "Enabled"
>> +#string STR_ADVANCED_SYSTAB_PROMPT   #language en-US "System Table 
>> Selection"
>> +#string STR_ADVANCED_SYSTAB_HELP     #language en-US "ACPI/DT choice 
>> for specific OSes"
>> +#string STR_ADVANCED_SYSTAB_ACPI     #language en-US "ACPI"
>> +#string STR_ADVANCED_SYSTAB_BOTH     #language en-US "ACPI + Devicetree"
>> +#string STR_ADVANCED_SYSTAB_DT       #language en-US "Devicetree"
>>   /*
>>    * MMC/SD configuration.
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
>> index 576eabe9..91a0ea2d 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
>> @@ -44,9 +44,9 @@ formset
>>         name  = RamLimitTo3GB,
>>         guid  = CONFIGDXE_FORM_SET_GUID;
>> -    efivarstore ADVANCED_DEVICE_TREE_VARSTORE_DATA,
>> +    efivarstore SYSTEM_TABLE_MODE_VARSTORE_DATA,
>>         attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | 
>> EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
>> -      name  = OptDeviceTree,
>> +      name  = SystemTableMode,
>>         guid  = CONFIGDXE_FORM_SET_GUID;
>>       efivarstore MMC_SD_VARSTORE_DATA,
>> @@ -164,12 +164,13 @@ formset
>>             endoneof;
>>           endif;
>> -        oneof varid = OptDeviceTree.Enabled,
>> -            prompt      = STRING_TOKEN(STR_ADVANCED_DT_PROMPT),
>> -            help        = STRING_TOKEN(STR_ADVANCED_DT_HELP),
>> +        oneof varid = SystemTableMode.Mode,
>> +            prompt      = STRING_TOKEN(STR_ADVANCED_SYSTAB_PROMPT),
>> +            help        = STRING_TOKEN(STR_ADVANCED_SYSTAB_HELP),
>>               flags       = NUMERIC_SIZE_4 | INTERACTIVE | 
>> RESET_REQUIRED,
>> -            option text = STRING_TOKEN(STR_ADVANCED_DT_OFF), value = 
>> 0, flags = 0;
>> -            option text = STRING_TOKEN(STR_ADVANCED_DT_ON), value = 
>> 1, flags = DEFAULT;
>> +            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_ACPI), 
>> value = SYSTEM_TABLE_MODE_ACPI, flags = 0;
>> +            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_BOTH), 
>> value = SYSTEM_TABLE_MODE_BOTH, flags = DEFAULT;
>> +            option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value 
>> = SYSTEM_TABLE_MODE_DT, flags = DEFAULT;
>>           endoneof;
>>       endform;
>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c 
>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>> index 3aaa0a7f..44c6c87c 100644
>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>> @@ -355,7 +355,8 @@ FdtDxeInitialize (
>>     UINTN      FdtSize;
>>     VOID       *FdtImage = NULL;
>> -  if (PcdGet32 (PcdOptDeviceTree) == 0) {
>> +  if (PcdGet32 (PcdSystemTableMode) != SYSTEM_TABLE_MODE_BOTH &&
>> +      PcdGet32 (PcdSystemTableMode) != SYSTEM_TABLE_MODE_DT) {
>>       DEBUG ((DEBUG_INFO, "Device Tree disabled per user 
>> configuration\n"));
>>       return EFI_SUCCESS;
>>     }
>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf 
>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
>> index 49ba5ba1..26f84e59 100644
>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf
>> @@ -46,4 +46,4 @@
>>     gRaspberryPiTokenSpaceGuid.PcdFdtBaseAddress
>>   [Pcd]
>> -  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree
>> +  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode
>> diff --git a/Platform/RaspberryPi/Include/ConfigVars.h 
>> b/Platform/RaspberryPi/Include/ConfigVars.h
>> index a0959b4b..28837f98 100644
>> --- a/Platform/RaspberryPi/Include/ConfigVars.h
>> +++ b/Platform/RaspberryPi/Include/ConfigVars.h
>> @@ -76,12 +76,11 @@ typedef struct {
>>   } ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA;
>>   typedef struct {
>> -  /*
>> -   * 0 - Do not provide a Device Tree to the OS
>> -   * 1 - Provide a Device Tree to the OS
>> -   */
>> -  UINT32 Enabled;
>> -} ADVANCED_DEVICE_TREE_VARSTORE_DATA;
>> +#define SYSTEM_TABLE_MODE_ACPI 0
>> +#define SYSTEM_TABLE_MODE_BOTH 1
>> +#define SYSTEM_TABLE_MODE_DT   2
>> +  UINT32 Mode;
>> +} SYSTEM_TABLE_MODE_VARSTORE_DATA;
>>   typedef struct {
>>     /*
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc 
>> b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> index 563fb891..a138c874 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> @@ -481,9 +481,13 @@
>>     
>> gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|L"RamLimitTo3GB"|gConfigDxeFormSetGuid|0x0|0 
>>
>>     #
>> -  # Device Tree
>> +  # Device Tree and ACPI selection.
>>     #
>> -  
>> gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|L"OptDeviceTree"|gConfigDxeFormSetGuid|0x0|1 
>>
>> +  # 0 - SYSTEM_TABLE_MODE_ACPI
>> +  # 1 - SYSTEM_TABLE_MODE_BOTH (default)
>> +  # 2 - SYSTEM_TABLE_MODE_DT
>> +  #
>> +  
>> gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|L"SystemTableMode"|gConfigDxeFormSetGuid|0x0|1 
>>
>>     #
>>     # Common UEFI ones.
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc 
>> b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> index 4deccd9d..75867f03 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> @@ -496,9 +496,13 @@
>>     
>> gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|L"RamLimitTo3GB"|gConfigDxeFormSetGuid|0x0|1 
>>
>>     #
>> -  # Device Tree
>> +  # Device Tree and ACPI selection.
>>     #
>> -  
>> gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|L"OptDeviceTree"|gConfigDxeFormSetGuid|0x0|0 
>>
>> +  # 0 - SYSTEM_TABLE_MODE_ACPI (default)
>> +  # 1 - SYSTEM_TABLE_MODE_BOTH
>> +  # 2 - SYSTEM_TABLE_MODE_DT
>> +  #
>> +  
>> gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|L"SystemTableMode"|gConfigDxeFormSetGuid|0x0|0 
>>
>>     #
>>     # Common UEFI ones.
>> diff --git a/Platform/RaspberryPi/RaspberryPi.dec 
>> b/Platform/RaspberryPi/RaspberryPi.dec
>> index 66ef6186..1a3c44e0 100644
>> --- a/Platform/RaspberryPi/RaspberryPi.dec
>> +++ b/Platform/RaspberryPi/RaspberryPi.dec
>> @@ -65,6 +65,6 @@
>>     gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock|0|UINT32|0x00000016
>>     
>> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes|0x3F|UINT8|0x00000017 
>>
>>     gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot|0|UINT32|0x00000018
>> -  gRaspberryPiTokenSpaceGuid.PcdOptDeviceTree|1|UINT32|0x0000001B
>> +  gRaspberryPiTokenSpaceGuid.PcdSystemTableMode|1|UINT32|0x0000001B
>>     gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|0|UINT32|0x00000019
>>     gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
>>
> 
> Reviewed-by: Pete Batard <pete@akeo.ie>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 0/2] RPi - add DT-only mode
  2020-05-10 21:34 [edk2-platforms][PATCH 0/2] RPi - add DT-only mode Andrei Warkentin
  2020-05-10 21:34 ` [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h Andrei Warkentin
  2020-05-10 21:34 ` [edk2-platforms][PATCH 2/2] RPi: allow selecting DT-only mode Andrei Warkentin
@ 2020-05-11 11:28 ` Ard Biesheuvel
  2 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2020-05-11 11:28 UTC (permalink / raw)
  To: Andrei Warkentin, devel; +Cc: leif, pete, philmd

On 5/10/20 11:34 PM, Andrei Warkentin wrote:
> Today the Pies can be booted in a way where only ACPI is exposed,
> or both ACPI and DT are exposed.
> 
> This patch set adds one more mode - DT only, no ACPI. The target
> audience is developers. When both are exposed, it's up to the OS
> to decide which gets used, and that choice can differ between OSes.
> 
> Note: this does _not_ change defaults. Pi 3 still defaults to
> ACPI + DT, while Pi 4 still defaults to ACPI only.
> 
> We don't really want to remove DT + ACPI mode - it is the default
> on Pi 3, and removing it is bound to just annoy users - WoA and
> NetBSD (voa UEFI) on Pi 3 only work with ACPI, while everything
> else (Linux, FreeBSD) only work with DT. I'd make an analogy of
> MPS and ACPI being exposed for the longest time ever together on
> PCs.
> 
> Andrei Warkentin (2):
>    RPi: move varstore structure defs to ConfigVars.h
>    RPi: allow selecting DT-only mode
> 

Pushed as 51da3c318e3d..d492639638ee (with Pete's R-b and his remark 
addressed)

Thanks!


>   .../RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c |  21 ++-
>   .../Drivers/ConfigDxe/ConfigDxe.inf           |   2 +-
>   .../Drivers/ConfigDxe/ConfigDxeHii.uni        |   9 +-
>   .../Drivers/ConfigDxe/ConfigDxeHii.vfr        | 146 ++----------------
>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c  |   6 +-
>   .../RaspberryPi/Drivers/FdtDxe/FdtDxe.inf     |   2 +-
>   Platform/RaspberryPi/Include/ConfigVars.h     | 131 ++++++++++++++++
>   Platform/RaspberryPi/RPi3/RPi3.dsc            |   8 +-
>   Platform/RaspberryPi/RPi4/RPi4.dsc            |   8 +-
>   Platform/RaspberryPi/RaspberryPi.dec          |   2 +-
>   10 files changed, 180 insertions(+), 155 deletions(-)
>   create mode 100644 Platform/RaspberryPi/Include/ConfigVars.h
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-11 11:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-10 21:34 [edk2-platforms][PATCH 0/2] RPi - add DT-only mode Andrei Warkentin
2020-05-10 21:34 ` [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h Andrei Warkentin
2020-05-11 10:57   ` Pete Batard
2020-05-10 21:34 ` [edk2-platforms][PATCH 2/2] RPi: allow selecting DT-only mode Andrei Warkentin
2020-05-11 10:58   ` Pete Batard
2020-05-11 11:25     ` Ard Biesheuvel
2020-05-11 11:28 ` [edk2-platforms][PATCH 0/2] RPi - add " Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox