Re: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

Ard,
I should have provided some of them in the last version.
Sorry about that.

We just found an internal/private SdMmcPciHc implementation
developed by other teams. We are evaluating whether your
proposed SdMmcOverride can be used to retire that private
implementation.


On 12/6/2017 2:01 AM, Ard Biesheuvel wrote:
> Invoke the newly introduced SD/MMC override protocol to override
> the capabilities register after reading it from the device registers,
> and to call the pre/post host init and reset hooks at the appropriate
> times.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c   | 134 +++++++++++++++++++-
>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   |   1 +
>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |   2 +
>   3 files changed, 133 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index 0be8828abfcc..f1ea78de809e 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -17,6 +17,8 @@
>   
>   #include "SdMmcPciHcDxe.h"
>   
> +STATIC EDKII_SD_MMC_OVERRIDE           *mOverride;
> +
>   //
>   // Driver Global Variables
>   //
> @@ -214,6 +216,104 @@ Done:
>   }
>   
>   /**
> +  Execute a SD/MMC host controller reset
> +
> +  @param[in] Private      A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
> +  @param[in] Slot         The slot number of the host controller to reset.
> +**/
> +STATIC
> +EFI_STATUS
> +SdMmcPciHcResetHost (
> +  IN  SD_MMC_HC_PRIVATE_DATA          *Private,
> +  IN  UINT8                           Slot
> +  )
I checked the implementation of SdMmcHcReset(). It's quite simple.
So how about we do not introduce this new function SdMmcPciHcResetHost,
but just put the two NotifyPhase call inside SdMmcHcReset().

Because the names of the two functions (SdMmcPciHcResetHost and
SdMmcHcReset) are quite similar. After not a long period, maintainer
may get confused about which is which.

I agree parameters of SdMmcHcReset need to change.

> +{
> +  EFI_STATUS    Status;
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          &Private->PassThru,
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcResetPre);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: SD/MMC pre reset notifier callback failed - %r\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +  }
> +
> +  Status = SdMmcHcReset (Private->PciIo, Slot);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          &Private->PassThru,
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcResetPost);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: SD/MMC post reset notifier callback failed - %r\n",
> +        __FUNCTION__, Status));
> +    }
> +  }
> +  return Status;
> +}
> +
> +/**
> +  Initialize a SD/MMC host controller
> +
> +  @param[in] Private      A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
> +  @param[in] Slot         The slot number of the host controller to initialize.
> +**/
> +STATIC
> +EFI_STATUS
> +SdMmcPciHcInitHost (
> +  IN  SD_MMC_HC_PRIVATE_DATA          *Private,
> +  IN  UINT8                           Slot
> +  )
> +{
> +  EFI_STATUS    Status;
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          &Private->PassThru,
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcInitHostPre);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: SD/MMC pre init notifier callback failed - %r\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +  }
> +
> +  Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          &Private->PassThru,
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcInitHostPost);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: SD/MMC post init notifier callback failed - %r\n",
> +        __FUNCTION__, Status));
> +    }
> +  }
> +  return Status;
> +}
> +
> +/**
>     Sd removable device enumeration callback function when the timer event is signaled.
>   
>     @param[in]  Event     The Event this notify function registered to.
> @@ -281,14 +381,14 @@ SdMmcPciHcEnumerateDevice (
>           //
>           // Reset the specified slot of the SD/MMC Pci Host Controller
>           //
> -        Status = SdMmcHcReset (Private->PciIo, Slot);
> +        Status = SdMmcPciHcResetHost (Private, Slot);
>           if (EFI_ERROR (Status)) {
>             continue;
>           }
>           //
>           // Reinitialize slot and restart identification process for the new attached device
>           //
> -        Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
> +        Status = SdMmcPciHcInitHost (Private, Slot);
>           if (EFI_ERROR (Status)) {
>             continue;
>           }
> @@ -601,6 +701,20 @@ SdMmcPciHcDriverBindingStart (
>       goto Done;
>     }
>   
> +  //
> +  // Attempt to locate the singleton instance of the SD/MMC override protocol,
> +  // which implements platform specific workarounds for non-standard SDHCI
> +  // implementations.
> +  //
> +  if (mOverride == NULL) {
> +    Status = gBS->HandleProtocol (Controller, &gEdkiiSdMmcOverrideProtocolGuid,
> +                    (VOID **)&mOverride);

HandleProtocol() on the Controller cannot work. HandleProtocol() looks
for the SdMmcOverride instance on the specific handle.

You should use LocateProtocol() to look for the SdMmcOverride instance
regardless the handle.

> +    if (!EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_INFO, "%a: found SD/MMC override protocol\n",
> +        __FUNCTION__));
> +    }
> +  }
> +
>     Support64BitDma = TRUE;
>     for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
>       Private->Slot[Slot].Enable = TRUE;
> @@ -609,6 +723,18 @@ SdMmcPciHcDriverBindingStart (
>       if (EFI_ERROR (Status)) {
>         continue;
>       }
> +    if (mOverride != NULL && mOverride->Capability != NULL) {
> +      Status = mOverride->Capability (
> +                            &Private->PassThru,
> +                            Controller,
> +                            Slot,
> +                            (VOID *)&Private->Capability[Slot]);
(VOID *) can be removed?

> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n",
> +          __FUNCTION__, Status));
> +        continue;
> +      }
> +    }
>       DumpCapabilityReg (Slot, &Private->Capability[Slot]);
>   
>       Support64BitDma &= Private->Capability[Slot].SysBus64;
> @@ -627,7 +753,7 @@ SdMmcPciHcDriverBindingStart (
>       //
>       // Reset the specified slot of the SD/MMC Pci Host Controller
>       //
> -    Status = SdMmcHcReset (PciIo, Slot);
> +    Status = SdMmcPciHcResetHost (Private, Slot);
>       if (EFI_ERROR (Status)) {
>         continue;
>       }
> @@ -642,7 +768,7 @@ SdMmcPciHcDriverBindingStart (
>         continue;
>       }
>   
> -    Status = SdMmcHcInitHost (PciIo, Slot, Private->Capability[Slot]);
> +    Status = SdMmcPciHcInitHost (Private, Slot);
>       if (EFI_ERROR (Status)) {
>         continue;
>       }
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index 6a2a27969936..8830cd437edd 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -35,6 +35,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>   #include <Protocol/DriverBinding.h>
>   #include <Protocol/ComponentName.h>
>   #include <Protocol/ComponentName2.h>
> +#include <Protocol/SdMmcOverride.h>
>   #include <Protocol/SdMmcPassThru.h>
>   
>   #include "SdMmcPciHci.h"
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> index e26e6a098c17..154ce45d8223 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> @@ -48,6 +48,7 @@ [Sources]
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>   
>   [LibraryClasses]
>     DevicePathLib
> @@ -61,6 +62,7 @@ [LibraryClasses]
>     DebugLib
>   
>   [Protocols]
> +  gEdkiiSdMmcOverrideProtocolGuid               ## SOMETIMES_CONSUMES
>     gEfiDevicePathProtocolGuid                    ## TO_START
>     gEfiPciIoProtocolGuid                         ## TO_START
>     gEfiSdMmcPassThruProtocolGuid                 ## BY_START
> 


-- 
Thanks,
Ray