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

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

On 11/21/19 4:48 PM, Leif Lindholm wrote:
> On Thu, Nov 21, 2019 at 16:29:16 +0100, Philippe Mathieu-Daudé wrote:
>> 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 ..."
> 
> The original is correct, the suggested alternative is not (an SBSA
> UART is a subset, so not fully compatible).

OK, thanks for clarifying.

> 
> If the comment was to change, I would suggest that dropping the model
> name completely and simply refer to it as "the UART" would be
> preferable.
> 
> /
>      Leif
> 
>> Regardless:
>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>
>>>
>>> /
>>>       Leif
>