[edk2,4/6] ArmPlatformPkg: Add support to configure PL011 UART clock

From: Evan Lloyd <evan.lloyd@arm.com>


On some platforms the UART clock is not the same for all the serial
ports. The PL011 driver must be capable of handling serial ports with
different clock rates, so must not rely on a PCD for the clock rate.

This patch allows the UART clock rate to be passed as a parameter
to PL011UartInitializePort(), which is called from the serial port
library. This patch also contains the corresponding changes in the
serial port library.

The PCD in Drivers/PL011Uart is replaced by an extra parameter for
PL011UartInitializePort.  The PCD is moved to Library/PL011SerialPortLib
to supply the value to pass.

A corresponding patch to ArmVirtPkg is included in the same bundle to
align that with these changes.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>

---
 ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf                   |  1 -
 ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf |  1 +
 ArmPlatformPkg/Include/Drivers/PL011Uart.h                       | 43 ++++++++++++--------
 ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c                     | 22 +++++++---
 ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c   | 30 +++++++-------
 5 files changed, 60 insertions(+), 37 deletions(-)

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 28 May 2016 at 14:54,  <evan.lloyd@arm.com> wrote:
> From: Evan Lloyd <evan.lloyd@arm.com>

>

> On some platforms the UART clock is not the same for all the serial

> ports. The PL011 driver must be capable of handling serial ports with

> different clock rates, so must not rely on a PCD for the clock rate.

>

> This patch allows the UART clock rate to be passed as a parameter

> to PL011UartInitializePort(), which is called from the serial port

> library. This patch also contains the corresponding changes in the

> serial port library.

>

> The PCD in Drivers/PL011Uart is replaced by an extra parameter for

> PL011UartInitializePort.  The PCD is moved to Library/PL011SerialPortLib

> to supply the value to pass.

>

> A corresponding patch to ArmVirtPkg is included in the same bundle to

> align that with these changes.

>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>


Minor comment below.  But I tested it on Juno R2 and FVP Foundation
and AEMV8 models anyway as it won't effect functionality.

I saw a hang on R2 that I know to be unrelated to this patch, but
thought I'd report it as I've not seen this specific hang before:

------------------------------------------------------------------------
Installation of the FDT using the device path
<VenHw(E7223039-5836-41E1-B542-D7EC736C5E59)/board.dtb> completed.
PCI: The size 0x7F000000 of the region 0x80000000 has been increased
to be a power of two for the AXI translation table.
PCI: The size 0x180000000 of the region 0x880000000 has been increased
to be a power of two for the AXI translation table.


Synchronous Exception at 0x00000000FDFAFDCC
/working/platforms/uefi/edk2/Build/ArmJuno/DEBUG_GCC49/AARCH64/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe/DEBUG/CpuIo2Dxe.dll
loaded at 0x00000000FDFAE000

  X0 0x0000000057F00000   X1 0x00000000FDFB4030   X2
0x0000000057F00000   X3 0x000000000000001A
  X4 0x00000000FD5ACD98   X5 0x00000000FDFAF4B4   X6
0x0000000000000008   X7 0x0000000000000020
  X8 0x00000000FE113010   X9 0x0000004100000000  X10
0x0000000000040000  X11 0x0000000000000000
 X12 0x00000000707FE07A  X13 0x0000000000000000  X14
0x0000000000000000  X15 0x0000000000000000
 X16 0x00000000FEFFFBF0  X17 0x0000000000000000  X18
0x0000000000000000  X19 0x00000000FE0C0018
 X20 0x0000000000000000  X21 0x0000000000000000  X22
0x0000000000000000  X23 0x0000000000000000
 X24 0x0000000000000000  X25 0x0000000000000000  X26
0x0000000000000000  X27 0x0000000000000000
 X28 0x0000000000000000   FP 0x0000000000000000   LR 0x00000000FDFAF550

  V0 0x3C83709547545153 BC990E9F897D4FA8   V1 0xBEBEF297C5EC0578
E1D99AE8C2B92607
  V2 0x13242833C5F05BAB 101C24C521387481   V3 0x0A50ABCAD0BDDA12
996865483E880969
  V4 0x8400848428600003 0000000008000538   V5 0x00010110001A4001
80000E0202A00001
  V6 0xA010290482010102 4042000A02400100   V7 0x8CC7020884C87814
78A5636F748F82EE
  V8 0x0000000000000000 A4506CEB90BEFFFA   V9 0x0000000000000000
88C1883495C7F76F
 V10 0x0000000000000000 3DB8D233CF470963  V11 0x0000000000000000
4416D9F479B08221
 V12 0x0000000000000000 55B8AE51435C1A1A  V13 0x0000000000000000
C95B757DA62B3A12
 V14 0x0000000000000000 E08479A1B557DFA8  V15 0x0000000000000000
927A0019CDEC1A76
 V16 0x21081A2640C0F178 0084C0048EC80F21  V17 0x1902182018B00C04
0001080284828304
 V18 0x001080000004000C 5900108030000800  V19 0x8001804051402A9B
8000004000080405
 V20 0x000000980800104D 82000C0000000282  V21 0x01B0A50000880048
38A0047001808220
 V22 0x8200024108502200 1200252080001002  V23 0x000F0080006290A2
B1601010040BA110
 V24 0x8401100100804000 0020010200812008  V25 0x2800000008A00108
2001000000E00C24
 V26 0x0840000020668104 4810C11700008400  V27 0x9000000000E00002
A6201000C0028C04
 V28 0x8020000281000200 890080400880A022  V29 0x6085000000000136
800489A520006304
 V30 0x0801200082000020 0384E25502959220  V31 0x4064084112940202
1000459804200800

  SP 0x00000000FEFFF3F0  ELR 0x00000000FDFAFDCC  SPSR 0x60000309  FPSR
0x00000000
 ESR 0x96000210          FAR 0x0000000057F00000

 ESR : EC 0x25  IL 0x1  ISS 0x00000210

Data abort: Synchronous external abort
ASSERT [ArmCpuDxe]
/working/platforms/uefi/edk2/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(194):
((BOOLEAN)(0==1))
------------------------------------------------------------------------

I reverted the patch and the problem still happens.  I reverted the
whole series and I can still reproduce the problem.  Ho hum.

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

> ---

>  ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf                   |  1 -

>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf |  1 +

>  ArmPlatformPkg/Include/Drivers/PL011Uart.h                       | 43 ++++++++++++--------

>  ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c                     | 22 +++++++---

>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c   | 30 +++++++-------

>  5 files changed, 60 insertions(+), 37 deletions(-)

>

> diff --git a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf

> index 5afce36d3935e7fd79c25c46360d72328b2a571f..0154f3bd2e3a3ab930227bde8e45d5e13eaf92ae 100644

> --- a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf

> +++ b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf

> @@ -37,6 +37,5 @@ [Packages]

>  [FixedPcd]

>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate

>

> -  gArmPlatformTokenSpaceGuid.PL011UartClkInHz

>    gArmPlatformTokenSpaceGuid.PL011UartInteger

>    gArmPlatformTokenSpaceGuid.PL011UartFractional

> diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf

> index 653c0b2dfc147f1d82155e4150812f0cb4c59e12..3683e06d27e1a084ba493b0bdf1bec4c0e8f117a 100644

> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf

> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf

> @@ -41,3 +41,4 @@ [FixedPcd]

>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits

>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity

>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits

> +  gArmPlatformTokenSpaceGuid.PL011UartClkInHz

> diff --git a/ArmPlatformPkg/Include/Drivers/PL011Uart.h b/ArmPlatformPkg/Include/Drivers/PL011Uart.h

> index 8c2616ede4131b7504088d656160ce184dadf914..6a97174c82fa8d3153af1327060a98e3b66a8a45 100644

> --- a/ArmPlatformPkg/Include/Drivers/PL011Uart.h

> +++ b/ArmPlatformPkg/Include/Drivers/PL011Uart.h

> @@ -89,32 +89,43 @@

>  #define PL011_UARTPID2_VER(X)     (((X) >> 4) & 0xF)

>  #define PL011_VER_R1P4            0x2

>

> -/*

> +/**

>

>    Initialise the serial port to the specified settings.

> -  Programmed hardware of Serial port.

> -  @param  UartBase                The base address of the serial device.

> -  @param  BaudRate                The baud rate of the serial device. If the baud rate is not supported,

> -                                  the speed will be reduced down to the nearest supported one and the

> +  All unspecified settings will be set to the default values.

> +

> +  @param[in]  UartBase            The base address of the serial device.

> +  @param[in]  UartClkInHz         The clock in Hz for the serial device.

> +                                  Ignored if the PCD PL011UartInteger is not 0

> +  @param[in out] BaudRate         The baud rate of the serial device. If the

> +                                  baud rate is not supported, the speed will be

> +                                  reduced to the nearest supported one and the

>                                    variable's value will be updated accordingly.

> -  @param  ReceiveFifoDepth        The number of characters the device will buffer on input.

> -                                  ReceiveFifoDepth value of 0 will use the device's default FIFO depth.

> -  @param  Parity                  If applicable, this is the EFI_PARITY_TYPE that is computed or checked

> -                                  as each character is transmitted or received. If the device does not

> -                                  support parity, the value is the default parity value.

> -  @param  DataBits                The number of data bits in each character

> -  @param  StopBits                If applicable, the EFI_STOP_BITS_TYPE number of stop bits per character.

> -                                  If the device does not support stop bits, the value is the default stop

> -                                  bit value.

> +  @param[in out] ReceiveFifoDepth The number of characters the device will

> +                                  buffer on input.  Value of 0 will use the

> +                                  device's default FIFO depth.

> +  @param[in out]  Parity          If applicable, this is the EFI_PARITY_TYPE

> +                                  that is computed or checked as each character

> +                                  is transmitted or received. If the device does

> +                                  not support parity, the value is the default

> +                                  parity value.

> +  @param[in out]  DataBits        The number of data bits in each character.

> +  @param[in out]  StopBits        If applicable, the EFI_STOP_BITS_TYPE number

> +                                  of stop bits per character.

> +                                  If the device does not support stop bits, the

> +                                  value is the default stop bit value.

>

> -  @retval RETURN_SUCCESS            All attributes were set correctly on the serial device.

> -  @retval RETURN_INVALID_PARAMETER  One or more of the attributes has an unsupported value.

> +  @retval RETURN_SUCCESS            All attributes were set correctly on the

> +                                    serial device.

> +  @retval RETURN_INVALID_PARAMETER  One or more of the attributes has an

> +                                    unsupported value.

>

>  **/

>  RETURN_STATUS

>  EFIAPI

>  PL011UartInitializePort (

>    IN     UINTN               UartBase,

> +  IN     UINT32              UartClkInHz,

>    IN OUT UINT64              *BaudRate,

>    IN OUT UINT32              *ReceiveFifoDepth,

>    IN OUT EFI_PARITY_TYPE     *Parity,

> diff --git a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c

> index dd5f88d3d629d345a50af468ed394a269b35f52a..61b122a67ab7354714adb429e401126973ab6c8d 100644

> --- a/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c

> +++ b/ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c

> @@ -35,6 +35,8 @@ STATIC CONST UINT32 mInvalidControlBits = EFI_SERIAL_SOFTWARE_LOOPBACK_ENABLE;

>    All unspecified settings will be set to the default values.

>

>    @param  UartBase                The base address of the serial device.

> +  @param  UartClkInHz             The clock in Hz for the serial device.

> +                                  Ignored if the PCD PL011UartInteger is not 0

>    @param  BaudRate                The baud rate of the serial device. If the

>                                    baud rate is not supported, the speed will be

>                                    reduced to the nearest supported one and the

> @@ -63,6 +65,7 @@ RETURN_STATUS

>  EFIAPI

>  PL011UartInitializePort (

>    IN     UINTN               UartBase,

> +  IN     UINT32              UartClkInHz,

>    IN OUT UINT64              *BaudRate,

>    IN OUT UINT32              *ReceiveFifoDepth,

>    IN OUT EFI_PARITY_TYPE     *Parity,

> @@ -72,6 +75,8 @@ PL011UartInitializePort (

>  {

>    UINT32      LineControl;

>    UINT32      Divisor;

> +  UINT32      Integer;

> +  UINT32      Fractional;

>

>    // The PL011 supports a buffer of 1, 16 or 32 chars. Therefore we can accept

>    // 1 char buffer as the minimum FIFO size. Because everything can be rounded

> @@ -168,19 +173,24 @@ PL011UartInitializePort (

>

>    // If PL011 Integer value has been defined then always ignore the BAUD rate

>    if (FixedPcdGet32 (PL011UartInteger) != 0) {

> -      MmioWrite32 (UartBase + UARTIBRD, FixedPcdGet32 (PL011UartInteger));

> -      MmioWrite32 (UartBase + UARTFBRD, FixedPcdGet32 (PL011UartFractional));

> +    Integer = FixedPcdGet32 (PL011UartInteger);

> +    Fractional = FixedPcdGet32 (PL011UartFractional);

>    } else {

>      // If BAUD rate is zero then replace it with the system default value

>      if (*BaudRate == 0) {

>        *BaudRate = FixedPcdGet32 (PcdSerialBaudRate);

> -      ASSERT (*BaudRate != 0);

> +      if (*BaudRate == 0) {


Should you check UartClkInHz at this point too?


> +        return RETURN_INVALID_PARAMETER;

> +      }

>      }

>

> -    Divisor = (FixedPcdGet32 (PL011UartClkInHz) * 4) / *BaudRate;

> -    MmioWrite32 (UartBase + UARTIBRD, Divisor >> FRACTION_PART_SIZE_IN_BITS);

> -    MmioWrite32 (UartBase + UARTFBRD, Divisor & FRACTION_PART_MASK);

> +    Divisor = (UartClkInHz * 4) / *BaudRate;

> +    Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;

> +    Fractional = Divisor & FRACTION_PART_MASK;

>    }

> +  // Set Baud Rate Registers

> +  MmioWrite32 (UartBase + UARTIBRD, Integer);

> +  MmioWrite32 (UartBase + UARTFBRD, Fractional);

>

>    // No parity, 1 stop, no fifo, 8 data bits

>    MmioWrite32 (UartBase + UARTLCR_H, LineControl);

> diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c

> index 9c998a63cfe4e1506e6c1f0aa25ab6566f6dbf65..b3dea6b3aca91b3bc7a296c28a02aaa6593e4cf2 100644

> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c

> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c

> @@ -50,13 +50,14 @@ SerialPortInitialize (

>    StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8 (PcdUartDefaultStopBits);

>

>    return PL011UartInitializePort (

> -      (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),

> -      &BaudRate,

> -      &ReceiveFifoDepth,

> -      &Parity,

> -      &DataBits,

> -      &StopBits

> -      );

> +           (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),

> +           FixedPcdGet32 (PL011UartClkInHz),

> +           &BaudRate,

> +           &ReceiveFifoDepth,

> +           &Parity,

> +           &DataBits,

> +           &StopBits

> +           );

>  }

>

>  /**

> @@ -150,13 +151,14 @@ SerialPortSetAttributes (

>    )

>  {

>    return PL011UartInitializePort (

> -    (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),

> -    BaudRate,

> -    ReceiveFifoDepth,

> -    Parity,

> -    DataBits,

> -    StopBits

> -    );

> +           (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),

> +           FixedPcdGet32 (PL011UartClkInHz),

> +           BaudRate,

> +           ReceiveFifoDepth,

> +           Parity,

> +           DataBits,

> +           StopBits

> +           );

>  }

>

>  /**

> --

> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

>

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel