Re: [edk2-devel][PATCH v2 6/6] Platform/RPi/ConfigDxe: Improve RPi configuration form

["Andrei Warkentin" <awarkentin@vmware.com>]

Nevermind, I missed all the current reviews and the code is already in.

A

________________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Andrei Warkentin via Groups.Io <awarkentin=vmware.com@groups.io>
Sent: Tuesday, March 3, 2020 11:17 AM
To: Pete Batard; devel@edk2.groups.io
Cc: ard.biesheuvel@linaro.org; leif@nuviainc.com; philmd@redhat.com
Subject: Re: [edk2-devel][PATCH v2 6/6] Platform/RPi/ConfigDxe: Improve RPi configuration form

Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>

________________________________________
From: Pete Batard <pete@akeo.ie>
Sent: Tuesday, March 3, 2020 8:08 AM
To: devel@edk2.groups.io
Cc: ard.biesheuvel@linaro.org; leif@nuviainc.com; philmd@redhat.com; Andrei Warkentin
Subject: [edk2-devel][PATCH v2 6/6] Platform/RPi/ConfigDxe: Improve RPi configuration form

Group all SD/MMC settings under the SD/MMC form.
Make CPU settings more prominent.
Harmonise form titles and text content.
Reorder forms in the order they are most likely to be queried.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni | 24 +++++-----
 Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 50 ++++++++++----------
 2 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
index 2e79e322e558..77eda96d8136 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
@@ -14,11 +14,11 @@
 #string STR_FORM_SET_TITLE_HELP  #language en-US "Press <Enter> to configure system settings."

 /*
- * Chipset config.
+ * Chipset configuration.
  */

-#string STR_CHIPSET_FORM_TITLE      #language en-US "Chipset Configuration"
-#string STR_CHIPSET_FORM_SUBTITLE   #language en-US "Note: OS may override settings when booted."
+#string STR_CHIPSET_FORM_TITLE      #language en-US "CPU Configuration"
+#string STR_CHIPSET_FORM_SUBTITLE   #language en-US "Note: OS may override settings."

 #string STR_CHIPSET_CLOCK_CPU_PROMPT #language en-US "CPU Clock"
 #string STR_CHIPSET_CLOCK_CPU_HELP   #language en-US "CPU Speed"
@@ -30,11 +30,6 @@
 #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_SD_PROMPT        #language en-US "uSD Routing"
-#string STR_CHIPSET_SD_HELP          #language en-US "Choose host controller to drive uSD slot"
-#string STR_CHIPSET_SD_SDHOST        #language en-US "Broadcom SDHOST"
-#string STR_CHIPSET_SD_ARASAN        #language en-US "Arasan SDHCI"
-
 /*
  * Advanced configuration.
  */
@@ -55,8 +50,13 @@
  * MMC/SD configuration.
  */

-#string STR_MMC_FORM_TITLE       #language en-US "SD/MMC Tweaks"
-#string STR_MMC_FORM_SUBTITLE    #language en-US "Note: UEFI only, OS will override settings when booted."
+#string STR_MMC_FORM_TITLE       #language en-US "SD/MMC Configuration"
+#string STR_MMC_FORM_SUBTITLE    #language en-US "Note: UEFI only, OS may override settings."
+
+#string STR_MMC_SD_PROMPT        #language en-US "uSD Routing"
+#string STR_MMC_SD_HELP          #language en-US "Choose host controller to drive uSD slot"
+#string STR_MMC_SD_SDHOST        #language en-US "Broadcom SDHOST"
+#string STR_MMC_SD_ARASAN        #language en-US "Arasan SDHCI"

 #string STR_MMC_DISMULTI_PROMPT  #language en-US "Multi-Block Support"
 #string STR_MMC_DISMULTI_HELP    #language en-US "Use CMD18/CMD25 for transfers when possible"
@@ -84,7 +84,7 @@
  * Display settings.
  */

-#string STR_DISPLAY_FORM_TITLE      #language en-US "Display"
+#string STR_DISPLAY_FORM_TITLE      #language en-US "Display Configuration"
 #string STR_DISPLAY_FORM_SUBTITLE   #language en-US "UEFI video driver settings"

 #string STR_DISPLAY_VMODES_640_PROMPT  #language en-US "Virtual 640x480"
@@ -109,7 +109,7 @@
 /*
  * Debugging settings go here.
  */
-#string STR_DEBUG_FORM_TITLE        #language en-US "Debugging"
+#string STR_DEBUG_FORM_TITLE        #language en-US "Debugging Configuration"
 #string STR_DEBUG_FORM_SUBTITLE     #language en-US "For UEFI/OS Developers"

 #string STR_DEBUG_JTAG_PROMPT       #language en-US "JTAG Routing"
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
index d16058da4926..9c2fd64a8e27 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
@@ -65,14 +65,6 @@ typedef struct {
   UINT32 Clock;
 } CHIPSET_CUSTOM_CPU_CLOCK_VARSTORE_DATA;

-typedef struct {
-  /*
-   * 0 - uSD slot routed to Broadcom SDHOST.
-   * 1 - uSD slot routed to Arasan SDHCI.
-   */
-  UINT32 Routing;
-} CHIPSET_SD_VARSTORE_DATA;
-
 typedef struct {
   /*
    * Always set by ConfigDxe prior to HII init to reflect
@@ -93,6 +85,14 @@ typedef struct {
   UINT32 Enabled;
 } ADVANCED_DEVICE_TREE_VARSTORE_DATA;

+typedef struct {
+  /*
+   * 0 - uSD slot routed to Broadcom SDHOST.
+   * 1 - uSD slot routed to Arasan SDHCI.
+   */
+  UINT32 Routing;
+} MMC_SD_VARSTORE_DATA;
+
 typedef struct {
   /*
    * 0 - Don't disable multi-block.
@@ -155,11 +155,6 @@ formset
       name  = CustomCpuClock,
       guid  = CONFIGDXE_FORM_SET_GUID;

-    efivarstore CHIPSET_SD_VARSTORE_DATA,
-      attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
-      name  = SdIsArasan,
-      guid  = CONFIGDXE_FORM_SET_GUID;
-
     efivarstore ADVANCED_RAM_MORE_THAN_3GB_VARSTORE_DATA,
       attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
       name  = RamMoreThan3GB,
@@ -175,6 +170,11 @@ formset
       name  = OptDeviceTree,
       guid  = CONFIGDXE_FORM_SET_GUID;

+    efivarstore MMC_SD_VARSTORE_DATA,
+      attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
+      name  = SdIsArasan,
+      guid  = CONFIGDXE_FORM_SET_GUID;
+
     efivarstore MMC_DISMULTI_VARSTORE_DATA,
       attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
       name  = MmcDisableMulti,
@@ -228,6 +228,10 @@ formset
            prompt = STRING_TOKEN(STR_CHIPSET_FORM_TITLE),
            help = STRING_TOKEN(STR_NULL_STRING);

+        goto 0x1004,
+            prompt = STRING_TOKEN(STR_DISPLAY_FORM_TITLE),
+            help = STRING_TOKEN(STR_NULL_STRING);
+
         goto 0x1006,
            prompt = STRING_TOKEN(STR_ADVANCED_FORM_TITLE),
            help = STRING_TOKEN(STR_NULL_STRING);
@@ -236,10 +240,6 @@ formset
            prompt = STRING_TOKEN(STR_MMC_FORM_TITLE),
            help = STRING_TOKEN(STR_NULL_STRING);

-        goto 0x1004,
-            prompt = STRING_TOKEN(STR_DISPLAY_FORM_TITLE),
-            help = STRING_TOKEN(STR_NULL_STRING);
-
         goto 0x1005,
             prompt = STRING_TOKEN(STR_DEBUG_FORM_TITLE),
             help = STRING_TOKEN(STR_NULL_STRING);
@@ -269,14 +269,6 @@ formset
               default = 600,
           endnumeric;
         endif;
-
-        oneof varid = SdIsArasan.Routing,
-            prompt      = STRING_TOKEN(STR_CHIPSET_SD_PROMPT),
-            help        = STRING_TOKEN(STR_CHIPSET_SD_HELP),
-            flags       = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
-            option text = STRING_TOKEN(STR_CHIPSET_SD_ARASAN), value = 1, flags = 0;
-            option text = STRING_TOKEN(STR_CHIPSET_SD_SDHOST), value = 0, flags = DEFAULT;
-        endoneof;
     endform;

     form formid = 0x1006,
@@ -306,6 +298,14 @@ formset
         title  = STRING_TOKEN(STR_MMC_FORM_TITLE);
         subtitle text = STRING_TOKEN(STR_MMC_FORM_SUBTITLE);

+        oneof varid = SdIsArasan.Routing,
+            prompt      = STRING_TOKEN(STR_MMC_SD_PROMPT),
+            help        = STRING_TOKEN(STR_MMC_SD_HELP),
+            flags       = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
+            option text = STRING_TOKEN(STR_MMC_SD_ARASAN), value = 1, flags = 0;
+            option text = STRING_TOKEN(STR_MMC_SD_SDHOST), value = 0, flags = DEFAULT;
+        endoneof;
+
         oneof varid = MmcDisableMulti.DisableMulti,
             prompt      = STRING_TOKEN(STR_MMC_DISMULTI_PROMPT),
             help        = STRING_TOKEN(STR_MMC_DISMULTI_HELP),
--
2.21.0.windows.1