[edk2-devel] [PATCH edk2-platforms v3 0/4] Provide XHCI USB controller only for newer hardware

[edk2-devel] [PATCH edk2-platforms v3 0/4] Provide XHCI USB controller only for newer hardware

[Marcin Juszkiewicz]

Platform version 0.3 introduced XHCI USB controller instead of EHCI one.
But we did it in a way that there is no in-EDK2 check for platform
version (XHCI is always given).

This behaviour works with Linux as it complains about being unable to
initialize EHCI and goes on. Free/Net/Open BSD systems hang in such
situation.

Now we check are we on Platform Version 0.3 at least and then initialize
XHCI controller. Otherwise we disable it's node in DSDT table.

I checked several ways to handle the situation. 

SSDT overlay enabling with LocateAndInstallAcpiFromFvConfitional() was
first one but this function had only DBG2 and FACP tables.

Then looked at trying to detect XHCI from _STA method. But this is
sysbus device which is there or not without way to discover it.

Next attempt was to have variable in DSDT and to use it in _STA. Too
much trouble.

Then looked again at code from
Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiDsdt.c and noticed
UpdateStatusMethodObject() function. Copied some code and it worked.

Booting OpenBSD reminded me to update table checksum.

Nhi Pham pointed me to AcpiLib/AcpiAmlObjectUpdateInteger() function. So
v3 got AML variable (called XHCI) which is set to 0xF (enabled, working)
by default or to 0x0 (disabled, ignore) on Platform Version below 0.3
one.

This allowed to remove copy of UpdateStatusMethodObject() function.

---
Changes in v3:
- use AML variable in DSDT

Changes in v2:
- XHCI initialized only on PlatVer 0.3+
- XHCI disabled in DSDT for older platforms
- no SSDT overlays for EHCI/XHCI
- no EHCI at all (it does not work anyway)
- no Pcd renaming

---
Marcin Juszkiewicz (4):
      SbsaQemu: introduce macro to compare platform version
      SbsaQemu: add AcpiLib
      SbsaQemu: initialize XHCI only if it exists
      SbsaQemu: disable XHCI in DSDT if not present

 Platform/Qemu/SbsaQemu/SbsaQemu.dsc                  |  2 +
 .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf      |  4 ++
 .../IndustryStandard/SbsaQemuPlatformVersion.h       | 25 ++++++++
 .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c        | 60 ++++++++++++++++++++
 .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c        | 47 ++++++++-------
 Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl            |  3 +-
 6 files changed, 118 insertions(+), 23 deletions(-)
---
base-commit: 06f6274d56422084281886fad447ab117fd0e487
change-id: 20231013-ehci-xhci-fix-c529356a7a8f

Best regards,
-- 
Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109681): https://edk2.groups.io/g/devel/message/109681
Mute This Topic: https://groups.io/mt/102017312/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms v3 1/4] SbsaQemu: introduce macro to compare platform version

[Marcin Juszkiewicz]

We want to check "if platver < 0.3" in an easy way.
---
 .../IndustryStandard/SbsaQemuPlatformVersion.h       | 25 ++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h
new file mode 100644
index 000000000000..dd2483787002
--- /dev/null
+++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h
@@ -0,0 +1,25 @@
+/** @file
+*
+*  Copyright (c) Linaro Limited. All rights reserved.
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef SBSAQEMUPLATFORM_VERSION_H
+#define SBSAQEMUPLATFORM_VERSION_H
+
+/*
+ * Compare PlatformVersion
+ *
+ */
+
+#define PLATFORM_VERSION_LESS_THAN(Major, Minor) (     \
+  (                                                    \
+    ( PcdGet32 (PcdPlatformVersionMajor) < Major)   || \
+    (                                                  \
+      ( PcdGet32 (PcdPlatformVersionMajor) == Major) && \
+      ( PcdGet32 (PcdPlatformVersionMinor) < Minor)    \
+    )                                                  \
+  )                                                    \
+)
+#endif

-- 
2.41.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109682): https://edk2.groups.io/g/devel/message/109682
Mute This Topic: https://groups.io/mt/102017313/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v3 1/4] SbsaQemu: introduce macro to compare platform version

[Ard Biesheuvel]

On Tue, 17 Oct 2023 at 15:23, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
> We want to check "if platver < 0.3" in an easy way.
> ---
>  .../IndustryStandard/SbsaQemuPlatformVersion.h       | 25 ++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h
> new file mode 100644
> index 000000000000..dd2483787002
> --- /dev/null
> +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h
> @@ -0,0 +1,25 @@
> +/** @file
> +*
> +*  Copyright (c) Linaro Limited. All rights reserved.
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef SBSAQEMUPLATFORM_VERSION_H
> +#define SBSAQEMUPLATFORM_VERSION_H
> +
> +/*
> + * Compare PlatformVersion
> + *
> + */
> +
> +#define PLATFORM_VERSION_LESS_THAN(Major, Minor) (     \
> +  (                                                    \
> +    ( PcdGet32 (PcdPlatformVersionMajor) < Major)   || \
> +    (                                                  \
> +      ( PcdGet32 (PcdPlatformVersionMajor) == Major) && \
> +      ( PcdGet32 (PcdPlatformVersionMinor) < Minor)    \
> +    )                                                  \
> +  )                                                    \
> +)
> +#endif
>

I don't mind adding this here but it is slightly unidiomatic so I'd
like Leif's take on this too.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109713): https://edk2.groups.io/g/devel/message/109713
Mute This Topic: https://groups.io/mt/102017313/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v3 1/4] SbsaQemu: introduce macro to compare platform version

[Ard Biesheuvel]

On Wed, 18 Oct 2023 at 10:45, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 17 Oct 2023 at 15:23, Marcin Juszkiewicz
> <marcin.juszkiewicz@linaro.org> wrote:
> >
> > We want to check "if platver < 0.3" in an easy way.
> > ---
> >  .../IndustryStandard/SbsaQemuPlatformVersion.h       | 25 ++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h
> > new file mode 100644
> > index 000000000000..dd2483787002
> > --- /dev/null
> > +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuPlatformVersion.h
> > @@ -0,0 +1,25 @@
> > +/** @file
> > +*
> > +*  Copyright (c) Linaro Limited. All rights reserved.
> > +*
> > +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +**/
> > +
> > +#ifndef SBSAQEMUPLATFORM_VERSION_H
> > +#define SBSAQEMUPLATFORM_VERSION_H
> > +
> > +/*
> > + * Compare PlatformVersion
> > + *
> > + */
> > +
> > +#define PLATFORM_VERSION_LESS_THAN(Major, Minor) (     \
> > +  (                                                    \
> > +    ( PcdGet32 (PcdPlatformVersionMajor) < Major)   || \
> > +    (                                                  \
> > +      ( PcdGet32 (PcdPlatformVersionMajor) == Major) && \
> > +      ( PcdGet32 (PcdPlatformVersionMinor) < Minor)    \
> > +    )                                                  \
> > +  )                                                    \
> > +)
> > +#endif
> >
>
> I don't mind adding this here but it is slightly unidiomatic so I'd
> like Leif's take on this too.

... and it also lacks a s-o-b line


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109714): https://edk2.groups.io/g/devel/message/109714
Mute This Topic: https://groups.io/mt/102017313/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v3 1/4] SbsaQemu: introduce macro to compare platform version

[Marcin Juszkiewicz]

W dniu 18.10.2023 o 10:46, Ard Biesheuvel pisze:
>> I don't mind adding this here but it is slightly unidiomatic so I'd
>> like Leif's take on this too.
> ... and it also lacks a s-o-b line

Oops. Added in local copy.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109725): https://edk2.groups.io/g/devel/message/109725
Mute This Topic: https://groups.io/mt/102017313/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms v3 2/4] SbsaQemu: add AcpiLib

[Marcin Juszkiewicz]

It will be needed for playing with disabling XHCI later.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 Platform/Qemu/SbsaQemu/SbsaQemu.dsc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
index 36723e21d7b5..1e650350cb63 100644
--- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
+++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
@@ -172,6 +172,8 @@ [LibraryClasses.common]
 
   ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
 
+  AcpiLib|EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
+
   ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
   ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
 

-- 
2.41.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109683): https://edk2.groups.io/g/devel/message/109683
Mute This Topic: https://groups.io/mt/102017314/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms v3 3/4] SbsaQemu: initialize XHCI only if it exists

[Marcin Juszkiewicz]

We need platform version to be at least 0.3 to have XHCI
in virtual hardware. On older platforms there is non-working
EHCI which we ignore.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c        | 47 +++++++++++---------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
index 36ada4270bbf..76a9cd62d4a4 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c
@@ -15,6 +15,7 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiDriverEntryPoint.h>
 #include <IndustryStandard/SbsaQemuSmc.h>
+#include <IndustryStandard/SbsaQemuPlatformVersion.h>
 
 #include <Protocol/FdtClient.h>
 
@@ -57,28 +58,6 @@ InitializeSbsaQemuPlatformDxe (
     return Status;
   }
 
-  Base = (VOID*)(UINTN)PcdGet64 (PcdPlatformXhciBase);
-  ASSERT (Base != NULL);
-  Size = (UINTN)PcdGet32 (PcdPlatformXhciSize);
-  ASSERT (Size != 0);
-
-  DEBUG ((DEBUG_INFO, "%a: Got platform XHCI %llx %u\n",
-          __FUNCTION__, Base, Size));
-
-  Status = RegisterNonDiscoverableMmioDevice (
-                   NonDiscoverableDeviceTypeXhci,
-                   NonDiscoverableDeviceDmaTypeCoherent,
-                   NULL,
-                   NULL,
-                   1,
-                   Base, Size);
-
-  if (EFI_ERROR(Status)) {
-    DEBUG ((DEBUG_ERROR, "%a: NonDiscoverable: Cannot install XHCI device @%p (Staus == %r)\n",
-            __FUNCTION__, Base, Status));
-    return Status;
-  }
-
   SmcResult = ArmCallSmc0 (SIP_SVC_VERSION, &Arg0, &Arg1, NULL);
   if (SmcResult == SMC_ARCH_CALL_SUCCESS) {
     Result = PcdSet32S (PcdPlatformVersionMajor, Arg0);
@@ -118,5 +97,29 @@ InitializeSbsaQemuPlatformDxe (
 
   DEBUG ((DEBUG_INFO, "GICI base: 0x%x\n", Arg0));
 
+  if (! PLATFORM_VERSION_LESS_THAN(0, 3) ) {
+    Base = (VOID*)(UINTN)PcdGet64 (PcdPlatformXhciBase);
+    ASSERT (Base != NULL);
+    Size = (UINTN)PcdGet32 (PcdPlatformXhciSize);
+    ASSERT (Size != 0);
+
+    DEBUG ((DEBUG_INFO, "%a: Got platform XHCI %llx %u\n",
+            __FUNCTION__, Base, Size));
+
+    Status = RegisterNonDiscoverableMmioDevice (
+                     NonDiscoverableDeviceTypeXhci,
+                     NonDiscoverableDeviceDmaTypeCoherent,
+                     NULL,
+                     NULL,
+                     1,
+                     Base, Size);
+
+    if (EFI_ERROR(Status)) {
+    DEBUG ((DEBUG_ERROR, "%a: NonDiscoverable: Cannot install XHCI device @%p (Staus == %r)\n",
+            __FUNCTION__, Base, Status));
+    return Status;
+    }
+  }
+
   return EFI_SUCCESS;
 }

-- 
2.41.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109684): https://edk2.groups.io/g/devel/message/109684
Mute This Topic: https://groups.io/mt/102017315/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-platforms v3 4/4] SbsaQemu: disable XHCI in DSDT if not present

[Marcin Juszkiewicz]

We need platform version to be at least 0.3 to have XHCI
in virtual hardware. On older platforms there is non-working
EHCI which we ignore.

Set DSDT node to be disabled so operating system will not try
to initialize not-existing hardware.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf      |  4 ++
 .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c        | 60 ++++++++++++++++++++
 Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl            |  3 +-
 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
index 7c7e08e0fd3a..d5ded892d6ea 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
@@ -29,6 +29,7 @@ [Packages]
   Silicon/Qemu/SbsaQemu/SbsaQemu.dec
 
 [LibraryClasses]
+  AcpiLib
   ArmLib
   BaseMemoryLib
   BaseLib
@@ -50,6 +51,8 @@ [Pcd]
   gArmTokenSpaceGuid.PcdGicRedistributorsBase
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdGicItsBase
   gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdSmmuBase
+  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor
+  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor
 
 [Depex]
   gEfiAcpiTableProtocolGuid                       ## CONSUMES
@@ -59,6 +62,7 @@ [Guids]
 
 [Protocols]
   gEfiAcpiTableProtocolGuid                       ## CONSUMES
+  gEfiAcpiSdtProtocolGuid
 
 [FixedPcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId
diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
index fd849ca1594b..cf6e534ca3a0 100644
--- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
+++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
@@ -10,6 +10,7 @@
 #include <IndustryStandard/AcpiAml.h>
 #include <IndustryStandard/IoRemappingTable.h>
 #include <IndustryStandard/SbsaQemuAcpi.h>
+#include <IndustryStandard/SbsaQemuPlatformVersion.h>
 #include <Library/AcpiLib.h>
 #include <Library/ArmLib.h>
 #include <Library/BaseMemoryLib.h>
@@ -682,6 +683,63 @@ AddGtdtTable (
   return Status;
 }
 
+EFI_STATUS
+DisableXhciOnOlderPlatVer (
+  VOID
+  )
+{
+  EFI_STATUS            Status;
+  EFI_ACPI_SDT_PROTOCOL                       *AcpiSdtProtocol;
+  EFI_ACPI_DESCRIPTION_HEADER                 *Table;
+  UINTN                                       TableKey;
+  UINTN                                       TableIndex;
+  EFI_ACPI_HANDLE                             TableHandle;
+
+  Status = EFI_SUCCESS;
+
+  if ( PLATFORM_VERSION_LESS_THAN(0, 3) ) {
+    DEBUG ((DEBUG_ERROR, "Platform Version < 0.3 - disabling XHCI\n"));
+    Status = gBS->LocateProtocol (
+                    &gEfiAcpiSdtProtocolGuid,
+                    NULL,
+                    (VOID **)&AcpiSdtProtocol
+                    );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "Unable to locate ACPI table protocol\n"));
+      return Status;
+    }
+
+    TableIndex = 0;
+    Status = AcpiLocateTableBySignature (
+               AcpiSdtProtocol,
+               EFI_ACPI_6_3_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
+               &TableIndex,
+               &Table,
+               &TableKey
+               );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "ACPI DSDT table not found!\n"));
+      ASSERT_EFI_ERROR (Status);
+      return Status;
+    }
+
+    Status = AcpiSdtProtocol->OpenSdt (TableKey, &TableHandle);
+    if (EFI_ERROR (Status)) {
+      ASSERT_EFI_ERROR (Status);
+      AcpiSdtProtocol->Close (TableHandle);
+      return Status;
+    }
+
+    AcpiAmlObjectUpdateInteger (AcpiSdtProtocol, TableHandle, "\\_SB.USB0.XHCI", 0x0);
+
+    AcpiSdtProtocol->Close (TableHandle);
+    AcpiUpdateChecksum ((UINT8 *)Table, Table->Length);
+  }
+
+  return Status;
+}
+
+
 EFI_STATUS
 EFIAPI
 InitializeSbsaQemuAcpiDxe (
@@ -738,5 +796,7 @@ InitializeSbsaQemuAcpiDxe (
     DEBUG ((DEBUG_ERROR, "Failed to add GTDT table\n"));
   }
 
+  Status = DisableXhciOnOlderPlatVer();
+
   return EFI_SUCCESS;
 }
diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl b/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
index 543b5782580a..ba3eefc975a5 100644
--- a/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
+++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
@@ -73,8 +73,9 @@ DefinitionBlock ("DsdtTable.aml", "DSDT",
         Name (_HID, "PNP0D10")      // _HID: Hardware ID
         Name (_UID, 0x00)            // _UID: Unique ID
         Name (_CCA, 0x01)            // _CCA: Cache Coherency Attribute
+        Name (XHCI, 0xF)            // will be set using AcpiLib
         Method (_STA) {
-          Return (0xF)
+          Return (XHCI)
         }
         Method (_CRS, 0x0, Serialized) {
             Name (RBUF, ResourceTemplate() {

-- 
2.41.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109685): https://edk2.groups.io/g/devel/message/109685
Mute This Topic: https://groups.io/mt/102017316/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v3 4/4] SbsaQemu: disable XHCI in DSDT if not present

[Nhi Pham via groups.io]

Hi Marcin,

There is a nitpicking below.

Other than, it looks good to me.

Acked-by: Nhi Pham <nhi@os.amperecomputing.com>

Regards,
Nhi

On 10/17/2023 8:23 PM, Marcin Juszkiewicz wrote:
> We need platform version to be at least 0.3 to have XHCI
> in virtual hardware. On older platforms there is non-working
> EHCI which we ignore.
> 
> Set DSDT node to be disabled so operating system will not try
> to initialize not-existing hardware.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf      |  4 ++
>   .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c        | 60 ++++++++++++++++++++
>   Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl            |  3 +-
>   3 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> index 7c7e08e0fd3a..d5ded892d6ea 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> @@ -29,6 +29,7 @@ [Packages]
>     Silicon/Qemu/SbsaQemu/SbsaQemu.dec
>   
>   [LibraryClasses]
> +  AcpiLib
>     ArmLib
>     BaseMemoryLib
>     BaseLib
> @@ -50,6 +51,8 @@ [Pcd]
>     gArmTokenSpaceGuid.PcdGicRedistributorsBase
>     gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdGicItsBase
>     gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdSmmuBase
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMajor
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformVersionMinor
>   
>   [Depex]
>     gEfiAcpiTableProtocolGuid                       ## CONSUMES
> @@ -59,6 +62,7 @@ [Guids]
>   
>   [Protocols]
>     gEfiAcpiTableProtocolGuid                       ## CONSUMES
> +  gEfiAcpiSdtProtocolGuid
>   
>   [FixedPcd]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> index fd849ca1594b..cf6e534ca3a0 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> @@ -10,6 +10,7 @@
>   #include <IndustryStandard/AcpiAml.h>
>   #include <IndustryStandard/IoRemappingTable.h>
>   #include <IndustryStandard/SbsaQemuAcpi.h>
> +#include <IndustryStandard/SbsaQemuPlatformVersion.h>
>   #include <Library/AcpiLib.h>
>   #include <Library/ArmLib.h>
>   #include <Library/BaseMemoryLib.h>
> @@ -682,6 +683,63 @@ AddGtdtTable (
>     return Status;
>   }
>   
> +EFI_STATUS
> +DisableXhciOnOlderPlatVer (
> +  VOID
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_ACPI_SDT_PROTOCOL                       *AcpiSdtProtocol;
> +  EFI_ACPI_DESCRIPTION_HEADER                 *Table;
> +  UINTN                                       TableKey;
> +  UINTN                                       TableIndex;
> +  EFI_ACPI_HANDLE                             TableHandle;
> +
> +  Status = EFI_SUCCESS;
> +
> +  if ( PLATFORM_VERSION_LESS_THAN(0, 3) ) {
> +    DEBUG ((DEBUG_ERROR, "Platform Version < 0.3 - disabling XHCI\n"));
> +    Status = gBS->LocateProtocol (
> +                    &gEfiAcpiSdtProtocolGuid,
> +                    NULL,
> +                    (VOID **)&AcpiSdtProtocol
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Unable to locate ACPI table protocol\n"));
> +      return Status;
> +    }
> +
> +    TableIndex = 0;
> +    Status = AcpiLocateTableBySignature (
> +               AcpiSdtProtocol,
> +               EFI_ACPI_6_3_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
> +               &TableIndex,
> +               &Table,
> +               &TableKey
> +               );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "ACPI DSDT table not found!\n"));
> +      ASSERT_EFI_ERROR (Status);
> +      return Status;
> +    }
> +
> +    Status = AcpiSdtProtocol->OpenSdt (TableKey, &TableHandle);
> +    if (EFI_ERROR (Status)) {
> +      ASSERT_EFI_ERROR (Status);
> +      AcpiSdtProtocol->Close (TableHandle);
> +      return Status;
> +    }
> +
> +    AcpiAmlObjectUpdateInteger (AcpiSdtProtocol, TableHandle, "\\_SB.USB0.XHCI", 0x0);
> +
> +    AcpiSdtProtocol->Close (TableHandle);
> +    AcpiUpdateChecksum ((UINT8 *)Table, Table->Length);
> +  }
> +
> +  return Status;
> +}
> +
> +
>   EFI_STATUS
>   EFIAPI
>   InitializeSbsaQemuAcpiDxe (
> @@ -738,5 +796,7 @@ InitializeSbsaQemuAcpiDxe (
>       DEBUG ((DEBUG_ERROR, "Failed to add GTDT table\n"));
>     }
>   
> +  Status = DisableXhciOnOlderPlatVer();

Nit: EDK2 Coding Style says that you need a space before (. Is it 
necessary to handle the result of Status?

> +
>     return EFI_SUCCESS;
>   }
> diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl b/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
> index 543b5782580a..ba3eefc975a5 100644
> --- a/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
> +++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl
> @@ -73,8 +73,9 @@ DefinitionBlock ("DsdtTable.aml", "DSDT",
>           Name (_HID, "PNP0D10")      // _HID: Hardware ID
>           Name (_UID, 0x00)            // _UID: Unique ID
>           Name (_CCA, 0x01)            // _CCA: Cache Coherency Attribute
> +        Name (XHCI, 0xF)            // will be set using AcpiLib
>           Method (_STA) {
> -          Return (0xF)
> +          Return (XHCI)
>           }
>           Method (_CRS, 0x0, Serialized) {
>               Name (RBUF, ResourceTemplate() {
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109703): https://edk2.groups.io/g/devel/message/109703
Mute This Topic: https://groups.io/mt/102017316/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v3 4/4] SbsaQemu: disable XHCI in DSDT if not present

[Marcin Juszkiewicz]

W dniu 18.10.2023 o 05:28, Nhi Pham pisze:
> Hi Marcin,
> 
> There is a nitpicking below.
> 
> Other than, it looks good to me.
> 
> Acked-by: Nhi Pham <nhi@os.amperecomputing.com>
> 
>> a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c 
>> b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
>> index fd849ca1594b..cf6e534ca3a0 100644
>> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
>> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
>> @@ -10,6 +10,7 @@
>>   #include <IndustryStandard/AcpiAml.h>
>>   #include <IndustryStandard/IoRemappingTable.h>
>>   #include <IndustryStandard/SbsaQemuAcpi.h>
>> +#include <IndustryStandard/SbsaQemuPlatformVersion.h>
>>   #include <Library/AcpiLib.h>
>>   #include <Library/ArmLib.h>
>>   #include <Library/BaseMemoryLib.h>
>> @@ -682,6 +683,63 @@ AddGtdtTable (
>>     return Status;
>>   }
>> +EFI_STATUS
>> +DisableXhciOnOlderPlatVer (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS            Status;
>> +  EFI_ACPI_SDT_PROTOCOL                       *AcpiSdtProtocol;
>> +  EFI_ACPI_DESCRIPTION_HEADER                 *Table;
>> +  UINTN                                       TableKey;
>> +  UINTN                                       TableIndex;
>> +  EFI_ACPI_HANDLE                             TableHandle;
>> +
>> +  Status = EFI_SUCCESS;
>> +
>> +  if ( PLATFORM_VERSION_LESS_THAN(0, 3) ) {
>> +    DEBUG ((DEBUG_ERROR, "Platform Version < 0.3 - disabling XHCI\n"));
>> +    Status = gBS->LocateProtocol (
>> +                    &gEfiAcpiSdtProtocolGuid,
>> +                    NULL,
>> +                    (VOID **)&AcpiSdtProtocol
>> +                    );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "Unable to locate ACPI table protocol\n"));
>> +      return Status;
>> +    }
>> +
>> +    TableIndex = 0;
>> +    Status = AcpiLocateTableBySignature (
>> +               AcpiSdtProtocol,
>> +               
>> EFI_ACPI_6_3_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
>> +               &TableIndex,
>> +               &Table,
>> +               &TableKey
>> +               );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "ACPI DSDT table not found!\n"));
>> +      ASSERT_EFI_ERROR (Status);
>> +      return Status;
>> +    }
>> +
>> +    Status = AcpiSdtProtocol->OpenSdt (TableKey, &TableHandle);
>> +    if (EFI_ERROR (Status)) {
>> +      ASSERT_EFI_ERROR (Status);
>> +      AcpiSdtProtocol->Close (TableHandle);
>> +      return Status;
>> +    }
>> +
>> +    AcpiAmlObjectUpdateInteger (AcpiSdtProtocol, TableHandle, 
>> "\\_SB.USB0.XHCI", 0x0);
>> +
>> +    AcpiSdtProtocol->Close (TableHandle);
>> +    AcpiUpdateChecksum ((UINT8 *)Table, Table->Length);
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>> +
>>   EFI_STATUS
>>   EFIAPI
>>   InitializeSbsaQemuAcpiDxe (
>> @@ -738,5 +796,7 @@ InitializeSbsaQemuAcpiDxe (
>>       DEBUG ((DEBUG_ERROR, "Failed to add GTDT table\n"));
>>     }
>> +  Status = DisableXhciOnOlderPlatVer();
> 
> Nit: EDK2 Coding Style says that you need a space before (. 

Ah, right. forgot to crucify the source.

> Is it necessary to handle the result of Status?

EDK2 is full of handling Status on touching ACPI tables. So I followed.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109711): https://edk2.groups.io/g/devel/message/109711
Mute This Topic: https://groups.io/mt/102017316/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v3 4/4] SbsaQemu: disable XHCI in DSDT if not present

[Ard Biesheuvel]

On Wed, 18 Oct 2023 at 07:56, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
> W dniu 18.10.2023 o 05:28, Nhi Pham pisze:
> > Hi Marcin,
> >
> > There is a nitpicking below.
> >
> > Other than, it looks good to me.
> >
> > Acked-by: Nhi Pham <nhi@os.amperecomputing.com>
> >
> >> a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> >> b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> >> index fd849ca1594b..cf6e534ca3a0 100644
> >> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> >> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> >> @@ -10,6 +10,7 @@
> >>   #include <IndustryStandard/AcpiAml.h>
> >>   #include <IndustryStandard/IoRemappingTable.h>
> >>   #include <IndustryStandard/SbsaQemuAcpi.h>
> >> +#include <IndustryStandard/SbsaQemuPlatformVersion.h>
> >>   #include <Library/AcpiLib.h>
> >>   #include <Library/ArmLib.h>
> >>   #include <Library/BaseMemoryLib.h>
> >> @@ -682,6 +683,63 @@ AddGtdtTable (
> >>     return Status;
> >>   }
> >> +EFI_STATUS
> >> +DisableXhciOnOlderPlatVer (
> >> +  VOID
> >> +  )
> >> +{
> >> +  EFI_STATUS            Status;
> >> +  EFI_ACPI_SDT_PROTOCOL                       *AcpiSdtProtocol;
> >> +  EFI_ACPI_DESCRIPTION_HEADER                 *Table;
> >> +  UINTN                                       TableKey;
> >> +  UINTN                                       TableIndex;
> >> +  EFI_ACPI_HANDLE                             TableHandle;
> >> +
> >> +  Status = EFI_SUCCESS;
> >> +
> >> +  if ( PLATFORM_VERSION_LESS_THAN(0, 3) ) {
> >> +    DEBUG ((DEBUG_ERROR, "Platform Version < 0.3 - disabling XHCI\n"));
> >> +    Status = gBS->LocateProtocol (
> >> +                    &gEfiAcpiSdtProtocolGuid,
> >> +                    NULL,
> >> +                    (VOID **)&AcpiSdtProtocol
> >> +                    );
> >> +    if (EFI_ERROR (Status)) {
> >> +      DEBUG ((DEBUG_ERROR, "Unable to locate ACPI table protocol\n"));
> >> +      return Status;
> >> +    }
> >> +
> >> +    TableIndex = 0;
> >> +    Status = AcpiLocateTableBySignature (
> >> +               AcpiSdtProtocol,
> >> +
> >> EFI_ACPI_6_3_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
> >> +               &TableIndex,
> >> +               &Table,
> >> +               &TableKey
> >> +               );
> >> +    if (EFI_ERROR (Status)) {
> >> +      DEBUG ((DEBUG_ERROR, "ACPI DSDT table not found!\n"));
> >> +      ASSERT_EFI_ERROR (Status);
> >> +      return Status;
> >> +    }
> >> +
> >> +    Status = AcpiSdtProtocol->OpenSdt (TableKey, &TableHandle);
> >> +    if (EFI_ERROR (Status)) {
> >> +      ASSERT_EFI_ERROR (Status);
> >> +      AcpiSdtProtocol->Close (TableHandle);
> >> +      return Status;
> >> +    }
> >> +
> >> +    AcpiAmlObjectUpdateInteger (AcpiSdtProtocol, TableHandle,
> >> "\\_SB.USB0.XHCI", 0x0);
> >> +
> >> +    AcpiSdtProtocol->Close (TableHandle);
> >> +    AcpiUpdateChecksum ((UINT8 *)Table, Table->Length);
> >> +  }
> >> +
> >> +  return Status;
> >> +}
> >> +
> >> +
> >>   EFI_STATUS
> >>   EFIAPI
> >>   InitializeSbsaQemuAcpiDxe (
> >> @@ -738,5 +796,7 @@ InitializeSbsaQemuAcpiDxe (
> >>       DEBUG ((DEBUG_ERROR, "Failed to add GTDT table\n"));
> >>     }
> >> +  Status = DisableXhciOnOlderPlatVer();
> >
> > Nit: EDK2 Coding Style says that you need a space before (.
>
> Ah, right. forgot to crucify the source.
>
> > Is it necessary to handle the result of Status?
>
> EDK2 is full of handling Status on touching ACPI tables. So I followed.
>

Can you just do 'return DisableXhciOnOlderPlatVer();' instead?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109715): https://edk2.groups.io/g/devel/message/109715
Mute This Topic: https://groups.io/mt/102017316/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH edk2-platforms v3 4/4] SbsaQemu: disable XHCI in DSDT if not present

[Marcin Juszkiewicz]

W dniu 18.10.2023 o 10:47, Ard Biesheuvel pisze:
>>> Nit: EDK2 Coding Style says that you need a space before (.
>> Ah, right. forgot to crucify the source.
>>
>>> Is it necessary to handle the result of Status?
>> EDK2 is full of handling Status on touching ACPI tables. So I followed.
>>
> Can you just do 'return DisableXhciOnOlderPlatVer();' instead?

Done. Sent v4.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109724): https://edk2.groups.io/g/devel/message/109724
Mute This Topic: https://groups.io/mt/102017316/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-