Re: [PATCH v1 06/14] DynamicTablesPkg: FdtHwInfoParser: Add Serial port parser

["PierreGondois" <pierre.gondois@arm.com>]

Hi Sami,

Please find my answer inlined:

On 11/5/21 14:27, Sami Mujawar wrote:
>
> Hi Pierre,
>
> Please find my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
>
> On 23/06/2021 01:38 PM, Pierre.Gondois@arm.com wrote:
>> From: Pierre Gondois <Pierre.Gondois@arm.com>
>>
>> The Microsoft Debug Port Table 2 (DBG2), the Serial Port Console
>> Redirector (SPCR) table are mandatory tables required for booting
>> a standards-based operating system. The DBG2 table is used by the
>> OS debugger while the SPCR table is used to configure the serial
>> terminal. Additionally, the serial ports available on a platform
>> for generic use also need to be described in DSDT/SSDT for an OS
>> to be able to use the serial ports.
>>
>> The Arm Base System Architecture 1.0 specification a lists of
>> supported serial port hardware for Arm Platforms. This list
>> includes the following serial port UARTs:
>>  - SBSA/Generic UART
>>  - a fully 16550 compatible UART.
>> Along, with these the PL011 UART is the most commonly used serial
>> port hardware on Arm platforms.
>>
>> The serial port hardware information is described in the platform
>> Device Tree, the bindings for which can be found at:
>>  - linux/Documentation/devicetree/bindings/serial/serial.yaml
>>  - linux/Documentation/devicetree/bindings/serial/8250.txt
>>  - linux/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
>>  - linux/Documentation/devicetree/bindings/serial/pl011.yaml
>>
>> The FdtHwInfoParser implements a Serial Port Parser that parses
>> the platform Device Tree to create CM_ARM_SERIAL_PORT_INFO objects
>> with the following IDs:
>>  - EArmObjSerialConsolePortInfo (for use by SPCR)
>>  - EArmObjSerialDebugPortInfo (for use by DBG2)
>>  - EArmObjSerialPortInfo (for use as generic Serial Ports)
>>
>> The Serial Port for use by SPCR is selected by parsing the Device
>> Tree for the '/chosen' node with the 'stdout-path' property. The
>> next Serial Port is selected for use as the Debug Serial Port and
>> the remaining serial ports are used as generic serial ports.
>>
>> The CM_ARM_SERIAL_PORT_INFO objects are encapsulated in Configuration
>> Manager descriptor objects with the respective IDs and are added to
>> the platform information repository.
>>
>> The platform Configuration Manager can then utilise this information
>> when generating the DBG2, SPCR and the SSDT serial port tables.
>>
>> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>  .../Serial/ArmSerialPortParser.c              | 621 ++++++++++++++++++
>>  .../Serial/ArmSerialPortParser.h              |  47 ++
>>  2 files changed, 668 insertions(+)
>>  create mode 100644 DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.c
>>  create mode 100644 DynamicTablesPkg/Library/FdtHwInfoParserLib/Serial/ArmSerialPortParser.h
[snip]
>> +
>> +  SerialPortInfo->Interrupt = FdtGetInterruptId ((CONST UINT32*)Data);
>> +
>> +  // Note: clock-frequency is optional for SBSA UART.
>> +  Data = fdt_getprop (Fdt, SerialPortNode, "clock-frequency", &DataSize);
>> +  if (Data != NULL) {
>> +    if (DataSize < sizeof (UINT32)) {
>> +      // If error or not enough space.
>> +      ASSERT (0);
>> +      return EFI_ABORTED;
>> +    } else if (fdt_node_offset_by_phandle (Fdt, fdt32_to_cpu (*Data)) >= 0) {
>> +      // "clock-frequency" can be a "clocks phandle to refer to the clk used".
>> +      // This is not supported.
>> +      ASSERT (0);
>> +      return EFI_UNSUPPORTED;
>> +    }
>> +    SerialPortInfo->Clock = fdt32_to_cpu (*(UINT32*)Data);
>> +  }
>> +
>> +  if (FdtNodeIsCompatible (Fdt, SerialPortNode, &Serial16550CompatibleInfo)) {
>> +    SerialPortInfo->PortSubtype = EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_FULL_16550;
> [SAMI] I think this needs to be set
> toEFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_16550_WITH_GAS.

[Pierre] From what I can found,the
EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_16550_WITH_GAS has only been added
recently in the edk2 and linux. I didn't find to what is correspond. In
the DynamicTablesPkg, we are generating  the
EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_FULL_16550 id. So I think we should
keep the non-GAS id.

>> +
>> +    /* reg-io-width:
>> +         description: |
>> +         The size (in bytes) of the IO accesses that should be performed on the
>> +         device. There are some systems that require 32-bit accesses to the
>> +         UART.
>> +    */
>> +    Data = fdt_getprop (Fdt, SerialPortNode, "reg-io-width", &DataSize);
>> +    if (Data != NULL) {
>> +      if (DataSize < sizeof (UINT32)) {
>> +        // If error or not enough space.
>> +        ASSERT (0);
>> +        return EFI_ABORTED;
>> +      }
>>
>>