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

["Vivek Kumar Gautam" <vivek.gautam@arm.com>]

Hi Sami,


On 4/26/23 21:47, Sami Mujawar wrote:
> 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>

Thank you for the review of this patch and other patches in the series. 
Please find my comments inline below.

> 
> 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.

I will keep these things in mind for future. Thanks for taking care of 
these in this patch.

Best regards
Vivek

> 
> [/SAMI]
> 

[snip]