Re: [edk2-devel] [PATCH v1 10/19] DynamicTablesPkg: Serial debug port initialisation

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 11/21/19 4:23 PM, Leif Lindholm wrote:
> On Thu, Nov 21, 2019 at 16:20:31 +0100, Philippe Mathieu-Daudé wrote:
>> On 8/23/19 12:55 PM, Sami Mujawar wrote:
>>> The ARM DCC serial port subtype is an option that is
>>> supported by the DBG2 generator. However, the serial
>>> port initialisation should only be done for PL011/SBSA
>>> compatible UARTs.
>>>
>>> Add check to conditionally initialise the serial port.
>>>
>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>> ---
>>>    DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c | 27 ++++++++++++--------
>>>    1 file changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>> index 346ab5b22f5402bf87c385558f68f080d1b454ed..51c843d25f75388104694855ce133b3d61860196 100644
>>> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
>>> @@ -175,7 +175,7 @@ GET_OBJECT_LIST (
>>>      CM_ARM_SERIAL_PORT_INFO
>>>      );
>>> -/** Initialize the PL011 UART with the parameters obtained from
>>> +/** Initialize the PL011/SBSA UART with the parameters obtained from
>>>        the Configuration Manager.
>>
>> Isn't the SBSA UART a PL011?
> 
> No. It's a compatible subset.
> So a PL011 can be used as an SBSA UART.

OK thanks.

Can you update the comment? Maybe:

"Initialize the PL011 compatible UART with the parameters ..."

Regardless:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
> /
>      Leif
> 
>>>      @param [in]  SerialPortInfo Pointer to the Serial Port Information.
>>> @@ -353,15 +353,22 @@ BuildDbg2Table (
>>>      AcpiDbg2.Dbg2DeviceInfo[DBG_PORT_INDEX_PORT1].Dbg2Device.PortSubtype =
>>>        SerialPortInfo->PortSubtype;
>>> -  // Initialize the serial port
>>> -  Status = SetupDebugUart (SerialPortInfo);
>>> -  if (EFI_ERROR (Status)) {
>>> -    DEBUG ((
>>> -      DEBUG_ERROR,
>>> -      "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
>>> -      Status
>>> -      ));
>>> -    goto error_handler;
>>> +  if ((SerialPortInfo->PortSubtype ==
>>> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_PL011_UART)           ||
>>> +      (SerialPortInfo->PortSubtype ==
>>> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART_2X) ||
>>> +      (SerialPortInfo->PortSubtype ==
>>> +      EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_SBSA_GENERIC_UART)) {
>>> +    // Initialize the serial port
>>> +    Status = SetupDebugUart (SerialPortInfo);
>>> +    if (EFI_ERROR (Status)) {
>>> +      DEBUG ((
>>> +        DEBUG_ERROR,
>>> +        "ERROR: DBG2: Failed to configure debug serial port. Status = %r\n",
>>> +        Status
>>> +        ));
>>> +      goto error_handler;
>>> +    }
>>>      }
>>>      *Table = (EFI_ACPI_DESCRIPTION_HEADER*)&AcpiDbg2;
>>>
>>
>