[edk2-devel] [PATCH 2/2] Platform/Sgi: Add serial debug controller to SSDT
Rohit Mathew
rohit.mathew at arm.com
Mon Jul 25 14:55:47 UTC 2022
Hi Sami/Thanu,
Please find my response inline -
On Mon, Jul 25, 2022 at 02:54 PM, Thanu Rangarajan <Thanu.Rangarajan at 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 at arm.com>
> *Date:* Monday, 25 July 2022 at 16:12
> *To:* Rohit Mathew <Rohit.Mathew at arm.com>, "devel at edk2.groups.io"
> <devel at edk2.groups.io>
> *Cc:* Thanu Rangarajan <Thanu.Rangarajan at arm.com>, nd <nd at 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 at arm.com> ( rohit.mathew at 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91808): https://edk2.groups.io/g/devel/message/91808
Mute This Topic: https://groups.io/mt/92169056/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20220725/bf28cb90/attachment-0001.htm>
More information about the edk2-devel-archive
mailing list