[edk2-devel] [edk2-platforms][PATCH V2 3/5] Platform/Sgi: Add SSDT table for IO virtualization SoC expansion block
Vivek Kumar Gautam
vivek.gautam at arm.com
Fri Feb 10 07:54:12 UTC 2023
Hi Pierre,
On 2/7/23 14:20, Pierre Gondois wrote:
> Hello Vivek,
> Thanks for the answers,
>
> On 2/7/23 07:59, Vivek Kumar Gautam wrote:
>> Hi Pierre
>>
>> On 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.gautam at 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 the
>>>
>>> I 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.
Cool. Thanks.
>
>>
>>>
>>>> +
>>>> +// 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 cur
>>
>> Sorry, I didn't understand your comment here.
>
> Sorry I didn't finish the sentence. I was wondering what TDL files were
> for.
I think this is a stray comment. I will remove it.
>
>>
>>>> +
>>>> + @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.
Yes, I also found it while looking for few references and it is
functional :-)
Please let me know if you want me to change anything here. I can send
the updated series soon after addressing your review comments.
[snip]
Best regards
Vivek
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99965): https://edk2.groups.io/g/devel/message/99965
Mute This Topic: https://groups.io/mt/96562647/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list