Re: [edk2-platforms][PATCH V3 4/5] Platform/Sgi: Initialize additional UART controllers

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Vivek,

Thank you for this patch.

I have some minor feedback that I will address before merging.

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

Regards,

Sami Mujawar

On 24/03/2023 11:03 am, Vivek Gautam wrote:
> From: Shriram K <shriram.k@arm.com>
>
> The IO virtualization block on reference design platforms allow
> connecting SoC expansion devices such as PL011 UART. On platforms
> that support this, initialize the UART controller connected to the
> IO virtualization block.
>
> Signed-off-by: Shriram K <shriram.k@arm.com>
> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> ---
>   Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf  |  9 ++-
>   Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf  |  6 +-
>   Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c    | 64 +++++++++++++++++++-
>   Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c | 44 +++++++++++++-
>   4 files changed, 116 insertions(+), 7 deletions(-)
>
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> index 9d89314a594e..3cd7e2329c22 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -1,5 +1,5 @@
>   #
> -#  Copyright (c) 2018, ARM Limited. All rights reserved.
> +#  Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.
>   #
>   #  SPDX-License-Identifier: BSD-2-Clause-Patent
>   #
> @@ -17,6 +17,7 @@
>     VirtioDevices.c
>   
>   [Packages]
> +  ArmPlatformPkg/ArmPlatformPkg.dec
>     EmbeddedPkg/EmbeddedPkg.dec
>     MdePkg/MdePkg.dec
>     OvmfPkg/OvmfPkg.dec
> @@ -37,10 +38,16 @@
>     gArmSgiTokenSpaceGuid.PcdVirtioNetSupported
>   
>   [FixedPcd]
> +  gArmSgiTokenSpaceGuid.PcdChipCount
> +  gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlk0Base
> +  gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkUartEnable
> +  gArmSgiTokenSpaceGuid.PcdMaxAddressBitsPerChip
>     gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
>     gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
>     gArmSgiTokenSpaceGuid.PcdVirtioNetBaseAddress
>     gArmSgiTokenSpaceGuid.PcdVirtioNetSize
>   
> +  gArmPlatformTokenSpaceGuid.PcdSerialDbgUartClkInHz
> +
>   [Depex]
>     TRUE
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> index 1ca7679b4191..020bde0d1f56 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> @@ -1,5 +1,5 @@
>   #
> -#  Copyright (c) 2018 - 2022, Arm Limited. All rights reserved.
> +#  Copyright (c) 2018 - 2023, Arm Limited. All rights reserved.
>   #
>   #  SPDX-License-Identifier: BSD-2-Clause-Patent
>   #
> @@ -41,10 +41,12 @@
>     gArmPlatformTokenSpaceGuid.PcdCoreCount
>     gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase
>   
> -  gArmSgiTokenSpaceGuid.PcdMaxAddressBitsPerChip
>     gArmSgiTokenSpaceGuid.PcdDramBlock2Base
>     gArmSgiTokenSpaceGuid.PcdDramBlock2Size
>     gArmSgiTokenSpaceGuid.PcdGicSize
> +  gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlk0Base
> +  gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkUartEnable
> +  gArmSgiTokenSpaceGuid.PcdMaxAddressBitsPerChip
>   
>     gArmTokenSpaceGuid.PcdSystemMemoryBase
>     gArmTokenSpaceGuid.PcdSystemMemorySize
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> index 2f72e7152ff3..b3a998bc1585 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> @@ -1,6 +1,6 @@
>   /** @file
>   *
> -*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*  Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.
>   *
>   *  SPDX-License-Identifier: BSD-2-Clause-Patent
>   *
> @@ -9,6 +9,9 @@
>   #include <Library/AcpiLib.h>
>   #include <Library/DebugLib.h>
>   #include <Library/HobLib.h>
> +#include <Library/PL011UartLib.h>
> +
> +#include <IoVirtSoCExp.h>
>   #include <SgiPlatform.h>
>   
>   VOID
> @@ -16,6 +19,64 @@ InitVirtioDevices (
>     VOID
>     );
>   
> +/**
> +  Initialize UART controllers connected to IO Virtualization block.
> +
> +  Use PL011UartLib Library to initialize UART controllers that are present in
> +  the SoC expansion block. This SoC expansion block is connected to the IO
> +  virtualization block on Arm infrastructure reference design (RD) platforms.
> +
> +  @retval  None
> +**/
> +STATIC
> +VOID
> +InitIoVirtSocExpBlkUartControllers (VOID)
> +{
> +  EFI_STATUS                 Status;
> +  EFI_PARITY_TYPE            Parity;
> +  EFI_STOP_BITS_TYPE         StopBits;
> +  UINT64                     BaudRate;
> +  UINT32                     ReceiveFifoDepth;
> +  UINT8                      DataBits;
> +  UINT8                      UartIdx;
> +  UINT32                     ChipIdx;
> +  UINT64                     UartAddr;
> +
> +  if (FixedPcdGet32 (PcdIoVirtSocExpBlkUartEnable) == 0)
> +    return;

[SAMI] edk2 coding standard requires to use opening and closing curly 
brackets even when the if condition has a single enclosing statement.

Also the function parameters must be on separate lines. However, I will 
address that locally before merging.

[/SAMI]

> +
> +  ReceiveFifoDepth = 0;
> +  Parity = 1;
> +  DataBits = 8;
> +  StopBits = 1;
> +  BaudRate = 115200;
> +
> +  for (ChipIdx = 0; ChipIdx < FixedPcdGet32 (PcdChipCount); ChipIdx++) {
> +    for (UartIdx = 0; UartIdx < 2; UartIdx++) {
> +      UartAddr = SGI_REMOTE_CHIP_MEM_OFFSET(ChipIdx) + UART_START(UartIdx);
> +
> +      Status = PL011UartInitializePort (
> +                 (UINTN)UartAddr,
> +                 FixedPcdGet32 (PcdSerialDbgUartClkInHz),
> +                 &BaudRate,
> +                 &ReceiveFifoDepth,
> +                 &Parity,
> +                 &DataBits,
> +                 &StopBits
> +                 );
> +
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((
> +          DEBUG_ERROR,
> +          "Failed to init PL011_UART%u on IO Virt Block port, status: %r\n",
> +          UartIdx,
> +          Status
> +          ));
> +      }
> +    }
> +  }
> +}
> +
>   EFI_STATUS
>   EFIAPI
>   ArmSgiPkgEntryPoint (
> @@ -32,6 +93,7 @@ ArmSgiPkgEntryPoint (
>     }
>   
>     InitVirtioDevices ();
> +  InitIoVirtSocExpBlkUartControllers ();
>   
>     return Status;
>   }
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> index 8139b75d8ee4..fa3cfbc730f6 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> @@ -1,6 +1,6 @@
>   /** @file
>   *
> -*  Copyright (c) 2018-2020, ARM Limited. All rights reserved.
> +*  Copyright (c) 2018-2023, ARM Limited. All rights reserved.
>   *
>   *  SPDX-License-Identifier: BSD-2-Clause-Patent
>   *
> @@ -13,11 +13,24 @@
>   #include <Library/IoLib.h>
>   #include <Library/MemoryAllocationLib.h>
>   
> +#include <IoVirtSoCExp.h>
>   #include <SgiPlatform.h>
>   
>   // Total number of descriptors, including the final "end-of-table" descriptor.
> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS                 \
> -          (14 + (FixedPcdGet32 (PcdChipCount) * 2))
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS                                     \
> +          ((14 + (FixedPcdGet32 (PcdChipCount) * 2)) +                         \
> +           (FixedPcdGet32 (PcdIoVirtSocExpBlkUartEnable) *                     \
> +            FixedPcdGet32 (PcdChipCount) * 2))
> +
> +// Memory Map descriptor for IO Virtualization SoC Expansion Block UART
> +#define IO_VIRT_SOC_EXP_BLK_UART_MMAP(UartIdx, ChipIdx)                        \
> +  VirtualMemoryTable[++Index].PhysicalBase =                                   \
> +    SGI_REMOTE_CHIP_MEM_OFFSET(ChipIdx) + UART_START(UartIdx);                 \
> +  VirtualMemoryTable[Index].VirtualBase    =                                   \
> +    SGI_REMOTE_CHIP_MEM_OFFSET(ChipIdx) + UART_START(UartIdx);                 \
> +  VirtualMemoryTable[Index].Length         = SIZE_64KB;                        \
> +  VirtualMemoryTable[Index].Attributes     = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
>   
>   /**
>     Returns the Virtual Memory Map of the platform.
> @@ -171,6 +184,31 @@ ArmPlatformGetVirtualMemoryMap (
>     VirtualMemoryTable[Index].Length          = SIZE_64KB;
>     VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>   
> +#if (FixedPcdGet32 (PcdIoVirtSocExpBlkUartEnable) == 1)
> +  // Chip-0 IO Virtualization SoC Expansion Block - UART0
> +  IO_VIRT_SOC_EXP_BLK_UART_MMAP(0, 0)
> +  // Chip-0 IO Virtualization SoC Expansion Block - UART1
> +  IO_VIRT_SOC_EXP_BLK_UART_MMAP(1, 0)
> +#if (FixedPcdGet32 (PcdChipCount) > 1)
> +  // Chip-1 IO Virtualization SoC Expansion Block - UART0
> +  IO_VIRT_SOC_EXP_BLK_UART_MMAP(0, 1)
> +  // Chip-1 IO Virtualization SoC Expansion Block - UART1
> +  IO_VIRT_SOC_EXP_BLK_UART_MMAP(1, 1)
> +#if (FixedPcdGet32 (PcdChipCount) > 2)
> +  // Chip-2 IO Virtualization SoC Expansion Block - UART0
> +  IO_VIRT_SOC_EXP_BLK_UART_MMAP(0, 2)
> +  // Chip-2 IO Virtualization SoC Expansion Block - UART1
> +  IO_VIRT_SOC_EXP_BLK_UART_MMAP(1, 2)
> +#if (FixedPcdGet32 (PcdChipCount) > 3)
> +  // Chip-3 IO Virtualization SoC Expansion Block - UART0
> +  IO_VIRT_SOC_EXP_BLK_UART_MMAP(0, 3)
> +  // Chip-3 IO Virtualization SoC Expansion Block - UART1
> +  IO_VIRT_SOC_EXP_BLK_UART_MMAP(1, 3)
> +#endif
> +#endif
> +#endif
> +#endif
> +
>     // DDR - (2GB - 16MB)
>     VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdSystemMemoryBase);
>     VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdSystemMemoryBase);