Re: [PATCH 4/4] ArmPlatformPkg: add PL011 UART runtime debug driver

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 20 April 2018 at 16:14, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
> Leif, Ard
>
>
> When do you plan to push this patch?
>

I don't. There was some pushback from the Intel guys regarding the use
of status codes instead of a runtime debug protocol, and nobody came
forward to say that they wanted this.

If you are interested in this functionality, please respond to the 0/4
cover letter and explain why this approach is preferred over status
codes.

Thanks,
Ard.


> ________________________________
> From: edk2-devel <edk2-devel-bounces@lists.01.org> on behalf of Leif
> Lindholm <leif.lindholm@linaro.org>
> Sent: 15 March 2018 12:49:27
> To: Ard Biesheuvel
> Cc: eric.dong@intel.com; edk2-devel@lists.01.org; liming.gao@intel.com;
> heyi.guo@linaro.org; michael.d.kinney@intel.com; lersek@redhat.com;
> star.zeng@intel.com
> Subject: Re: [edk2] [PATCH 4/4] ArmPlatformPkg: add PL011 UART runtime debug
> driver
>
> On Thu, Mar 01, 2018 at 06:11:42PM +0000, Ard Biesheuvel wrote:
>> Implement the new runtime debug output protocol on top of a PL011 UART.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> LGTM
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
>> ---
>>
>> ArmPlatformPkg/Drivers/PL011RuntimeDebugOutputDxe/PL011RuntimeDebugOutputDxe.c
>> | 144 ++++++++++++++++++++
>>
>> ArmPlatformPkg/Drivers/PL011RuntimeDebugOutputDxe/PL011RuntimeDebugOutputDxe.inf
>> |  62 +++++++++
>>  2 files changed, 206 insertions(+)
>>
>> diff --git
>> a/ArmPlatformPkg/Drivers/PL011RuntimeDebugOutputDxe/PL011RuntimeDebugOutputDxe.c
>> b/ArmPlatformPkg/Drivers/PL011RuntimeDebugOutputDxe/PL011RuntimeDebugOutputDxe.c
>> new file mode 100644
>> index 000000000000..155b2c50d463
>> --- /dev/null
>> +++
>> b/ArmPlatformPkg/Drivers/PL011RuntimeDebugOutputDxe/PL011RuntimeDebugOutputDxe.c
>> @@ -0,0 +1,144 @@
>> +/** @file
>> +  Runtime driver to produce debug output on a PL011 UART
>> +
>> +  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
>> +
>> +  This program and the accompanying materials are licensed and made
>> available
>> +  under the terms and conditions of the BSD License which accompanies
>> this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> IMPLIED.
>> +
>> +**/
>> +
>> +#include <PiDxe.h>
>> +#include <Library/DxeServicesTableLib.h>
>> +#include <Library/PL011UartLib.h>
>> +#include <Library/UefiDriverEntryPoint.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiRuntimeLib.h>
>> +#include <Protocol/RuntimeDebugOutput.h>
>> +
>> +STATIC UINTN        mUartBase;
>> +STATIC EFI_EVENT    mVirtualAddressChangeEvent;
>> +
>> +/**
>> +  Write data from buffer to debug output device
>> +
>> +  Writes NumberOfBytes data bytes from Buffer to the debug output device.
>> +  The number of bytes actually written to the device is returned.
>> +  If the return value is less than NumberOfBytes, then the write
>> operation
>> +  failed.
>> +  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 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.
>> +
>> +**/
>> +STATIC
>> +UINTN
>> +PL011RuntimeDebugOutputWrite (
>> +  IN  EDK2_RUNTIME_DEBUG_OUTPUT_PROTOCOL  *This,
>> +  IN  UINT8                               *Buffer,
>> +  IN  UINTN                               NumberOfBytes
>> +  )
>> +{
>> +  return PL011UartWrite (mUartBase, Buffer, NumberOfBytes);
>> +}
>> +
>> +STATIC EDK2_RUNTIME_DEBUG_OUTPUT_PROTOCOL mRuntimeDebugOutput = {
>> +  PL011RuntimeDebugOutputWrite
>> +};
>> +
>> +/**
>> +  Fixup internal data so that EFI can be called in virtual mode.
>> +
>> +  @param[in]    Event   The Event that is being processed
>> +  @param[in]    Context Event Context
>> +**/
>> +STATIC
>> +VOID
>> +EFIAPI
>> +VirtualNotifyEvent (
>> +  IN EFI_EVENT        Event,
>> +  IN VOID             *Context
>> +  )
>> +{
>> +  EfiConvertPointer (0x0, (VOID **)&mUartBase);
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +PL011RuntimeDebugOutputDxeEntry (
>> +  IN EFI_HANDLE        ImageHandle,
>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS          Status;
>> +  EFI_HANDLE          Handle;
>> +  UINT64              BaudRate;
>> +  UINT32              ReceiveFifoDepth;
>> +  EFI_PARITY_TYPE     Parity;
>> +  UINT8               DataBits;
>> +  EFI_STOP_BITS_TYPE  StopBits;
>> +
>> +  mUartBase        = (UINTN)FixedPcdGet64 (PcdSerialRegisterBase);
>> +  BaudRate         = FixedPcdGet64 (PcdUartDefaultBaudRate);
>> +  ReceiveFifoDepth = 0;         // Use default FIFO depth
>> +  Parity           = (EFI_PARITY_TYPE)FixedPcdGet8
>> (PcdUartDefaultParity);
>> +  DataBits         = FixedPcdGet8 (PcdUartDefaultDataBits);
>> +  StopBits         = (EFI_STOP_BITS_TYPE) FixedPcdGet8
>> (PcdUartDefaultStopBits);
>> +
>> +  Status = PL011UartInitializePort (mUartBase, FixedPcdGet32
>> (PL011UartClkInHz),
>> +             &BaudRate, &ReceiveFifoDepth, &Parity, &DataBits,
>> &StopBits);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  //
>> +  // Register for the virtual address change event
>> +  //
>> +  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_NOTIFY,
>> +                  VirtualNotifyEvent, NULL,
>> &gEfiEventVirtualAddressChangeGuid,
>> +                  &mVirtualAddressChangeEvent);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  //
>> +  // Declare the UART MMIO region as EFI_MEMORY_RUNTIME
>> +  //
>> +  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
>> mUartBase,
>> +                  SIZE_4KB, EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
>> +  if (EFI_ERROR (Status)) {
>> +    goto CloseEvent;
>> +  }
>> +
>> +  Status = gDS->SetMemorySpaceAttributes (mUartBase, SIZE_4KB,
>> +                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
>> +  if (EFI_ERROR (Status)) {
>> +    goto CloseEvent;
>> +  }
>> +
>> +  Handle = NULL;
>> +  Status = gBS->InstallMultipleProtocolInterfaces (&Handle,
>> +                  &gEdkiiRuntimeDebugOutputProtocolGuid,
>> &mRuntimeDebugOutput,
>> +                  NULL);
>> +  if (EFI_ERROR (Status)) {
>> +    goto CloseEvent;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +
>> +CloseEvent:
>> +  gBS->CloseEvent (mVirtualAddressChangeEvent);
>> +
>> +  return Status;
>> +}
>> diff --git
>> a/ArmPlatformPkg/Drivers/PL011RuntimeDebugOutputDxe/PL011RuntimeDebugOutputDxe.inf
>> b/ArmPlatformPkg/Drivers/PL011RuntimeDebugOutputDxe/PL011RuntimeDebugOutputDxe.inf
>> new file mode 100644
>> index 000000000000..28a8e514552e
>> --- /dev/null
>> +++
>> b/ArmPlatformPkg/Drivers/PL011RuntimeDebugOutputDxe/PL011RuntimeDebugOutputDxe.inf
>> @@ -0,0 +1,62 @@
>> +#/** @file
>> +#  Runtime driver to produce debug output on a PL011 UART
>> +#
>> +#  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
>> +#
>> +#  This program and the accompanying materials are licensed and made
>> available
>> +#  under the terms and conditions of the BSD License which accompanies
>> this
>> +#  distribution. The full text of the license may be found at
>> +#  http://opensource.org/licenses/bsd-license.php
>> +#
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> IMPLIED.
>> +#
>> +#
>> +#**/
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x0001001A
>> +  BASE_NAME                      = PL011RuntimeDebugOutputDxe
>> +  FILE_GUID                      = 494297ca-9205-463a-aae5-215ffd067cbb
>> +  MODULE_TYPE                    = DXE_RUNTIME_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  ENTRY_POINT                    = PL011RuntimeDebugOutputDxeEntry
>> +
>> +#
>> +# The following information is for reference only and not required by the
>> build tools.
>> +#
>> +#  VALID_ARCHITECTURES           = AARCH64 ARM
>> +#
>> +
>> +[Sources.common]
>> +  PL011RuntimeDebugOutputDxe.c
>> +
>> +[Packages]
>> +  ArmPlatformPkg/ArmPlatformPkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  MdePkg/MdePkg.dec
>> +
>> +[LibraryClasses]
>> +  DxeServicesTableLib
>> +  PcdLib
>> +  PL011UartLib
>> +  UefiBootServicesTableLib
>> +  UefiDriverEntryPoint
>> +  UefiRuntimeLib
>> +
>> +[Guids]
>> +  gEfiEventVirtualAddressChangeGuid             ## CONSUMES # Event
>> +
>> +[Protocols]
>> +  gEdkiiRuntimeDebugOutputProtocolGuid          ## PROTOCOL
>> ALWAYS_PRODUCED
>> +
>> +[FixedPcd]
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
>> +  gArmPlatformTokenSpaceGuid.PL011UartClkInHz
>> +
>> +[Depex]
>> +  TRUE
>> --
>> 2.11.0
>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.