Hello Vivek, Thanks for the answers, On 2/7/23 07:59, Vivek Kumar Gautam wrote:
Hi PierreOn 2/3/23 21:26, Pierre Gondois wrote:Hello Vivek,Thanks for review the changes, please find my responses inline below.On 1/27/23 10:23, Vivek Gautam wrote:Arm reference design platforms have multiple IO virtualization blocks that allow connecting PCIe root bus or non-PCIe SoC peripherals to the system. Each of these IO virtualization blocks consists of an instance of SMMUv3, a GIC-ITS and a NCI (network chip interconnect) to support traffic flow and address mapping, as required. The SoC expansion blocks that connect to the IO virtualization block include devices such as UARTs, DMAs and few additional memory nodes. For platforms having SoC expansion block connected to the IO virtualization block add a SSDT table to describe devices included in the SoC expansion block. Preprocessor macros are also added in this change to allow scalability for platforms that implement multiple instances of these SoC expansion blocks. Signed-off-by: Vivek Gautam <vivek.gau...@arm.com> --- Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc | 5 + Platform/ARM/SgiPkg/Include/IoVirtSoCExp.h | 189 ++++++++++++++++++++ Platform/ARM/SgiPkg/AcpiTables/SsdtIoVirtSocExp.asl | 96 ++++++++++ Platform/ARM/SgiPkg/SgiPlatform.dec | 5 + 4 files changed, 295 insertions(+) diff --git a/Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc b/Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc index 12dcd82eb132..14734fb65828 100644 --- a/Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc +++ b/Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc @@ -72,3 +72,8 @@ gArmSgiTokenSpaceGuid.PcdGpioController0BaseAddress|0x0C1D0000 gArmSgiTokenSpaceGuid.PcdGpioController0Size|0x00010000 gArmSgiTokenSpaceGuid.PcdGpioController0Interrupt|392 + + # IO virtualization block + gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlk0Base|0x1080000000 + gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkPeriOffset|0x10000000 + gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkResourceSize|0x10000 diff --git a/Platform/ARM/SgiPkg/Include/IoVirtSoCExp.h b/Platform/ARM/SgiPkg/Include/IoVirtSoCExp.h new file mode 100644 index 000000000000..8e73b8989b16 --- /dev/null +++ b/Platform/ARM/SgiPkg/Include/IoVirtSoCExp.h @@ -0,0 +1,189 @@ +/** @file +* +* Copyright (c) 2023, Arm Limited. All rights reserved. +* +* SPDX-License-Identifier: BSD-2-Clause-Patent +* +**/ + +#include "SgiPlatform.h" + +#define IO_VIRT_BLK_BASE FixedPcdGet64 (PcdIoVirtSocExpBlk0Base) +#define RESOURCE_SIZE FixedPcdGet32 (PcdIoVirtSocExpBlkResourceSize) + +/** Macros to calculate base addresses of UART and DMA devices within IO + virtualization SoC expansion block address space. + + @param [in] n Index of UART or DMA device within SoC expansion block. + Should be either 0 or 1. + + The base address offsets of UART and DMA devices within a SoC expansion block + are shown below. The UARTs are at offset (2 * index + 0x1000_0000), while theI think it's (2 * index * offset) (the offset is missing).Right it should have been (2 * index * offset). I will correct this comment and the one below for DMA.+ DMAs are at offsets ((2 * index + 1) + 0x1000_0000). + +----------------------------------------------+ + | Port # | Peripheral | Base address offset | + |--------|---------------|---------------------| + | x4_0 | PL011_UART0 | 0x0000_0000 | + |--------|---------------|---------------------| + | x4_1 | PL011_DMA0_NS | 0x1000_0000 | + |--------|---------------|---------------------| + | x8 | PL011_UART1 | 0x2000_0000 | + |--------|---------------|---------------------| + | x16 | PL011_DMA1_NS | 0x3000_0000 | + +----------------------------------------------+ +**/ +#define UART_START(n) IO_VIRT_BLK_BASE + \ + (2 * n * FixedPcdGet32 (PcdIoVirtSocExpBlkPeriOffset)) +#define DMA_START(n) IO_VIRT_BLK_BASE + \ + (((2 * n) + 1) * FixedPcdGet32 (PcdIoVirtSocExpBlkPeriOffset))The values of: - PcdIoVirtSocExpBlk0Base - PcdIoVirtSocExpBlkPeriOffset - PcdIoVirtSocExpBlkResourceSize are the same for all Rdn2[|Cfg1|Cfg2] platforms and the documentation above is referring to hard-coded values (e.g. 0x1000_0000), so would it be worth defining them as local macros only ?I agree that these PCDs are same for RDN2 {|Cfg1|Cfg2}. I think now that I can add local macros for PcdIoVirtSocExpBlkPeriOffset and PcdIoVirtSocExpBlkResourceSize. But I plan to keep the PCD for base address - PcdIoVirtSocExpBlk0Base as that can be reused for other platforms considering that other platforms may map the IoVirtSocExpBlk in a different expansion base address. Let me know what do you think.
Ok right, works for me.
+ +// Interrupt numbers of PL330 DMA-0 and DMA-1 devices in the SoC expansion +// connected to the IO Virtualization block. Each DMA PL330 controller uses +// eight data channel interrupts and one instruction channel interrupt to +// notify aborts. +#define RD_IOVIRT_SOC_EXP_DMA0_INTERRUPTS_INIT \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + 493, 494, 495, 496, 497, 498, 499, 500, 501 \ + } +#define RD_IOVIRT_SOC_EXP_DMA1_INTERRUPTS_INIT \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + 503, 504, 505, 506, 507, 508, 509, 510, 511 \ + } + +#define RD_IOVIRT_SOC_EXP_DMA2_INTERRUPTS_INIT \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + 973, 974, 975, 976, 977, 978, 979, 980, 981 \ + } + +#define RD_IOVIRT_SOC_EXP_DMA3_INTERRUPTS_INIT \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + 983, 984, 985, 986, 987, 988, 989, 990, 991 \ + } + +#define RD_IOVIRT_SOC_EXP_DMA4_INTERRUPTS_INIT \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + 4557, 4558, 4559, 4560, 4561, 4562, 4563, 4564, 4565 \ + } + +#define RD_IOVIRT_SOC_EXP_DMA5_INTERRUPTS_INIT \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + 4567, 4568, 4569, 4570, 4571, 4572, 4573, 4574, 4575 \ + } + +#define RD_IOVIRT_SOC_EXP_DMA6_INTERRUPTS_INIT \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + 5037, 5038, 5039, 5040, 5041, 5042, 5043, 5044, 5045 \ + } + +#define RD_IOVIRT_SOC_EXP_DMA7_INTERRUPTS_INIT \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + 5047, 5048, 5049, 5050, 5051, 5052, 5053, 5054, 5055 \ + } + +/** Macro for PL011 UART controller node instantiation in SSDT table. + + See section 5.2.11.2 of ACPI specification v6.4 for the definition of SSDT + table. Use of this macro is restricted to ASL file and not to TDL file.Out of curSorry, I didn't understand your comment here.
Sorry I didn't finish the sentence. I was wondering what TDL files were for.
+ + @param [in] ComIdx Index of Com device to be initializaed; + to be passed as 2-digit index, such as 01 to + support multichip platforms as well. + @param [in] ChipIdx Index of chip to which this DMA device belongs + @param [in] StartOff Starting offset of this device within IO + virtualization block memory map + @param [in] IrqNum Interrupt ID used for the device +**/ +#define RD_IOVIRT_SOC_EXP_COM_INIT(ComIdx, ChipIdx, StartOff, IrqNum) \ + Device (COM ##ComIdx) { \ + Name (_HID, "ARMH0011") \ + Name (_UID, ComIdx) \ + Name (_STA, 0xF) \ + \ + Method (_CRS, 0, Serialized) { \ + Name (RBUF, ResourceTemplate () { \ + QWordMemory ( \ + ResourceProducer, \ + PosDecode, \ + MinFixed, \ + MaxFixed, \ + NonCacheable, \ + ReadWrite, \ + 0x0, \ + 0, \ + 1, \ + 0x0, \ + 2, \ + , \ + , \ + MMI1, \ + AddressRangeMemory, \ + TypeStatic \ + ) \ + \ + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { \ + IrqNum \ + } \ + }) /* end Name(RBUF) */ \ + /* Work around ASL's inability to add in a resource definition */ \ + CreateQwordField (RBUF, MMI1._MIN, MIN1) \ + CreateQwordField (RBUF, MMI1._MAX, MAX1) \ + CreateQwordField (RBUF, MMI1._LEN, LEN1) \ + Add (SGI_REMOTE_CHIP_MEM_OFFSET(ChipIdx), StartOff, MIN1) \ + Add (MIN1, RESOURCE_SIZE - 1, MAX1) \ + Add (RESOURCE_SIZE, 0, LEN1) \This seems like a complicated way to do additions. I guess the asl compiler doesn't allow doing this. The DynamicTablesPkg could allow generating such resources. Is there a reason not to use it ?We have not used the DynamicTablePkg maintain the readability and maintainability of the SSDT tables on these Arm reference design platforms. I took a reference from RaspbarryPi platform [1] to use the CreateQWordField() and Add() methods to calculate the resultant start and end addresses whereever necessary. Let me know if this doesn't look right.
This is the first time I see this, but it seems to be functional.
+ \ + Return (RBUF) \ + } /* end Method(_CRS) */ \ + } +[snip]+-------------------------------------------------------------------------------+ + + This SSDT ACPI table lists the SoC expansion block devices connected via the + IO Virtualization block on RD-N2 platform variants and mapped to SoC expansion + address at an offset of 0x10_8000_0000 from each chip's base address. + + Copyright (c) 2023, Arm Limited. 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 +**/ + +#include "IoVirtSoCExp.h" +#include "SgiAcpiHeader.h" + +DefinitionBlock ("SsdtIoVirtSocExp.aml", "SSDT", 2, "ARMLTD", "ARMSGI", + EFI_ACPI_ARM_OEM_REVISION) { + Scope (_SB) + { + + // IO Virtualization SoC Expansion - PL011 UART + if (LEqual (FixedPcdGet32 (PcdIoVirtSocExpBlkUartEnable), 1)) { + RD_IOVIRT_SOC_EXP_COM_INIT(2, 0, UART_START(0), 492) + RD_IOVIRT_SOC_EXP_COM_INIT(3, 0, UART_START(1), 502) + + if (LGreater (FixedPcdGet32 (PcdChipCount), 1)) { + RD_IOVIRT_SOC_EXP_COM_INIT(4, 1, UART_START(0), 972) + RD_IOVIRT_SOC_EXP_COM_INIT(5, 1, UART_START(1), 982) + } + + if (LGreater (FixedPcdGet32 (PcdChipCount), 2)) { + RD_IOVIRT_SOC_EXP_COM_INIT(6, 2, UART_START(0), 4556) + RD_IOVIRT_SOC_EXP_COM_INIT(7, 2, UART_START(1), 4566) + } + + if (LGreater (FixedPcdGet32 (PcdChipCount), 3)) { + RD_IOVIRT_SOC_EXP_COM_INIT(8, 3, UART_START(0), 5036) + RD_IOVIRT_SOC_EXP_COM_INIT(9, 3, UART_START(1), 5046) + } + } + + // IO Virtualization SoC Expansion - PL330 DMA + RD_IOVIRT_SOC_EXP_DMA_INIT(0, 0, DMA_START(0)) + RD_IOVIRT_SOC_EXP_DMA_INIT(1, 0, DMA_START(1)) + + if (LGreater (FixedPcdGet32 (PcdChipCount), 1)) { + RD_IOVIRT_SOC_EXP_DMA_INIT(2, 1, DMA_START(0)) + RD_IOVIRT_SOC_EXP_DMA_INIT(3, 1, DMA_START(1)) + } + + if (LGreater (FixedPcdGet32 (PcdChipCount), 2)) { + RD_IOVIRT_SOC_EXP_DMA_INIT(4, 2, DMA_START(0)) + RD_IOVIRT_SOC_EXP_DMA_INIT(5, 2, DMA_START(1)) + } + + if (LGreater (FixedPcdGet32 (PcdChipCount), 3)) { + RD_IOVIRT_SOC_EXP_DMA_INIT(6, 3, DMA_START(0)) + RD_IOVIRT_SOC_EXP_DMA_INIT(7, 3, DMA_START(1)) + }Is it possible to decide to include the definitions at build time with: #if (FixedPcdGet32 (PcdChipCount) > 3) ? Same comment for other 'if (LGreater (...'Yes, it should be possible. I will update the patch with the suggested change. [1] https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/AcpiTables/Pci.asl#L116 Best regards Vivek+ } // Scope(_SB) +} diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec index e878af24d56b..407f03c1c3e8 100644 --- a/Platform/ARM/SgiPkg/SgiPlatform.dec +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec @@ -98,5 +98,10 @@ # Address bus width gArmSgiTokenSpaceGuid.PcdMaxAddressBitsPerChip|0x0|UINT64|0x00000027 + # IO virtualization block + gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlk0Base|0|UINT64|0x0000002B + gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkPeriOffset|0|UINT32|0x0000002C + gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkResourceSize|0|UINT32|0x0000002D + [Ppis] gNtFwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#99724): https://edk2.groups.io/g/devel/message/99724 Mute This Topic: https://groups.io/mt/96562647/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
