* [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell
@ 2020-05-05 14:50 Ard Biesheuvel
2020-05-05 14:50 ` [PATCH edk2-platforms 1/5] Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse Ard Biesheuvel
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 14:50 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin, Ard Biesheuvel
This proposes an alternative approach to the issue described in
https://github.com/raspberrypi/firmware/issues/1376.
Instead of fiddling with HobLib internals and relying on headers being
included in the correct order, this replaces all DEBUG uses of SerialPortLib
with an implementation that doesn't reprogram the UART at all (and so does
not need to know the value of the base clock), and updates the two remaining
users to query the firmware for the correct value before touching the h/w.
NOTE: build tested only.
Ard Biesheuvel (5):
Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse
Platform/RaspberryPi: introduce DebugDualSerialPortLib
Platform/RaspberryPi: fix 16550 divisor calculation logic
Platform/RaspberryPi3: query firmware for 16550 input clock at boot
Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3
Platform/RaspberryPi/RPi3/RPi3.dsc | 16 +-
Platform/RaspberryPi/RPi4/RPi4.dsc | 14 +-
.../DebugDualSerialPortLib.inf | 46 ++++
...alPortLib.inf => DualSerialPortDxeLib.inf} | 30 ++-
.../DualSerialPortLib/DualSerialPortLib.inf | 5 +-
.../DualSerialPortLib/DualSerialPortLib.h | 82 ++++++
.../DebugDualSerialPortLib.c | 28 ++
.../DualSerialPortLib/DualSerialPortLib.c | 243 +-----------------
.../DualSerialPortLibCommon.c | 218 ++++++++++++++++
.../DualSerialPortLibConstructor.c | 40 +++
.../PlatformLib/AArch64/RaspberryPiHelper.S | 25 +-
11 files changed, 494 insertions(+), 253 deletions(-)
create mode 100644 Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
copy Platform/RaspberryPi/Library/DualSerialPortLib/{DualSerialPortLib.inf => DualSerialPortDxeLib.inf} (69%)
create mode 100644 Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h
create mode 100644 Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c
create mode 100644 Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c
create mode 100644 Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH edk2-platforms 1/5] Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse
2020-05-05 14:50 [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
@ 2020-05-05 14:50 ` Ard Biesheuvel
2020-05-06 10:18 ` Pete Batard
2020-05-05 14:50 ` [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib Ard Biesheuvel
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 14:50 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin, Ard Biesheuvel
In preparation of creating different versions of DualSerialPortLib,
split off the parts that will be shared between all versions.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf | 5 +-
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h | 82 +++++++
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 229 +-------------------
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c | 218 +++++++++++++++++++
4 files changed, 305 insertions(+), 229 deletions(-)
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
index af1e6b026fe6..fda9ff2bcbf9 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
@@ -19,9 +19,8 @@ [Defines]
[Packages]
ArmPlatformPkg/ArmPlatformPkg.dec
- EmbeddedPkg/EmbeddedPkg.dec
- MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
Silicon/Broadcom/Bcm283x/Bcm283x.dec
[LibraryClasses]
@@ -32,6 +31,8 @@ [LibraryClasses]
[Sources]
DualSerialPortLib.c
+ DualSerialPortLib.h
+ DualSerialPortLibCommon.c
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth ## SOMETIMES_CONSUMES
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h
new file mode 100644
index 000000000000..a8d150f516b9
--- /dev/null
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h
@@ -0,0 +1,82 @@
+/** @file
+ 16550 and PL011 Serial Port library functions for Raspberry Pi
+
+ 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>
+ Copyright (c) 2012 - 2016, ARM Ltd. All rights reserved.<BR>
+ Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+ Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi/UefiBaseType.h>
+#include <IndustryStandard/Bcm2836.h>
+#include <IndustryStandard/Bcm2836Gpio.h>
+
+#define PL011_UART_REGISTER_BASE BCM2836_PL011_UART_BASE_ADDRESS
+#define MINI_UART_REGISTER_BASE (BCM2836_MINI_UART_BASE_ADDRESS + 0x40)
+
+//
+// 16550 UART register offsets and bitfields
+//
+#define R_UART_RXBUF 0 // LCR_DLAB = 0
+#define R_UART_TXBUF 0 // LCR_DLAB = 0
+#define R_UART_BAUD_LOW 0 // LCR_DLAB = 1
+#define R_UART_BAUD_HIGH 1 // LCR_DLAB = 1
+#define R_UART_IER 1 // LCR_DLAB = 0
+#define R_UART_FCR 2
+#define B_UART_FCR_FIFOE BIT0
+#define B_UART_FCR_FIFO64 BIT5
+#define R_UART_LCR 3
+#define B_UART_LCR_DLAB BIT7
+#define R_UART_MCR 4
+#define B_UART_MCR_DTRC BIT0
+#define B_UART_MCR_RTS BIT1
+#define R_UART_LSR 5
+#define B_UART_LSR_RXRDY BIT0
+#define B_UART_LSR_TXRDY BIT5
+#define B_UART_LSR_TEMT BIT6
+#define R_UART_MSR 6
+#define B_UART_MSR_CTS BIT4
+#define B_UART_MSR_DSR BIT5
+#define B_UART_MSR_RI BIT6
+#define B_UART_MSR_DCD BIT7
+
+extern BOOLEAN UsePl011Uart;
+extern BOOLEAN UsePl011UartSet;
+
+/**
+ Read an 8-bit 16550 register.
+
+ @param Base The base address register of UART device.
+ @param Offset The offset of the 16550 register to read.
+
+ @return The value read from the 16550 register.
+
+**/
+UINT8
+SerialPortReadRegister (
+ UINTN Base,
+ UINTN Offset
+ );
+
+/**
+ Write an 8-bit 16550 register.
+
+ @param Base The base address register of UART device.
+ @param Offset The offset of the 16550 register to write.
+ @param Value The value to write to the 16550 register specified by Offset.
+
+ @return The value written to the 16550 register.
+
+**/
+UINT8
+SerialPortWriteRegister (
+ UINTN Base,
+ UINTN Offset,
+ UINT8 Value
+ );
+
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index 05e12f383785..b1d17d3fa04a 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -13,132 +13,13 @@
**/
#include <Base.h>
-#include <IndustryStandard/Bcm2836.h>
-#include <IndustryStandard/Bcm2836Gpio.h>
-#include <Library/BaseLib.h>
#include <Library/IoLib.h>
#include <Library/PcdLib.h>
#include <Library/PL011UartClockLib.h>
#include <Library/PL011UartLib.h>
#include <Library/SerialPortLib.h>
-BOOLEAN UsePl011Uart = FALSE;
-BOOLEAN UsePl011UartSet = FALSE;
-
-#define PL011_UART_REGISTER_BASE BCM2836_PL011_UART_BASE_ADDRESS
-#define MINI_UART_REGISTER_BASE (BCM2836_MINI_UART_BASE_ADDRESS + 0x40)
-
-//
-// 16550 UART register offsets and bitfields
-//
-#define R_UART_RXBUF 0 // LCR_DLAB = 0
-#define R_UART_TXBUF 0 // LCR_DLAB = 0
-#define R_UART_BAUD_LOW 0 // LCR_DLAB = 1
-#define R_UART_BAUD_HIGH 1 // LCR_DLAB = 1
-#define R_UART_IER 1 // LCR_DLAB = 0
-#define R_UART_FCR 2
-#define B_UART_FCR_FIFOE BIT0
-#define B_UART_FCR_FIFO64 BIT5
-#define R_UART_LCR 3
-#define B_UART_LCR_DLAB BIT7
-#define R_UART_MCR 4
-#define B_UART_MCR_DTRC BIT0
-#define B_UART_MCR_RTS BIT1
-#define R_UART_LSR 5
-#define B_UART_LSR_RXRDY BIT0
-#define B_UART_LSR_TXRDY BIT5
-#define B_UART_LSR_TEMT BIT6
-#define R_UART_MSR 6
-#define B_UART_MSR_CTS BIT4
-#define B_UART_MSR_DSR BIT5
-#define B_UART_MSR_RI BIT6
-#define B_UART_MSR_DCD BIT7
-
-/**
- Read an 8-bit 16550 register.
-
- @param Base The base address register of UART device.
- @param Offset The offset of the 16550 register to read.
-
- @return The value read from the 16550 register.
-
-**/
-UINT8
-SerialPortReadRegister (
- UINTN Base,
- UINTN Offset
- )
-{
- return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
-}
-
-/**
- Write an 8-bit 16550 register.
-
- @param Base The base address register of UART device.
- @param Offset The offset of the 16550 register to write.
- @param Value The value to write to the 16550 register specified by Offset.
-
- @return The value written to the 16550 register.
-
-**/
-UINT8
-SerialPortWriteRegister (
- UINTN Base,
- UINTN Offset,
- UINT8 Value
- )
-{
- return MmioWrite8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride), Value);
-}
-
-/**
- Return whether the hardware flow control signal allows writing.
-
- @param SerialRegisterBase The base address register of UART device.
-
- @retval TRUE The serial port is writable.
- @retval FALSE The serial port is not writable.
-**/
-BOOLEAN
-SerialPortWritable (
- UINTN SerialRegisterBase
- )
-{
- if (PcdGetBool (PcdSerialUseHardwareFlowControl)) {
- if (PcdGetBool (PcdSerialDetectCable)) {
- //
- // Wait for both DSR and CTS to be set
- // DSR is set if a cable is connected.
- // CTS is set if it is ok to transmit data
- //
- // DSR CTS Description Action
- // === === ======================================== ========
- // 0 0 No cable connected. Wait
- // 0 1 No cable connected. Wait
- // 1 0 Cable connected, but not clear to send. Wait
- // 1 1 Cable connected, and clear to send. Transmit
- //
- return (BOOLEAN) ((SerialPortReadRegister (SerialRegisterBase, R_UART_MSR) & (B_UART_MSR_DSR | B_UART_MSR_CTS)) == (B_UART_MSR_DSR | B_UART_MSR_CTS));
- } else {
- //
- // Wait for both DSR and CTS to be set OR for DSR to be clear.
- // DSR is set if a cable is connected.
- // CTS is set if it is ok to transmit data
- //
- // DSR CTS Description Action
- // === === ======================================== ========
- // 0 0 No cable connected. Transmit
- // 0 1 No cable connected. Transmit
- // 1 0 Cable connected, but not clear to send. Wait
- // 1 1 Cable connected, and clar to send. Transmit
- //
- return (BOOLEAN) ((SerialPortReadRegister (SerialRegisterBase, R_UART_MSR) & (B_UART_MSR_DSR | B_UART_MSR_CTS)) != (B_UART_MSR_DSR));
- }
- }
-
- return TRUE;
-}
+#include "DualSerialPortLib.h"
/**
Return the baud generator divisor to use for 16650 setup.
@@ -147,6 +28,7 @@ SerialPortWritable (
@return The baud generator divisor.
**/
+STATIC
UINT32
SerialPortGetDivisor (
UINT32 SerialBaudRate
@@ -288,113 +170,6 @@ SerialPortInitialize (
}
}
-/**
- Write data from buffer to serial device.
-
- Writes NumberOfBytes data bytes from Buffer to the serial device.
- The number of bytes actually written to the serial device is returned.
- If the return value is less than NumberOfBytes, then the write operation failed.
-
- If Buffer is NULL, then ASSERT().
-
- If NumberOfBytes is zero, then return 0.
-
- @param Buffer Pointer to the data buffer to be written.
- @param NumberOfBytes Number of bytes to written to the serial device.
-
- @retval 0 NumberOfBytes is 0.
- @retval >0 The number of bytes written to the serial device.
- If this value is less than NumberOfBytes, then the write operation failed.
-
-**/
-UINTN
-EFIAPI
-SerialPortWrite (
- IN UINT8 *Buffer,
- IN UINTN NumberOfBytes
- )
-{
- UINTN SerialRegisterBase;
- UINTN Result;
- UINTN Index;
- UINTN FifoSize;
-
- //
- // Serial writes may happen *before* the UART has been initialized
- // and if we use the wrong UART then, all kind of bad things happen.
- // To alleviate this, we add UART detection in SerialPortWrite and
- // guard the UART detection with a second boolean.
- //
- if (!UsePl011UartSet) {
- UsePl011Uart = ((MmioRead32(GPIO_BASE_ADDRESS + 4) & 0x0003F000) == 0x00024000);
- UsePl011UartSet = TRUE;
- }
-
- if (UsePl011Uart) {
- return PL011UartWrite (PL011_UART_REGISTER_BASE, Buffer, NumberOfBytes);
- } else {
- if (Buffer == NULL) {
- return 0;
- }
-
- SerialRegisterBase = MINI_UART_REGISTER_BASE;
-
- if (NumberOfBytes == 0) {
- //
- // Flush the hardware
- //
-
- //
- // Wait for both the transmit FIFO and shift register empty.
- //
- while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
-
- //
- // Wait for the hardware flow control signal
- //
- while (!SerialPortWritable (SerialRegisterBase));
- return 0;
- }
-
- //
- // Compute the maximum size of the Tx FIFO
- //
- FifoSize = 1;
- if ((PcdGet8 (PcdSerialFifoControl) & B_UART_FCR_FIFOE) != 0) {
- if ((PcdGet8 (PcdSerialFifoControl) & B_UART_FCR_FIFO64) == 0) {
- FifoSize = 16;
- } else {
- FifoSize = PcdGet32 (PcdSerialExtendedTxFifoSize);
- }
- }
-
- Result = NumberOfBytes;
- while (NumberOfBytes != 0) {
- //
- // Wait for the serial port to be ready, to make sure both the transmit FIFO
- // and shift register empty.
- //
- while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
-
- //
- // Fill then entire Tx FIFO
- //
- for (Index = 0; Index < FifoSize && NumberOfBytes != 0; Index++, NumberOfBytes--, Buffer++) {
- //
- // Wait for the hardware flow control signal
- //
- while (!SerialPortWritable (SerialRegisterBase));
-
- //
- // Write byte to the transmit buffer.
- //
- SerialPortWriteRegister (SerialRegisterBase, R_UART_TXBUF, *Buffer);
- }
- }
- return Result;
- }
-}
-
/**
Reads data from a serial device into a buffer.
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c
new file mode 100644
index 000000000000..48d8df280640
--- /dev/null
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c
@@ -0,0 +1,218 @@
+/** @file
+ 16550 and PL011 Serial Port library functions for Raspberry Pi
+
+ 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>
+ Copyright (c) 2012 - 2016, ARM Ltd. All rights reserved.<BR>
+ Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+ Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Library/IoLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PL011UartLib.h>
+#include <Library/SerialPortLib.h>
+
+#include "DualSerialPortLib.h"
+
+BOOLEAN UsePl011Uart = FALSE;
+BOOLEAN UsePl011UartSet = FALSE;
+
+/**
+ Read an 8-bit 16550 register.
+
+ @param Base The base address register of UART device.
+ @param Offset The offset of the 16550 register to read.
+
+ @return The value read from the 16550 register.
+
+**/
+UINT8
+SerialPortReadRegister (
+ UINTN Base,
+ UINTN Offset
+ )
+{
+ return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
+}
+
+/**
+ Write an 8-bit 16550 register.
+
+ @param Base The base address register of UART device.
+ @param Offset The offset of the 16550 register to write.
+ @param Value The value to write to the 16550 register specified by Offset.
+
+ @return The value written to the 16550 register.
+
+**/
+UINT8
+SerialPortWriteRegister (
+ UINTN Base,
+ UINTN Offset,
+ UINT8 Value
+ )
+{
+ return MmioWrite8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride), Value);
+}
+
+/**
+ Return whether the hardware flow control signal allows writing.
+
+ @param SerialRegisterBase The base address register of UART device.
+
+ @retval TRUE The serial port is writable.
+ @retval FALSE The serial port is not writable.
+**/
+STATIC
+BOOLEAN
+SerialPortWritable (
+ UINTN SerialRegisterBase
+ )
+{
+ if (PcdGetBool (PcdSerialUseHardwareFlowControl)) {
+ if (PcdGetBool (PcdSerialDetectCable)) {
+ //
+ // Wait for both DSR and CTS to be set
+ // DSR is set if a cable is connected.
+ // CTS is set if it is ok to transmit data
+ //
+ // DSR CTS Description Action
+ // === === ======================================== ========
+ // 0 0 No cable connected. Wait
+ // 0 1 No cable connected. Wait
+ // 1 0 Cable connected, but not clear to send. Wait
+ // 1 1 Cable connected, and clear to send. Transmit
+ //
+ return (BOOLEAN) ((SerialPortReadRegister (SerialRegisterBase, R_UART_MSR) & (B_UART_MSR_DSR | B_UART_MSR_CTS)) == (B_UART_MSR_DSR | B_UART_MSR_CTS));
+ } else {
+ //
+ // Wait for both DSR and CTS to be set OR for DSR to be clear.
+ // DSR is set if a cable is connected.
+ // CTS is set if it is ok to transmit data
+ //
+ // DSR CTS Description Action
+ // === === ======================================== ========
+ // 0 0 No cable connected. Transmit
+ // 0 1 No cable connected. Transmit
+ // 1 0 Cable connected, but not clear to send. Wait
+ // 1 1 Cable connected, and clar to send. Transmit
+ //
+ return (BOOLEAN) ((SerialPortReadRegister (SerialRegisterBase, R_UART_MSR) & (B_UART_MSR_DSR | B_UART_MSR_CTS)) != (B_UART_MSR_DSR));
+ }
+ }
+
+ return TRUE;
+}
+
+/**
+ Write data from buffer to serial device.
+
+ Writes NumberOfBytes data bytes from Buffer to the serial device.
+ The number of bytes actually written to the serial device is returned.
+ If the return value is less than NumberOfBytes, then the write operation failed.
+
+ If Buffer is NULL, then ASSERT().
+
+ If NumberOfBytes is zero, then return 0.
+
+ @param Buffer Pointer to the data buffer to be written.
+ @param NumberOfBytes Number of bytes to written to the serial device.
+
+ @retval 0 NumberOfBytes is 0.
+ @retval >0 The number of bytes written to the serial device.
+ If this value is less than NumberOfBytes, then the write operation failed.
+
+**/
+UINTN
+EFIAPI
+SerialPortWrite (
+ IN UINT8 *Buffer,
+ IN UINTN NumberOfBytes
+ )
+{
+ UINTN SerialRegisterBase;
+ UINTN Result;
+ UINTN Index;
+ UINTN FifoSize;
+
+ //
+ // Serial writes may happen *before* the UART has been initialized
+ // and if we use the wrong UART then, all kind of bad things happen.
+ // To alleviate this, we add UART detection in SerialPortWrite and
+ // guard the UART detection with a second boolean.
+ //
+ if (!UsePl011UartSet) {
+ UsePl011Uart = ((MmioRead32(GPIO_BASE_ADDRESS + 4) & 0x0003F000) == 0x00024000);
+ UsePl011UartSet = TRUE;
+ }
+
+ if (UsePl011Uart) {
+ return PL011UartWrite (PL011_UART_REGISTER_BASE, Buffer, NumberOfBytes);
+ } else {
+ if (Buffer == NULL) {
+ return 0;
+ }
+
+ SerialRegisterBase = MINI_UART_REGISTER_BASE;
+
+ if (NumberOfBytes == 0) {
+ //
+ // Flush the hardware
+ //
+
+ //
+ // Wait for both the transmit FIFO and shift register empty.
+ //
+ while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
+
+ //
+ // Wait for the hardware flow control signal
+ //
+ while (!SerialPortWritable (SerialRegisterBase));
+ return 0;
+ }
+
+ //
+ // Compute the maximum size of the Tx FIFO
+ //
+ FifoSize = 1;
+ if ((PcdGet8 (PcdSerialFifoControl) & B_UART_FCR_FIFOE) != 0) {
+ if ((PcdGet8 (PcdSerialFifoControl) & B_UART_FCR_FIFO64) == 0) {
+ FifoSize = 16;
+ } else {
+ FifoSize = PcdGet32 (PcdSerialExtendedTxFifoSize);
+ }
+ }
+
+ Result = NumberOfBytes;
+ while (NumberOfBytes != 0) {
+ //
+ // Wait for the serial port to be ready, to make sure both the transmit FIFO
+ // and shift register empty.
+ //
+ while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
+
+ //
+ // Fill then entire Tx FIFO
+ //
+ for (Index = 0; Index < FifoSize && NumberOfBytes != 0; Index++, NumberOfBytes--, Buffer++) {
+ //
+ // Wait for the hardware flow control signal
+ //
+ while (!SerialPortWritable (SerialRegisterBase));
+
+ //
+ // Write byte to the transmit buffer.
+ //
+ SerialPortWriteRegister (SerialRegisterBase, R_UART_TXBUF, *Buffer);
+ }
+ }
+ return Result;
+ }
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib
2020-05-05 14:50 [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
2020-05-05 14:50 ` [PATCH edk2-platforms 1/5] Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse Ard Biesheuvel
@ 2020-05-05 14:50 ` Ard Biesheuvel
2020-05-06 10:18 ` Pete Batard
2020-05-05 14:50 ` [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic Ard Biesheuvel
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 14:50 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin, Ard Biesheuvel
On DEBUG builds that use the serial port directly for debug output,
every module reinitializes the UART hardware, through the DebugLib
constructor calling SerialPortInitialize.
This is unnecessary, but usually harmless. However, in cases where this
requires information that is non-trivial to obtain (e.g., the rate of
the clock source feeding the baud clock), it results in a special kind
of dependency hell that can only be fully appreciated by seasoned EDK2
connoisseurs [0].
As a first step towards solving this mess, implement a special version
of the Raspberry Pi dual serial port library that only implements the
SerialPortInitialize() and SerialPortWrite() library functions, and make
the former an empty stub. This makes it only suitable for use by modules
that inherit a dependency on SerialPortLib via DebugLib, and requires us
to ensure that the baud clock is programmed correctly by the SEC phase.
Use this version of the library to satisfy all SerialPortLib dependencies
except the ones in PrePi and in SerialDxe. These will retain the full
version, which is the only one that still consumes PcdSerialClockRate.
[0] There are two distinct problems making this mess almost unsolvable:
- SerialPortInitialize() is called directly in various places instead
of relying on constructor ordering, so adding a constructor to a
SerialPortLib implementation does not help,
- Constructor ordering resolution in the EDK2 tooling fails to take
transitive dependencies into account if an intermediate library has
no constructor it self. For instance, if LibA depends on LibB, which
depends on LibC, the constructors of LibA and LibC could be called in
any order if LibB does not have a constructor itself (and fixing this
breaks all the platforms in the tree)
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
Platform/RaspberryPi/RPi3/RPi3.dsc | 12 +++--
Platform/RaspberryPi/RPi4/RPi4.dsc | 12 +++--
Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf | 46 ++++++++++++++++++++
Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c | 28 ++++++++++++
4 files changed, 92 insertions(+), 6 deletions(-)
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index 563fb891b841..d7218219fc5a 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -127,7 +127,7 @@ [LibraryClasses.common]
# Dual serial port library
PL011UartClockLib|ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf
PL011UartLib|ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.inf
- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
# Cryptographic libraries
IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
@@ -521,7 +521,10 @@ [Components.common]
#
# PEI Phase modules
#
- ArmPlatformPkg/PrePi/PeiUniCore.inf
+ ArmPlatformPkg/PrePi/PeiUniCore.inf {
+ <LibraryClasses>
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+ }
#
# DXE
@@ -569,7 +572,10 @@ [Components.common]
MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
- MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
+ MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
+ <LibraryClasses>
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+ }
Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf
MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 4deccd9d3ecc..4fb015b077e6 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -127,7 +127,7 @@ [LibraryClasses.common]
# Dual serial port library
PL011UartClockLib|ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf
PL011UartLib|ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.inf
- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
# Cryptographic libraries
IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
@@ -536,7 +536,10 @@ [Components.common]
#
# PEI Phase modules
#
- ArmPlatformPkg/PrePi/PeiUniCore.inf
+ ArmPlatformPkg/PrePi/PeiUniCore.inf {
+ <LibraryClasses>
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+ }
#
# DXE
@@ -584,7 +587,10 @@ [Components.common]
MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
- MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
+ MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
+ <LibraryClasses>
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+ }
Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf
EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf b/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
new file mode 100644
index 000000000000..cd14d44c59d8
--- /dev/null
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
@@ -0,0 +1,46 @@
+## @file
+#
+# SerialPortLib instance for both PL011 and 16550 UART that satisfies
+# only the dependencies of DebugLib.
+#
+# Copyright (c) 2020, Pete Batard <pete@akeo.ie>
+# Copyright (c) 2012 - 2020, ARM Ltd. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 1.27
+ BASE_NAME = DebugDualSerialPortLib
+ FILE_GUID = 323bae1b-c2fc-4929-a2fe-9e9174f8ce0f
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = SerialPortLib
+
+[Packages]
+ ArmPlatformPkg/ArmPlatformPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Silicon/Broadcom/Bcm283x/Bcm283x.dec
+
+[LibraryClasses]
+ IoLib
+ PcdLib
+ PL011UartLib
+
+[Sources]
+ DebugDualSerialPortLib.c
+ DualSerialPortLib.h
+ DualSerialPortLibCommon.c
+
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ## CONSUMES
+
+[FixedPcd]
+ gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress ## CONSUMES
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c
new file mode 100644
index 000000000000..fbb9fde08bac
--- /dev/null
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c
@@ -0,0 +1,28 @@
+/** @file
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Library/SerialPortLib.h>
+
+/**
+ Initialize the serial device hardware.
+
+ If no initialization is required, then return RETURN_SUCCESS.
+ If the serial device was successfully initialized, then return RETURN_SUCCESS.
+ If the serial device could not be initialized, then return RETURN_DEVICE_ERROR.
+
+ @retval RETURN_SUCCESS The serial device was initialized.
+ @retval RETURN_DEVICE_ERROR The serial device could not be initialized.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortInitialize (
+ VOID
+ )
+{
+ return RETURN_SUCCESS;
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic
2020-05-05 14:50 [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
2020-05-05 14:50 ` [PATCH edk2-platforms 1/5] Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse Ard Biesheuvel
2020-05-05 14:50 ` [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib Ard Biesheuvel
@ 2020-05-05 14:50 ` Ard Biesheuvel
2020-05-05 18:10 ` Ard Biesheuvel
2020-05-05 14:50 ` [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot Ard Biesheuvel
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 14:50 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin, Ard Biesheuvel
The 16550 'miniUART' on the Raspberry Pi gets its input clock from
different sources on RPi3 and RPi3. Fix the logic that derives the
divisor for the 16550 baud clock on the respective platforms.
While at it, make the input clock PCD patchable for RPi3 so we can
manipulate it at runtime in a future patch.
Co-authored-by: Pete Batard <pete@akeo.ie>
Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Pete Batard <pete@akeo.ie>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
Platform/RaspberryPi/RPi3/RPi3.dsc | 4 +++-
Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 14 ++++++++------
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index d7218219fc5a..96b27400eef8 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -409,7 +409,6 @@ [PcdsFixedAtBuild.common]
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
@@ -441,6 +440,9 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
+[PcdsPatchableInModule]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|250000000
+
[PcdsDynamicHii.common.DEFAULT]
#
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index 4fb015b077e6..5d8bd88d7e34 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -420,7 +420,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/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index b1d17d3fa04a..5e83bbf022eb 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -41,18 +41,20 @@ SerialPortGetDivisor (
// 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.
//
- BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate) * 4;
+ BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);
+#if (RPI_MODEL == 4)
Divisor = MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
if (Divisor != 0)
BaseClockRate = (BaseClockRate << 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)BaseClockRate / (SerialBaudRate * 8);
+ if (Divisor != 0) {
+ Divisor--;
}
return Divisor;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot
2020-05-05 14:50 [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
` (2 preceding siblings ...)
2020-05-05 14:50 ` [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic Ard Biesheuvel
@ 2020-05-05 14:50 ` Ard Biesheuvel
2020-05-05 18:11 ` Ard Biesheuvel
2020-05-05 14:50 ` [PATCH edk2-platforms 5/5] Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3 Ard Biesheuvel
2020-05-06 16:16 ` [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
5 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 14:50 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin, Ard Biesheuvel
Query the firmware for the clock rate that is used to drive the
16550 baud clock, so that we can program the correct baud rate.
Co-authored-by: Pete Batard <pete@akeo.ie>
Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Pete Batard <pete@akeo.ie>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 25 +++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
index 91dfe1bb981e..35580e4ed73a 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
+++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
@@ -3,7 +3,7 @@
* 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.
+ * Copyright (c) 2011-2020, ARM Limited. All rights reserved.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*
@@ -85,6 +85,14 @@ ASM_FUNC (ArmPlatformPeiBootAction)
adr x2, mBoardRevision
str w0, [x2]
+#if (RPI_MODEL == 3)
+ run .Lclkinfo_buffer
+
+ ldr w0, .Lfrequency
+ adrp x2, _gPcd_BinaryPatch_PcdSerialClockRate
+ str w0, [x2, :lo12:_gPcd_BinaryPatch_PcdSerialClockRate]
+#endif
+
ret
.align 4
@@ -127,6 +135,21 @@ ASM_FUNC (ArmPlatformPeiBootAction)
.long 0 // end tag
.set .Lrevinfo_size, . - .Lrevinfo_buffer
+#if (RPI_MODEL == 3)
+ .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
+#endif
+
//UINTN
//ArmPlatformGetPrimaryCoreMpId (
// VOID
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH edk2-platforms 5/5] Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3
2020-05-05 14:50 [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
` (3 preceding siblings ...)
2020-05-05 14:50 ` [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot Ard Biesheuvel
@ 2020-05-05 14:50 ` Ard Biesheuvel
2020-05-06 10:19 ` Pete Batard
2020-05-06 16:16 ` [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
5 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 14:50 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin, Ard Biesheuvel
The Raspberry Pi 3 derives its 16550 baud clock from the variable core
clock, and so any reprogramming of the baud rate needs to take the
actual core clock value into account.
Introduce a DXE phase version of DualSerialPortLib that discovers this
value in its constructor, using the RPi firmware protocol, and wire it
up for the RPi3 platform.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
Platform/RaspberryPi/RPi3/RPi3.dsc | 2 +-
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf | 67 ++++++++++++++++++++
Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c | 40 ++++++++++++
3 files changed, 108 insertions(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index 96b27400eef8..2b8ad1c4bdbd 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -576,7 +576,7 @@ [Components.common]
MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
<LibraryClasses>
- SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
+ SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf
}
Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf
new file mode 100644
index 000000000000..4c22b39daa7f
--- /dev/null
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf
@@ -0,0 +1,67 @@
+## @file
+#
+# DXE phase SerialPortLib instance for both PL011 and 16550 UART.
+#
+# Copyright (c) 2020, Pete Batard <pete@akeo.ie>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 1.27
+ BASE_NAME = DualSerialPortDxeLib
+ FILE_GUID = d586667e-ec50-4bf6-9701-fb4e29055a60
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = SerialPortLib|DXE_DRIVER
+ CONSTRUCTOR = DualSerialPortDxeLibConstructor
+
+[Packages]
+ ArmPlatformPkg/ArmPlatformPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Platform/RaspberryPi/RaspberryPi.dec
+ Silicon/Broadcom/Bcm283x/Bcm283x.dec
+
+[LibraryClasses]
+ IoLib
+ PcdLib
+ PL011UartClockLib
+ PL011UartLib
+
+[Sources]
+ DualSerialPortLib.c
+ DualSerialPortLib.h
+ DualSerialPortLibCommon.c
+ DualSerialPortLibConstructor.c
+
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth ## SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable ## SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ## CONSUMES
+
+[FixedPcd]
+ gArmPlatformTokenSpaceGuid.PL011UartClkInHz
+ gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
+
+[Protocols]
+ gRaspberryPiFirmwareProtocolGuid ## CONSUMES
+
+[PatchPcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
+
+[Depex]
+ gRaspberryPiFirmwareProtocolGuid
diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c
new file mode 100644
index 000000000000..c6d695181ab7
--- /dev/null
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c
@@ -0,0 +1,40 @@
+/** @file
+
+ Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+#include <IndustryStandard/RpiMbox.h>
+#include <Library/SerialPortLib.h>
+#include <Library/PcdLib.h>
+#include <Protocol/RpiFirmware.h>
+
+EFI_STATUS
+EFIAPI
+DualSerialPortDxeLibConstructor (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ RASPBERRY_PI_FIRMWARE_PROTOCOL *Firmware;
+ UINT32 ClockRate;
+ EFI_STATUS Status;
+
+ Status = SystemTable->BootServices->LocateProtocol (
+ &gRaspberryPiFirmwareProtocolGuid,
+ NULL, (VOID **)&Firmware);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = Firmware->GetClockRate (RPI_MBOX_CLOCK_RATE_CORE, &ClockRate);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ PatchPcdSet32 (PcdSerialClockRate, ClockRate);
+ return EFI_SUCCESS;
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic
2020-05-05 14:50 ` [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic Ard Biesheuvel
@ 2020-05-05 18:10 ` Ard Biesheuvel
2020-05-06 10:18 ` [edk2-devel] " Pete Batard
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 18:10 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin
On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
> The 16550 'miniUART' on the Raspberry Pi gets its input clock from
> different sources on RPi3 and RPi3. Fix the logic that derives the
This should be 'Rpi3 and RPi4'
> divisor for the 16550 baud clock on the respective platforms.
>
> While at it, make the input clock PCD patchable for RPi3 so we can
> manipulate it at runtime in a future patch.
>
> Co-authored-by: Pete Batard <pete@akeo.ie>
> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Pete Batard <pete@akeo.ie>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> Platform/RaspberryPi/RPi3/RPi3.dsc | 4 +++-
> Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 14 ++++++++------
> 3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> index d7218219fc5a..96b27400eef8 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> @@ -409,7 +409,6 @@ [PcdsFixedAtBuild.common]
> gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
> - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
> gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
> @@ -441,6 +440,9 @@ [PcdsFixedAtBuild.common]
> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>
> +[PcdsPatchableInModule]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|250000000
> +
> [PcdsDynamicHii.common.DEFAULT]
>
> #
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index 4fb015b077e6..5d8bd88d7e34 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -420,7 +420,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/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
> index b1d17d3fa04a..5e83bbf022eb 100644
> --- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
> @@ -41,18 +41,20 @@ SerialPortGetDivisor (
> // 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.
> //
> - BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate) * 4;
> + BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);
> +#if (RPI_MODEL == 4)
> Divisor = MmioRead32(BCM2836_CM_BASE + BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
> if (Divisor != 0)
> BaseClockRate = (BaseClockRate << 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)BaseClockRate / (SerialBaudRate * 8);
> + if (Divisor != 0) {
> + Divisor--;
> }
> return Divisor;
> }
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot
2020-05-05 14:50 ` [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot Ard Biesheuvel
@ 2020-05-05 18:11 ` Ard Biesheuvel
2020-05-06 10:18 ` [edk2-devel] " Pete Batard
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-05 18:11 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin
On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
> Query the firmware for the clock rate that is used to drive the
> 16550 baud clock, so that we can program the correct baud rate.
>
> Co-authored-by: Pete Batard <pete@akeo.ie>
> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Pete Batard <pete@akeo.ie>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 25 +++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> index 91dfe1bb981e..35580e4ed73a 100644
> --- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> +++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> @@ -3,7 +3,7 @@
> * 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.
> + * Copyright (c) 2011-2020, ARM Limited. All rights reserved.
> *
> * SPDX-License-Identifier: BSD-2-Clause-Patent
> *
> @@ -85,6 +85,14 @@ ASM_FUNC (ArmPlatformPeiBootAction)
> adr x2, mBoardRevision
> str w0, [x2]
>
> +#if (RPI_MODEL == 3)
As noted by Pete off-list, doing this doesn't work unless we add
something like
GCC:*_*_*_PP_FLAGS = -DRPI_MODEL=3
to the [BuildOptions] in RPi3.dsc
> + run .Lclkinfo_buffer
> +
> + ldr w0, .Lfrequency
> + adrp x2, _gPcd_BinaryPatch_PcdSerialClockRate
> + str w0, [x2, :lo12:_gPcd_BinaryPatch_PcdSerialClockRate]
> +#endif
> +
> ret
>
> .align 4
> @@ -127,6 +135,21 @@ ASM_FUNC (ArmPlatformPeiBootAction)
> .long 0 // end tag
> .set .Lrevinfo_size, . - .Lrevinfo_buffer
>
> +#if (RPI_MODEL == 3)
> + .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
> +#endif
> +
> //UINTN
> //ArmPlatformGetPrimaryCoreMpId (
> // VOID
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH edk2-platforms 1/5] Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse
2020-05-05 14:50 ` [PATCH edk2-platforms 1/5] Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse Ard Biesheuvel
@ 2020-05-06 10:18 ` Pete Batard
0 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2020-05-06 10:18 UTC (permalink / raw)
To: Ard Biesheuvel, devel; +Cc: leif, andrey.warkentin
One minor whitespace issue inline:
On 2020.05.05 15:50, Ard Biesheuvel wrote:
> In preparation of creating different versions of DualSerialPortLib,
> split off the parts that will be shared between all versions.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf | 5 +-
> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h | 82 +++++++
> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c | 229 +-------------------
> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c | 218 +++++++++++++++++++
> 4 files changed, 305 insertions(+), 229 deletions(-)
>
> diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> index af1e6b026fe6..fda9ff2bcbf9 100644
> --- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> @@ -19,9 +19,8 @@ [Defines]
>
> [Packages]
> ArmPlatformPkg/ArmPlatformPkg.dec
> - EmbeddedPkg/EmbeddedPkg.dec
> - MdePkg/MdePkg.dec
> MdeModulePkg/MdeModulePkg.dec
> + MdePkg/MdePkg.dec
> Silicon/Broadcom/Bcm283x/Bcm283x.dec
>
> [LibraryClasses]
> @@ -32,6 +31,8 @@ [LibraryClasses]
>
> [Sources]
> DualSerialPortLib.c
> + DualSerialPortLib.h
> + DualSerialPortLibCommon.c
>
> [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth ## SOMETIMES_CONSUMES
> diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h
> new file mode 100644
> index 000000000000..a8d150f516b9
> --- /dev/null
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.h
> @@ -0,0 +1,82 @@
> +/** @file
> + 16550 and PL011 Serial Port library functions for Raspberry Pi
> +
> + 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>
> + Copyright (c) 2012 - 2016, ARM Ltd. All rights reserved.<BR>
> + Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> + Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <IndustryStandard/Bcm2836.h>
> +#include <IndustryStandard/Bcm2836Gpio.h>
> +
> +#define PL011_UART_REGISTER_BASE BCM2836_PL011_UART_BASE_ADDRESS
> +#define MINI_UART_REGISTER_BASE (BCM2836_MINI_UART_BASE_ADDRESS + 0x40)
> +
> +//
> +// 16550 UART register offsets and bitfields
> +//
> +#define R_UART_RXBUF 0 // LCR_DLAB = 0
> +#define R_UART_TXBUF 0 // LCR_DLAB = 0
> +#define R_UART_BAUD_LOW 0 // LCR_DLAB = 1
> +#define R_UART_BAUD_HIGH 1 // LCR_DLAB = 1
> +#define R_UART_IER 1 // LCR_DLAB = 0
> +#define R_UART_FCR 2
> +#define B_UART_FCR_FIFOE BIT0
> +#define B_UART_FCR_FIFO64 BIT5
> +#define R_UART_LCR 3
> +#define B_UART_LCR_DLAB BIT7
> +#define R_UART_MCR 4
> +#define B_UART_MCR_DTRC BIT0
> +#define B_UART_MCR_RTS BIT1
> +#define R_UART_LSR 5
> +#define B_UART_LSR_RXRDY BIT0
> +#define B_UART_LSR_TXRDY BIT5
> +#define B_UART_LSR_TEMT BIT6
> +#define R_UART_MSR 6
> +#define B_UART_MSR_CTS BIT4
> +#define B_UART_MSR_DSR BIT5
> +#define B_UART_MSR_RI BIT6
> +#define B_UART_MSR_DCD BIT7
> +
> +extern BOOLEAN UsePl011Uart;
> +extern BOOLEAN UsePl011UartSet;
> +
> +/**
> + Read an 8-bit 16550 register.
> +
> + @param Base The base address register of UART device.
> + @param Offset The offset of the 16550 register to read.
> +
> + @return The value read from the 16550 register.
> +
> +**/
> +UINT8
> +SerialPortReadRegister (
> + UINTN Base,
> + UINTN Offset
> + );
> +
> +/**
> + Write an 8-bit 16550 register.
> +
> + @param Base The base address register of UART device.
> + @param Offset The offset of the 16550 register to write.
> + @param Value The value to write to the 16550 register specified by Offset.
> +
> + @return The value written to the 16550 register.
> +
> +**/
> +UINT8
> +SerialPortWriteRegister (
> + UINTN Base,
> + UINTN Offset,
> + UINT8 Value
> + );
> +
There's an extra trailing space here that could be removed.
> diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
> index 05e12f383785..b1d17d3fa04a 100644
> --- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
> @@ -13,132 +13,13 @@
> **/
>
> #include <Base.h>
> -#include <IndustryStandard/Bcm2836.h>
> -#include <IndustryStandard/Bcm2836Gpio.h>
> -#include <Library/BaseLib.h>
> #include <Library/IoLib.h>
> #include <Library/PcdLib.h>
> #include <Library/PL011UartClockLib.h>
> #include <Library/PL011UartLib.h>
> #include <Library/SerialPortLib.h>
>
> -BOOLEAN UsePl011Uart = FALSE;
> -BOOLEAN UsePl011UartSet = FALSE;
> -
> -#define PL011_UART_REGISTER_BASE BCM2836_PL011_UART_BASE_ADDRESS
> -#define MINI_UART_REGISTER_BASE (BCM2836_MINI_UART_BASE_ADDRESS + 0x40)
> -
> -//
> -// 16550 UART register offsets and bitfields
> -//
> -#define R_UART_RXBUF 0 // LCR_DLAB = 0
> -#define R_UART_TXBUF 0 // LCR_DLAB = 0
> -#define R_UART_BAUD_LOW 0 // LCR_DLAB = 1
> -#define R_UART_BAUD_HIGH 1 // LCR_DLAB = 1
> -#define R_UART_IER 1 // LCR_DLAB = 0
> -#define R_UART_FCR 2
> -#define B_UART_FCR_FIFOE BIT0
> -#define B_UART_FCR_FIFO64 BIT5
> -#define R_UART_LCR 3
> -#define B_UART_LCR_DLAB BIT7
> -#define R_UART_MCR 4
> -#define B_UART_MCR_DTRC BIT0
> -#define B_UART_MCR_RTS BIT1
> -#define R_UART_LSR 5
> -#define B_UART_LSR_RXRDY BIT0
> -#define B_UART_LSR_TXRDY BIT5
> -#define B_UART_LSR_TEMT BIT6
> -#define R_UART_MSR 6
> -#define B_UART_MSR_CTS BIT4
> -#define B_UART_MSR_DSR BIT5
> -#define B_UART_MSR_RI BIT6
> -#define B_UART_MSR_DCD BIT7
> -
> -/**
> - Read an 8-bit 16550 register.
> -
> - @param Base The base address register of UART device.
> - @param Offset The offset of the 16550 register to read.
> -
> - @return The value read from the 16550 register.
> -
> -**/
> -UINT8
> -SerialPortReadRegister (
> - UINTN Base,
> - UINTN Offset
> - )
> -{
> - return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
> -}
> -
> -/**
> - Write an 8-bit 16550 register.
> -
> - @param Base The base address register of UART device.
> - @param Offset The offset of the 16550 register to write.
> - @param Value The value to write to the 16550 register specified by Offset.
> -
> - @return The value written to the 16550 register.
> -
> -**/
> -UINT8
> -SerialPortWriteRegister (
> - UINTN Base,
> - UINTN Offset,
> - UINT8 Value
> - )
> -{
> - return MmioWrite8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride), Value);
> -}
> -
> -/**
> - Return whether the hardware flow control signal allows writing.
> -
> - @param SerialRegisterBase The base address register of UART device.
> -
> - @retval TRUE The serial port is writable.
> - @retval FALSE The serial port is not writable.
> -**/
> -BOOLEAN
> -SerialPortWritable (
> - UINTN SerialRegisterBase
> - )
> -{
> - if (PcdGetBool (PcdSerialUseHardwareFlowControl)) {
> - if (PcdGetBool (PcdSerialDetectCable)) {
> - //
> - // Wait for both DSR and CTS to be set
> - // DSR is set if a cable is connected.
> - // CTS is set if it is ok to transmit data
> - //
> - // DSR CTS Description Action
> - // === === ======================================== ========
> - // 0 0 No cable connected. Wait
> - // 0 1 No cable connected. Wait
> - // 1 0 Cable connected, but not clear to send. Wait
> - // 1 1 Cable connected, and clear to send. Transmit
> - //
> - return (BOOLEAN) ((SerialPortReadRegister (SerialRegisterBase, R_UART_MSR) & (B_UART_MSR_DSR | B_UART_MSR_CTS)) == (B_UART_MSR_DSR | B_UART_MSR_CTS));
> - } else {
> - //
> - // Wait for both DSR and CTS to be set OR for DSR to be clear.
> - // DSR is set if a cable is connected.
> - // CTS is set if it is ok to transmit data
> - //
> - // DSR CTS Description Action
> - // === === ======================================== ========
> - // 0 0 No cable connected. Transmit
> - // 0 1 No cable connected. Transmit
> - // 1 0 Cable connected, but not clear to send. Wait
> - // 1 1 Cable connected, and clar to send. Transmit
> - //
> - return (BOOLEAN) ((SerialPortReadRegister (SerialRegisterBase, R_UART_MSR) & (B_UART_MSR_DSR | B_UART_MSR_CTS)) != (B_UART_MSR_DSR));
> - }
> - }
> -
> - return TRUE;
> -}
> +#include "DualSerialPortLib.h"
>
> /**
> Return the baud generator divisor to use for 16650 setup.
> @@ -147,6 +28,7 @@ SerialPortWritable (
>
> @return The baud generator divisor.
> **/
> +STATIC
> UINT32
> SerialPortGetDivisor (
> UINT32 SerialBaudRate
> @@ -288,113 +170,6 @@ SerialPortInitialize (
> }
> }
>
> -/**
> - Write data from buffer to serial device.
> -
> - Writes NumberOfBytes data bytes from Buffer to the serial device.
> - The number of bytes actually written to the serial device is returned.
> - If the return value is less than NumberOfBytes, then the write operation failed.
> -
> - If Buffer is NULL, then ASSERT().
> -
> - If NumberOfBytes is zero, then return 0.
> -
> - @param Buffer Pointer to the data buffer to be written.
> - @param NumberOfBytes Number of bytes to written to the serial device.
> -
> - @retval 0 NumberOfBytes is 0.
> - @retval >0 The number of bytes written to the serial device.
> - If this value is less than NumberOfBytes, then the write operation failed.
> -
> -**/
> -UINTN
> -EFIAPI
> -SerialPortWrite (
> - IN UINT8 *Buffer,
> - IN UINTN NumberOfBytes
> - )
> -{
> - UINTN SerialRegisterBase;
> - UINTN Result;
> - UINTN Index;
> - UINTN FifoSize;
> -
> - //
> - // Serial writes may happen *before* the UART has been initialized
> - // and if we use the wrong UART then, all kind of bad things happen.
> - // To alleviate this, we add UART detection in SerialPortWrite and
> - // guard the UART detection with a second boolean.
> - //
> - if (!UsePl011UartSet) {
> - UsePl011Uart = ((MmioRead32(GPIO_BASE_ADDRESS + 4) & 0x0003F000) == 0x00024000);
> - UsePl011UartSet = TRUE;
> - }
> -
> - if (UsePl011Uart) {
> - return PL011UartWrite (PL011_UART_REGISTER_BASE, Buffer, NumberOfBytes);
> - } else {
> - if (Buffer == NULL) {
> - return 0;
> - }
> -
> - SerialRegisterBase = MINI_UART_REGISTER_BASE;
> -
> - if (NumberOfBytes == 0) {
> - //
> - // Flush the hardware
> - //
> -
> - //
> - // Wait for both the transmit FIFO and shift register empty.
> - //
> - while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
> -
> - //
> - // Wait for the hardware flow control signal
> - //
> - while (!SerialPortWritable (SerialRegisterBase));
> - return 0;
> - }
> -
> - //
> - // Compute the maximum size of the Tx FIFO
> - //
> - FifoSize = 1;
> - if ((PcdGet8 (PcdSerialFifoControl) & B_UART_FCR_FIFOE) != 0) {
> - if ((PcdGet8 (PcdSerialFifoControl) & B_UART_FCR_FIFO64) == 0) {
> - FifoSize = 16;
> - } else {
> - FifoSize = PcdGet32 (PcdSerialExtendedTxFifoSize);
> - }
> - }
> -
> - Result = NumberOfBytes;
> - while (NumberOfBytes != 0) {
> - //
> - // Wait for the serial port to be ready, to make sure both the transmit FIFO
> - // and shift register empty.
> - //
> - while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
> -
> - //
> - // Fill then entire Tx FIFO
> - //
> - for (Index = 0; Index < FifoSize && NumberOfBytes != 0; Index++, NumberOfBytes--, Buffer++) {
> - //
> - // Wait for the hardware flow control signal
> - //
> - while (!SerialPortWritable (SerialRegisterBase));
> -
> - //
> - // Write byte to the transmit buffer.
> - //
> - SerialPortWriteRegister (SerialRegisterBase, R_UART_TXBUF, *Buffer);
> - }
> - }
> - return Result;
> - }
> -}
> -
> /**
> Reads data from a serial device into a buffer.
>
> diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c
> new file mode 100644
> index 000000000000..48d8df280640
> --- /dev/null
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibCommon.c
> @@ -0,0 +1,218 @@
> +/** @file
> + 16550 and PL011 Serial Port library functions for Raspberry Pi
> +
> + 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>
> + Copyright (c) 2012 - 2016, ARM Ltd. All rights reserved.<BR>
> + Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> + Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PL011UartLib.h>
> +#include <Library/SerialPortLib.h>
> +
> +#include "DualSerialPortLib.h"
> +
> +BOOLEAN UsePl011Uart = FALSE;
> +BOOLEAN UsePl011UartSet = FALSE;
> +
> +/**
> + Read an 8-bit 16550 register.
> +
> + @param Base The base address register of UART device.
> + @param Offset The offset of the 16550 register to read.
> +
> + @return The value read from the 16550 register.
> +
> +**/
> +UINT8
> +SerialPortReadRegister (
> + UINTN Base,
> + UINTN Offset
> + )
> +{
> + return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
> +}
> +
> +/**
> + Write an 8-bit 16550 register.
> +
> + @param Base The base address register of UART device.
> + @param Offset The offset of the 16550 register to write.
> + @param Value The value to write to the 16550 register specified by Offset.
> +
> + @return The value written to the 16550 register.
> +
> +**/
> +UINT8
> +SerialPortWriteRegister (
> + UINTN Base,
> + UINTN Offset,
> + UINT8 Value
> + )
> +{
> + return MmioWrite8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride), Value);
> +}
> +
> +/**
> + Return whether the hardware flow control signal allows writing.
> +
> + @param SerialRegisterBase The base address register of UART device.
> +
> + @retval TRUE The serial port is writable.
> + @retval FALSE The serial port is not writable.
> +**/
> +STATIC
> +BOOLEAN
> +SerialPortWritable (
> + UINTN SerialRegisterBase
> + )
> +{
> + if (PcdGetBool (PcdSerialUseHardwareFlowControl)) {
> + if (PcdGetBool (PcdSerialDetectCable)) {
> + //
> + // Wait for both DSR and CTS to be set
> + // DSR is set if a cable is connected.
> + // CTS is set if it is ok to transmit data
> + //
> + // DSR CTS Description Action
> + // === === ======================================== ========
> + // 0 0 No cable connected. Wait
> + // 0 1 No cable connected. Wait
> + // 1 0 Cable connected, but not clear to send. Wait
> + // 1 1 Cable connected, and clear to send. Transmit
> + //
> + return (BOOLEAN) ((SerialPortReadRegister (SerialRegisterBase, R_UART_MSR) & (B_UART_MSR_DSR | B_UART_MSR_CTS)) == (B_UART_MSR_DSR | B_UART_MSR_CTS));
> + } else {
> + //
> + // Wait for both DSR and CTS to be set OR for DSR to be clear.
> + // DSR is set if a cable is connected.
> + // CTS is set if it is ok to transmit data
> + //
> + // DSR CTS Description Action
> + // === === ======================================== ========
> + // 0 0 No cable connected. Transmit
> + // 0 1 No cable connected. Transmit
> + // 1 0 Cable connected, but not clear to send. Wait
> + // 1 1 Cable connected, and clar to send. Transmit
> + //
> + return (BOOLEAN) ((SerialPortReadRegister (SerialRegisterBase, R_UART_MSR) & (B_UART_MSR_DSR | B_UART_MSR_CTS)) != (B_UART_MSR_DSR));
> + }
> + }
> +
> + return TRUE;
> +}
> +
> +/**
> + Write data from buffer to serial device.
> +
> + Writes NumberOfBytes data bytes from Buffer to the serial device.
> + The number of bytes actually written to the serial device is returned.
> + If the return value is less than NumberOfBytes, then the write operation failed.
> +
> + If Buffer is NULL, then ASSERT().
> +
> + If NumberOfBytes is zero, then return 0.
> +
> + @param Buffer Pointer to the data buffer to be written.
> + @param NumberOfBytes Number of bytes to written to the serial device.
> +
> + @retval 0 NumberOfBytes is 0.
> + @retval >0 The number of bytes written to the serial device.
> + If this value is less than NumberOfBytes, then the write operation failed.
> +
> +**/
> +UINTN
> +EFIAPI
> +SerialPortWrite (
> + IN UINT8 *Buffer,
> + IN UINTN NumberOfBytes
> + )
> +{
> + UINTN SerialRegisterBase;
> + UINTN Result;
> + UINTN Index;
> + UINTN FifoSize;
> +
> + //
> + // Serial writes may happen *before* the UART has been initialized
> + // and if we use the wrong UART then, all kind of bad things happen.
> + // To alleviate this, we add UART detection in SerialPortWrite and
> + // guard the UART detection with a second boolean.
> + //
> + if (!UsePl011UartSet) {
> + UsePl011Uart = ((MmioRead32(GPIO_BASE_ADDRESS + 4) & 0x0003F000) == 0x00024000);
> + UsePl011UartSet = TRUE;
> + }
> +
> + if (UsePl011Uart) {
> + return PL011UartWrite (PL011_UART_REGISTER_BASE, Buffer, NumberOfBytes);
> + } else {
> + if (Buffer == NULL) {
> + return 0;
> + }
> +
> + SerialRegisterBase = MINI_UART_REGISTER_BASE;
> +
> + if (NumberOfBytes == 0) {
> + //
> + // Flush the hardware
> + //
> +
> + //
> + // Wait for both the transmit FIFO and shift register empty.
> + //
> + while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
> +
> + //
> + // Wait for the hardware flow control signal
> + //
> + while (!SerialPortWritable (SerialRegisterBase));
> + return 0;
> + }
> +
> + //
> + // Compute the maximum size of the Tx FIFO
> + //
> + FifoSize = 1;
> + if ((PcdGet8 (PcdSerialFifoControl) & B_UART_FCR_FIFOE) != 0) {
> + if ((PcdGet8 (PcdSerialFifoControl) & B_UART_FCR_FIFO64) == 0) {
> + FifoSize = 16;
> + } else {
> + FifoSize = PcdGet32 (PcdSerialExtendedTxFifoSize);
> + }
> + }
> +
> + Result = NumberOfBytes;
> + while (NumberOfBytes != 0) {
> + //
> + // Wait for the serial port to be ready, to make sure both the transmit FIFO
> + // and shift register empty.
> + //
> + while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
> +
> + //
> + // Fill then entire Tx FIFO
> + //
> + for (Index = 0; Index < FifoSize && NumberOfBytes != 0; Index++, NumberOfBytes--, Buffer++) {
> + //
> + // Wait for the hardware flow control signal
> + //
> + while (!SerialPortWritable (SerialRegisterBase));
> +
> + //
> + // Write byte to the transmit buffer.
> + //
> + SerialPortWriteRegister (SerialRegisterBase, R_UART_TXBUF, *Buffer);
> + }
> + }
> + return Result;
> + }
> +}
>
Reviewed-by: Pete Batard <pete@akeo.ie>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib
2020-05-05 14:50 ` [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib Ard Biesheuvel
@ 2020-05-06 10:18 ` Pete Batard
0 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2020-05-06 10:18 UTC (permalink / raw)
To: Ard Biesheuvel, devel; +Cc: leif, andrey.warkentin
On 2020.05.05 15:50, Ard Biesheuvel wrote:
> On DEBUG builds that use the serial port directly for debug output,
> every module reinitializes the UART hardware, through the DebugLib
> constructor calling SerialPortInitialize.
>
> This is unnecessary, but usually harmless. However, in cases where this
> requires information that is non-trivial to obtain (e.g., the rate of
> the clock source feeding the baud clock), it results in a special kind
> of dependency hell that can only be fully appreciated by seasoned EDK2
> connoisseurs [0].
>
> As a first step towards solving this mess, implement a special version
> of the Raspberry Pi dual serial port library that only implements the
> SerialPortInitialize() and SerialPortWrite() library functions, and make
> the former an empty stub. This makes it only suitable for use by modules
> that inherit a dependency on SerialPortLib via DebugLib, and requires us
> to ensure that the baud clock is programmed correctly by the SEC phase.
>
> Use this version of the library to satisfy all SerialPortLib dependencies
> except the ones in PrePi and in SerialDxe. These will retain the full
> version, which is the only one that still consumes PcdSerialClockRate.
>
> [0] There are two distinct problems making this mess almost unsolvable:
> - SerialPortInitialize() is called directly in various places instead
> of relying on constructor ordering, so adding a constructor to a
> SerialPortLib implementation does not help,
> - Constructor ordering resolution in the EDK2 tooling fails to take
> transitive dependencies into account if an intermediate library has
> no constructor it self. For instance, if LibA depends on LibB, which
> depends on LibC, the constructors of LibA and LibC could be called in
> any order if LibB does not have a constructor itself (and fixing this
> breaks all the platforms in the tree)
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> Platform/RaspberryPi/RPi3/RPi3.dsc | 12 +++--
> Platform/RaspberryPi/RPi4/RPi4.dsc | 12 +++--
> Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf | 46 ++++++++++++++++++++
> Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c | 28 ++++++++++++
> 4 files changed, 92 insertions(+), 6 deletions(-)
>
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> index 563fb891b841..d7218219fc5a 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> @@ -127,7 +127,7 @@ [LibraryClasses.common]
> # Dual serial port library
> PL011UartClockLib|ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf
> PL011UartLib|ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.inf
> - SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> + SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
>
> # Cryptographic libraries
> IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> @@ -521,7 +521,10 @@ [Components.common]
> #
> # PEI Phase modules
> #
> - ArmPlatformPkg/PrePi/PeiUniCore.inf
> + ArmPlatformPkg/PrePi/PeiUniCore.inf {
> + <LibraryClasses>
> + SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> + }
>
> #
> # DXE
> @@ -569,7 +572,10 @@ [Components.common]
> MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
> MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
> MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
> - MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
> + MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
> + <LibraryClasses>
> + SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> + }
> Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf
>
> MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index 4deccd9d3ecc..4fb015b077e6 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -127,7 +127,7 @@ [LibraryClasses.common]
> # Dual serial port library
> PL011UartClockLib|ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf
> PL011UartLib|ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.inf
> - SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> + SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
>
> # Cryptographic libraries
> IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> @@ -536,7 +536,10 @@ [Components.common]
> #
> # PEI Phase modules
> #
> - ArmPlatformPkg/PrePi/PeiUniCore.inf
> + ArmPlatformPkg/PrePi/PeiUniCore.inf {
> + <LibraryClasses>
> + SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> + }
>
> #
> # DXE
> @@ -584,7 +587,10 @@ [Components.common]
> MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
> MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
> MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
> - MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
> + MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
> + <LibraryClasses>
> + SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> + }
> Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf
> EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
>
> diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf b/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
> new file mode 100644
> index 000000000000..cd14d44c59d8
> --- /dev/null
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.inf
> @@ -0,0 +1,46 @@
> +## @file
> +#
> +# SerialPortLib instance for both PL011 and 16550 UART that satisfies
> +# only the dependencies of DebugLib.
> +#
> +# Copyright (c) 2020, Pete Batard <pete@akeo.ie>
> +# Copyright (c) 2012 - 2020, ARM Ltd. All rights reserved.<BR>
> +# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 1.27
> + BASE_NAME = DebugDualSerialPortLib
> + FILE_GUID = 323bae1b-c2fc-4929-a2fe-9e9174f8ce0f
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = SerialPortLib
> +
> +[Packages]
> + ArmPlatformPkg/ArmPlatformPkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + MdePkg/MdePkg.dec
> + Silicon/Broadcom/Bcm283x/Bcm283x.dec
> +
> +[LibraryClasses]
> + IoLib
> + PcdLib
> + PL011UartLib
> +
> +[Sources]
> + DebugDualSerialPortLib.c
> + DualSerialPortLib.h
> + DualSerialPortLibCommon.c
> +
> +[Pcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ## CONSUMES
> +
> +[FixedPcd]
> + gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress ## CONSUMES
> diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c
> new file mode 100644
> index 000000000000..fbb9fde08bac
> --- /dev/null
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DebugDualSerialPortLib.c
> @@ -0,0 +1,28 @@
> +/** @file
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/SerialPortLib.h>
> +
> +/**
> + Initialize the serial device hardware.
> +
> + If no initialization is required, then return RETURN_SUCCESS.
> + If the serial device was successfully initialized, then return RETURN_SUCCESS.
> + If the serial device could not be initialized, then return RETURN_DEVICE_ERROR.
> +
> + @retval RETURN_SUCCESS The serial device was initialized.
> + @retval RETURN_DEVICE_ERROR The serial device could not be initialized.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SerialPortInitialize (
> + VOID
> + )
> +{
> + return RETURN_SUCCESS;
> +}
>
Reviewed-by: Pete Batard <pete@akeo.ie>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic
2020-05-05 18:10 ` Ard Biesheuvel
@ 2020-05-06 10:18 ` Pete Batard
2020-05-06 10:25 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Pete Batard @ 2020-05-06 10:18 UTC (permalink / raw)
To: devel, ard.biesheuvel; +Cc: leif, andrey.warkentin
One general remark below:
On 2020.05.05 19:10, Ard Biesheuvel wrote:
> On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
>> The 16550 'miniUART' on the Raspberry Pi gets its input clock from
>> different sources on RPi3 and RPi3. Fix the logic that derives the
>
> This should be 'Rpi3 and RPi4'
>
>> divisor for the 16550 baud clock on the respective platforms.
>>
>> While at it, make the input clock PCD patchable for RPi3 so we can
>> manipulate it at runtime in a future patch.
For the sake of harmonization between platforms, wouldn't we want to
also declare the PCD patchable on RPi4 as well (even though we obviously
aren't going to patch it for the time being)?
There is some plan of factorizing the DSC content, so if that doesn't
hurt us, I'd propose we try to keep the Pi3 and Pi4 version as close as
possible.
This also ties in with my remark for the next patch.
>>
>> Co-authored-by: Pete Batard <pete@akeo.ie>
>> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
>> Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>> Platform/RaspberryPi/RPi3/RPi3.dsc
>> | 4 +++-
>> Platform/RaspberryPi/RPi4/RPi4.dsc
>> | 2 +-
>> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c |
>> 14 ++++++++------
>> 3 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc
>> b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> index d7218219fc5a..96b27400eef8 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> @@ -409,7 +409,6 @@ [PcdsFixedAtBuild.common]
>> gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
>> - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
>> gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
>> @@ -441,6 +440,9 @@ [PcdsFixedAtBuild.common]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>> +[PcdsPatchableInModule]
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|250000000
>> +
>> [PcdsDynamicHii.common.DEFAULT]
>> #
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> index 4fb015b077e6..5d8bd88d7e34 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> @@ -420,7 +420,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/Library/DualSerialPortLib/DualSerialPortLib.c
>> b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>> index b1d17d3fa04a..5e83bbf022eb 100644
>> --- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>> @@ -41,18 +41,20 @@ SerialPortGetDivisor (
>> // 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.
>> //
>> - BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate) * 4;
>> + BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);
>> +#if (RPI_MODEL == 4)
>> Divisor = MmioRead32(BCM2836_CM_BASE +
>> BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
>> if (Divisor != 0)
>> BaseClockRate = (BaseClockRate << 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)BaseClockRate / (SerialBaudRate * 8);
>> + if (Divisor != 0) {
>> + Divisor--;
>> }
>> return Divisor;
>> }
>>
>
>
>
>
Reviewed-by: Pete Batard <pete@akeo.ie>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot
2020-05-05 18:11 ` Ard Biesheuvel
@ 2020-05-06 10:18 ` Pete Batard
2020-05-06 10:31 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Pete Batard @ 2020-05-06 10:18 UTC (permalink / raw)
To: devel, ard.biesheuvel; +Cc: leif, andrey.warkentin
One remark below:
On 2020.05.05 19:11, Ard Biesheuvel wrote:
> On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
>> Query the firmware for the clock rate that is used to drive the
>> 16550 baud clock, so that we can program the correct baud rate.
>>
>> Co-authored-by: Pete Batard <pete@akeo.ie>
>> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
>> Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>> Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>> | 25 +++++++++++++++++++-
>> 1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git
>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>> b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>> index 91dfe1bb981e..35580e4ed73a 100644
>> ---
>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>> +++
>> b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>> @@ -3,7 +3,7 @@
>> * 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.
>> + * Copyright (c) 2011-2020, ARM Limited. All rights reserved.
>> *
>> * SPDX-License-Identifier: BSD-2-Clause-Patent
>> *
>> @@ -85,6 +85,14 @@ ASM_FUNC (ArmPlatformPeiBootAction)
>> adr x2, mBoardRevision
>> str w0, [x2]
>> +#if (RPI_MODEL == 3)
>
> As noted by Pete off-list, doing this doesn't work unless we add
> something like
>
> GCC:*_*_*_PP_FLAGS = -DRPI_MODEL=3
>
> to the [BuildOptions] in RPi3.dsc
>
>
>
>> + run .Lclkinfo_buffer
>> +
>> + ldr w0, .Lfrequency
>> + adrp x2, _gPcd_BinaryPatch_PcdSerialClockRate
>> + str w0, [x2, :lo12:_gPcd_BinaryPatch_PcdSerialClockRate]
>> +#endif
>> +
Since we're modifying a patchable PCD here, shouldn't we add a:
[PatchPcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate
section in PlatformLib.inf?
Of course, if we do that, we can't keep PcdSerialClockRate fixed for
RPi4, as the build process will complain about PCD mismatch.
I also wouldn't mind a comment that explains how one arrives at figuring
out that "_gPcd_BinaryPatch_PcdSerialClockRate" should be used to locate
our address (and possibly the addition of :lo12:), because I don't think
it's going to be that straightforward for people reading the code for
the first time, though I fear that the explanation will boil down to "we
need to do it this specific way for a gcc aarch64 relocation"...
>> ret
>> .align 4
>> @@ -127,6 +135,21 @@ ASM_FUNC (ArmPlatformPeiBootAction)
>> .long 0 // end tag
>> .set .Lrevinfo_size, . - .Lrevinfo_buffer
>> +#if (RPI_MODEL == 3)
>> + .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
>> +#endif
>> +
>> //UINTN
>> //ArmPlatformGetPrimaryCoreMpId (
>> // VOID
>>
>
>
>
>
With addition of "GCC:*_*_*_PP_FLAGS = -DRPI_MODEL=3" in the .dsc:
Reviewed-by: Pete Batard <pete@akeo.ie>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH edk2-platforms 5/5] Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3
2020-05-05 14:50 ` [PATCH edk2-platforms 5/5] Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3 Ard Biesheuvel
@ 2020-05-06 10:19 ` Pete Batard
0 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2020-05-06 10:19 UTC (permalink / raw)
To: Ard Biesheuvel, devel; +Cc: leif, andrey.warkentin
On 2020.05.05 15:50, Ard Biesheuvel wrote:
> The Raspberry Pi 3 derives its 16550 baud clock from the variable core
> clock, and so any reprogramming of the baud rate needs to take the
> actual core clock value into account.
>
> Introduce a DXE phase version of DualSerialPortLib that discovers this
> value in its constructor, using the RPi firmware protocol, and wire it
> up for the RPi3 platform.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> Platform/RaspberryPi/RPi3/RPi3.dsc | 2 +-
> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf | 67 ++++++++++++++++++++
> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c | 40 ++++++++++++
> 3 files changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> index 96b27400eef8..2b8ad1c4bdbd 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> @@ -576,7 +576,7 @@ [Components.common]
> MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
> MdeModulePkg/Universal/SerialDxe/SerialDxe.inf {
> <LibraryClasses>
> - SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.inf
> + SerialPortLib|Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf
> }
> Platform/RaspberryPi/Drivers/DisplayDxe/DisplayDxe.inf
>
> diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf
> new file mode 100644
> index 000000000000..4c22b39daa7f
> --- /dev/null
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortDxeLib.inf
> @@ -0,0 +1,67 @@
> +## @file
> +#
> +# DXE phase SerialPortLib instance for both PL011 and 16550 UART.
> +#
> +# Copyright (c) 2020, Pete Batard <pete@akeo.ie>
> +# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 1.27
> + BASE_NAME = DualSerialPortDxeLib
> + FILE_GUID = d586667e-ec50-4bf6-9701-fb4e29055a60
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = SerialPortLib|DXE_DRIVER
> + CONSTRUCTOR = DualSerialPortDxeLibConstructor
> +
> +[Packages]
> + ArmPlatformPkg/ArmPlatformPkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + MdePkg/MdePkg.dec
> + Platform/RaspberryPi/RaspberryPi.dec
> + Silicon/Broadcom/Bcm283x/Bcm283x.dec
> +
> +[LibraryClasses]
> + IoLib
> + PcdLib
> + PL011UartClockLib
> + PL011UartLib
> +
> +[Sources]
> + DualSerialPortLib.c
> + DualSerialPortLib.h
> + DualSerialPortLibCommon.c
> + DualSerialPortLibConstructor.c
> +
> +[Pcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth ## SOMETIMES_CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable ## SOMETIMES_CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ## CONSUMES
> +
> +[FixedPcd]
> + gArmPlatformTokenSpaceGuid.PL011UartClkInHz
> + gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
> +
> +[Protocols]
> + gRaspberryPiFirmwareProtocolGuid ## CONSUMES
> +
> +[PatchPcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
> +
> +[Depex]
> + gRaspberryPiFirmwareProtocolGuid
> diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c
> new file mode 100644
> index 000000000000..c6d695181ab7
> --- /dev/null
> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLibConstructor.c
> @@ -0,0 +1,40 @@
> +/** @file
> +
> + Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <IndustryStandard/RpiMbox.h>
> +#include <Library/SerialPortLib.h>
> +#include <Library/PcdLib.h>
> +#include <Protocol/RpiFirmware.h>
> +
> +EFI_STATUS
> +EFIAPI
> +DualSerialPortDxeLibConstructor (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + RASPBERRY_PI_FIRMWARE_PROTOCOL *Firmware;
> + UINT32 ClockRate;
> + EFI_STATUS Status;
> +
> + Status = SystemTable->BootServices->LocateProtocol (
> + &gRaspberryPiFirmwareProtocolGuid,
> + NULL, (VOID **)&Firmware);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + Status = Firmware->GetClockRate (RPI_MBOX_CLOCK_RATE_CORE, &ClockRate);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + PatchPcdSet32 (PcdSerialClockRate, ClockRate);
> + return EFI_SUCCESS;
> +}
>
Reviewed-by: Pete Batard <pete@akeo.ie>
For the series:
Tested-by: Pete Batard <pete@akeo.ie>
Tested-on-platforms: RPi3 Model B+, RPi4 Model B
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic
2020-05-06 10:18 ` [edk2-devel] " Pete Batard
@ 2020-05-06 10:25 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-06 10:25 UTC (permalink / raw)
To: Pete Batard, devel; +Cc: leif, andrey.warkentin
On 5/6/20 12:18 PM, Pete Batard wrote:
> One general remark below:
>
> On 2020.05.05 19:10, Ard Biesheuvel wrote:
>> On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
>>> The 16550 'miniUART' on the Raspberry Pi gets its input clock from
>>> different sources on RPi3 and RPi3. Fix the logic that derives the
>>
>> This should be 'Rpi3 and RPi4'
>>
>>> divisor for the 16550 baud clock on the respective platforms.
>>>
>>> While at it, make the input clock PCD patchable for RPi3 so we can
>>> manipulate it at runtime in a future patch.
>
> For the sake of harmonization between platforms, wouldn't we want to
> also declare the PCD patchable on RPi4 as well (even though we obviously
> aren't going to patch it for the time being)?
>
I'd prefer not to. That way, we can easily spot problems when the
patching code is incorporated inadvertently.
> There is some plan of factorizing the DSC content, so if that doesn't
> hurt us, I'd propose we try to keep the Pi3 and Pi4 version as close as
> possible.
>
> This also ties in with my remark for the next patch.
>
>>>
>>> Co-authored-by: Pete Batard <pete@akeo.ie>
>>> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
>>> Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> ---
>>> Platform/RaspberryPi/RPi3/RPi3.dsc | 4 +++-
>>> Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
>>> Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>>> | 14 ++++++++------
>>> 3 files changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc
>>> b/Platform/RaspberryPi/RPi3/RPi3.dsc
>>> index d7218219fc5a..96b27400eef8 100644
>>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
>>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
>>> @@ -409,7 +409,6 @@ [PcdsFixedAtBuild.common]
>>> gArmPlatformTokenSpaceGuid.PL011UartClkInHz|48000000
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride|4
>>> - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|500000000
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|0x27
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|8
>>> gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
>>> @@ -441,6 +440,9 @@ [PcdsFixedAtBuild.common]
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>>> +[PcdsPatchableInModule]
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|250000000
>>> +
>>> [PcdsDynamicHii.common.DEFAULT]
>>> #
>>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc
>>> b/Platform/RaspberryPi/RPi4/RPi4.dsc
>>> index 4fb015b077e6..5d8bd88d7e34 100644
>>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
>>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
>>> @@ -420,7 +420,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/Library/DualSerialPortLib/DualSerialPortLib.c
>>> b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>>> index b1d17d3fa04a..5e83bbf022eb 100644
>>> --- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>>> +++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
>>> @@ -41,18 +41,20 @@ SerialPortGetDivisor (
>>> // 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.
>>> //
>>> - BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate) * 4;
>>> + BaseClockRate = (UINT64)PcdGet32 (PcdSerialClockRate);
>>> +#if (RPI_MODEL == 4)
>>> Divisor = MmioRead32(BCM2836_CM_BASE +
>>> BCM2836_CM_VPU_CLOCK_DIVISOR) & 0xFFFFFF;
>>> if (Divisor != 0)
>>> BaseClockRate = (BaseClockRate << 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)BaseClockRate / (SerialBaudRate * 8);
>>> + if (Divisor != 0) {
>>> + Divisor--;
>>> }
>>> return Divisor;
>>> }
>>>
>>
>>
>>
>>
>
> Reviewed-by: Pete Batard <pete@akeo.ie>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot
2020-05-06 10:18 ` [edk2-devel] " Pete Batard
@ 2020-05-06 10:31 ` Ard Biesheuvel
2020-05-06 10:38 ` Pete Batard
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-06 10:31 UTC (permalink / raw)
To: Pete Batard, devel; +Cc: leif, andrey.warkentin
On 5/6/20 12:18 PM, Pete Batard wrote:
> One remark below:
>
> On 2020.05.05 19:11, Ard Biesheuvel wrote:
>> On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
>>> Query the firmware for the clock rate that is used to drive the
>>> 16550 baud clock, so that we can program the correct baud rate.
>>>
>>> Co-authored-by: Pete Batard <pete@akeo.ie>
>>> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
>>> Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> ---
>>>
>>> Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>> | 25 +++++++++++++++++++-
>>> 1 file changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>>
>>> index 91dfe1bb981e..35580e4ed73a 100644
>>> ---
>>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>> +++
>>> b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>> @@ -3,7 +3,7 @@
>>> * 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.
>>> + * Copyright (c) 2011-2020, ARM Limited. All rights reserved.
>>> *
>>> * SPDX-License-Identifier: BSD-2-Clause-Patent
>>> *
>>> @@ -85,6 +85,14 @@ ASM_FUNC (ArmPlatformPeiBootAction)
>>> adr x2, mBoardRevision
>>> str w0, [x2]
>>> +#if (RPI_MODEL == 3)
>>
>> As noted by Pete off-list, doing this doesn't work unless we add
>> something like
>>
>> GCC:*_*_*_PP_FLAGS = -DRPI_MODEL=3
>>
>> to the [BuildOptions] in RPi3.dsc
>>
>>
>>
>>> + run .Lclkinfo_buffer
>>> +
>>> + ldr w0, .Lfrequency
>>> + adrp x2, _gPcd_BinaryPatch_PcdSerialClockRate
>>> + str w0, [x2, :lo12:_gPcd_BinaryPatch_PcdSerialClockRate]
>>> +#endif
>>> +
>
> Since we're modifying a patchable PCD here, shouldn't we add a:
>
> [PatchPcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate
>
> section in PlatformLib.inf?
>
We should add it, but we can add it to the [Pcd] section.
Alternatively, we could have different .INFs for RPi3 and RPi4, but that
is really overkill IMO.
Making it patchable on both platforms just to patch it on one is also
unnecessary I think. The current approach will ensure that we catch any
issues at build time, without any major hacks,].
> Of course, if we do that, we can't keep PcdSerialClockRate fixed for
> RPi4, as the build process will complain about PCD mismatch.
>
> I also wouldn't mind a comment that explains how one arrives at figuring
> out that "_gPcd_BinaryPatch_PcdSerialClockRate" should be used to locate
> our address (and possibly the addition of :lo12:), because I don't think
> it's going to be that straightforward for people reading the code for
> the first time, though I fear that the explanation will boil down to "we
> need to do it this specific way for a gcc aarch64 relocation"...
>
We don't actually need the adrp/str pair with the lo12 here, I will
replace it with adr. (Just muscle memory)
>>> ret
>>> .align 4
>>> @@ -127,6 +135,21 @@ ASM_FUNC (ArmPlatformPeiBootAction)
>>> .long 0 // end tag
>>> .set .Lrevinfo_size, . - .Lrevinfo_buffer
>>> +#if (RPI_MODEL == 3)
>>> + .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
>>> +#endif
>>> +
>>> //UINTN
>>> //ArmPlatformGetPrimaryCoreMpId (
>>> // VOID
>>>
>>
>>
>>
>>
>
> With addition of "GCC:*_*_*_PP_FLAGS = -DRPI_MODEL=3" in the .dsc:
> Reviewed-by: Pete Batard <pete@akeo.ie>
>
Thanks
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot
2020-05-06 10:31 ` Ard Biesheuvel
@ 2020-05-06 10:38 ` Pete Batard
0 siblings, 0 replies; 17+ messages in thread
From: Pete Batard @ 2020-05-06 10:38 UTC (permalink / raw)
To: Ard Biesheuvel, devel; +Cc: leif, andrey.warkentin
On 2020.05.06 11:31, Ard Biesheuvel wrote:
> On 5/6/20 12:18 PM, Pete Batard wrote:
>> One remark below:
>>
>> On 2020.05.05 19:11, Ard Biesheuvel wrote:
>>> On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
>>>> Query the firmware for the clock rate that is used to drive the
>>>> 16550 baud clock, so that we can program the correct baud rate.
>>>>
>>>> Co-authored-by: Pete Batard <pete@akeo.ie>
>>>> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
>>>> Co-authored-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>> ---
>>>> Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>>> | 25 +++++++++++++++++++-
>>>> 1 file changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git
>>>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>>> b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>>> index 91dfe1bb981e..35580e4ed73a 100644
>>>> ---
>>>> a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>>> +++
>>>> b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
>>>> @@ -3,7 +3,7 @@
>>>> * 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.
>>>> + * Copyright (c) 2011-2020, ARM Limited. All rights reserved.
>>>> *
>>>> * SPDX-License-Identifier: BSD-2-Clause-Patent
>>>> *
>>>> @@ -85,6 +85,14 @@ ASM_FUNC (ArmPlatformPeiBootAction)
>>>> adr x2, mBoardRevision
>>>> str w0, [x2]
>>>> +#if (RPI_MODEL == 3)
>>>
>>> As noted by Pete off-list, doing this doesn't work unless we add
>>> something like
>>>
>>> GCC:*_*_*_PP_FLAGS = -DRPI_MODEL=3
>>>
>>> to the [BuildOptions] in RPi3.dsc
>>>
>>>
>>>
>>>> + run .Lclkinfo_buffer
>>>> +
>>>> + ldr w0, .Lfrequency
>>>> + adrp x2, _gPcd_BinaryPatch_PcdSerialClockRate
>>>> + str w0, [x2, :lo12:_gPcd_BinaryPatch_PcdSerialClockRate]
>>>> +#endif
>>>> +
>>
>> Since we're modifying a patchable PCD here, shouldn't we add a:
>>
>> [PatchPcd]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate
>>
>> section in PlatformLib.inf?
>>
>
> We should add it, but we can add it to the [Pcd] section.
>
> Alternatively, we could have different .INFs for RPi3 and RPi4, but that
> is really overkill IMO.
>
> Making it patchable on both platforms just to patch it on one is also
> unnecessary I think. The current approach will ensure that we catch any
> issues at build time, without any major hacks,].
Yeah, I'm also fine with what we have at the moment, and I sure don't
want separate .INFs for Pi3 and Pi4.
>
>> Of course, if we do that, we can't keep PcdSerialClockRate fixed for
>> RPi4, as the build process will complain about PCD mismatch.
>>
>> I also wouldn't mind a comment that explains how one arrives at
>> figuring out that "_gPcd_BinaryPatch_PcdSerialClockRate" should be
>> used to locate our address (and possibly the addition of :lo12:),
>> because I don't think it's going to be that straightforward for people
>> reading the code for the first time, though I fear that the
>> explanation will boil down to "we need to do it this specific way for
>> a gcc aarch64 relocation"...
>>
>
> We don't actually need the adrp/str pair with the lo12 here, I will
> replace it with adr. (Just muscle memory)
Sounds good, thanks!
/Pete
>
>
>
>>>> ret
>>>> .align 4
>>>> @@ -127,6 +135,21 @@ ASM_FUNC (ArmPlatformPeiBootAction)
>>>> .long 0 // end tag
>>>> .set .Lrevinfo_size, . - .Lrevinfo_buffer
>>>> +#if (RPI_MODEL == 3)
>>>> + .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
>>>> +#endif
>>>> +
>>>> //UINTN
>>>> //ArmPlatformGetPrimaryCoreMpId (
>>>> // VOID
>>>>
>>>
>>>
>>>
>>>
>>
>> With addition of "GCC:*_*_*_PP_FLAGS = -DRPI_MODEL=3" in the .dsc:
>> Reviewed-by: Pete Batard <pete@akeo.ie>
>>
>
>
> Thanks
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell
2020-05-05 14:50 [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
` (4 preceding siblings ...)
2020-05-05 14:50 ` [PATCH edk2-platforms 5/5] Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3 Ard Biesheuvel
@ 2020-05-06 16:16 ` Ard Biesheuvel
5 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2020-05-06 16:16 UTC (permalink / raw)
To: devel; +Cc: leif, pete, andrey.warkentin
On 5/5/20 4:50 PM, Ard Biesheuvel wrote:
> This proposes an alternative approach to the issue described in
> https://github.com/raspberrypi/firmware/issues/1376.
>
> Instead of fiddling with HobLib internals and relying on headers being
> included in the correct order, this replaces all DEBUG uses of SerialPortLib
> with an implementation that doesn't reprogram the UART at all (and so does
> not need to know the value of the base clock), and updates the two remaining
> users to query the firmware for the correct value before touching the h/w.
>
> NOTE: build tested only.
>
> Ard Biesheuvel (5):
> Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse
> Platform/RaspberryPi: introduce DebugDualSerialPortLib
> Platform/RaspberryPi: fix 16550 divisor calculation logic
> Platform/RaspberryPi3: query firmware for 16550 input clock at boot
> Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3
>
I have pushed this as 11189124fbc2..644e223bb371 with Pete's ack, after
making the changes as discussed in the individual replies.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-06 16:16 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-05 14:50 [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
2020-05-05 14:50 ` [PATCH edk2-platforms 1/5] Platform/RaspberryPi/DualSerialPortLib: split up to ease reuse Ard Biesheuvel
2020-05-06 10:18 ` Pete Batard
2020-05-05 14:50 ` [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib Ard Biesheuvel
2020-05-06 10:18 ` Pete Batard
2020-05-05 14:50 ` [PATCH edk2-platforms 3/5] Platform/RaspberryPi: fix 16550 divisor calculation logic Ard Biesheuvel
2020-05-05 18:10 ` Ard Biesheuvel
2020-05-06 10:18 ` [edk2-devel] " Pete Batard
2020-05-06 10:25 ` Ard Biesheuvel
2020-05-05 14:50 ` [PATCH edk2-platforms 4/5] Platform/RaspberryPi3: query firmware for 16550 input clock at boot Ard Biesheuvel
2020-05-05 18:11 ` Ard Biesheuvel
2020-05-06 10:18 ` [edk2-devel] " Pete Batard
2020-05-06 10:31 ` Ard Biesheuvel
2020-05-06 10:38 ` Pete Batard
2020-05-05 14:50 ` [PATCH edk2-platforms 5/5] Platform/RaspberryPi: create DXE phase SerialPortLib version for RPi3 Ard Biesheuvel
2020-05-06 10:19 ` Pete Batard
2020-05-06 16:16 ` [PATCH edk2-platforms 0/5] Platform/RaspberryPi: fix serialportlib dependency hell Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox