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 ( 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. >> >> > > > > [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 ( > https://github.com/torvalds/linux/commit/ac442a077acf9a6bf1db4320ec0c3f303be092b3 > ) > > > > 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.