public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <quic_llindhol@quicinc.com>
To: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Cc: <devel@edk2.groups.io>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>,
	Jeremy Linton <jeremy.linton@arm.com>,
	Nhi Pham <nhi@os.amperecomputing.com>,
	Chuong Tran <chuong@os.amperecomputing.com>,
	Rebecca Cran <rebecca@os.amperecomputing.com>
Subject: Re: [edk2-devel] [PATCH edk2-platforms v5 4/4] SbsaQemu: disable XHCI in DSDT if not present
Date: Thu, 26 Oct 2023 17:56:29 +0100	[thread overview]
Message-ID: <ZTqaPYmkQB39NGwz@qc-i7.hemma.eciton.net> (raw)
In-Reply-To: <20231018-ehci-xhci-fix-v5-4-f42995a3dfd9@linaro.org>

Couple of minor comments on this patch, rest of set good to go:

On Wed, Oct 18, 2023 at 13:55:41 +0200, 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        | 68 +++++++++++++++++++-
>  Silicon/Qemu/SbsaQemu/AcpiTables/Dsdt.asl            |  3 +-
>  3 files changed, 73 insertions(+), 2 deletions(-)
> 

> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> index fd849ca1594b..464119de1457 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,71 @@ AddGtdtTable (
>    return Status;
>  }
>  
> +/*
> + * A function to disable XHCI node on Platform Version lower than 0.3
> + */

STATIC

> +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;
> +    }
> +
> +    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;
> +    }
> +
> +    Status = AcpiAmlObjectUpdateInteger (AcpiSdtProtocol, TableHandle, "\\_SB.USB0.XHCI", 0x0);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failed to disable XHCI!\n"));
> +      ASSERT_EFI_ERROR (Status);
> +      AcpiSdtProtocol->Close (TableHandle);
> +      return Status;
> +    }
> +
> +    AcpiSdtProtocol->Close (TableHandle);
> +    AcpiUpdateChecksum ((UINT8 *)Table, Table->Length);
> +  }
> +
> +  return Status;
> +}
> +
> +
>  EFI_STATUS
>  EFIAPI
>  InitializeSbsaQemuAcpiDxe (
> @@ -738,5 +804,5 @@ InitializeSbsaQemuAcpiDxe (
>      DEBUG ((DEBUG_ERROR, "Failed to add GTDT table\n"));
>    }
>  
> -  return EFI_SUCCESS;
> +  return DisableXhciOnOlderPlatVer ();

Replacing the normal exit path like this unnecessarily ties this call
to the end of the function.

Please add another boring "if (EFI_ERROR ..." stanza instead.

/
    Leif

>  }
> 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 (#110129): https://edk2.groups.io/g/devel/message/110129
Mute This Topic: https://groups.io/mt/102037246/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



      reply	other threads:[~2023-10-26 16:56 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-18 11:55 [edk2-devel] [PATCH edk2-platforms v5 0/4] Provide XHCI USB controller only for newer hardware Marcin Juszkiewicz
2023-10-18 11:55 ` [edk2-devel] [PATCH edk2-platforms v5 1/4] SbsaQemu: introduce macro to compare platform version Marcin Juszkiewicz
2023-10-18 11:55 ` [edk2-devel] [PATCH edk2-platforms v5 2/4] SbsaQemu: add AcpiLib Marcin Juszkiewicz
2023-10-18 11:55 ` [edk2-devel] [PATCH edk2-platforms v5 3/4] SbsaQemu: initialize XHCI only if it exists Marcin Juszkiewicz
2023-10-18 11:55 ` [edk2-devel] [PATCH edk2-platforms v5 4/4] SbsaQemu: disable XHCI in DSDT if not present Marcin Juszkiewicz
2023-10-26 16:56   ` Leif Lindholm [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZTqaPYmkQB39NGwz@qc-i7.hemma.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox