From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [PATCH 2/2] Platform/Sgi: Add serial debug controller to SSDT To: Sami Mujawar ,devel@edk2.groups.io From: "Rohit Mathew" X-Originating-Location: Cambridge, England, GB (217.140.99.251) X-Originating-Platform: Windows Chrome 103 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Fri, 22 Jul 2022 05:46:44 -0700 References: In-Reply-To: Message-ID: <3602.1658494004063183052@groups.io> Content-Type: multipart/alternative; boundary="wLhyyqbfRwkXf5L27rwt" --wLhyyqbfRwkXf5L27rwt Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Sami, Thank you for the review. Regarding the use of Dynamic Tables Framework, there are no short term plan= s to migrate to it. Please find my response for your comment inline - On Thu, Jul 21, 2022 at 01:42 PM, Sami Mujawar wrote: >=20 > Hi Rohit, >=20 > 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. >=20 > Apart from this I have a comment marked inline as [SAMI]. >=20 > Regards, >=20 > Sami Mujawar >=20 > On 04/07/2022 05:59 pm, Rohit Mathew wrote: >=20 >> 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. >>=20 >> 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(+) >>=20 >> 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 >>=20 >> 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 >>=20 >> 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 >>=20 >> 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 >>=20 >> [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 >>=20 >> [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 >>=20 >> [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 >>=20 >> [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 >>=20 >> 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", >> }) >> } >>=20 >> + Device (COM1) { >> + Name (_HID, "ARMH0011") >> + Name (_CID, "ARMH0011") >=20 > [SAMI] Any reason for not using=C2=A0 ARMHB000 (see Section 2.3 of the AC= PI > for Arm Components 1.1 specification)? [Rohit]: COM1 is based on PL011. Since PL011 would satisfy SBSA compliance,= we have used PL011's HID. >=20 >=20 >> + 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") >=20 >=20 Regards, Rohit --wLhyyqbfRwkXf5L27rwt Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Sami,

Thank you for the review.
Regarding the use of Dyn= amic Tables Framework, there are no short term plans to migrate to it. Plea= se 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 wh= ich can be easily avoided. It
will also make the code more maintainab= le.

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 s= erial
port used as the debug port. On the Neoverse reference design pl= atforms,
the UART0 port of the SoC is used as the debug port.
Signed-off-by: Rohit Mathew <rohit.mathew@arm.com>
---
P= latform/ARM/SgiPkg/AcpiTables/RdE1EdgeAcpiTables.inf | 1 +
Platform/AR= M/SgiPkg/AcpiTables/RdN1EdgeAcpiTables.inf | 1 +
Platform/ARM/SgiPkg/A= cpiTables/RdN1EdgeX2AcpiTables.inf | 1 +
Platform/ARM/SgiPkg/AcpiTable= s/RdN2AcpiTables.inf | 1 +
Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1Acpi= Tables.inf | 1 +
Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf | 1= +
Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf | 1 +
Plat= form/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf | 1 +
Platform/ARM/Sgi= Pkg/AcpiTables/SsdtRos.asl | 15 +++++++++++++++
9 files changed, 23 in= sertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdE1Edge= AcpiTables.inf b/Platform/ARM/SgiPkg/AcpiTables/RdE1EdgeAcpiTables.inf
index d2935f1e73e1..d46ae0274d90 100644
--- a/Platform/ARM/SgiPkg/Acp= iTables/RdE1EdgeAcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/Rd= E1EdgeAcpiTables.inf
@@ -39,6 +39,7 @@ [Packages]
[FixedPcd]
gArmPlatformTokenSpaceGuid.PcdCoreCount
gArmPlatformTokenSpaceGuid.Pc= dClusterCount
+ gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
= gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
gArmPlatformTokenS= paceGuid.PL011UartInterrupt

diff --git a/Platform/ARM/SgiPkg/Acp= iTables/RdN1EdgeAcpiTables.inf b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeAcp= iTables.inf
index 73f47ece7718..4bf681d3bc2e 100644
--- a/Platfor= m/ARM/SgiPkg/AcpiTables/RdN1EdgeAcpiTables.inf
+++ b/Platform/ARM/SgiP= kg/AcpiTables/RdN1EdgeAcpiTables.inf
@@ -39,6 +39,7 @@ [Packages]
[FixedPcd]
gArmPlatformTokenSpaceGuid.PcdCoreCount
gArmPlatformT= okenSpaceGuid.PcdClusterCount
+ gArmPlatformTokenSpaceGuid.PcdSerialDb= gInterrupt
gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
gA= rmPlatformTokenSpaceGuid.PL011UartInterrupt

diff --git a/Platfor= m/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf b/Platform/ARM/SgiPkg/Acpi= Tables/RdN1EdgeX2AcpiTables.inf
index da14120bde69..89f532217ceb 10064= 4
--- a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
+= ++ b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
@@ -41,6 = +41,7 @@ [Packages]
[FixedPcd]
gArmPlatformTokenSpaceGuid.PcdCore= Count
gArmPlatformTokenSpaceGuid.PcdClusterCount
+ gArmPlatformTo= kenSpaceGuid.PcdSerialDbgInterrupt
gArmPlatformTokenSpaceGuid.PcdSeria= lDbgRegisterBase
gArmPlatformTokenSpaceGuid.PL011UartInterrupt
diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf b/Platfo= rm/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
index 90976250445e..66d542= 2df36b 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf<= br />+++ b/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
@@ -37,6 = +37,7 @@ [Packages]
Platform/ARM/SgiPkg/SgiPlatform.dec

[Fi= xedPcd]
+ gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
gArmPl= atformTokenSpaceGuid.PcdSerialDbgRegisterBase
gArmPlatformTokenSpaceGu= id.PL011UartInterrupt
gArmPlatformTokenSpaceGuid.PcdCoreCount
dif= f --git a/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf b/Platform/= ARM/SgiPkg/AcpiTables/RdN2Cfg1AcpiTables.inf
index 95fb446c105d..74273= 4ab7348 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.PcdSerialDbgInterruptgArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
gArmPlatformTok= enSpaceGuid.PcdCoreCount
gArmPlatformTokenSpaceGuid.PcdClusterCountdiff --git a/Platform/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf b/Platfor= m/ARM/SgiPkg/AcpiTables/RdV1AcpiTables.inf
index 3540575dd641..cc41dda= 1a135 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

[Fix= edPcd]
+ gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
gArmPla= tformTokenSpaceGuid.PcdSerialDbgRegisterBase
gArmPlatformTokenSpaceGui= d.PL011UartInterrupt
gArmPlatformTokenSpaceGuid.PcdCoreCount
diff= --git a/Platform/ARM/SgiPkg/AcpiTables/RdV1McAcpiTables.inf b/Platform/ARM= /SgiPkg/AcpiTables/RdV1McAcpiTables.inf
index c6bd69b4a538..ecb42bf3cc= 33 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

[Fi= xedPcd]
+ gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
gArmPl= atformTokenSpaceGuid.PcdSerialDbgRegisterBase
gArmPlatformTokenSpaceGu= id.PL011UartInterrupt
gArmPlatformTokenSpaceGuid.PcdCoreCount
dif= f --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf b/Platform/AR= M/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
index cb3f3fcdb9b6..379b5c9e6= 122 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf+++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
@@ -39,6= +39,7 @@ [Packages]
[FixedPcd]
gArmPlatformTokenSpaceGuid.PcdCor= eCount
gArmPlatformTokenSpaceGuid.PcdClusterCount
+ gArmPlatformT= okenSpaceGuid.PcdSerialDbgInterrupt
gArmPlatformTokenSpaceGuid.PcdSeri= alDbgRegisterBase
gArmPlatformTokenSpaceGuid.PL011UartInterrupt
<= br />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, "ARMH= 0011")
[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, R= esourceTemplate () {
+ Memory32Fixed (
+ ReadWrite,
+ FixedP= cdGet64 (PcdSerialDbgRegisterBase),
+ 0x1000
+ )
+ Interrupt= (ResourceConsumer, Level, ActiveHigh, Exclusive) { FixedPcdGet32 (PcdSeria= lDbgInterrupt) }
+ })
+ }
+
// VIRTIO DISK
Device = (VR00) {
Name (_HID, "LNRO0005")
Regards,
Rohit --wLhyyqbfRwkXf5L27rwt--