From: "Pete Batard" <pete@akeo.ie>
To: Andrei Warkentin <andrey.warkentin@gmail.com>, devel@edk2.groups.io
Cc: ard.biesheuvel@arm.com, leif@nuviainc.com, philmd@redhat.com
Subject: Re: [edk2-platforms][PATCH 2/2] RPi: allow selecting DT-only mode
Date: Mon, 11 May 2020 11:58:13 +0100 [thread overview]
Message-ID: <7175d5c7-fd47-245b-f593-44b091966ea7@akeo.ie> (raw)
In-Reply-To: <20200510213450.12642-3-andrey.warkentin@gmail.com>
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>
next prev parent reply other threads:[~2020-05-11 10:58 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-05-11 11:25 ` Ard Biesheuvel
2020-05-11 11:28 ` [edk2-platforms][PATCH 0/2] RPi - add " 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=7175d5c7-fd47-245b-f593-44b091966ea7@akeo.ie \
--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