Re: [edk2-devel] [PATCH 2/2] Platform/Sgi: Add serial debug controller to SSDT

["Rohit Mathew" <rohit.mathew@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 11065 bytes --]

Hi Sami/Thanu,

Please find my response inline -

On Mon, Jul 25, 2022 at 02:54 PM, Thanu Rangarajan <Thanu.Rangarajan@arm.com> 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 <Sami.Mujawar@arm.com>
> *Date:* Monday, 25 July 2022 at 16:12
> *To:* Rohit Mathew <Rohit.Mathew@arm.com>, "devel@edk2.groups.io"
> <devel@edk2.groups.io>
> *Cc:* Thanu Rangarajan <Thanu.Rangarajan@arm.com>, nd <nd@arm.com>
> *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> ( 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.

[-- Attachment #2: Type: text/html, Size: 12666 bytes --]