public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
To: devel@edk2.groups.io
Cc: leif@nuviainc.com, pete@akeo.ie, andrey.warkentin@gmail.com,
	Ard Biesheuvel <ard.biesheuvel@arm.com>
Subject: [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib
Date: Tue,  5 May 2020 16:50:26 +0200	[thread overview]
Message-ID: <20200505145029.29826-3-ard.biesheuvel@arm.com> (raw)
In-Reply-To: <20200505145029.29826-1-ard.biesheuvel@arm.com>

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


  parent reply	other threads:[~2020-05-05 14:50 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 ` Ard Biesheuvel [this message]
2020-05-06 10:18   ` [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib 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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200505145029.29826-3-ard.biesheuvel@arm.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox