On 6 December 2017 at 03:33, Ni, Ruiyu <ruiyu...@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.biesheu...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
