From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 61917220FB315 for ; Tue, 5 Dec 2017 19:29:15 -0800 (PST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Dec 2017 19:33:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,366,1508828400"; d="scan'208";a="156425440" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.14]) ([10.239.9.14]) by orsmga004.jf.intel.com with ESMTP; 05 Dec 2017 19:33:45 -0800 To: Ard Biesheuvel , "edk2-devel@lists.01.org" Cc: "Tian, Feng" , "Wu, Hao A" , "leif.lindholm@linaro.org" , "Kinney, Michael D" , "Zeng, Star" References: <20171205180152.15758-1-ard.biesheuvel@linaro.org> <20171205180152.15758-3-ard.biesheuvel@linaro.org> From: "Ni, Ruiyu" Message-ID: Date: Wed, 6 Dec 2017 11:33:44 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171205180152.15758-3-ard.biesheuvel@linaro.org> Subject: Re: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Dec 2017 03:29:15 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > --- > 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 > #include > #include > +#include > #include > > #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