From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web10.9742.1589194677349660588 for ; Mon, 11 May 2020 03:57:57 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=RW79GGaT; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.68, mailfrom: pete@akeo.ie) Received: by mail-wr1-f68.google.com with SMTP id x17so10379069wrt.5 for ; Mon, 11 May 2020 03:57:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=CeW3aBQZgfBXA+EP0GdxPDYo20ia356Xau8GPQp0CWc=; b=RW79GGaTMz56RmPHJxpjQRQ/ynQAPBZVeT3Bhb0kamThcQCuKXx3MmP20bzAohEiDC EctGVU5S+roH7e5BiFqFSZj7EjvTjOAz+LPd0Ud++CfwrHa1xwkiNKKnnmcewQkpFuYI LMohz5KctVLUmn12bxPBBXVKi574S7f2qBC1HpjLCM3eAGRoAzMCmTsRBoFb0yYbIO7d EG637Y7iel5O6r1kQIdsXv5L3bizU8lfXxaaFpZ/Ehf69T10+meHfCdp3TUck2HBSzbm VMcJZqH3OyjGqP59/6aaEd4+JFkXoh+NFMvI+wAWXN4EORVKGVYXYybjAW43YdrdVYLL JT+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=CeW3aBQZgfBXA+EP0GdxPDYo20ia356Xau8GPQp0CWc=; b=pK149WC3V3IXSB771d4zaZlNR561bX1ATwUOlMCuCdV+1Wtq3wPmQqgootYIvWwLiV 2rizz9WBA5C9hF7pIrECiVqElElwJrAiTblIPPXWnHdf5BOYwfhhCJdEq3YCq7Wj/BOM TY496WR+UT74HzjN3eTCjjJ2g4Aw+iIREdivACKvP2/wRvJ7WnSpyVTu3S7twdOXFWxr 7TqeU/K66O3Rk6NfFJEbn+MC6KlF+4Z0viLbXg6mMf7QK9pTx9lHEhXtgSTtz4n74EAv kXFE5NeSjaA0tz8hCn8doVhGFkABF1bBZb0HuPpoemZnte2vBe8c9ZybacaLpzusGrdH M17w== X-Gm-Message-State: AGi0PuaGKZ/XBDpqfHXqTGkGRIihLt7PVOpHAHfKy4pQsJDQC00I9W40 vqKxXEzf+OAyfn5JrzPOKnTRAw== X-Google-Smtp-Source: APiQypKPAnXvwmhD+0BlouwPdDLfj6dlrM3s8C0fVNaK3WjLrnQT9b3qKy1AhxNm+7kJSoYFrf1QiA== X-Received: by 2002:a5d:4248:: with SMTP id s8mr18057538wrr.216.1589194675735; Mon, 11 May 2020 03:57:55 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.75.87]) by smtp.googlemail.com with ESMTPSA id f26sm26127283wmj.11.2020.05.11.03.57.54 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 11 May 2020 03:57:54 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH 1/2] RPi: move varstore structure defs to ConfigVars.h To: Andrei Warkentin , devel@edk2.groups.io Cc: ard.biesheuvel@arm.com, leif@nuviainc.com, philmd@redhat.com References: <20200510213450.12642-1-andrey.warkentin@gmail.com> <20200510213450.12642-2-andrey.warkentin@gmail.com> From: "Pete Batard" Message-ID: <0c82c341-29db-5e27-c786-b6682337358b@akeo.ie> Date: Mon, 11 May 2020 11:57:53 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <20200510213450.12642-2-andrey.warkentin@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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 > --- > 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 > #include > #include > +#include > #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 > #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 > > // > // 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 > #include > #include > - > #include > - > #include > +#include > > 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 > + * > + * 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