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

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

On 6 December 2017 at 03:33, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> 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.
>

OK, good to know.

>
>
> 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.
>

OK, that works for me.

>
>> +{
>> +  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.
>

Oops. Yes, I missed that when updating the code. Will fix.

>> +    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?
>

No, Capability () takes a UINT64 pointer, because the capability
struct is exactly 64 bits. So the cast is needed.

But now that I think of it, the struct may not be 64-bit aligned, so
casting it is incorrect. I will revert that change.

>
>> +      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