Re: [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h

["Pete Batard" <pete@akeo.ie>]

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>