Hi Sami,

Thank you for the review.
Regarding the use of Dynamic Tables Framework, there are no short term plans to migrate to it. Please find my response for your comment inline -

On Thu, Jul 21, 2022 at 01:42 PM, Sami Mujawar wrote:

> 
> Hi Rohit,
> 
> Have you considered moving to use Dynamic Tables Framework? There is
> just too much repetition in this series which can be easily avoided. It
> will also make the code more maintainable.
> 
> Apart from this I have a comment marked inline as [SAMI].
> 
> Regards,
> 
> Sami Mujawar
> 
> On 04/07/2022 05:59 pm, Rohit Mathew wrote:
> 
>> Add a new device entry in the SSDT ACPI table to describe the serial
>> port used as the debug port. On the Neoverse reference design platforms,
>> the UART0 port of the SoC is used as the debug port.
>> 
>> Signed-off-by: Rohit Mathew <rohit.mathew@arm.com>
>> ---
>> Platform/ARM/SgiPkg/AcpiTables/RdE1EdgeAcpiTables.inf | 1 +
>> Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeAcpiTables.inf | 1 +
>> Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf | 1 +
>> Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf | 1 +
>> Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf | 1 +
>> Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf | 1 +
>> Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf | 1 +
>> Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf | 1 +
>> Platform/ARM/SgiPkg/AcpiTables/SsdtRos.asl | 15 +++++++++++++++
>> 9 files changed, 23 insertions(+)
>> 
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdE1EdgeAcpiTables.inf
>> b/Platform/ARM/SgiPkg/AcpiTables/RdE1EdgeAcpiTables.inf
>> index d2935f1e73e1..d46ae0274d90 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/RdE1EdgeAcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/RdE1EdgeAcpiTables.inf
>> @@ -39,6 +39,7 @@ [Packages]
>> [FixedPcd]
>> gArmPlatformTokenSpaceGuid.PcdCoreCount
>> gArmPlatformTokenSpaceGuid.PcdClusterCount
>> + gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
>> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>> gArmPlatformTokenSpaceGuid.PL011UartInterrupt
>> 
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeAcpiTables.inf
>> b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeAcpiTables.inf
>> index 73f47ece7718..4bf681d3bc2e 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeAcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeAcpiTables.inf
>> @@ -39,6 +39,7 @@ [Packages]
>> [FixedPcd]
>> gArmPlatformTokenSpaceGuid.PcdCoreCount
>> gArmPlatformTokenSpaceGuid.PcdClusterCount
>> + gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
>> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>> gArmPlatformTokenSpaceGuid.PL011UartInterrupt
>> 
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
>> b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
>> index da14120bde69..89f532217ceb 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
>> @@ -41,6 +41,7 @@ [Packages]
>> [FixedPcd]
>> gArmPlatformTokenSpaceGuid.PcdCoreCount
>> gArmPlatformTokenSpaceGuid.PcdClusterCount
>> + gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
>> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>> gArmPlatformTokenSpaceGuid.PL011UartInterrupt
>> 
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
>> b/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
>> index 90976250445e..66d5422df36b 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
>> @@ -37,6 +37,7 @@ [Packages]
>> Platform/ARM/SgiPkg/SgiPlatform.dec
>> 
>> [FixedPcd]
>> + gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
>> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>> gArmPlatformTokenSpaceGuid.PL011UartInterrupt
>> gArmPlatformTokenSpaceGuid.PcdCoreCount
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf
>> b/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf
>> index 95fb446c105d..742734ab7348 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf
>> @@ -37,6 +37,7 @@ [Packages]
>> Platform/ARM/SgiPkg/SgiPlatform.dec
>> 
>> [FixedPcd]
>> + gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
>> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>> gArmPlatformTokenSpaceGuid.PcdCoreCount
>> gArmPlatformTokenSpaceGuid.PcdClusterCount
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf
>> b/Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf
>> index 3540575dd641..cc41dda1a135 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf
>> @@ -37,6 +37,7 @@ [Packages]
>> Platform/ARM/SgiPkg/SgiPlatform.dec
>> 
>> [FixedPcd]
>> + gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
>> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>> gArmPlatformTokenSpaceGuid.PL011UartInterrupt
>> gArmPlatformTokenSpaceGuid.PcdCoreCount
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf
>> b/Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf
>> index c6bd69b4a538..ecb42bf3cc33 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf
>> @@ -39,6 +39,7 @@ [Packages]
>> Platform/ARM/SgiPkg/SgiPlatform.dec
>> 
>> [FixedPcd]
>> + gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
>> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>> gArmPlatformTokenSpaceGuid.PL011UartInterrupt
>> gArmPlatformTokenSpaceGuid.PcdCoreCount
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
>> b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
>> index cb3f3fcdb9b6..379b5c9e6122 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
>> @@ -39,6 +39,7 @@ [Packages]
>> [FixedPcd]
>> gArmPlatformTokenSpaceGuid.PcdCoreCount
>> gArmPlatformTokenSpaceGuid.PcdClusterCount
>> + gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
>> gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
>> gArmPlatformTokenSpaceGuid.PL011UartInterrupt
>> 
>> diff --git a/Platform/ARM/SgiPkg/AcpiTables/SsdtRos.asl
>> b/Platform/ARM/SgiPkg/AcpiTables/SsdtRos.asl
>> index fd20c67e1225..ab8578072836 100644
>> --- a/Platform/ARM/SgiPkg/AcpiTables/SsdtRos.asl
>> +++ b/Platform/ARM/SgiPkg/AcpiTables/SsdtRos.asl
>> @@ -29,6 +29,21 @@ DefinitionBlock ("SsdtRosTable.aml", "SSDT", 2,
>> "ARMLTD", "ARMSGI",
>> })
>> }
>> 
>> + Device (COM1) {
>> + Name (_HID, "ARMH0011")
>> + Name (_CID, "ARMH0011")
> 
> [SAMI] Any reason for not using  ARMHB000 (see Section 2.3 of the ACPI
> for Arm Components 1.1 specification)?

[Rohit]: COM1 is based on PL011. Since PL011 would satisfy SBSA compliance, we have used PL011's HID.

> 
> 
>> + Name (_UID, One)
>> + Name (_STA, 0xF)
>> + Name (_CRS, ResourceTemplate () {
>> + Memory32Fixed (
>> + ReadWrite,
>> + FixedPcdGet64 (PcdSerialDbgRegisterBase),
>> + 0x1000
>> + )
>> + Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
>> FixedPcdGet32 (PcdSerialDbgInterrupt) }
>> + })
>> + }
>> +
>> // VIRTIO DISK
>> Device (VR00) {
>> Name (_HID, "LNRO0005")
> 
> 

Regards,
Rohit