* [edk2-platforms][PATCH 0/3] Platform/RPi: Fix compatibility with recent start.elf
@ 2020-05-04 11:15 Pete Batard
2020-05-04 11:15 ` [edk2-platforms][PATCH 1/3] Platform/RPi: Fortify mailbox code Pete Batard
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Pete Batard @ 2020-05-04 11:15 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif, awarkentin
As per https://github.com/raspberrypi/firmware/issues/1376 we found two
issues with recent versions of the Raspberry Pi Foundation's start.elf
on the Pi 3:
1. Our timeout delay for mbox reply was too short, leading to loss of
USB due to USB mbox power state request not being applied.
2. The MiniUART baudrate shifted from the expected value due to the
Raspberry Pi Foundation deviating from their official documentation
and increasing the default core clock (which the MiniUART bases its
baudrate divisor on) to 400 MHz, instead using a fixed 250 MHz,
when 'enable_uart=1' is in effect.
Fixing the first issue is fairly straightforward, and is done in patch
1/3, where we also take this opportunity to improve the overall mbox
code.
Fixing the second issue requires a little more involvement as we want
to ensure that we no longer depend on a fixed PCD clock rate to derive
the MiniUART baudrate divisor, but instead use the actual core clock
frequency retreived from mbox. However, because there basically exists
two instances of DualSerialLib in the firmware (one in PEI phase and
one in DXE) and the serial initialization is done very early, we have
to use the following process:
1. ArmPlatformPeiBootAction set the core clock of the PEI instance of
DualSerialLib, which enables the PEI serial instance to be set
with a proper baudrate.
2. A newly introduced PlatformPeiLib provides a PlatformPeim () call
that reads the global value from the PEI serial instance and
stores it into a GUID Hob.
3. In DXE phase, when the DXE instance of DualSerialLib attempts to
read the core clock value for the first time, it detects that it
has not been set and sets its value from the GUID Hob.
It needs to be pointed out however that we can't use HobLib helper
functions such as GetFirstGuidHob () or even GetHobList () directly
because DualSerialLib gets instantiated before the DXE version of
HobLib. However we work around that by using PrePeiGetHobList ().
This serial baudrate fixup is accomplished in patch 2/3. We also
take this opportunity to improve the Pi 4 code, which currently
doesn't use the core clock frequency at all, andt instead applies
a variable 12.12 fixed divisor (which we can always access without
mbox transactions) onto a 1 GHz fixed clock.
It should be noted that, with 2/3 applied, users of the Pi firmware
have the new capability of being able to override the core frequency
to whichever value they like (by adding 'core_freq=###' in their
'config.txt') without losing MiniUART baudrate, as was the case
without this patch.
Finally, considering that the knowing the actual core frequency
the system booted with can be useful during early init, we add
patch 3/3 to display this value for DEBUG builds.
Andrei Warkentin (2):
Platform/RPi: Fortify mailbox code
Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor
computation
Pete Batard (1):
Platform/RPi: Report core clock frequency during early init
Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 11 +-
Platform/RaspberryPi/Include/Guid/DualSerialPortLibHobGuid.h | 22 ++++
Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h | 11 +-
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 106 +++++++++++++++++---
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf | 6 ++
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 83 ++++++++-------
Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c | 8 +-
Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.c | 33 ++++++
Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.inf | 47 +++++++++
Platform/RaspberryPi/RPi3/RPi3.dsc | 8 +-
Platform/RaspberryPi/RPi4/RPi4.dsc | 9 +-
Platform/RaspberryPi/RaspberryPi.dec | 1 +
12 files changed, 278 insertions(+), 67 deletions(-)
create mode 100644 Platform/RaspberryPi/Include/Guid/DualSerialPortLibHobGuid.h
create mode 100644 Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.c
create mode 100644 Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.inf
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [edk2-platforms][PATCH 1/3] Platform/RPi: Fortify mailbox code
2020-05-04 11:15 [edk2-platforms][PATCH 0/3] Platform/RPi: Fix compatibility with recent start.elf Pete Batard
@ 2020-05-04 11:15 ` Pete Batard
2020-05-06 12:39 ` Ard Biesheuvel
2020-05-04 11:15 ` [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation Pete Batard
2020-05-04 11:15 ` [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init Pete Batard
2 siblings, 1 reply; 11+ messages in thread
From: Pete Batard @ 2020-05-04 11:15 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif, awarkentin, Andrei Warkentin
From: Andrei Warkentin <andrey.warkentin@gmail.com>
As part of the analysis done in:
https://github.com/raspberrypi/firmware/issues/1376:
* Bump up max tries, to avoid command time-outs.
* Macro-ify RaspberryPiHelper.S some more to make code more maintainable.
* Add ".align 4" before every command buffer.
Co-authored-by: Pete Batard <pete@akeo.ie>
Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 11 +---
Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h | 11 +++-
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 60 ++++++++------------
3 files changed, 37 insertions(+), 45 deletions(-)
diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index 40c78b5d57cf..6c45cf47f082 100644
--- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -1,5 +1,6 @@
/** @file
*
+ * Copyright (c) 2020, Pete Batard <pete@akeo.ie>
* Copyright (c) 2019, ARM Limited. All rights reserved.
* Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
* Copyright (c) 2016, Linaro, Ltd. All rights reserved.
@@ -30,12 +31,6 @@
//
#define NUM_PAGES 1
-//
-// The number of iterations to perform when waiting for the mailbox
-// status to change
-//
-#define MAX_TRIES 0x100000
-
STATIC VOID *mDmaBuffer;
STATIC VOID *mDmaBufferMapping;
STATIC UINTN mDmaBufferBusAddress;
@@ -62,7 +57,7 @@ DrainMailbox (
}
ArmDataSynchronizationBarrier ();
MmioRead32 (BCM2836_MBOX_BASE_ADDRESS + BCM2836_MBOX_READ_OFFSET);
- } while (++Tries < MAX_TRIES);
+ } while (++Tries < RPI_MBOX_MAX_TRIES);
return FALSE;
}
@@ -86,7 +81,7 @@ MailboxWaitForStatusCleared (
return TRUE;
}
ArmDataSynchronizationBarrier ();
- } while (++Tries < MAX_TRIES);
+ } while (++Tries < RPI_MBOX_MAX_TRIES);
return FALSE;
}
diff --git a/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h b/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
index 584786a61dfd..3328be582df1 100644
--- a/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
+++ b/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
@@ -1,6 +1,6 @@
/** @file
*
- * Copyright (c) 2019, Pete Batard <pete@akeo.ie>
+ * Copyright (c) 2019-2020, Pete Batard <pete@akeo.ie>
* Copyright (c) 2016, Linaro Limited. All rights reserved.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -10,6 +10,15 @@
#ifndef __RASPBERRY_PI_MAILBOX_H__
#define __RASPBERRY_PI_MAILBOX_H__
+/*
+ * Number of iterations to perform when waiting for the mailbox.
+ *
+ * This number was arrived at empirically, following discussion
+ * at https://github.com/raspberrypi/firmware/issues/1376, to
+ * avoid mailbox time-outs on some commands.
+ */
+#define RPI_MBOX_MAX_TRIES 0x8000000
+
/* Mailbox channels */
#define RPI_MBOX_PM_CHANNEL 0
#define RPI_MBOX_FB_CHANNEL 1
diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
index cc58406e1bfc..91dfe1bb981e 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
@@ -1,6 +1,7 @@
/** @file
*
- * Copyright (c) 2019, Pete Batard <pete@akeo.ie>
+ * Copyright (c) 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
+ * Copyright (c) 2019-2020, Pete Batard <pete@akeo.ie>
* Copyright (c) 2016, Linaro Limited. All rights reserved.
* Copyright (c) 2011-2013, ARM Limited. All rights reserved.
*
@@ -13,10 +14,8 @@
#include <IndustryStandard/Bcm2836.h>
#include <IndustryStandard/RpiMbox.h>
-#define MAX_TRIES 0x100000
-
.macro drain
- mov x5, #MAX_TRIES
+ mov x5, #RPI_MBOX_MAX_TRIES
0: ldr w6, [x4, #BCM2836_MBOX_STATUS_OFFSET]
tbnz w6, #BCM2836_MBOX_STATUS_EMPTY, 1f
dmb ld
@@ -27,7 +26,7 @@
.endm
.macro poll, status
- mov x5, #MAX_TRIES
+ mov x5, #RPI_MBOX_MAX_TRIES
0: ldr w6, [x4, #BCM2836_MBOX_STATUS_OFFSET]
tbz w6, #\status, 1f
dmb ld
@@ -36,23 +35,30 @@
1:
.endm
+ .macro run, command_buffer
+ adr x0, \command_buffer
+ orr x0, x0, #RPI_MBOX_VC_CHANNEL
+ add x0, x0, x1
+
+ poll BCM2836_MBOX_STATUS_FULL
+ str w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
+ dmb sy
+ poll BCM2836_MBOX_STATUS_EMPTY
+ dmb sy
+ ldr wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
+ dmb ld
+ .endm
+
ASM_FUNC (ArmPlatformPeiBootAction)
- adr x0, .Lmeminfo_buffer
mov x1, #FixedPcdGet64 (PcdDmaDeviceOffset)
orr x0, x0, #RPI_MBOX_VC_CHANNEL
// x1 holds the value of PcdDmaDeviceOffset throughout this function
- add x0, x0, x1
MOV32 (x4, BCM2836_MBOX_BASE_ADDRESS)
drain
- poll BCM2836_MBOX_STATUS_FULL
- str w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
- dmb sy
- poll BCM2836_MBOX_STATUS_EMPTY
- dmb sy
- ldr wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
- dmb ld
+
+ run .Lmeminfo_buffer
ldr w0, .Lmembase
adr x2, mSystemMemoryBase
@@ -63,17 +69,7 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mSystemMemoryEnd
str x0, [x2]
- adr x0, .Lvcinfo_buffer
- orr x0, x0, #RPI_MBOX_VC_CHANNEL
- add x0, x0, x1
-
- poll BCM2836_MBOX_STATUS_FULL
- str w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
- dmb sy
- poll BCM2836_MBOX_STATUS_EMPTY
- dmb sy
- ldr wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
- dmb ld
+ run .Lvcinfo_buffer
ldr w0, .Lvcbase
adr x2, mVideoCoreBase
@@ -83,17 +79,7 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mVideoCoreSize
str x0, [x2]
- adr x0, .Lrevinfo_buffer
- orr x0, x0, #RPI_MBOX_VC_CHANNEL
- add x0, x0, x1
-
- poll BCM2836_MBOX_STATUS_FULL
- str w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
- dmb sy
- poll BCM2836_MBOX_STATUS_EMPTY
- dmb sy
- ldr wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
- dmb ld
+ run .Lrevinfo_buffer
ldr w0, .Lrevision
adr x2, mBoardRevision
@@ -115,6 +101,7 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag
.set .Lmeminfo_size, . - .Lmeminfo_buffer
+ .align 4
.Lvcinfo_buffer:
.long .Lvcinfo_size
.long 0x0
@@ -128,6 +115,7 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag
.set .Lvcinfo_size, . - .Lvcinfo_buffer
+ .align 4
.Lrevinfo_buffer:
.long .Lrevinfo_size
.long 0x0
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation
2020-05-04 11:15 [edk2-platforms][PATCH 0/3] Platform/RPi: Fix compatibility with recent start.elf Pete Batard
2020-05-04 11:15 ` [edk2-platforms][PATCH 1/3] Platform/RPi: Fortify mailbox code Pete Batard
@ 2020-05-04 11:15 ` Pete Batard
2020-05-05 10:05 ` Ard Biesheuvel
2020-05-04 11:15 ` [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init Pete Batard
2 siblings, 1 reply; 11+ messages in thread
From: Pete Batard @ 2020-05-04 11:15 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif, awarkentin, Andrei Warkentin
From: Andrei Warkentin <andrey.warkentin@gmail.com>
Fix for https://github.com/raspberrypi/firmware/issues/1376.
For the Pi 3, to properly configure miniUART, we need the core clock,
which can be vary between VideoCore firmare release or config.txt options.
Unfortunately, it's painful to get - it's only available via the mailbox
interface. Additionally, SerialLib is a very limited environment, even
when linked in a DXE-mode component, because as part of a DebugLib
implementation it ends being the base prerequisite of everything.
That means a "normal" mailbox implementation like the one from
RpiFirmwareDxe (with dependencies on DmaLib) is out of the question.
Using a basic implementation such as the one in PlatformLib doesn't work
either because it operates in both environments with MMU on (DXE phase)
and MMU off (SEC/PrePi).
Ideally, we read the value via mbox exactly once at boot (PlatformLib),
and then somehow stash it. A GUID Hob sounds appropriate, yet when
SerialPortLib operates in DXE components, we can't use the HobLib to
*locate* the Hob list itself (remember, SerialPortLib initializes
before any of HobLib's dependencies, like UeflLib...).
FORTUNATELY, there is a way out - we can use PrePeiGetHobList to cut
through to the HOB list pointer stashed by PrePi without dealing with
any of the libraries meant for DXE.
Once we have the Hob list pointer, we can use the regular DXE HobLib
routines safely (so long as they are the ones taking a HobList pointer
as a parameter). So we can link DualSerialPortLib with whatever HobLib
that is appropriate for the environment - just need to also link in
PrePiHobListPointerLib.
gSerialLibCoreClockFreq is always externally set when DualSerialPortLib
run as part of PrePi. So - if gSerialLibCoreClockFreq is not set, then
we must be in the DXE phase and can fetch the Hob list via
PrePeiGetHobList. The GUID Hob is registered as part of a Pi-customized
PlatformPeiLib.
For the Pi 4, the situation is a bit different, as the system clock used
to compute the baud rate is not derived directly from the core clock, but
instead from a fixed clock (current 1 GHz) to which a 12.12 fixed point
divisor is applied. But even though we don't use it, we still initialize
gSerialLibCoreClockFreq a.k.a. the VPU core frequency, as this can be
useful to have.
As a result of the above, the PcdSerialClockRate values in the .dsc are
also updated, so that it can be used as a fallback to the default core
frequency in case of the Pi 3, and as the fixed frequency to which the
fixed point divisor is applied in case of the Pi 4.
Co-authored-by: Pete Batard <pete@akeo.ie>
Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Platform/RaspberryPi/Include/Guid/DualSerialPortLibHobGuid.h | 22 ++++
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 106 +++++++++++++++++---
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf | 6 ++
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 23 +++++
Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.c | 33 ++++++
Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.inf | 47 +++++++++
Platform/RaspberryPi/RPi3/RPi3.dsc | 8 +-
Platform/RaspberryPi/RPi4/RPi4.dsc | 9 +-
Platform/RaspberryPi/RaspberryPi.dec | 1 +
9 files changed, 234 insertions(+), 21 deletions(-)
diff --git a/Platform/RaspberryPi/Include/Guid/DualSerialPortLibHobGuid.h b/Platform/RaspberryPi/Include/Guid/DualSerialPortLibHobGuid.h
new file mode 100644
index 000000000000..82864eb33268
--- /dev/null
+++ b/Platform/RaspberryPi/Include/Guid/DualSerialPortLibHobGuid.h
@@ -0,0 +1,22 @@
+/** @file
+*
+* Copyright (c) 2020, Pete Batard <pete@akeo.ie>
+* Copyright (c) 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#ifndef _DUAL_SERIAL_PORT_LIB_HOB_GUID_H_
+#define _DUAL_SERIAL_PORT_LIB_HOB_GUID_H_
+
+#define DUAL_SERIAL_PORT_LIB_HOB_GUID \
+ { 0x1CB0EB5B, 0x4F73, 0x4308, { 0xA3, 0x33, 0x1D, 0x95, 0xBE, 0x5F, 0x30, 0xB5 } }
+
+extern GUID gDualSerialPortLibHobGuid;
+
+typedef struct {
+ UINT32 CoreClockFreq;
+} DUAL_SERIAL_PORT_LIB_VARS;
+
+#endif /* _DUAL_SERIAL_PORT_LIB_HOB_GUID_H_ */
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index 05e12f383785..77b5f586b6c5 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -1,6 +1,7 @@
/** @file
16550 and PL011 Serial Port library functions for Raspberry Pi
+ Copyright (c) 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
Copyright (c) 2020, Pete Batard <pete@akeo.ie>
Copyright (c) 2018, AMD Incorporated. All rights reserved.<BR>
Copyright (c) 2014, Hewlett-Packard Development Company, L.P.<BR>
@@ -12,7 +13,8 @@
**/
-#include <Base.h>
+#include <Uefi.h>
+#include <Guid/DualSerialPortLibHobGuid.h>
#include <IndustryStandard/Bcm2836.h>
#include <IndustryStandard/Bcm2836Gpio.h>
#include <Library/BaseLib.h>
@@ -21,9 +23,21 @@
#include <Library/PL011UartClockLib.h>
#include <Library/PL011UartLib.h>
#include <Library/SerialPortLib.h>
+//
+// The order of the following includes should not be altered
+//
+#include <Library/PrePiHobListPointerLib.h>
+#include <Pi/PiMultiPhase.h>
+#include <Library/HobLib.h>
-BOOLEAN UsePl011Uart = FALSE;
-BOOLEAN UsePl011UartSet = FALSE;
+STATIC BOOLEAN UsePl011Uart = FALSE;
+STATIC BOOLEAN UsePl011UartSet = FALSE;
+UINT32 gSerialLibCoreClockFreq = 0;
+//
+// Set by ArmPlatformPeiBootAction to skip PrePeiGetHobList,
+// as it won't be initialized and could return garbage.
+//
+UINT32 gSerialLibCoreSkipHob = 0;
#define PL011_UART_REGISTER_BASE BCM2836_PL011_UART_BASE_ADDRESS
#define MINI_UART_REGISTER_BASE (BCM2836_MINI_UART_BASE_ADDRESS + 0x40)
@@ -152,26 +166,86 @@ SerialPortGetDivisor (
UINT32 SerialBaudRate
)
{
- UINT64 BaseClockRate;
- UINT32 Divisor;
+ UINT64 BaseSerialClockFreq;
+ UINT32 Divisor;
+
+#if (RPI_MODEL == 3)
+ VOID *HobList;
+ EFI_HOB_GUID_TYPE *GuidHob;
+ DUAL_SERIAL_PORT_LIB_VARS *Vars;
+
+ //
+ // For the Pi 3, the base serial clock is the VPU core clock.
+ //
+ // Unfortunately, it's painful to get - it's only available via
+ // the mailbox interface. Additionally, SerialLib is a very
+ // limited environment, even when linked in a DXE-mode component, because
+ // as part of a DebugLib implementation it ends being the base prerequisite
+ // of /everything/. That means a "normal" mailbox implementation like
+ // the one im RpiFirmwareDxe (with dependencies on DmaLib) is out
+ // of the question. Using a basic implementation such as the one
+ // in PlatformLib doesn't work either because operate in both
+ // environments with MMU on (DXE phase) and MMU off (SEC/PrePi).
+ //
+ // Ideally, we read the value via mbox exactly once (PlatformLib),
+ // then somehow stash it. A GUID Hob sounds pretty nice, but
+ // we can't use the DXE HobLib to *locate* the Hob list itself
+ // (remember, SerialPortLib initializes before any of HobLib's
+ // dependencies, like UeflLib).
+ //
+ // FORTUNATELY, there is a way out - we can use PrePeiGetHobList
+ // to cut through to the HOB list pointer stashed by PrePi without.
+ // Once we have the Hob list pointer, we can use the DXE HobLib routines
+ // safely (so long as they are the ones taking a HobList pointer
+ // as a parameter).
+ //
+ // gSerialLibCoreClockFreq is always externally set when we run
+ // as part of PrePi. So - if gSerialLibCoreClockFreq is not set,
+ // then we must be in the DXE phase and can fetch the Hob list
+ // via our little hack.
+ //
+ if (gSerialLibCoreClockFreq == 0 && !gSerialLibCoreSkipHob) {
+ HobList = PrePeiGetHobList ();
+ if (HobList != NULL) {
+ GuidHob = (EFI_HOB_GUID_TYPE *) GetNextGuidHob (&gDualSerialPortLibHobGuid,
+ HobList);
+ if (GuidHob != NULL) {
+ Vars = (DUAL_SERIAL_PORT_LIB_VARS *) GET_GUID_HOB_DATA (GuidHob);
+ gSerialLibCoreClockFreq = Vars->CoreClockFreq;
+ gSerialLibCoreSkipHob = 1;
+ }
+ }
+ }
//
- // On the Raspberry Pi, the clock to use for the 16650-compatible UART
- // is the base clock divided by the 12.12 fixed point VPU clock divisor.
+ // Fall back to PCD if we have a nonsensical value.
+ //
+ if (gSerialLibCoreClockFreq < 10000000) {
+ gSerialLibCoreClockFreq = PcdGet32 (PcdSerialClockRate);
+ }
+ BaseSerialClockFreq = (UINT64) gSerialLibCoreClockFreq;
+#else
//
- BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate) * 4;
- Divisor = MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
- if (Divisor != 0)
- BaseClockRate = (BaseClockRate << 12) / Divisor;
+ // For the Pi 4, the base serial clock is not derived from the VPU
+ // core clock, but from a fixed clock (currently 1 GHz) to which a
+ // variable 12.12 fixed point clock divisor is applied.
+ //
+ BaseSerialClockFreq = (UINT64) PcdGet32 (PcdSerialClockRate);
+ Divisor = MmioRead32 (BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
+ if (Divisor != 0) {
+ BaseSerialClockFreq = (BaseSerialClockFreq << 12) / Divisor;
+ }
+#endif
//
- // Now calculate divisor for baud generator
- // Ref_Clk_Rate / Baud_Rate / 16
+ // As per the BCM2xxx datasheets:
+ // baudrate = system_clock_freq / (8 * (divisor + 1)).
//
- Divisor = (UINT32)BaseClockRate / (SerialBaudRate * 16);
- if (((UINT32)BaseClockRate % (SerialBaudRate * 16)) >= SerialBaudRate * 8) {
- Divisor++;
+ Divisor = (UINT32)BaseSerialClockFreq / (SerialBaudRate * 8);
+ if (Divisor != 0) {
+ Divisor--;
}
+
return Divisor;
}
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
index af1e6b026fe6..7430eede18db 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
@@ -22,17 +22,23 @@ [Packages]
EmbeddedPkg/EmbeddedPkg.dec
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
+ Platform/RaspberryPi/RaspberryPi.dec
Silicon/Broadcom/Bcm283x/Bcm283x.dec
[LibraryClasses]
IoLib
+ HobLib
PcdLib
PL011UartClockLib
PL011UartLib
+ PrePiHobListPointerLib
[Sources]
DualSerialPortLib.c
+[Guids]
+ gDualSerialPortLibHobGuid
+
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio ## CONSUMES
diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
index 91dfe1bb981e..83a3511d3507 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
@@ -85,6 +85,16 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mBoardRevision
str w0, [x2]
+ run .Lclkinfo_buffer
+
+ ldr w0, .Lfrequency
+ adr x2, gSerialLibCoreClockFreq
+ str w0, [x2]
+ adr x2, gSerialLibCoreSkipHob
+ //
+ // Any non-zero value will do.
+ //
+ str w0, [x2]
ret
.align 4
@@ -127,6 +137,19 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag
.set .Lrevinfo_size, . - .Lrevinfo_buffer
+ .align 4
+.Lclkinfo_buffer:
+ .long .Lclkinfo_size
+ .long 0x0
+ .long RPI_MBOX_GET_CLOCK_RATE
+ .long 8 // buf size
+ .long 4 // input len
+ .long 4 // clock id: 0x04 = Core/VPU
+.Lfrequency:
+ .long 0 // frequency
+ .long 0 // end tag
+ .set .Lclkinfo_size, . - .Lclkinfo_buffer
+
//UINTN
//ArmPlatformGetPrimaryCoreMpId (
// VOID
diff --git a/Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.c b/Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.c
new file mode 100644
index 000000000000..367a32bd4ca7
--- /dev/null
+++ b/Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.c
@@ -0,0 +1,33 @@
+/** @file
+*
+* Copyright (c) 2011-2014, ARM Limited. All rights reserved.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <PiPei.h>
+
+#include <Library/ArmPlatformLib.h>
+#include <Library/HobLib.h>
+#include <Library/PcdLib.h>
+#include <Library/DebugLib.h>
+#include <Guid/DualSerialPortLibHobGuid.h>
+
+extern UINT32 gSerialLibCoreClockFreq, gSerialLibCoreSkipHob;
+
+static DUAL_SERIAL_PORT_LIB_VARS mUartLibVars;
+
+EFI_STATUS
+EFIAPI
+PlatformPeim (
+ VOID
+ )
+{
+ BuildFvHob (PcdGet64 (PcdFvBaseAddress), PcdGet32 (PcdFvSize));
+
+ mUartLibVars.CoreClockFreq = gSerialLibCoreClockFreq;
+ BuildGuidDataHob (&gDualSerialPortLibHobGuid, &mUartLibVars, sizeof(mUartLibVars));
+
+ return EFI_SUCCESS;
+}
diff --git a/Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.inf b/Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.inf
new file mode 100644
index 000000000000..8e2bab14a89a
--- /dev/null
+++ b/Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.inf
@@ -0,0 +1,47 @@
+#/** @file
+#
+# Copyright (c) 2011-2012, ARM Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#**/
+
+[Defines]
+ INF_VERSION = 0x0001001A
+ BASE_NAME = ArmPlatformPeiLib
+ FILE_GUID = 49d37060-70b5-11e0-aa2d-0002a5d5c51b
+ MODULE_TYPE = SEC
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = PlatformPeiLib
+
+[Sources]
+ PlatformPeiLib.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ ArmPlatformPkg/ArmPlatformPkg.dec
+ MdePkg/MdePkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ Platform/RaspberryPi/RaspberryPi.dec
+
+[LibraryClasses]
+ ArmPlatformLib
+ DebugLib
+ HobLib
+
+[Guids]
+ gDualSerialPortLibHobGuid
+
+[Ppis]
+ gEfiPeiMasterBootModePpiGuid # PPI ALWAYS_PRODUCED
+ gEfiPeiBootInRecoveryModePpiGuid # PPI SOMETIMES_PRODUCED
+
+[FixedPcd]
+ gArmTokenSpaceGuid.PcdFdBaseAddress
+ gArmTokenSpaceGuid.PcdFdSize
+ gArmTokenSpaceGuid.PcdFvBaseAddress
+ gArmTokenSpaceGuid.PcdFvSize
+
+[Depex]
+ TRUE
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index 563fb891b841..efebf1f823b3 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -77,6 +77,10 @@ [LibraryClasses.common]
UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
+ #
+ # PrePiHobListPointerLib is necessary for DualSerialPortLib.
+ #
+ PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
@@ -174,7 +178,7 @@ [LibraryClasses.common.SEC]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
MemoryInitPeiLib|Platform/RaspberryPi/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
- PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+ PlatformPeiLib|Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.inf
ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.inf
LzmaDecompressLib|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
@@ -409,7 +413,7 @@ [PcdsFixedAtBuild.common]
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|250000000
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 4deccd9d3ecc..4dc9b493cf79 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -75,6 +75,10 @@ [LibraryClasses.common]
UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
+ #
+ # PrePiHobListPointerLib is necessary for DualSerialPortLib.
+ #
+ PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
@@ -182,12 +186,11 @@ [LibraryClasses.common.SEC]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
MemoryInitPeiLib|Platform/RaspberryPi/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf
- PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+ PlatformPeiLib|Platform/RaspberryPi/Library/PlatformPeiLib/PlatformPeiLib.inf
ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.inf
LzmaDecompressLib|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
HobLib|EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
- PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
[LibraryClasses.common.DXE_CORE]
@@ -420,7 +423,7 @@ [PcdsFixedAtBuild.common]
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1000000000
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
index 66ef6186644b..a915387fc822 100644
--- a/Platform/RaspberryPi/RaspberryPi.dec
+++ b/Platform/RaspberryPi/RaspberryPi.dec
@@ -26,6 +26,7 @@ [Guids]
gRaspberryPiTokenSpaceGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
gRaspberryPiEventResetGuid = {0xCD7CC258, 0x31DB, 0x11E6, {0x9F, 0xD3, 0x63, 0xB4, 0xB4, 0xE4, 0xD4, 0xB4}}
gConfigDxeFormSetGuid = {0xCD7CC258, 0x31DB, 0x22E6, {0x9F, 0x22, 0x63, 0xB0, 0xB8, 0xEE, 0xD6, 0xB5}}
+ gDualSerialPortLibHobGuid = { 0x1CB0EB5B, 0x4F73, 0x4308, { 0xA3, 0x33, 0x1D, 0x95, 0xBE, 0x5F, 0x30, 0xB5 } }
[PcdsFixedAtBuild.common]
#
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init
2020-05-04 11:15 [edk2-platforms][PATCH 0/3] Platform/RPi: Fix compatibility with recent start.elf Pete Batard
2020-05-04 11:15 ` [edk2-platforms][PATCH 1/3] Platform/RPi: Fortify mailbox code Pete Batard
2020-05-04 11:15 ` [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation Pete Batard
@ 2020-05-04 11:15 ` Pete Batard
2020-05-04 11:37 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-05-05 6:04 ` Andrei Warkentin
2 siblings, 2 replies; 11+ messages in thread
From: Pete Batard @ 2020-05-04 11:15 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif, awarkentin
The previous commit ensures that gSerialLibCoreClockFreq contains the VPU
core frequency by the time we execute ArmPlatformGetVirtualMemoryMap ().
This value can prove useful for troubleshooting, so report it.
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
index aae189ec8136..015d3dccc27c 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
+++ b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
@@ -22,7 +22,12 @@ extern UINT64 mSystemMemoryEnd;
UINT64 mVideoCoreBase;
UINT64 mVideoCoreSize;
UINT32 mBoardRevision;
-
+//
+// gSerialLibCoreClockFreq, which resides in DualSerialLib is set
+// to the VPU Core Clock frequency by ArmPlatformPeiBootAction ().
+// We use it to report the core frequency during early boot.
+//
+extern UINT32 gSerialLibCoreClockFreq;
// The total number of descriptors, including the final "end-of-table" descriptor.
#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 11
@@ -66,6 +71,7 @@ ArmPlatformGetVirtualMemoryMap (
DEBUG ((DEBUG_INFO, "Board Rev: 0x%lX\n", mBoardRevision));
DEBUG ((DEBUG_INFO, "Base RAM : 0x%ll08X (Size 0x%ll08X)\n", mSystemMemoryBase, mSystemMemoryEnd + 1));
DEBUG ((DEBUG_INFO, "VideoCore: 0x%ll08X (Size 0x%ll08X)\n", mVideoCoreBase, mVideoCoreSize));
+ DEBUG ((DEBUG_INFO, "Core Freq: %d MHz\n", gSerialLibCoreClockFreq / 1000000));
ASSERT (mSystemMemoryBase == 0);
ASSERT (VirtualMemoryMap != NULL);
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init
2020-05-04 11:15 ` [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init Pete Batard
@ 2020-05-04 11:37 ` Philippe Mathieu-Daudé
2020-05-05 6:04 ` Andrei Warkentin
1 sibling, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-04 11:37 UTC (permalink / raw)
To: devel, pete; +Cc: ard.biesheuvel, leif, awarkentin
On 5/4/20 1:15 PM, Pete Batard wrote:
> The previous commit ensures that gSerialLibCoreClockFreq contains the VPU
> core frequency by the time we execute ArmPlatformGetVirtualMemoryMap ().
>
> This value can prove useful for troubleshooting, so report it.
>
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
> Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> index aae189ec8136..015d3dccc27c 100644
> --- a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> +++ b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> @@ -22,7 +22,12 @@ extern UINT64 mSystemMemoryEnd;
> UINT64 mVideoCoreBase;
> UINT64 mVideoCoreSize;
> UINT32 mBoardRevision;
> -
> +//
> +// gSerialLibCoreClockFreq, which resides in DualSerialLib is set
> +// to the VPU Core Clock frequency by ArmPlatformPeiBootAction ().
> +// We use it to report the core frequency during early boot.
> +//
> +extern UINT32 gSerialLibCoreClockFreq;
>
> // The total number of descriptors, including the final "end-of-table" descriptor.
> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 11
> @@ -66,6 +71,7 @@ ArmPlatformGetVirtualMemoryMap (
> DEBUG ((DEBUG_INFO, "Board Rev: 0x%lX\n", mBoardRevision));
> DEBUG ((DEBUG_INFO, "Base RAM : 0x%ll08X (Size 0x%ll08X)\n", mSystemMemoryBase, mSystemMemoryEnd + 1));
> DEBUG ((DEBUG_INFO, "VideoCore: 0x%ll08X (Size 0x%ll08X)\n", mVideoCoreBase, mVideoCoreSize));
> + DEBUG ((DEBUG_INFO, "Core Freq: %d MHz\n", gSerialLibCoreClockFreq / 1000000));
>
> ASSERT (mSystemMemoryBase == 0);
> ASSERT (VirtualMemoryMap != NULL);
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init
2020-05-04 11:15 ` [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init Pete Batard
2020-05-04 11:37 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2020-05-05 6:04 ` Andrei Warkentin
1 sibling, 0 replies; 11+ messages in thread
From: Andrei Warkentin @ 2020-05-05 6:04 UTC (permalink / raw)
To: Pete Batard, devel@edk2.groups.io
Cc: ard.biesheuvel@arm.com, leif@nuviainc.com
[-- Attachment #1: Type: text/plain, Size: 2170 bytes --]
Reviewed-by: Andrei Warkentin <andrey.warkentin@gmail.com>
A
________________________________
From: Pete Batard <pete@akeo.ie>
Sent: Monday, May 4, 2020 6:15 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: ard.biesheuvel@arm.com <ard.biesheuvel@arm.com>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>
Subject: [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init
The previous commit ensures that gSerialLibCoreClockFreq contains the VPU
core frequency by the time we execute ArmPlatformGetVirtualMemoryMap ().
This value can prove useful for troubleshooting, so report it.
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
index aae189ec8136..015d3dccc27c 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
+++ b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
@@ -22,7 +22,12 @@ extern UINT64 mSystemMemoryEnd;
UINT64 mVideoCoreBase;
UINT64 mVideoCoreSize;
UINT32 mBoardRevision;
-
+//
+// gSerialLibCoreClockFreq, which resides in DualSerialLib is set
+// to the VPU Core Clock frequency by ArmPlatformPeiBootAction ().
+// We use it to report the core frequency during early boot.
+//
+extern UINT32 gSerialLibCoreClockFreq;
// The total number of descriptors, including the final "end-of-table" descriptor.
#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 11
@@ -66,6 +71,7 @@ ArmPlatformGetVirtualMemoryMap (
DEBUG ((DEBUG_INFO, "Board Rev: 0x%lX\n", mBoardRevision));
DEBUG ((DEBUG_INFO, "Base RAM : 0x%ll08X (Size 0x%ll08X)\n", mSystemMemoryBase, mSystemMemoryEnd + 1));
DEBUG ((DEBUG_INFO, "VideoCore: 0x%ll08X (Size 0x%ll08X)\n", mVideoCoreBase, mVideoCoreSize));
+ DEBUG ((DEBUG_INFO, "Core Freq: %d MHz\n", gSerialLibCoreClockFreq / 1000000));
ASSERT (mSystemMemoryBase == 0);
ASSERT (VirtualMemoryMap != NULL);
--
2.21.0.windows.1
[-- Attachment #2: Type: text/html, Size: 4464 bytes --]
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation
2020-05-04 11:15 ` [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation Pete Batard
@ 2020-05-05 10:05 ` Ard Biesheuvel
2020-05-05 11:54 ` Pete Batard
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 10:05 UTC (permalink / raw)
To: Pete Batard, devel; +Cc: leif, awarkentin, Andrei Warkentin
On 5/4/20 1:15 PM, Pete Batard wrote:
> From: Andrei Warkentin <andrey.warkentin@gmail.com>
>
> Fix for https://github.com/raspberrypi/firmware/issues/1376.
>
> For the Pi 3, to properly configure miniUART, we need the core clock,
> which can be vary between VideoCore firmare release or config.txt options.
>
> Unfortunately, it's painful to get - it's only available via the mailbox
> interface. Additionally, SerialLib is a very limited environment, even
> when linked in a DXE-mode component, because as part of a DebugLib
> implementation it ends being the base prerequisite of everything.
> That means a "normal" mailbox implementation like the one from
> RpiFirmwareDxe (with dependencies on DmaLib) is out of the question.
> Using a basic implementation such as the one in PlatformLib doesn't work
> either because it operates in both environments with MMU on (DXE phase)
> and MMU off (SEC/PrePi).
>
> Ideally, we read the value via mbox exactly once at boot (PlatformLib),
> and then somehow stash it. A GUID Hob sounds appropriate, yet when
> SerialPortLib operates in DXE components, we can't use the HobLib to
> *locate* the Hob list itself (remember, SerialPortLib initializes
> before any of HobLib's dependencies, like UeflLib...).
>
Yeah, the gift that keeps on giving :-) NXP are struggling with a
similar issue.
So the problem is that SerialPortInitialize() is called before we know
what value to program, and we cannot rely on other libraries to discover
this value because they may not work before their constructor has been
called.
So can we break the contents of SerialPortInitialize() into things that
need to happen once (program the divisors etc) and things that need to
happen each and every time (figure out which UART we are using in the
first place)? If the second set does not suffer from the same issue, can
we just move the entire logic that programs the UART block into PrePi,
so that all subsequent users don't have to touch those registers at all?
This means we may need two versions of DualSerialPortLib, where the one
that PrePi uses may need to be attached to SerialDxe as well, so that it
can reprogram the baud rate as needed. But for all the remaining
DebugLib related uses, we don't really need to reprogram the UART at all
afaics.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation
2020-05-05 10:05 ` Ard Biesheuvel
@ 2020-05-05 11:54 ` Pete Batard
2020-05-05 12:05 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: Pete Batard @ 2020-05-05 11:54 UTC (permalink / raw)
To: Ard Biesheuvel, devel; +Cc: leif, awarkentin, Andrei Warkentin
Hi Ard,
On 2020.05.05 11:05, Ard Biesheuvel wrote:
> On 5/4/20 1:15 PM, Pete Batard wrote:
>> From: Andrei Warkentin <andrey.warkentin@gmail.com>
>>
>> Fix for https://github.com/raspberrypi/firmware/issues/1376.
>>
>> For the Pi 3, to properly configure miniUART, we need the core clock,
>> which can be vary between VideoCore firmare release or config.txt
>> options.
>>
>> Unfortunately, it's painful to get - it's only available via the mailbox
>> interface. Additionally, SerialLib is a very limited environment, even
>> when linked in a DXE-mode component, because as part of a DebugLib
>> implementation it ends being the base prerequisite of everything.
>> That means a "normal" mailbox implementation like the one from
>> RpiFirmwareDxe (with dependencies on DmaLib) is out of the question.
>> Using a basic implementation such as the one in PlatformLib doesn't work
>> either because it operates in both environments with MMU on (DXE phase)
>> and MMU off (SEC/PrePi).
>>
>> Ideally, we read the value via mbox exactly once at boot (PlatformLib),
>> and then somehow stash it. A GUID Hob sounds appropriate, yet when
>> SerialPortLib operates in DXE components, we can't use the HobLib to
>> *locate* the Hob list itself (remember, SerialPortLib initializes
>> before any of HobLib's dependencies, like UeflLib...).
>>
>
> Yeah, the gift that keeps on giving :-) NXP are struggling with a
> similar issue.
>
> So the problem is that SerialPortInitialize() is called before we know
> what value to program, and we cannot rely on other libraries to discover
> this value because they may not work before their constructor has been
> called.
I wouldn't qualify the problem that way.
The problem is that we have everything we need to program the UART by
the time we get into SerialPortInitialize(), because
ArmPlatformPeiBootAction() has long retrieved that one variable/value we
need, which everything downstream relies on, but sharing that one
variable properly, so that it available in both PEI and DXE by the time
it is needed, is a headache.
For all intent and purposes, we always have the value we need, because
that's pretty much the very first thing we query. On the other hand,
directing that value to the places that consume it is problematic
because most of the mechanisms we have to do that have some kind of
reliance that serial output would already have been initialized...
So we have been treating it as mere problem of sharing a global variable
between PEI and DXE and nothing else, hence the proposal. As a matter of
fact, we started with an alternate solution where we just added an extra
page on top of the NvStorage reserved region, and stored the variable
there at an address that DualSerialPortLib could easily retrieve in
either PEI or DXE mode (with no need for the extra PlatformPeiLib then).
Hob is just the natural progression of that, to achieve it in a more
EDK2-like way.
> So can we break the contents of SerialPortInitialize() into things that
> need to happen once (program the divisors etc) and things that need to > happen each and every time (figure out which UART we are using in the
> first place)?
I'm trying to understand what you're getting at here.
By once and multiple time, I am assuming that you're referring to the
various instantiations of the library, because I only expect
SerialPortInitialize() to be called once per instance for DebugLib
related uses, so technically, everything from SerialPortInitialize()
could be considered a one-time operation.
So I guess what you're suggesting is that we somehow drop setting up the
baudrate in DXE phase and just assume that it has already been set from PEI.
> If the second set does not suffer from the same issue, can
> we just move the entire logic that programs the UART block into PrePi,
> so that all subsequent users don't have to touch those registers at all?
Well, someone can and will want to switch baudrate from Shell, which
means we need to compute a dynamic divisor from the base serial clock
frequency. So the only way I can see your idea work is if we re-query
the mbox to obtain the base serial clock frequency in
SerialPortSetAttributes(), but that means that, for the compiler to be
happy, it will need to set up a DXE dependency with RpiFirmwareDxe,
which we can't have in PEI phase...
> This means we may need two versions of DualSerialPortLib,
Unless there exists a compile time macros that indicate whether code is
compiling in PEI or DXE phase (and even then I suspect that .inf
sections will not cooperate that easily), then I don't see how we can
avoid having two separate versions of DualSerialPortLib indeed, and I
see that as becoming more of a headache than what we're proposing here...
> where the one
> that PrePi uses may need to be attached to SerialDxe as well, so that it
> can reprogram the baud rate as needed. But for all the remaining
> DebugLib related uses, we don't really need to reprogram the UART at all
> afaics.
From a design standpoint, this may look fine, but from an
implementation standpoint, when, again, the one problem we are really
trying to solve is the sharing of a global variable, I fear that we are
going to shoot ourselves in the foot if we try to go in that direction,
because duplication of code, when it can be avoided, doesn't strike me
like a good idea.
If we are going to push for something like that, I'd much rather work on
another EDK2 library that solves the problem of early global variable
sharing between DXE and PEI, than something that's custom to RPi and
that's not going to help anybody else in the process.
However, considering the time that has already spent trying to solve
this issue, I'd rather not have to do either of these, really, because I
don't think there is much to gain from adding bells and whistle to a
problem that really boils down to "We need to share a global variable,
that we set in early PEI, between PEI and DXE" and which we have solved
using what I believe to be the mandated EDK2 way of doing it (HOBs).
Now, if you are *really* that opposed to the current solution, I can see
what's achievable. But I'd rather only have to do that on a major
objection ("This proposal may create problems in the long run because X,
Y...") rather than preference of how things may look be better organized
if we did it in this other fashion...
Besides the need to add an extra PlatformPeiLib and the small hack we
had to use to get to the Hob in early DXE phase, do you have specific
concerns about the current proposal that you identify as reasonable
ground to want to push it back?
Regards,
/Pete
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation
2020-05-05 11:54 ` Pete Batard
@ 2020-05-05 12:05 ` Ard Biesheuvel
2020-05-05 12:09 ` Pete Batard
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 12:05 UTC (permalink / raw)
To: Pete Batard, devel; +Cc: leif, awarkentin, Andrei Warkentin
On 5/5/20 1:54 PM, Pete Batard wrote:
> Hi Ard,
>
> On 2020.05.05 11:05, Ard Biesheuvel wrote:
>> On 5/4/20 1:15 PM, Pete Batard wrote:
>>> From: Andrei Warkentin <andrey.warkentin@gmail.com>
>>>
>>> Fix for https://github.com/raspberrypi/firmware/issues/1376.
>>>
>>> For the Pi 3, to properly configure miniUART, we need the core clock,
>>> which can be vary between VideoCore firmare release or config.txt
>>> options.
>>>
>>> Unfortunately, it's painful to get - it's only available via the mailbox
>>> interface. Additionally, SerialLib is a very limited environment, even
>>> when linked in a DXE-mode component, because as part of a DebugLib
>>> implementation it ends being the base prerequisite of everything.
>>> That means a "normal" mailbox implementation like the one from
>>> RpiFirmwareDxe (with dependencies on DmaLib) is out of the question.
>>> Using a basic implementation such as the one in PlatformLib doesn't work
>>> either because it operates in both environments with MMU on (DXE phase)
>>> and MMU off (SEC/PrePi).
>>>
>>> Ideally, we read the value via mbox exactly once at boot (PlatformLib),
>>> and then somehow stash it. A GUID Hob sounds appropriate, yet when
>>> SerialPortLib operates in DXE components, we can't use the HobLib to
>>> *locate* the Hob list itself (remember, SerialPortLib initializes
>>> before any of HobLib's dependencies, like UeflLib...).
>>>
>>
>> Yeah, the gift that keeps on giving :-) NXP are struggling with a
>> similar issue.
>>
>> So the problem is that SerialPortInitialize() is called before we know
>> what value to program, and we cannot rely on other libraries to
>> discover this value because they may not work before their constructor
>> has been called.
>
> I wouldn't qualify the problem that way.
>
> The problem is that we have everything we need to program the UART by
> the time we get into SerialPortInitialize(), because
> ArmPlatformPeiBootAction() has long retrieved that one variable/value we
> need, which everything downstream relies on, but sharing that one
> variable properly, so that it available in both PEI and DXE by the time
> it is needed, is a headache.
>
> For all intent and purposes, we always have the value we need, because
> that's pretty much the very first thing we query. On the other hand,
> directing that value to the places that consume it is problematic
> because most of the mechanisms we have to do that have some kind of
> reliance that serial output would already have been initialized...
>
> So we have been treating it as mere problem of sharing a global variable
> between PEI and DXE and nothing else, hence the proposal. As a matter of
> fact, we started with an alternate solution where we just added an extra
> page on top of the NvStorage reserved region, and stored the variable
> there at an address that DualSerialPortLib could easily retrieve in
> either PEI or DXE mode (with no need for the extra PlatformPeiLib then).
> Hob is just the natural progression of that, to achieve it in a more
> EDK2-like way.
>
>> So can we break the contents of SerialPortInitialize() into things
>> that need to happen once (program the divisors etc) and things that
>> need to > happen each and every time (figure out which UART we are
>> using in the
>> first place)?
>
> I'm trying to understand what you're getting at here.
>
> By once and multiple time, I am assuming that you're referring to the
> various instantiations of the library, because I only expect
> SerialPortInitialize() to be called once per instance for DebugLib
> related uses, so technically, everything from SerialPortInitialize()
> could be considered a one-time operation.
>
> So I guess what you're suggesting is that we somehow drop setting up the
> baudrate in DXE phase and just assume that it has already been set from
> PEI.
>
>> If the second set does not suffer from the same issue, can we just
>> move the entire logic that programs the UART block into PrePi, so that
>> all subsequent users don't have to touch those registers at all?
>
> Well, someone can and will want to switch baudrate from Shell, which
> means we need to compute a dynamic divisor from the base serial clock
> frequency. So the only way I can see your idea work is if we re-query
> the mbox to obtain the base serial clock frequency in
> SerialPortSetAttributes(), but that means that, for the compiler to be
> happy, it will need to set up a DXE dependency with RpiFirmwareDxe,
> which we can't have in PEI phase...
>
>> This means we may need two versions of DualSerialPortLib,
>
> Unless there exists a compile time macros that indicate whether code is
> compiling in PEI or DXE phase (and even then I suspect that .inf
> sections will not cooperate that easily), then I don't see how we can
> avoid having two separate versions of DualSerialPortLib indeed, and I
> see that as becoming more of a headache than what we're proposing here...
>
>> where the one that PrePi uses may need to be attached to SerialDxe as
>> well, so that it can reprogram the baud rate as needed. But for all
>> the remaining DebugLib related uses, we don't really need to reprogram
>> the UART at all afaics.
>
> From a design standpoint, this may look fine, but from an
> implementation standpoint, when, again, the one problem we are really
> trying to solve is the sharing of a global variable, I fear that we are
> going to shoot ourselves in the foot if we try to go in that direction,
> because duplication of code, when it can be avoided, doesn't strike me
> like a good idea.
>
> If we are going to push for something like that, I'd much rather work on
> another EDK2 library that solves the problem of early global variable
> sharing between DXE and PEI, than something that's custom to RPi and
> that's not going to help anybody else in the process.
>
> However, considering the time that has already spent trying to solve
> this issue, I'd rather not have to do either of these, really, because I
> don't think there is much to gain from adding bells and whistle to a
> problem that really boils down to "We need to share a global variable,
> that we set in early PEI, between PEI and DXE" and which we have solved
> using what I believe to be the mandated EDK2 way of doing it (HOBs).
>
> Now, if you are *really* that opposed to the current solution, I can see
> what's achievable. But I'd rather only have to do that on a major
> objection ("This proposal may create problems in the long run because X,
> Y...") rather than preference of how things may look be better organized
> if we did it in this other fashion...
>
> Besides the need to add an extra PlatformPeiLib and the small hack we
> had to use to get to the Hob in early DXE phase, do you have specific
> concerns about the current proposal that you identify as reasonable
> ground to want to push it back?
>
I'll have a stab at coding it up myself.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation
2020-05-05 12:05 ` Ard Biesheuvel
@ 2020-05-05 12:09 ` Pete Batard
0 siblings, 0 replies; 11+ messages in thread
From: Pete Batard @ 2020-05-05 12:09 UTC (permalink / raw)
To: Ard Biesheuvel, devel; +Cc: leif, awarkentin, Andrei Warkentin
On 2020.05.05 13:05, Ard Biesheuvel wrote:
> On 5/5/20 1:54 PM, Pete Batard wrote:
>> Hi Ard,
>>
>> On 2020.05.05 11:05, Ard Biesheuvel wrote:
>>> On 5/4/20 1:15 PM, Pete Batard wrote:
>>>> From: Andrei Warkentin <andrey.warkentin@gmail.com>
>>>>
>>>> Fix for https://github.com/raspberrypi/firmware/issues/1376.
>>>>
>>>> For the Pi 3, to properly configure miniUART, we need the core clock,
>>>> which can be vary between VideoCore firmare release or config.txt
>>>> options.
>>>>
>>>> Unfortunately, it's painful to get - it's only available via the
>>>> mailbox
>>>> interface. Additionally, SerialLib is a very limited environment, even
>>>> when linked in a DXE-mode component, because as part of a DebugLib
>>>> implementation it ends being the base prerequisite of everything.
>>>> That means a "normal" mailbox implementation like the one from
>>>> RpiFirmwareDxe (with dependencies on DmaLib) is out of the question.
>>>> Using a basic implementation such as the one in PlatformLib doesn't
>>>> work
>>>> either because it operates in both environments with MMU on (DXE phase)
>>>> and MMU off (SEC/PrePi).
>>>>
>>>> Ideally, we read the value via mbox exactly once at boot (PlatformLib),
>>>> and then somehow stash it. A GUID Hob sounds appropriate, yet when
>>>> SerialPortLib operates in DXE components, we can't use the HobLib to
>>>> *locate* the Hob list itself (remember, SerialPortLib initializes
>>>> before any of HobLib's dependencies, like UeflLib...).
>>>>
>>>
>>> Yeah, the gift that keeps on giving :-) NXP are struggling with a
>>> similar issue.
>>>
>>> So the problem is that SerialPortInitialize() is called before we
>>> know what value to program, and we cannot rely on other libraries to
>>> discover this value because they may not work before their
>>> constructor has been called.
>>
>> I wouldn't qualify the problem that way.
>>
>> The problem is that we have everything we need to program the UART by
>> the time we get into SerialPortInitialize(), because
>> ArmPlatformPeiBootAction() has long retrieved that one variable/value
>> we need, which everything downstream relies on, but sharing that one
>> variable properly, so that it available in both PEI and DXE by the
>> time it is needed, is a headache.
>>
>> For all intent and purposes, we always have the value we need, because
>> that's pretty much the very first thing we query. On the other hand,
>> directing that value to the places that consume it is problematic
>> because most of the mechanisms we have to do that have some kind of
>> reliance that serial output would already have been initialized...
>>
>> So we have been treating it as mere problem of sharing a global
>> variable between PEI and DXE and nothing else, hence the proposal. As
>> a matter of fact, we started with an alternate solution where we just
>> added an extra page on top of the NvStorage reserved region, and
>> stored the variable there at an address that DualSerialPortLib could
>> easily retrieve in either PEI or DXE mode (with no need for the extra
>> PlatformPeiLib then). Hob is just the natural progression of that, to
>> achieve it in a more EDK2-like way.
>>
>>> So can we break the contents of SerialPortInitialize() into things
>>> that need to happen once (program the divisors etc) and things that
>>> need to > happen each and every time (figure out which UART we are
>>> using in the
>>> first place)?
>>
>> I'm trying to understand what you're getting at here.
>>
>> By once and multiple time, I am assuming that you're referring to the
>> various instantiations of the library, because I only expect
>> SerialPortInitialize() to be called once per instance for DebugLib
>> related uses, so technically, everything from SerialPortInitialize()
>> could be considered a one-time operation.
>>
>> So I guess what you're suggesting is that we somehow drop setting up
>> the baudrate in DXE phase and just assume that it has already been set
>> from PEI.
>>
>>> If the second set does not suffer from the same issue, can we just
>>> move the entire logic that programs the UART block into PrePi, so
>>> that all subsequent users don't have to touch those registers at all?
>>
>> Well, someone can and will want to switch baudrate from Shell, which
>> means we need to compute a dynamic divisor from the base serial clock
>> frequency. So the only way I can see your idea work is if we re-query
>> the mbox to obtain the base serial clock frequency in
>> SerialPortSetAttributes(), but that means that, for the compiler to be
>> happy, it will need to set up a DXE dependency with RpiFirmwareDxe,
>> which we can't have in PEI phase...
>>
>>> This means we may need two versions of DualSerialPortLib,
>>
>> Unless there exists a compile time macros that indicate whether code
>> is compiling in PEI or DXE phase (and even then I suspect that .inf
>> sections will not cooperate that easily), then I don't see how we can
>> avoid having two separate versions of DualSerialPortLib indeed, and I
>> see that as becoming more of a headache than what we're proposing here...
>>
>>> where the one that PrePi uses may need to be attached to SerialDxe as
>>> well, so that it can reprogram the baud rate as needed. But for all
>>> the remaining DebugLib related uses, we don't really need to
>>> reprogram the UART at all afaics.
>>
>> From a design standpoint, this may look fine, but from an
>> implementation standpoint, when, again, the one problem we are really
>> trying to solve is the sharing of a global variable, I fear that we
>> are going to shoot ourselves in the foot if we try to go in that
>> direction, because duplication of code, when it can be avoided,
>> doesn't strike me like a good idea.
>>
>> If we are going to push for something like that, I'd much rather work
>> on another EDK2 library that solves the problem of early global
>> variable sharing between DXE and PEI, than something that's custom to
>> RPi and that's not going to help anybody else in the process.
>>
>> However, considering the time that has already spent trying to solve
>> this issue, I'd rather not have to do either of these, really, because
>> I don't think there is much to gain from adding bells and whistle to a
>> problem that really boils down to "We need to share a global variable,
>> that we set in early PEI, between PEI and DXE" and which we have
>> solved using what I believe to be the mandated EDK2 way of doing it
>> (HOBs).
>>
>> Now, if you are *really* that opposed to the current solution, I can
>> see what's achievable. But I'd rather only have to do that on a major
>> objection ("This proposal may create problems in the long run because
>> X, Y...") rather than preference of how things may look be better
>> organized if we did it in this other fashion...
>>
>> Besides the need to add an extra PlatformPeiLib and the small hack we
>> had to use to get to the Hob in early DXE phase, do you have specific
>> concerns about the current proposal that you identify as reasonable
>> ground to want to push it back?
>>
>
> I'll have a stab at coding it up myself.
>
Obviously, that works for me. :)
Thanks!
/Pete
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-platforms][PATCH 1/3] Platform/RPi: Fortify mailbox code
2020-05-04 11:15 ` [edk2-platforms][PATCH 1/3] Platform/RPi: Fortify mailbox code Pete Batard
@ 2020-05-06 12:39 ` Ard Biesheuvel
0 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2020-05-06 12:39 UTC (permalink / raw)
To: Pete Batard, devel; +Cc: leif, awarkentin, Andrei Warkentin
On 5/4/20 1:15 PM, Pete Batard wrote:
> From: Andrei Warkentin <andrey.warkentin@gmail.com>
>
> As part of the analysis done in:
> https://github.com/raspberrypi/firmware/issues/1376:
> * Bump up max tries, to avoid command time-outs.
> * Macro-ify RaspberryPiHelper.S some more to make code more maintainable.
> * Add ".align 4" before every command buffer.
>
> Co-authored-by: Pete Batard <pete@akeo.ie>
> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> Signed-off-by: Pete Batard <pete@akeo.ie>
This patch
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Pushed as 725d1198e262..35a5402a718f
> ---
> Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 11 +---
> Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h | 11 +++-
> Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 60 ++++++++------------
> 3 files changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> index 40c78b5d57cf..6c45cf47f082 100644
> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> @@ -1,5 +1,6 @@
> /** @file
> *
> + * Copyright (c) 2020, Pete Batard <pete@akeo.ie>
> * Copyright (c) 2019, ARM Limited. All rights reserved.
> * Copyright (c) 2017-2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> * Copyright (c) 2016, Linaro, Ltd. All rights reserved.
> @@ -30,12 +31,6 @@
> //
> #define NUM_PAGES 1
>
> -//
> -// The number of iterations to perform when waiting for the mailbox
> -// status to change
> -//
> -#define MAX_TRIES 0x100000
> -
> STATIC VOID *mDmaBuffer;
> STATIC VOID *mDmaBufferMapping;
> STATIC UINTN mDmaBufferBusAddress;
> @@ -62,7 +57,7 @@ DrainMailbox (
> }
> ArmDataSynchronizationBarrier ();
> MmioRead32 (BCM2836_MBOX_BASE_ADDRESS + BCM2836_MBOX_READ_OFFSET);
> - } while (++Tries < MAX_TRIES);
> + } while (++Tries < RPI_MBOX_MAX_TRIES);
>
> return FALSE;
> }
> @@ -86,7 +81,7 @@ MailboxWaitForStatusCleared (
> return TRUE;
> }
> ArmDataSynchronizationBarrier ();
> - } while (++Tries < MAX_TRIES);
> + } while (++Tries < RPI_MBOX_MAX_TRIES);
>
> return FALSE;
> }
> diff --git a/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h b/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
> index 584786a61dfd..3328be582df1 100644
> --- a/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
> +++ b/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
> @@ -1,6 +1,6 @@
> /** @file
> *
> - * Copyright (c) 2019, Pete Batard <pete@akeo.ie>
> + * Copyright (c) 2019-2020, Pete Batard <pete@akeo.ie>
> * Copyright (c) 2016, Linaro Limited. All rights reserved.
> *
> * SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -10,6 +10,15 @@
> #ifndef __RASPBERRY_PI_MAILBOX_H__
> #define __RASPBERRY_PI_MAILBOX_H__
>
> +/*
> + * Number of iterations to perform when waiting for the mailbox.
> + *
> + * This number was arrived at empirically, following discussion
> + * at https://github.com/raspberrypi/firmware/issues/1376, to
> + * avoid mailbox time-outs on some commands.
> + */
> +#define RPI_MBOX_MAX_TRIES 0x8000000
> +
> /* Mailbox channels */
> #define RPI_MBOX_PM_CHANNEL 0
> #define RPI_MBOX_FB_CHANNEL 1
> diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> index cc58406e1bfc..91dfe1bb981e 100644
> --- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> +++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> @@ -1,6 +1,7 @@
> /** @file
> *
> - * Copyright (c) 2019, Pete Batard <pete@akeo.ie>
> + * Copyright (c) 2020, Andrei Warkentin <andrey.warkentin@gmail.com>
> + * Copyright (c) 2019-2020, Pete Batard <pete@akeo.ie>
> * Copyright (c) 2016, Linaro Limited. All rights reserved.
> * Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> *
> @@ -13,10 +14,8 @@
> #include <IndustryStandard/Bcm2836.h>
> #include <IndustryStandard/RpiMbox.h>
>
> -#define MAX_TRIES 0x100000
> -
> .macro drain
> - mov x5, #MAX_TRIES
> + mov x5, #RPI_MBOX_MAX_TRIES
> 0: ldr w6, [x4, #BCM2836_MBOX_STATUS_OFFSET]
> tbnz w6, #BCM2836_MBOX_STATUS_EMPTY, 1f
> dmb ld
> @@ -27,7 +26,7 @@
> .endm
>
> .macro poll, status
> - mov x5, #MAX_TRIES
> + mov x5, #RPI_MBOX_MAX_TRIES
> 0: ldr w6, [x4, #BCM2836_MBOX_STATUS_OFFSET]
> tbz w6, #\status, 1f
> dmb ld
> @@ -36,23 +35,30 @@
> 1:
> .endm
>
> + .macro run, command_buffer
> + adr x0, \command_buffer
> + orr x0, x0, #RPI_MBOX_VC_CHANNEL
> + add x0, x0, x1
> +
> + poll BCM2836_MBOX_STATUS_FULL
> + str w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
> + dmb sy
> + poll BCM2836_MBOX_STATUS_EMPTY
> + dmb sy
> + ldr wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
> + dmb ld
> + .endm
> +
> ASM_FUNC (ArmPlatformPeiBootAction)
> - adr x0, .Lmeminfo_buffer
> mov x1, #FixedPcdGet64 (PcdDmaDeviceOffset)
> orr x0, x0, #RPI_MBOX_VC_CHANNEL
> // x1 holds the value of PcdDmaDeviceOffset throughout this function
> - add x0, x0, x1
>
> MOV32 (x4, BCM2836_MBOX_BASE_ADDRESS)
>
> drain
> - poll BCM2836_MBOX_STATUS_FULL
> - str w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
> - dmb sy
> - poll BCM2836_MBOX_STATUS_EMPTY
> - dmb sy
> - ldr wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
> - dmb ld
> +
> + run .Lmeminfo_buffer
>
> ldr w0, .Lmembase
> adr x2, mSystemMemoryBase
> @@ -63,17 +69,7 @@ ASM_FUNC (ArmPlatformPeiBootAction)
> adr x2, mSystemMemoryEnd
> str x0, [x2]
>
> - adr x0, .Lvcinfo_buffer
> - orr x0, x0, #RPI_MBOX_VC_CHANNEL
> - add x0, x0, x1
> -
> - poll BCM2836_MBOX_STATUS_FULL
> - str w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
> - dmb sy
> - poll BCM2836_MBOX_STATUS_EMPTY
> - dmb sy
> - ldr wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
> - dmb ld
> + run .Lvcinfo_buffer
>
> ldr w0, .Lvcbase
> adr x2, mVideoCoreBase
> @@ -83,17 +79,7 @@ ASM_FUNC (ArmPlatformPeiBootAction)
> adr x2, mVideoCoreSize
> str x0, [x2]
>
> - adr x0, .Lrevinfo_buffer
> - orr x0, x0, #RPI_MBOX_VC_CHANNEL
> - add x0, x0, x1
> -
> - poll BCM2836_MBOX_STATUS_FULL
> - str w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
> - dmb sy
> - poll BCM2836_MBOX_STATUS_EMPTY
> - dmb sy
> - ldr wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
> - dmb ld
> + run .Lrevinfo_buffer
>
> ldr w0, .Lrevision
> adr x2, mBoardRevision
> @@ -115,6 +101,7 @@ ASM_FUNC (ArmPlatformPeiBootAction)
> .long 0 // end tag
> .set .Lmeminfo_size, . - .Lmeminfo_buffer
>
> + .align 4
> .Lvcinfo_buffer:
> .long .Lvcinfo_size
> .long 0x0
> @@ -128,6 +115,7 @@ ASM_FUNC (ArmPlatformPeiBootAction)
> .long 0 // end tag
> .set .Lvcinfo_size, . - .Lvcinfo_buffer
>
> + .align 4
> .Lrevinfo_buffer:
> .long .Lrevinfo_size
> .long 0x0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-06 12:39 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-04 11:15 [edk2-platforms][PATCH 0/3] Platform/RPi: Fix compatibility with recent start.elf Pete Batard
2020-05-04 11:15 ` [edk2-platforms][PATCH 1/3] Platform/RPi: Fortify mailbox code Pete Batard
2020-05-06 12:39 ` Ard Biesheuvel
2020-05-04 11:15 ` [edk2-platforms][PATCH 2/3] Platform/RPi/DualSerialPortLib: Fix miniUART serial divisor computation Pete Batard
2020-05-05 10:05 ` Ard Biesheuvel
2020-05-05 11:54 ` Pete Batard
2020-05-05 12:05 ` Ard Biesheuvel
2020-05-05 12:09 ` Pete Batard
2020-05-04 11:15 ` [edk2-platforms][PATCH 3/3] Platform/RPi: Report core clock frequency during early init Pete Batard
2020-05-04 11:37 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-05-05 6:04 ` Andrei Warkentin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox