Re: [edk2-devel] [PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: move EMMC SSDT handling to core routine

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Mon, 2 Dec 2019 at 10:26, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 11/29/19 11:47 AM, Ard Biesheuvel via Groups.Io wrote:
> > In preparation of adding support for describing the presence of OP-TEE
> > via a SSDT ACPI table, refactor the existing code so we will be able to
> > reuse it more easily.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >   Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c        | 55 -----------------
> >   Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c | 65 ++++++++++++++++++++
> >   2 files changed, 65 insertions(+), 55 deletions(-)
> >
> > diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> > index 0d0e5edad901..90b67152c768 100644
> > --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> > +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> > @@ -53,10 +53,6 @@
> >
> >   STATIC EFI_HANDLE mSdMmcControllerHandle;
> >
> > -STATIC EFI_ACPI_DESCRIPTION_HEADER      *mSsdt;
> > -STATIC UINTN                            mSsdtSize;
> > -STATIC VOID                             *mEventRegistration;
> > -
> >   /**
> >
> >     Override function for SDHCI capability bits
> > @@ -185,31 +181,6 @@ STATIC EDKII_SD_MMC_OVERRIDE mSdMmcOverride = {
> >     SynQuacerSdMmcNotifyPhase,
> >   };
> >
> > -STATIC
> > -VOID
> > -EFIAPI
> > -InstallAcpiTable (
> > -  IN EFI_EVENT                      Event,
> > -  IN VOID*                          Context
> > -  )
> > -{
> > -  UINTN                             TableKey;
> > -  EFI_STATUS                        Status;
> > -  EFI_ACPI_TABLE_PROTOCOL           *AcpiTable;
> > -
> > -  Status = gBS->LocateProtocol (&gEfiAcpiTableProtocolGuid, NULL,
> > -                  (VOID **)&AcpiTable);
> > -  if (EFI_ERROR (Status)) {
> > -    return;
> > -  }
> > -
> > -  Status = AcpiTable->InstallAcpiTable (AcpiTable, mSsdt, mSsdtSize, &TableKey);
> > -  if (EFI_ERROR (Status)) {
> > -    DEBUG ((DEBUG_WARN, "%a: failed to install SSDT table for eMMC - %r\n",
> > -      __FUNCTION__, Status));
> > -  }
> > -}
> > -
> >   EFI_STATUS
> >   EFIAPI
> >   RegisterEmmc (
> > @@ -218,32 +189,6 @@ RegisterEmmc (
> >   {
> >     EFI_STATUS                      Status;
> >     EFI_HANDLE                      Handle;
> > -  UINTN                           Index;
> > -
> > -  if (mHiiSettings->AcpiPref == ACPIPREF_ACPI) {
> > -    //
> > -    // Load the SSDT table from a raw section in this FFS file.
> > -    //
> > -    for (Index = 0;; Index++) {
> > -      Status = GetSectionFromFv (&gEfiCallerIdGuid, EFI_SECTION_RAW, Index,
> > -                 (VOID **)&mSsdt, &mSsdtSize);
> > -      if (EFI_ERROR (Status)) {
> > -        break;
> > -      }
> > -
> > -      if (mSsdt->OemTableId != EMMC_TABLE_ID) {
> > -        continue;
> > -      }
> > -
> > -      //
> > -      // Register for the ACPI table protocol
> > -      //
> > -      EfiCreateProtocolNotifyEvent (&gEfiAcpiTableProtocolGuid, TPL_CALLBACK,
> > -        InstallAcpiTable, NULL, &mEventRegistration);
> > -
> > -      break;
> > -    }
> > -  }
> >
> >     Status = RegisterNonDiscoverableMmioDevice (
> >                NonDiscoverableDeviceTypeSdhci,
> > diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > index 73cc560fa8d8..c9cc37dd2478 100644
> > --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> > @@ -113,6 +113,11 @@ STATIC EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR mI2c1Desc[] = {
> >     }
> >   };
> >
> > +STATIC EFI_ACPI_DESCRIPTION_HEADER      *mEmmcSsdt;
> > +STATIC UINTN                            mEmmcSsdtSize;
> > +
> > +STATIC VOID                             *mAcpiTableEventRegistration;
> > +
> >   STATIC
> >   EFI_STATUS
> >   RegisterDevice (
> > @@ -256,6 +261,32 @@ EnableSettingsForm (
> >     return InstallHiiPages ();
> >   }
> >
> > +STATIC
> > +VOID
> > +EFIAPI
> > +InstallAcpiTables (
>
> Maybe rename as InstallEmmcSsdtAcpiTables?
>

The next patch adds a caller that uses it for another SSDT, but I take
your point - it could be more precise.

> Regardless the name chosen:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>

Thanks,

> > +  IN EFI_EVENT                      Event,
> > +  IN VOID*                          Context
> > +  )
> > +{
> > +  UINTN                             TableKey;
> > +  EFI_STATUS                        Status;
> > +  EFI_ACPI_TABLE_PROTOCOL           *AcpiTable;
> > +
> > +  Status = gBS->LocateProtocol (&gEfiAcpiTableProtocolGuid, NULL,
> > +                  (VOID **)&AcpiTable);
> > +  if (EFI_ERROR (Status)) {
> > +    return;
> > +  }
> > +
> > +  Status = AcpiTable->InstallAcpiTable (AcpiTable, mEmmcSsdt, mEmmcSsdtSize,
> > +                        &TableKey);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_WARN, "%a: failed to install SSDT table for eMMC - %r\n",
> > +      __FUNCTION__, Status));
> > +  }
> > +}
> > +
> >   EFI_STATUS
> >   EFIAPI
> >   PlatformDxeEntryPoint (
> > @@ -267,6 +298,9 @@ PlatformDxeEntryPoint (
> >     VOID                            *Dtb;
> >     UINTN                           DtbSize;
> >     EFI_HANDLE                      Handle;
> > +  EFI_ACPI_DESCRIPTION_HEADER     *Ssdt;
> > +  UINTN                           SsdtSize;
> > +  UINTN                           Index;
> >
> >     mHiiSettingsVal = PcdGet64 (PcdPlatformSettings);
> >     mHiiSettings = (SYNQUACER_PLATFORM_VARSTORE_DATA *)&mHiiSettingsVal;
> > @@ -344,5 +378,36 @@ PlatformDxeEntryPoint (
> >       ASSERT_EFI_ERROR (Status);
> >     }
> >
> > +  if (mHiiSettings->AcpiPref == ACPIPREF_ACPI) {
> > +    //
> > +    // Load the SSDT tables from a raw section in this FFS file.
> > +    //
> > +    for (Index = 0;; Index++) {
> > +      Status = GetSectionFromFv (&gEfiCallerIdGuid, EFI_SECTION_RAW, Index,
> > +                 (VOID **)&Ssdt, &SsdtSize);
> > +      if (EFI_ERROR (Status)) {
> > +        break;
> > +      }
> > +
> > +      switch (Ssdt->OemTableId) {
> > +      case EMMC_TABLE_ID:
> > +        if (mHiiSettings->EnableEmmc != EMMC_ENABLED) {
> > +          break;
> > +        }
> > +        mEmmcSsdt = Ssdt;
> > +        mEmmcSsdtSize = SsdtSize;
> > +        break;
> > +      }
> > +    }
> > +
> > +    if (mEmmcSsdtSize > 0) {
> > +      //
> > +      // Register for the ACPI table protocol if we found any SSDTs to install
> > +      //
> > +      EfiCreateProtocolNotifyEvent (&gEfiAcpiTableProtocolGuid, TPL_CALLBACK,
> > +        InstallAcpiTables, NULL, &mAcpiTableEventRegistration);
> > +    }
> > +  }
> > +
> >     return EFI_SUCCESS;
> >   }
> >
>