From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::231; helo=mail-io0-x231.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D404B2217CE33 for ; Wed, 6 Dec 2017 03:11:49 -0800 (PST) Received: by mail-io0-x231.google.com with SMTP id h12so2259665iof.6 for ; Wed, 06 Dec 2017 03:16:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=AP/xvKtQtFe0f0x/J24OD1lfibSbH9GpcHhF7BdZAEk=; b=kf1Y+gmFBUZpIafIr4Zohtvmt/Lj5BdIc0KhE4UfWPy2rQck9TDXrvMIUOrNA0Xal4 YzHoNQaAc87NwEki/o0t1LgCbYGtIiNTPw/ZnA/I0bDU6J5qyfpDUdceCY5wIpX2qOCJ I/YmSjGgC45yqm7Bk21Jtsu0nd7O3kcEMS1pk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=AP/xvKtQtFe0f0x/J24OD1lfibSbH9GpcHhF7BdZAEk=; b=BlzRwaa92vqYPzUqR81tSO43kI4u/vhpT+LcqLEEGgSs+iI6kaZX54ax7nEUgDwrMm JM8I6fk7xEbR9QCfp5u1HBevqxJI2F31/bBaP8d01yccubSz99kvwqYGaOyzHm2vuWAv hjr01NYLuzUKnXHFylhoCpbr4R/kDY7GOepeJ/prGMEtrIAylpz7jjc41mdx3HUgD+m6 TUW6dZ9FFrgLXVPvaAlymM4sD+0CPino4qMpLv3OF6zHlsJxSfiF2Y++LX5/8miEZucB HxlVorpaOrX9SsqwdFlLYdC2AuuoM657Z7L2Qt7lHoqfYhNL0bAqNjshDe5jkpjmkQhN 0iWA== X-Gm-Message-State: AJaThX56346eJ1J1Mp3jBu6ZzfqliJ+R24ECcMjLQbdwfkufq11aPR5p c4eQQY1f4ZdMTMPWBqhOZ3k7DDxYhT8xoOzAMk4Aww== X-Google-Smtp-Source: AGs4zMYgLWeUUTJhJGwefjRu4/USqwgoieWhx7HnV/p7YEyfYgaXlvWXuiFU596oL5Mtl1eJctgCshCGp9XOusQYOYY= X-Received: by 10.107.174.222 with SMTP id n91mr31551707ioo.43.1512558980634; Wed, 06 Dec 2017 03:16:20 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Wed, 6 Dec 2017 03:16:19 -0800 (PST) In-Reply-To: References: <20171205180152.15758-1-ard.biesheuvel@linaro.org> <20171205180152.15758-3-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Wed, 6 Dec 2017 11:16:19 +0000 Message-ID: To: "Ni, Ruiyu" , "leif.lindholm@linaro.org" Cc: "edk2-devel@lists.01.org" , "Tian, Feng" , "Wu, Hao A" , "Kinney, Michael D" , "Zeng, Star" 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 11:11:50 -0000 Content-Type: text/plain; charset="UTF-8" On 6 December 2017 at 03:33, Ni, Ruiyu 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 >> --- >> 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 >> #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