Gentle reminder for this patch series. Regards, Rohit From: Rohit Mathew Sent: 02 August 2022 14:51 To: devel@edk2.groups.io; Sami Mujawar Cc: Thanu Rangarajan ; Thomas Abraham ; nd Subject: RE: [edk2-devel] [PATCH 2/2] Platform/Sgi: Add serial debug controller to SSDT Hi Sami, Gentle reminder for this patch series. Regards, Rohit. From: devel@edk2.groups.io > On Behalf Of Rohit Mathew via groups.io Sent: 25 July 2022 15:56 To: Thanu Rangarajan >; devel@edk2.groups.io Subject: Re: [edk2-devel] [PATCH 2/2] Platform/Sgi: Add serial debug controller to SSDT Hi Sami/Thanu, Please find my response inline - On Mon, Jul 25, 2022 at 02:54 PM, Thanu Rangarajan > wrote: Hi Rohit, The decision to use the SBSA defined HID for the Generic UART was taken after extensive discussions within the Arm ecosystem. And as Sami points out, now that formal Linux driver support for this HID is available, it would be good if it is used by other components in the stack as well. + Samer for any additional comments/clarifications. Regards, Thanu From: Sami Mujawar > Date: Monday, 25 July 2022 at 16:12 To: Rohit Mathew >, "devel@edk2.groups.io" > Cc: Thanu Rangarajan >, nd > Subject: Re: [edk2-devel] [PATCH 2/2] Platform/Sgi: Add serial debug controller to SSDT Hi Rohit, Please find my response inline marked [SAMI]. Regards, Sami Mujawar On 22/07/2022 01:46 pm, Rohit Mathew wrote: 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 --- 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. [SAMI] Following is an extract from Section 2.3 of the 'ACPI for Arm Components 1.1' specification. "Some operating systems use the PL011 HID (see Table 5 above) to bind to the Arm Generic UART in the system. While this practice is flawed and not encouraged by Arm, Arm acknowledges that it must be permitted until formal support for the Arm Generic UART HID is made available in these operating systems. Arm strongly recommends use of the Arm Generic UART HID going forward." 1. The Arm Generic UART is a subset of PL011. This means using the ARMHB000 should not be an issue. 2. This file is common to all platforms in SgiPkg, which are infrastructure platforms. 3. Some opreating systems (like Linux) have already integrated support for ARMHB000. Ref: serial: pl011: Add ACPI SBSA UART match id Considering the above, I think ARMHB000 should be used, here. [/SAMI] [Rohit] Apologies for not adding more context earlier. RD FVPs instantiates PL011 UART and not the Generic SBSA UART. Although 'Generic UART' would work as it is a subset of PL011, PL011 HID accurately describes the UART controllers for infra platforms. I'm not sure if this is any better explanation; Please let me know if you think otherwise. [/Rohit] + 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 IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.