Re: [edk2-devel] [edk2-platforms][PATCH V1 2/6] Platform/Sgi: add ssdt table for non-discoverable IO virtualization block

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

Hi Pierre,

On 12/7/22 19:04, Pierre Gondois wrote:
> Hello Vivek,
> Sorry for the long wait. I think the whole patchset needs to be
> rebased on latest master. I just have some comments for patches:
> - [PATCH V1 2/6] Platform/Sgi: add ssdt table for non-discoverable IO 
> virtualization block
> - [PATCH V1 3/6] Platform/Sgi: Initialize additional uart controllers
> The other patches look good to me.
>

Thank you for your review and apologies for responding late. I was able 
to rework the patches and get a cleaner SSDT table implementation. I 
will post the patches soon.

Please see my responses inline.

> Regards,
> Pierre
>
> On 2/14/22 13:13, Vivek Kumar Gautam via groups.io wrote:
>> Arm reference design platforms such as RD-N2 and RD-N2-Cfg1 have 
>> multiple
>> IO virtualization blocks that allow connecting PCIe root bus or non-PCIe
>> devices to the system. For platforms that connect non-discoverable (non-
>> PCI) devices to IO virtualization block, add a SSDT table to describe
>> such devices and use PCDs for the memory region and interrupts of these
>> devices in the table entry.
>> There are two PL011 UART controllers and two PL330 DMA controllers
>> connected to the non-PCIe IO virtualization block on RD-N2 and
>> RD-N2-Cfg1 platforms. List them in the SSDT ACPI table.
>>
>> While we are adding SSDT table entries for RD-N2 and RD-N2-Cfg1
>> remove the source file entries for incorrect SSDT and MCFG tables
>> for RD-N2 and RD-N2-Cfg1 platforms.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
>> ---
>>   Platform/ARM/SgiPkg/SgiPlatform.dec                    |  42 ++++
>>   Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc              |  40 ++++
>>   Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf      |  45 ++++-
>>   Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf  |  45 ++++-
>>   Platform/ARM/SgiPkg/AcpiTables/SsdtNonPciIoVirtBlk.asl | 203 
>> ++++++++++++++++++++
>>   5 files changed, 369 insertions(+), 6 deletions(-)
>>
>> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
>> b/Platform/ARM/SgiPkg/SgiPlatform.dec
>> index 05079743c452..6b3e28c3a08e 100644
>> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
>> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
>> @@ -95,5 +95,47 @@
>>     gArmSgiTokenSpaceGuid.PcdOscLpiEnable|0|UINT32|0x00000025
>>     gArmSgiTokenSpaceGuid.PcdOscCppcEnable|0|UINT32|0x00000026
>>   +  # IO virtualization block PL011 UARTs
>> + gArmSgiTokenSpaceGuid.PcdIoVirtBlkUart0Base|0|UINT64|0x0000002C
>> + gArmSgiTokenSpaceGuid.PcdIoVirtBlkUart0End|0|UINT64|0x0000002D
>
> I think it should be possible to remove all the Pcd*End addresses and
> replace them with (PcdIoVirtBlkUart0Base + PcdIoVirtBlkUart0Size - 1).

I will post the reworked patch-set addressing this.

>
>> + gArmSgiTokenSpaceGuid.PcdIoVirtBlkUart0Size|0|UINT64|0x0000002E
>> + gArmSgiTokenSpaceGuid.PcdIoVirtBlkUart0Interrupt|0|UINT32|0x0000002F
>> +
>> + gArmSgiTokenSpaceGuid.PcdIoVirtBlkUart1Base|0|UINT64|0x00000030
>> + gArmSgiTokenSpaceGuid.PcdIoVirtBlkUart1End|0|UINT64|0x00000031
>> + gArmSgiTokenSpaceGuid.PcdIoVirtBlkUart1Size|0|UINT64|0x00000032
>> + gArmSgiTokenSpaceGuid.PcdIoVirtBlkUart1Interrupt|0|UINT32|0x00000033
>> +
>
> [...]
>
>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/SsdtNonPciIoVirtBlk.asl 
>> b/Platform/ARM/SgiPkg/AcpiTables/SsdtNonPciIoVirtBlk.asl
>> new file mode 100644
>> index 000000000000..a035186b88db
>> --- /dev/null
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/SsdtNonPciIoVirtBlk.asl
>> @@ -0,0 +1,203 @@
>> +/** @file
>> +  Secondary System Description Table (SSDT) for Non-PCIe IO
>> +  Virtualization Block.
>> +
>> +  The IO virtualization block present on reference design platforms
>> +  such as RD-N2 and RD-N2-Cfg1 allows connecting PCIe and non-PCIe
>> +  devices. The non-discoverable (non-PCIe) devices that are connected
>> +  to the IO virtualization block include two PL011 UART and two PL330
>> +  DMA controllers.
>> +
>> +  Copyright (c) 2022, Arm Ltd. All rights reserved.
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +  @par Specification Reference:
>> +    - ACPI 6.4, Chapter 5, Section 5.2.11.2, Secondary System 
>> Description Table
>> +**/

[snip]

>>
>> +
>> +    // IO Virtualization Block - PL330 DMA0
>> +    Device (\_SB.DMA0) {
>> +      Name (_HID, "ARMH0330")
>
> Is there a specification for the description of this _HID and how
> it should be represented in ACPI ?

Yes, this can be found in the ACPI specification here [1]. The Linux 
kernel documentation also describes it here [2] as the primary object to 
use in device probing.

[1] 
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#device-identification-objects
[2] https://docs.kernel.org/arm64/acpi_object_usage.html


Best regards
Vivek