From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
To: Pete Batard <pete@akeo.ie>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
"Leif Lindholm" <leif@nuviainc.com>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Andrei Warkentin" <awarkentin@vmware.com>
Subject: Re: [edk2-devel][PATCH 3/5] Platform/RPi: Make 3GB/4GB a runtime (BIOS setup) choice
Date: Tue, 3 Mar 2020 11:42:13 +0100 [thread overview]
Message-ID: <CAKv+Gu_KjVVXfB+zcm+km+BTHKBm2+20k2OE1E6xzxXV6KCKNg@mail.gmail.com> (raw)
In-Reply-To: <20200303103339.7468-4-pete@akeo.ie>
On Tue, 3 Mar 2020 at 11:33, Pete Batard <pete@akeo.ie> wrote:
>
> From: Andrei Warkentin <andrey.warkentin@gmail.com>
>
> Currently some OSes (e.g FreeBSD) can make full use of the maximum
> 4 GB of RAM a Raspberry Pi 4 can offer, whereas others (e.g. Linux)
> must be restricted to only the first 3 GB.
>
> Previously this was a compile-time choice chosen by PcdAcpiBasicMode,
> and now we make it user-selectable. The default is a 3 GB limit.
>
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
> Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 58 ++++++++++++++++----
> Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf | 8 ++-
> Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni | 11 ++++
> Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 41 ++++++++++++++
> Platform/RaspberryPi/RPi3/RPi3.dsc | 6 ++
> Platform/RaspberryPi/RPi4/RPi4.dsc | 6 ++
> Platform/RaspberryPi/RaspberryPi.dec | 4 +-
> 7 files changed, 120 insertions(+), 14 deletions(-)
>
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> index 2f48ca0dd758..451a419a5358 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> @@ -11,11 +11,13 @@
> #include <Library/AcpiLib.h>
> #include <Library/HiiLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/DxeServicesTableLib.h>
> #include <Library/IoLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiRuntimeServicesTableLib.h>
> #include <Library/DevicePathLib.h>
> #include <IndustryStandard/RpiMbox.h>
> +#include <IndustryStandard/Bcm2711.h>
> #include <IndustryStandard/Bcm2836.h>
> #include <IndustryStandard/Bcm2836Gpio.h>
> #include <Library/GpioLib.h>
> @@ -26,6 +28,8 @@ extern UINT8 ConfigDxeHiiBin[];
> extern UINT8 ConfigDxeStrings[];
>
> STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
> +STATIC UINT32 mModelFamily = 0;
> +STATIC UINT32 mModelInstalledMB = 0;
>
> /*
> * The GUID inside Platform/RaspberryPi/RPi3/AcpiTables/AcpiTables.inf and
> @@ -129,6 +133,24 @@ SetupVariables (
> PcdSet32 (PcdCustomCpuClock, PcdGet32 (PcdCustomCpuClock));
> }
>
> + if (mModelFamily >= 4 && mModelInstalledMB > 3 * 1024) {
> + /*
> + * This allows changing PcdRamLimitTo3GB in forms.
> + */
> + PcdSet32 (PcdRamMoreThan3GB, 1);
> +
> + Size = sizeof (UINT32);
> + Status = gRT->GetVariable (L"RamLimitTo3GB",
> + &gConfigDxeFormSetGuid,
> + NULL, &Size, &Var32);
> + if (EFI_ERROR (Status)) {
> + PcdSet32 (PcdRamLimitTo3GB, PcdGet32 (PcdRamLimitTo3GB));
> + }
> + } else {
> + PcdSet32 (PcdRamMoreThan3GB, 0);
> + PcdSet32 (PcdRamLimitTo3GB, 0);
> + }
> +
> Size = sizeof (UINT32);
> Status = gRT->GetVariable (L"SdIsArasan",
> &gConfigDxeFormSetGuid,
> @@ -224,7 +246,6 @@ ApplyVariables (
> UINT32 CpuClock = PcdGet32 (PcdCpuClock);
> UINT32 CustomCpuClock = PcdGet32 (PcdCustomCpuClock);
> UINT32 Rate = 0;
> - UINT32 ModelFamily = 0;
>
> if (CpuClock != 0) {
> if (CpuClock == 2) {
> @@ -258,15 +279,18 @@ ApplyVariables (
> DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
> }
>
> - Status = mFwProtocol->GetModelFamily (&ModelFamily);
> - if (Status != EFI_SUCCESS) {
> - DEBUG ((DEBUG_ERROR, "Couldn't get the Raspberry Pi model family: %r\n", Status));
> - } else {
> - DEBUG ((DEBUG_INFO, "Current Raspberry Pi model family is 0x%x\n", ModelFamily));
> + if (mModelFamily >= 4 && PcdGet32 (PcdRamLimitTo3GB) == 0) {
> + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, BASE_1GB + SIZE_2GB,
> + SIZE_1GB - (SIZE_4GB - BCM2711_SOC_REGISTERS),
> + EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB);
> + ASSERT_EFI_ERROR (Status);
> + Status = gDS->SetMemorySpaceAttributes (BASE_1GB + SIZE_2GB,
> + SIZE_1GB - (SIZE_4GB - BCM2711_SOC_REGISTERS),
> + EFI_MEMORY_WB);
> + ASSERT_EFI_ERROR (Status);
> }
>
This looks a bit hacky and ad-hoc to me.
What is
SIZE_1GB - (SIZE_4GB - BCM2711_SOC_REGISTERS),
supposed to mean? Can we declare it somewhere and document it, rather
than drop it into the middle of a function call?
> -
> - if (ModelFamily == 3) {
> + if (mModelFamily == 3) {
> /*
> * Pi 3: either Arasan or SdHost goes to SD card.
> *
> @@ -316,7 +340,7 @@ ApplyVariables (
> GpioPinFuncSet (52, Gpio48Group);
> GpioPinFuncSet (53, Gpio48Group);
>
> - } else if (ModelFamily == 4) {
> + } else if (mModelFamily == 4) {
> /*
> * Pi 4: either Arasan or eMMC2 goes to SD card.
> */
> @@ -352,7 +376,7 @@ ApplyVariables (
> GpioPinFuncSet (39, GPIO_FSEL_ALT3);
> }
> } else {
> - DEBUG ((DEBUG_ERROR, "Model Family %d not supported...\n", ModelFamily));
> + DEBUG ((DEBUG_ERROR, "Model Family %d not supported...\n", mModelFamily));
> }
>
> /*
> @@ -400,6 +424,20 @@ ConfigInitialize (
> return Status;
> }
>
> + Status = mFwProtocol->GetModelFamily (&mModelFamily);
> + if (Status != EFI_SUCCESS) {
> + DEBUG ((DEBUG_ERROR, "Couldn't get the Raspberry Pi model family: %r\n", Status));
> + } else {
> + DEBUG ((DEBUG_INFO, "Current Raspberry Pi model family is %d\n", mModelFamily));
> + }
> +
> + Status = mFwProtocol->GetModelInstalledMB (&mModelInstalledMB);
> + if (Status != EFI_SUCCESS) {
> + DEBUG ((DEBUG_ERROR, "Couldn't get the Raspberry Pi installed RAM size: %r\n", Status));
> + } else {
> + DEBUG ((DEBUG_INFO, "Current Raspberry Pi installed RAM size is %d MB\n", mModelInstalledMB));
> + }
> +
> Status = SetupVariables ();
> if (Status != EFI_SUCCESS) {
> DEBUG ((DEBUG_ERROR, "Couldn't not setup NV vars: %r\n", Status));
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> index dc726cc6d934..407aac89c7b3 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> @@ -33,6 +33,7 @@ [Packages]
> ArmPlatformPkg/ArmPlatformPkg.dec
> MdePkg/MdePkg.dec
> MdeModulePkg/MdeModulePkg.dec
> + Silicon/Broadcom/Bcm27xx/Bcm27xx.dec
> Silicon/Broadcom/Bcm283x/Bcm283x.dec
> Platform/RaspberryPi/RaspberryPi.dec
> EmbeddedPkg/EmbeddedPkg.dec
> @@ -53,10 +54,11 @@ [Guids]
> gConfigDxeFormSetGuid
>
> [Protocols]
> - gRaspberryPiFirmwareProtocolGuid ## CONSUMES
> + gRaspberryPiFirmwareProtocolGuid ## CONSUMES
> gRaspberryPiConfigAppliedProtocolGuid ## PRODUCES
>
> [Pcd]
> + gBcm27xxTokenSpaceGuid.PcdBcm27xxRegistersAddress
> gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
> gRaspberryPiTokenSpaceGuid.PcdCpuClock
> gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock
> @@ -70,8 +72,8 @@ [Pcd]
> gRaspberryPiTokenSpaceGuid.PcdDebugShowUEFIExit
> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes
> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot
> -
> -[FeaturePcd]
> + gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB
> + gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB
>
> [Depex]
> gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> index 9b4076635f05..830533a9dc49 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.uni
> @@ -35,6 +35,17 @@
> #string STR_CHIPSET_SD_SDHOST #language en-US "Broadcom SDHOST"
> #string STR_CHIPSET_SD_ARASAN #language en-US "Arasan SDHCI"
>
> +/*
> + * Advanced configuration.
> + */
> +
> +#string STR_ADVANCED_FORM_TITLE #language en-US "Advanced Configuration"
> +
> +#string STR_ADVANCED_3GB_PROMPT #language en-US "Limit RAM to 3 GB"
> +#string STR_ADVANCED_3GB_HELP #language en-US "OSes not supporting ACPI DMA constraints require a 3 GB limit or face broken xHCI USB"
> +#string STR_ADVANCED_3GB_OFF #language en-US "Disabled"
> +#string STR_ADVANCED_3GB_ON #language en-US "Enabled"
> +
> /*
> * MMC/SD configuration.
> */
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> index 60bfdbd4d17e..483edd7459c5 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr
> @@ -73,6 +73,18 @@ typedef struct {
> UINT32 Routing;
> } CHIPSET_SD_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 - Don't disable multi-block.
> @@ -140,6 +152,16 @@ formset
> 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,
> + guid = CONFIGDXE_FORM_SET_GUID;
> +
> + efivarstore ADVANCED_RAM_LIMIT_TO_3GB_VARSTORE_DATA,
> + attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
> + name = RamLimitTo3GB,
> + 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,
> @@ -193,6 +215,10 @@ formset
> prompt = STRING_TOKEN(STR_CHIPSET_FORM_TITLE),
> help = STRING_TOKEN(STR_NULL_STRING);
>
> + goto 0x1006,
> + prompt = STRING_TOKEN(STR_ADVANCED_FORM_TITLE),
> + help = STRING_TOKEN(STR_NULL_STRING);
> +
> goto 0x1003,
> prompt = STRING_TOKEN(STR_MMC_FORM_TITLE),
> help = STRING_TOKEN(STR_NULL_STRING);
> @@ -240,6 +266,21 @@ formset
> endoneof;
> endform;
>
> + form formid = 0x1006,
> + title = STRING_TOKEN(STR_ADVANCED_FORM_TITLE);
> + subtitle text = STRING_TOKEN(STR_NULL_STRING);
> +
> + grayoutif ideqval RamMoreThan3GB.Supported == 0;
> + oneof varid = RamLimitTo3GB.Enabled,
> + prompt = STRING_TOKEN(STR_ADVANCED_3GB_PROMPT),
> + help = STRING_TOKEN(STR_ADVANCED_3GB_HELP),
> + flags = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED,
> + option text = STRING_TOKEN(STR_ADVANCED_3GB_OFF), value = 0, flags = DEFAULT;
> + option text = STRING_TOKEN(STR_ADVANCED_3GB_ON), value = 1, flags = 0;
> + endoneof;
> + endif;
> + endform;
> +
> form formid = 0x1003,
> title = STRING_TOKEN(STR_MMC_FORM_TITLE);
> subtitle text = STRING_TOKEN(STR_MMC_FORM_SUBTITLE);
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> index df5b246af1f8..48e1a32e1d24 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> @@ -439,6 +439,12 @@ [PcdsDynamicHii.common.DEFAULT]
> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes|L"DisplayEnableScaledVModes"|gConfigDxeFormSetGuid|0x0|0xff
> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot|L"DisplayEnableSShot"|gConfigDxeFormSetGuid|0x0|1
>
> + #
> + # Supporting > 3GB of memory.
> + #
> + gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|L"RamMoreThan3GB"|gConfigDxeFormSetGuid|0x0|0
> + gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|L"RamLimitTo3GB"|gConfigDxeFormSetGuid|0x0|0
> +
> #
> # Common UEFI ones.
> #
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index 60a5e38da778..3ce2c3e4d519 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -477,6 +477,12 @@ [PcdsDynamicHii.common.DEFAULT]
> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes|L"DisplayEnableScaledVModes"|gConfigDxeFormSetGuid|0x0|0xff
> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot|L"DisplayEnableSShot"|gConfigDxeFormSetGuid|0x0|1
>
> + #
> + # Supporting > 3GB of memory.
> + #
> + gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|L"RamMoreThan3GB"|gConfigDxeFormSetGuid|0x0|0
> + gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|L"RamLimitTo3GB"|gConfigDxeFormSetGuid|0x0|1
> +
> #
> # Common UEFI ones.
> #
> diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
> index bc378ffbfb8d..7f2c37ac9a7f 100644
> --- a/Platform/RaspberryPi/RaspberryPi.dec
> +++ b/Platform/RaspberryPi/RaspberryPi.dec
> @@ -57,6 +57,8 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> gRaspberryPiTokenSpaceGuid.PcdCustomCpuClock|0|UINT32|0x00000016
> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes|0|UINT8|0x00000017
> gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot|0|UINT32|0x00000018
> + gRaspberryPiTokenSpaceGuid.PcdRamMoreThan3GB|0|UINT32|0x00000019
> + gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
>
> [PcdsFeatureFlag.common]
> - gRaspberryPiTokenSpaceGuid.PcdAcpiBasicMode|FALSE|BOOLEAN|0x00000019
> + gRaspberryPiTokenSpaceGuid.PcdAcpiBasicMode|FALSE|BOOLEAN|0x0000001B
> --
> 2.21.0.windows.1
>
next prev parent reply other threads:[~2020-03-03 10:42 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-03 10:33 [edk2-devel][PATCH 0/5] Platform/RPi: User config improvements Pete Batard
2020-03-03 10:33 ` [edk2-devel][PATCH 1/5] Platform/RPi: Add firmware call to read installed memory size Pete Batard
2020-03-03 10:37 ` Ard Biesheuvel
2020-03-03 10:33 ` [edk2-devel][PATCH 2/5] Platform/RPi: Separate RAM descriptors between 0-3 GB and 3+ GB Pete Batard
2020-03-03 10:39 ` Ard Biesheuvel
2020-03-03 10:33 ` [edk2-devel][PATCH 3/5] Platform/RPi: Make 3GB/4GB a runtime (BIOS setup) choice Pete Batard
2020-03-03 10:42 ` Ard Biesheuvel [this message]
2020-03-03 10:33 ` [edk2-devel][PATCH 4/5] Platform/RPi: Make Device Tree provision " Pete Batard
2020-03-03 10:45 ` Ard Biesheuvel
2020-03-03 10:33 ` [edk2-devel][PATCH 5/5] Platform/RPi/ConfigDxe: Improve RPi configuration forms Pete Batard
2020-03-03 10:45 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu_KjVVXfB+zcm+km+BTHKBm2+20k2OE1E6xzxXV6KCKNg@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox