* [edk2-platforms: PATCH 0/1] Platforms/RPi3: Add multiple embedded Device Tree selection
@ 2019-08-12 13:06 Pete Batard
2019-08-12 13:06 ` [edk2-platforms: PATCH 1/1] " Pete Batard
0 siblings, 1 reply; 4+ messages in thread
From: Pete Batard @ 2019-08-12 13:06 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif.lindholm
The purpose of this patch is to enable the automatic provision of a relevant
Raspberry Pi 3 Device Tree at runtime, as there currently exists 2 models of
Raspberry Pi 3 (model B and model B+) but we only embed the Device Tree for a
single one (model B).
Obviously, the provision of only a single Device Tree is inconvenient, as it
currently forces users of the model B+ to locate and provide their own Device
Tree in config.txt, which might not be a straightforward operation. It also
adds to the number of extra files that must be copied to the boot media.
With this patch, both Device Tree binaries are embedded in the firmware, with
the relevant being served at runtime according to the detected hardware.
Note that we tried to future-proof the modified code with regards to expected
introduction of new Raspberry Pi platforms (such as the Pi 4). Therefore,
while the switch in FdtDxe.c may seem like overkill for now (with cases that
could be merged), it should make the reuse of that driver easier.
We also tried to harmonize debug output if FtdDxe (avoid acronyms and provide
human readable error codes).
Pete Batard (1):
Platforms/RPi3: Add multiple embedded Device Tree selection
Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 54 ++++++++++++++++----
Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf | 3 +-
Platform/RaspberryPi/RPi3/RPi3.dec | 3 +-
Platform/RaspberryPi/RPi3/RPi3.fdf | 6 ++-
4 files changed, 52 insertions(+), 14 deletions(-)
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection
2019-08-12 13:06 [edk2-platforms: PATCH 0/1] Platforms/RPi3: Add multiple embedded Device Tree selection Pete Batard
@ 2019-08-12 13:06 ` Pete Batard
2019-08-15 12:13 ` Leif Lindholm
0 siblings, 1 reply; 4+ messages in thread
From: Pete Batard @ 2019-08-12 13:06 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif.lindholm
The Raspberry Pi 3 platform currently has 2 different models, each with a
different Device Tree. Rather than embedding a single one, and requiring
users to manually provide the other, this patch ensures that we now embed
both and and serve the relevant one at runtime.
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 57 ++++++++++++++++----
Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf | 3 +-
Platform/RaspberryPi/RPi3/RPi3.dec | 3 +-
Platform/RaspberryPi/RPi3/RPi3.fdf | 6 ++-
4 files changed, 55 insertions(+), 14 deletions(-)
diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
index c84e5b2767e2..7c0ab75e7cb1 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
@@ -20,6 +20,11 @@
#include <Guid/Fdt.h>
+CONST EFI_GUID *mFdtGuid[] = {
+ &gRaspberryPiFdtGuid1,
+ &gRaspberryPiFdtGuid2,
+ };
+
STATIC VOID *mFdtImage;
STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
@@ -368,7 +373,9 @@ FdtDxeInitialize (
EFI_STATUS Status;
VOID *FdtImage;
UINTN FdtSize;
+ UINTN FdtIndex;
INT32 Retval;
+ UINT32 BoardRevision;
BOOLEAN Internal;
Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL,
@@ -383,13 +390,41 @@ FdtDxeInitialize (
* Have FDT passed via config.txt.
*/
FdtSize = fdt_totalsize (FdtImage);
- DEBUG ((DEBUG_INFO, "DTB passed via config.txt of 0x%lx bytes\n", FdtSize));
+ DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize));
Status = EFI_SUCCESS;
} else {
+ /*
+ * Use one of the embedded FDT's.
+ */
Internal = TRUE;
- DEBUG ((DEBUG_INFO, "No/bad FDT at %p (%a), trying internal DTB...\n",
- FdtImage, fdt_strerror (Retval)));
- Status = GetSectionFromAnyFv (&gRaspberryPiFdtFileGuid, EFI_SECTION_RAW, 0,
+ DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), "
+ "looking up internal one...\n", FdtImage, fdt_strerror (Retval)));
+ /*
+ * Query the board revision to differentiate between models.
+ */
+ Status = mFwProtocol->GetModelRevision (&BoardRevision);
+ if (EFI_ERROR (Status)) {
+ FdtIndex = 0;
+ DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status));
+ } else {
+ // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+ switch ((BoardRevision >> 4) & 0xFF) {
+ case 0x08: // Raspberry 3 Model B
+ DEBUG ((DEBUG_INFO, "Using RPi3 Model B internal Device Tree\n"));
+ FdtIndex = 0;
+ break;
+ case 0x0D: // Raspberry 3 Model B+
+ DEBUG ((DEBUG_INFO, "Using RPi3 Model B+ internal Device Tree\n"));
+ FdtIndex = 1;
+ break;
+ default:
+ DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
+ FdtIndex = 0;
+ break;
+ }
+ }
+ ASSERT (FdtIndex < ARRAY_SIZE (mFdtGuid));
+ Status = GetSectionFromAnyFv (mFdtGuid[FdtIndex], EFI_SECTION_RAW, 0,
&FdtImage, &FdtSize);
if (Status == EFI_SUCCESS) {
if (fdt_check_header (FdtImage) != 0) {
@@ -419,27 +454,27 @@ FdtDxeInitialize (
Status = SanitizePSCI ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to sanitize PSCI (error %d)\n", Status);
+ Print (L"Failed to sanitize PSCI: %r\n", Status);
}
Status = CleanMemoryNodes ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to clean memory nodes (error %d)\n", Status);
+ Print (L"Failed to clean memory nodes: %r\n", Status);
}
Status = CleanSimpleFramebuffer ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to clean frame buffer (error %d)\n", Status);
+ Print (L"Failed to clean frame buffer: %r\n", Status);
}
Status = FixEthernetAliases ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
+ Print (L"Failed to fix ethernet aliases: %r\n", Status);
}
Status = UpdateMacAddress ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to update MAC address (error %d)\n", Status);
+ Print (L"Failed to update MAC address: %r\n", Status);
}
if (Internal) {
@@ -448,11 +483,11 @@ FdtDxeInitialize (
*/
Status = UpdateBootArgs ();
if (EFI_ERROR (Status)) {
- Print (L"Failed to update boot arguments (error %d)\n", Status);
+ Print (L"Failed to update boot arguments: %r\n", Status);
}
}
- DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
+ DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage));
Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
ASSERT_EFI_ERROR (Status);
diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
index 5b0b1a09f374..c994a2b69d78 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
@@ -35,7 +35,8 @@ [LibraryClasses]
[Guids]
gFdtTableGuid
- gRaspberryPiFdtFileGuid
+ gRaspberryPiFdtGuid1
+ gRaspberryPiFdtGuid2
[Protocols]
gRaspberryPiFirmwareProtocolGuid ## CONSUMES
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dec b/Platform/RaspberryPi/RPi3/RPi3.dec
index 22de439fde8f..d90ea8329818 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dec
+++ b/Platform/RaspberryPi/RPi3/RPi3.dec
@@ -24,7 +24,8 @@ [Protocols]
[Guids]
gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
- gRaspberryPiFdtFileGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
+ gRaspberryPiFdtGuid1 = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
+ gRaspberryPiFdtGuid2 = {0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C}}
gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf
index c62d649834c7..83753d84badc 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.fdf
+++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
@@ -300,11 +300,15 @@ [FV.FvMain]
INF Platform/RaspberryPi/$(PLATFORM_NAME)/Drivers/LogoDxe/LogoDxe.inf
#
- # FDT (GUID matches gRaspberryPiFdtFileGuid in FdtDxe)
+ # Device Tree support
+ # GUIDs match gRaspberryPiFdtGuid# from FdtDxe.
#
FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC {
SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb
}
+ FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C {
+ SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb
+ }
[FV.FVMAIN_COMPACT]
FvAlignment = 16
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection
2019-08-12 13:06 ` [edk2-platforms: PATCH 1/1] " Pete Batard
@ 2019-08-15 12:13 ` Leif Lindholm
2019-08-15 13:11 ` Pete Batard
0 siblings, 1 reply; 4+ messages in thread
From: Leif Lindholm @ 2019-08-15 12:13 UTC (permalink / raw)
To: Pete Batard; +Cc: devel, ard.biesheuvel
On Mon, Aug 12, 2019 at 02:06:34PM +0100, Pete Batard wrote:
> The Raspberry Pi 3 platform currently has 2 different models, each with a
> different Device Tree. Rather than embedding a single one, and requiring
> users to manually provide the other, this patch ensures that we now embed
> both and and serve the relevant one at runtime.
>
> Signed-off-by: Pete Batard <pete@akeo.ie>
Changeset looks very useful. Some style issues. And one question.
First the question: is there no practical size restriction on the
firmware image? This is a 24k increase in image size.
> ---
> Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 57 ++++++++++++++++----
> Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf | 3 +-
> Platform/RaspberryPi/RPi3/RPi3.dec | 3 +-
> Platform/RaspberryPi/RPi3/RPi3.fdf | 6 ++-
> 4 files changed, 55 insertions(+), 14 deletions(-)
>
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> index c84e5b2767e2..7c0ab75e7cb1 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> @@ -20,6 +20,11 @@
>
> #include <Guid/Fdt.h>
>
> +CONST EFI_GUID *mFdtGuid[] = {
> + &gRaspberryPiFdtGuid1,
> + &gRaspberryPiFdtGuid2,
> + };
> +
> STATIC VOID *mFdtImage;
>
> STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
> @@ -368,7 +373,9 @@ FdtDxeInitialize (
> EFI_STATUS Status;
> VOID *FdtImage;
> UINTN FdtSize;
> + UINTN FdtIndex;
> INT32 Retval;
> + UINT32 BoardRevision;
> BOOLEAN Internal;
>
> Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL,
> @@ -383,13 +390,41 @@ FdtDxeInitialize (
> * Have FDT passed via config.txt.
> */
> FdtSize = fdt_totalsize (FdtImage);
This
> - DEBUG ((DEBUG_INFO, "DTB passed via config.txt of 0x%lx bytes\n", FdtSize));
> + DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize));
looks unrelated to the changeset.
> Status = EFI_SUCCESS;
> } else {
> + /*
> + * Use one of the embedded FDT's.
> + */
> Internal = TRUE;
This DEBUG changes bit
> - DEBUG ((DEBUG_INFO, "No/bad FDT at %p (%a), trying internal DTB...\n",
> - FdtImage, fdt_strerror (Retval)));
> - Status = GetSectionFromAnyFv (&gRaspberryPiFdtFileGuid, EFI_SECTION_RAW, 0,
> + DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), "
> + "looking up internal one...\n", FdtImage, fdt_strerror (Retval)));
to here looks completely unrelated to the changeset.
Which garbles the diff somewhat with regards to the functional line in
the middle..
> + /*
> + * Query the board revision to differentiate between models.
> + */
> + Status = mFwProtocol->GetModelRevision (&BoardRevision);
> + if (EFI_ERROR (Status)) {
> + FdtIndex = 0;
> + DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status));
> + } else {
> + // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
> + switch ((BoardRevision >> 4) & 0xFF) {
> + case 0x08: // Raspberry 3 Model B
> + DEBUG ((DEBUG_INFO, "Using RPi3 Model B internal Device Tree\n"));
> + FdtIndex = 0;
> + break;
> + case 0x0D: // Raspberry 3 Model B+
> + DEBUG ((DEBUG_INFO, "Using RPi3 Model B+ internal Device Tree\n"));
> + FdtIndex = 1;
> + break;
> + default:
> + DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
> + FdtIndex = 0;
> + break;
> + }
> + }
> + ASSERT (FdtIndex < ARRAY_SIZE (mFdtGuid));
> + Status = GetSectionFromAnyFv (mFdtGuid[FdtIndex], EFI_SECTION_RAW, 0,
> &FdtImage, &FdtSize);
> if (Status == EFI_SUCCESS) {
> if (fdt_check_header (FdtImage) != 0) {
Everything from here...
> @@ -419,27 +454,27 @@ FdtDxeInitialize (
>
> Status = SanitizePSCI ();
> if (EFI_ERROR (Status)) {
> - Print (L"Failed to sanitize PSCI (error %d)\n", Status);
> + Print (L"Failed to sanitize PSCI: %r\n", Status);
> }
>
> Status = CleanMemoryNodes ();
> if (EFI_ERROR (Status)) {
> - Print (L"Failed to clean memory nodes (error %d)\n", Status);
> + Print (L"Failed to clean memory nodes: %r\n", Status);
> }
>
> Status = CleanSimpleFramebuffer ();
> if (EFI_ERROR (Status)) {
> - Print (L"Failed to clean frame buffer (error %d)\n", Status);
> + Print (L"Failed to clean frame buffer: %r\n", Status);
> }
>
> Status = FixEthernetAliases ();
> if (EFI_ERROR (Status)) {
> - Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
> + Print (L"Failed to fix ethernet aliases: %r\n", Status);
> }
>
> Status = UpdateMacAddress ();
> if (EFI_ERROR (Status)) {
> - Print (L"Failed to update MAC address (error %d)\n", Status);
> + Print (L"Failed to update MAC address: %r\n", Status);
> }
>
> if (Internal) {
> @@ -448,11 +483,11 @@ FdtDxeInitialize (
> */
> Status = UpdateBootArgs ();
> if (EFI_ERROR (Status)) {
> - Print (L"Failed to update boot arguments (error %d)\n", Status);
> + Print (L"Failed to update boot arguments: %r\n", Status);
> }
> }
>
> - DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
> + DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage));
> Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
> ASSERT_EFI_ERROR (Status);
>
...to here is unrelated printout message changes.
So, I don't object to the debug printout changes as such, but they
obscure the functional changeset. Can you please break them out as a
separate patch?
Best Regards,
Leif
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
> index 5b0b1a09f374..c994a2b69d78 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
> @@ -35,7 +35,8 @@ [LibraryClasses]
>
> [Guids]
> gFdtTableGuid
> - gRaspberryPiFdtFileGuid
> + gRaspberryPiFdtGuid1
> + gRaspberryPiFdtGuid2
>
> [Protocols]
> gRaspberryPiFirmwareProtocolGuid ## CONSUMES
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dec b/Platform/RaspberryPi/RPi3/RPi3.dec
> index 22de439fde8f..d90ea8329818 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dec
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dec
> @@ -24,7 +24,8 @@ [Protocols]
>
> [Guids]
> gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
> - gRaspberryPiFdtFileGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
> + gRaspberryPiFdtGuid1 = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
> + gRaspberryPiFdtGuid2 = {0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C}}
> gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
> gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
>
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf
> index c62d649834c7..83753d84badc 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.fdf
> +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
> @@ -300,11 +300,15 @@ [FV.FvMain]
> INF Platform/RaspberryPi/$(PLATFORM_NAME)/Drivers/LogoDxe/LogoDxe.inf
>
> #
> - # FDT (GUID matches gRaspberryPiFdtFileGuid in FdtDxe)
> + # Device Tree support
> + # GUIDs match gRaspberryPiFdtGuid# from FdtDxe.
> #
> FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC {
> SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb
> }
> + FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C {
> + SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb
> + }
>
> [FV.FVMAIN_COMPACT]
> FvAlignment = 16
> --
> 2.21.0.windows.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-platforms: PATCH 1/1] Platforms/RPi3: Add multiple embedded Device Tree selection
2019-08-15 12:13 ` Leif Lindholm
@ 2019-08-15 13:11 ` Pete Batard
0 siblings, 0 replies; 4+ messages in thread
From: Pete Batard @ 2019-08-15 13:11 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, ard.biesheuvel
Hi Leif,
On 2019.08.15 13:13, Leif Lindholm wrote:
> On Mon, Aug 12, 2019 at 02:06:34PM +0100, Pete Batard wrote:
>> The Raspberry Pi 3 platform currently has 2 different models, each with a
>> different Device Tree. Rather than embedding a single one, and requiring
>> users to manually provide the other, this patch ensures that we now embed
>> both and and serve the relevant one at runtime.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>
> Changeset looks very useful. Some style issues. And one question.
>
> First the question: is there no practical size restriction on the
> firmware image? This is a 24k increase in image size.
I thought about that too, but we still have plenty of unused payload to
go by, which is why I've been adding stuff like the EBC driver, and now
this, without worrying too much about the extra space taken.
For one, in the current 2 MB firmware image, we're still barely
scratching 1.2 MB of effective data (for RELEASE. For DEBUG that's ~1.5
MB), so we have plenty of space to add additional Device Trees, logos,
and similar non critical data. As a matter of fact, I was almost tempted
to have this patchset also include the Raspberry Pi 4 Device Tree, so
that we would just have all of the DT's we might ever be interested in
available at runtime, regardless of whether they are applicable or not
(as this would simplify FdtDxe reuse a well GUID handling).
Also, I would expect the build process to complain loudly if we go over
capacity.
Finally, I'm pretty sure we could relatively easily switch to a 4 MB or
8 MB firmware image, if we start to run out of space with the current 2
MB, since, from what I am aware of, the SoC is designed to be able to
boot large Linux kernels directly and doesn't enforce a hardcoded limit
on our FV size.
In other words, even if we start to regularly add a tens of KB here and
there, it's going to take some time before we have to start to worry
about going over capacity...
>> ---
>> Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 57 ++++++++++++++++----
>> Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf | 3 +-
>> Platform/RaspberryPi/RPi3/RPi3.dec | 3 +-
>> Platform/RaspberryPi/RPi3/RPi3.fdf | 6 ++-
>> 4 files changed, 55 insertions(+), 14 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
>> index c84e5b2767e2..7c0ab75e7cb1 100644
>> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
>> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
>> @@ -20,6 +20,11 @@
>>
>> #include <Guid/Fdt.h>
>>
>> +CONST EFI_GUID *mFdtGuid[] = {
>> + &gRaspberryPiFdtGuid1,
>> + &gRaspberryPiFdtGuid2,
>> + };
>> +
>> STATIC VOID *mFdtImage;
>>
>> STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
>> @@ -368,7 +373,9 @@ FdtDxeInitialize (
>> EFI_STATUS Status;
>> VOID *FdtImage;
>> UINTN FdtSize;
>> + UINTN FdtIndex;
>> INT32 Retval;
>> + UINT32 BoardRevision;
>> BOOLEAN Internal;
>>
>> Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL,
>> @@ -383,13 +390,41 @@ FdtDxeInitialize (
>> * Have FDT passed via config.txt.
>> */
>> FdtSize = fdt_totalsize (FdtImage);
>
> This
>> - DEBUG ((DEBUG_INFO, "DTB passed via config.txt of 0x%lx bytes\n", FdtSize));
>> + DEBUG ((DEBUG_INFO, "Device Tree passed via config.txt (0x%lx bytes)\n", FdtSize));
> looks unrelated to the changeset.
>
>> Status = EFI_SUCCESS;
>> } else {
>> + /*
>> + * Use one of the embedded FDT's.
>> + */
>> Internal = TRUE;
>
> This DEBUG changes bit
>> - DEBUG ((DEBUG_INFO, "No/bad FDT at %p (%a), trying internal DTB...\n",
>> - FdtImage, fdt_strerror (Retval)));
>> - Status = GetSectionFromAnyFv (&gRaspberryPiFdtFileGuid, EFI_SECTION_RAW, 0,
>> + DEBUG ((DEBUG_INFO, "No/Bad Device Tree found at address 0x%p (%a), "
>> + "looking up internal one...\n", FdtImage, fdt_strerror (Retval)));
> to here looks completely unrelated to the changeset.
> Which garbles the diff somewhat with regards to the functional line in
> the middle..
I mentioned in the cover letter that changes were also added to
harmonize the debug output to avoid acronyms and provide human readable
error codes. But you're right, at the very least, these changes should
have been mentioned in the commit message for actual patch itself, and
are probably better off on their own.
I'll break them out as a separate patch as requested.
Regards,
/Pete
>
>> + /*
>> + * Query the board revision to differentiate between models.
>> + */
>> + Status = mFwProtocol->GetModelRevision (&BoardRevision);
>> + if (EFI_ERROR (Status)) {
>> + FdtIndex = 0;
>> + DEBUG ((DEBUG_ERROR, "Failed to get board type: %r\n", Status));
>> + } else {
>> + // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
>> + switch ((BoardRevision >> 4) & 0xFF) {
>> + case 0x08: // Raspberry 3 Model B
>> + DEBUG ((DEBUG_INFO, "Using RPi3 Model B internal Device Tree\n"));
>> + FdtIndex = 0;
>> + break;
>> + case 0x0D: // Raspberry 3 Model B+
>> + DEBUG ((DEBUG_INFO, "Using RPi3 Model B+ internal Device Tree\n"));
>> + FdtIndex = 1;
>> + break;
>> + default:
>> + DEBUG ((DEBUG_INFO, "Using default internal Device Tree\n"));
>> + FdtIndex = 0;
>> + break;
>> + }
>> + }
>> + ASSERT (FdtIndex < ARRAY_SIZE (mFdtGuid));
>> + Status = GetSectionFromAnyFv (mFdtGuid[FdtIndex], EFI_SECTION_RAW, 0,
>> &FdtImage, &FdtSize);
>> if (Status == EFI_SUCCESS) {
>> if (fdt_check_header (FdtImage) != 0) {
>
> Everything from here...
>
>> @@ -419,27 +454,27 @@ FdtDxeInitialize (
>>
>> Status = SanitizePSCI ();
>> if (EFI_ERROR (Status)) {
>> - Print (L"Failed to sanitize PSCI (error %d)\n", Status);
>> + Print (L"Failed to sanitize PSCI: %r\n", Status);
>> }
>>
>> Status = CleanMemoryNodes ();
>> if (EFI_ERROR (Status)) {
>> - Print (L"Failed to clean memory nodes (error %d)\n", Status);
>> + Print (L"Failed to clean memory nodes: %r\n", Status);
>> }
>>
>> Status = CleanSimpleFramebuffer ();
>> if (EFI_ERROR (Status)) {
>> - Print (L"Failed to clean frame buffer (error %d)\n", Status);
>> + Print (L"Failed to clean frame buffer: %r\n", Status);
>> }
>>
>> Status = FixEthernetAliases ();
>> if (EFI_ERROR (Status)) {
>> - Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
>> + Print (L"Failed to fix ethernet aliases: %r\n", Status);
>> }
>>
>> Status = UpdateMacAddress ();
>> if (EFI_ERROR (Status)) {
>> - Print (L"Failed to update MAC address (error %d)\n", Status);
>> + Print (L"Failed to update MAC address: %r\n", Status);
>> }
>>
>> if (Internal) {
>> @@ -448,11 +483,11 @@ FdtDxeInitialize (
>> */
>> Status = UpdateBootArgs ();
>> if (EFI_ERROR (Status)) {
>> - Print (L"Failed to update boot arguments (error %d)\n", Status);
>> + Print (L"Failed to update boot arguments: %r\n", Status);
>> }
>> }
>>
>> - DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
>> + DEBUG ((DEBUG_INFO, "Installed Device Tree at address 0x%p\n", mFdtImage));
>> Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mFdtImage);
>> ASSERT_EFI_ERROR (Status);
>>
>
> ...to here is unrelated printout message changes.
>
> So, I don't object to the debug printout changes as such, but they
> obscure the functional changeset. Can you please break them out as a
> separate patch?
>
> Best Regards,
>
> Leif
>
>> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
>> index 5b0b1a09f374..c994a2b69d78 100644
>> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
>> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.inf
>> @@ -35,7 +35,8 @@ [LibraryClasses]
>>
>> [Guids]
>> gFdtTableGuid
>> - gRaspberryPiFdtFileGuid
>> + gRaspberryPiFdtGuid1
>> + gRaspberryPiFdtGuid2
>>
>> [Protocols]
>> gRaspberryPiFirmwareProtocolGuid ## CONSUMES
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dec b/Platform/RaspberryPi/RPi3/RPi3.dec
>> index 22de439fde8f..d90ea8329818 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.dec
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dec
>> @@ -24,7 +24,8 @@ [Protocols]
>>
>> [Guids]
>> gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
>> - gRaspberryPiFdtFileGuid = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
>> + gRaspberryPiFdtGuid1 = {0xDF5DA223, 0x1D27, 0x47C3, { 0x8D, 0x1B, 0x9A, 0x41, 0xB5, 0x5A, 0x18, 0xBC}}
>> + gRaspberryPiFdtGuid2 = {0x3D523012, 0x73FE, 0x40E5, { 0x89, 0x2E, 0x1A, 0x4D, 0xF6, 0x0F, 0x3C, 0x0C}}
>> gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
>> gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
>>
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.fdf b/Platform/RaspberryPi/RPi3/RPi3.fdf
>> index c62d649834c7..83753d84badc 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.fdf
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.fdf
>> @@ -300,11 +300,15 @@ [FV.FvMain]
>> INF Platform/RaspberryPi/$(PLATFORM_NAME)/Drivers/LogoDxe/LogoDxe.inf
>>
>> #
>> - # FDT (GUID matches gRaspberryPiFdtFileGuid in FdtDxe)
>> + # Device Tree support
>> + # GUIDs match gRaspberryPiFdtGuid# from FdtDxe.
>> #
>> FILE FREEFORM = DF5DA223-1D27-47C3-8D1B-9A41B55A18BC {
>> SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b.dtb
>> }
>> + FILE FREEFORM = 3D523012-73FE-40E5-892E-1A4DF60F3C0C {
>> + SECTION RAW = Platform/RaspberryPi/$(PLATFORM_NAME)/DeviceTree/bcm2710-rpi-3-b-plus.dtb
>> + }
>>
>> [FV.FVMAIN_COMPACT]
>> FvAlignment = 16
>> --
>> 2.21.0.windows.1
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-15 13:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-12 13:06 [edk2-platforms: PATCH 0/1] Platforms/RPi3: Add multiple embedded Device Tree selection Pete Batard
2019-08-12 13:06 ` [edk2-platforms: PATCH 1/1] " Pete Batard
2019-08-15 12:13 ` Leif Lindholm
2019-08-15 13:11 ` Pete Batard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox