Re: [PATCH edk2-platforms 2/5] Platform/RaspberryPi: introduce DebugDualSerialPortLib

["Pete Batard" <pete@akeo.ie>]

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>