* [PATCH v2 0/2] quirks handling for SDHCI controllers
@ 2017-11-30 10:11 Ard Biesheuvel
2017-11-30 10:11 ` [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol Ard Biesheuvel
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2017-11-30 10:11 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, star.zeng, hao.a.wu, feng.tian, Ard Biesheuvel
Many SDHCI implementations exist that are almost spec complicant, and
could be driven by the generic SD/MMC host controller driver except
for some minimal necessary init time tweaks.
Adding such tweaks to the generic driver is undesirable. On the other
hand, forking the driver for every platform that has such a SDHCI
controller is problematic when it comes to upstreaming and ongoing
maintenance (which is arguably the point of upstreaming in the first
place).
So these patches propose a workaround that is minimally invasive on the
EDK2 side, but gives platforms a lot of leeway when it comes to applying
SDHCI quirks.
Changes since RFC/v1:
- add EFI_SD_MMC_PASS_THRU_PROTOCOL* member to override methods
- use UINT64* not VOID* to pass capability structure (which is alwys 64 bits
in size)
Ard Biesheuvel (2):
MdeModulePkg: introduce SD/MMC override protocol
MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 116 +++++++++++++++++++-
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 6 +
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf | 2 +
MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 +++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 +
5 files changed, 225 insertions(+), 5 deletions(-)
create mode 100644 MdeModulePkg/Include/Protocol/SdMmcOverride.h
--
2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-11-30 10:11 [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
@ 2017-11-30 10:11 ` Ard Biesheuvel
2017-12-05 7:08 ` Ni, Ruiyu
2017-12-05 10:12 ` Ni, Ruiyu
2017-11-30 10:11 ` [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden Ard Biesheuvel
2017-12-04 14:53 ` [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
2 siblings, 2 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2017-11-30 10:11 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, star.zeng, hao.a.wu, feng.tian, Ard Biesheuvel
Many ARM based SoCs have integrated SDHCI controllers, and often,
these implementations deviate in subtle ways from the pertinent
specifications. On the one hand, these deviations are quite easy
to work around, but on the other hand, having a collection of SoC
specific workarounds in the generic driver stack is undesirable.
So let's introduce an optional SD/MMC override protocol that we
can invoke at the appropriate moments in the device initialization.
That way, the workaround itself remains platform specific, but we
can still use the generic driver stack on such platforms.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 +
2 files changed, 106 insertions(+)
diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
new file mode 100644
index 000000000000..5a5c393896f4
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -0,0 +1,103 @@
+/** @file
+ Protocol to describe overrides required to support non-standard SDHCI
+ implementations
+
+ Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD License
+ which accompanies this distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __SD_MMC_OVERRIDE_H__
+#define __SD_MMC_OVERRIDE_H__
+
+#include <Protocol/SdMmcPassThru.h>
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
+ { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
+
+typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
+
+typedef enum {
+ SD_MMC_OVERRIDE_RESET_PRE_HOOK,
+ SD_MMC_OVERRIDE_RESET_POST_HOOK,
+ SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
+ SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
+} SD_MMC_OVERRIDE_HOOK;
+
+/**
+
+ Override function for SDHCI capability bits
+
+ @param[in] PassThru A pointer to the
+ EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+ @param[in] ControllerHandle The EFI_HANDLE of the controller.
+ @param[in] Slot The 0 based slot index.
+ @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure.
+
+ @retval EFI_SUCCESS The override function completed successfully.
+ @retval EFI_NOT_FOUND The specified controller or slot does not exist.
+ @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
+ IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
+ IN EFI_HANDLE ControllerHandle,
+ IN UINT8 Slot,
+ IN OUT UINT64 *SdMmcHcSlotCapability
+ );
+
+/**
+
+ Override function for SDHCI controller operations
+
+ @param[in] PassThru A pointer to the
+ EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+ @param[in] ControllerHandle The EFI_HANDLE of the controller.
+ @param[in] Slot The 0 based slot index.
+ @param[in,out] HookType The type of operation and whether the
+ hook is invoked right before (pre) or
+ right after (post)
+
+ @retval EFI_SUCCESS The override function completed successfully.
+ @retval EFI_NOT_FOUND The specified controller or slot does not exist.
+ @retval EFI_INVALID_PARAMETER HookType is invalid
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
+ IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
+ IN EFI_HANDLE ControllerHandle,
+ IN UINT8 Slot,
+ IN SD_MMC_OVERRIDE_HOOK HookType
+ );
+
+struct _SD_MMC_OVERRIDE {
+ //
+ // Protocol version of this implementation
+ //
+ UINTN Version;
+ //
+ // Callback to override SD/MMC host controller capability bits
+ //
+ SD_MMC_OVERRIDE_CAPABILITY OverrideCapability;
+ //
+ // Callback to invoke SD/MMC override hooks
+ //
+ SD_MMC_OVERRIDE_INVOKE_HOOK InvokeHook;
+};
+
+extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 856d67aceb21..64ceea029f94 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -562,6 +562,9 @@ [Protocols]
## Include/Protocol/SmmMemoryAttribute.h
gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce, 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
+ ## Include/Protocol/SdMmcOverride.h
+ gEdkiiSdMmcOverrideProtocolGuid = { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
+
#
# [Error.gEfiMdeModulePkgTokenSpaceGuid]
# 0x80000001 | Invalid value provided.
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden
2017-11-30 10:11 [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
2017-11-30 10:11 ` [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol Ard Biesheuvel
@ 2017-11-30 10:11 ` Ard Biesheuvel
2017-12-05 12:46 ` Wu, Hao A
2017-12-04 14:53 ` [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
2 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2017-11-30 10:11 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, star.zeng, hao.a.wu, feng.tian, Ard Biesheuvel
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 | 116 +++++++++++++++++++-
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 6 +
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf | 2 +
3 files changed, 119 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index 0be8828abfcc..61f64285807d 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -60,7 +60,8 @@ SD_MMC_HC_PRIVATE_DATA gSdMmcPciHcTemplate = {
{ // MaxCurrent
0,
},
- 0 // ControllerVersion
+ 0, // ControllerVersion
+ NULL // Override
};
SD_DEVICE_PATH mSdDpTemplate = {
@@ -213,6 +214,92 @@ Done:
return;
}
+STATIC
+EFI_STATUS
+SdMmcPciHcResetHost (
+ IN SD_MMC_HC_PRIVATE_DATA *Private,
+ IN UINT8 Slot
+ )
+{
+ EFI_STATUS Status;
+
+ if (Private->Override != NULL &&
+ Private->Override->InvokeHook != NULL) {
+ Status = Private->Override->InvokeHook (
+ &Private->PassThru,
+ Private->ControllerHandle,
+ Slot,
+ SD_MMC_OVERRIDE_RESET_PRE_HOOK);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_WARN, "%a: SD/MMC pre reset hook failed - %r\n",
+ __FUNCTION__, Status));
+ return Status;
+ }
+ }
+
+ Status = SdMmcHcReset (Private->PciIo, Slot);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (Private->Override != NULL &&
+ Private->Override->InvokeHook != NULL) {
+ Status = Private->Override->InvokeHook (
+ &Private->PassThru,
+ Private->ControllerHandle,
+ Slot,
+ SD_MMC_OVERRIDE_RESET_POST_HOOK);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_WARN, "%a: SD/MMC post reset hook failed - %r\n",
+ __FUNCTION__, Status));
+ }
+ }
+ return Status;
+}
+
+STATIC
+EFI_STATUS
+SdMmcPciHcInitHost (
+ IN SD_MMC_HC_PRIVATE_DATA *Private,
+ IN UINT8 Slot
+ )
+{
+ EFI_STATUS Status;
+
+ if (Private->Override != NULL &&
+ Private->Override->InvokeHook != NULL) {
+ Status = Private->Override->InvokeHook (
+ &Private->PassThru,
+ Private->ControllerHandle,
+ Slot,
+ SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_WARN, "%a: SD/MMC pre init hook failed - %r\n",
+ __FUNCTION__, Status));
+ return Status;
+ }
+ }
+
+ Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (Private->Override != NULL &&
+ Private->Override->InvokeHook != NULL) {
+ Status = Private->Override->InvokeHook (
+ &Private->PassThru,
+ Private->ControllerHandle,
+ Slot,
+ SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_WARN, "%a: SD/MMC post init hook failed - %r\n",
+ __FUNCTION__, Status));
+ }
+ }
+ return Status;
+}
+
/**
Sd removable device enumeration callback function when the timer event is signaled.
@@ -281,14 +368,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 +688,12 @@ SdMmcPciHcDriverBindingStart (
goto Done;
}
+ Status = gBS->HandleProtocol (Controller, &gEdkiiSdMmcOverrideProtocolGuid,
+ (VOID **)&Private->Override);
+ if (!EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_INFO, "%a: using SD/MMC override protocol\n", __FUNCTION__));
+ }
+
Support64BitDma = TRUE;
for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
Private->Slot[Slot].Enable = TRUE;
@@ -609,6 +702,19 @@ SdMmcPciHcDriverBindingStart (
if (EFI_ERROR (Status)) {
continue;
}
+ if (Private->Override != NULL &&
+ Private->Override->OverrideCapability != NULL) {
+ Status = Private->Override->OverrideCapability (
+ &Private->PassThru,
+ Controller,
+ Slot,
+ (VOID *)&Private->Capability[Slot]);
+ 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 +733,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 +748,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..b51e0529f885 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"
@@ -115,6 +116,11 @@ typedef struct {
UINT64 MaxCurrent[SD_MMC_HC_MAX_SLOT];
UINT32 ControllerVersion;
+
+ //
+ // Optional protcol to deal with non-standard SDHCI implementations
+ //
+ SD_MMC_OVERRIDE *Override;
} SD_MMC_HC_PRIVATE_DATA;
#define SD_MMC_HC_TRB_SIG SIGNATURE_32 ('T', 'R', 'B', 'T')
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
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] quirks handling for SDHCI controllers
2017-11-30 10:11 [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
2017-11-30 10:11 ` [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol Ard Biesheuvel
2017-11-30 10:11 ` [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden Ard Biesheuvel
@ 2017-12-04 14:53 ` Ard Biesheuvel
2017-12-05 2:08 ` Zeng, Star
2 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2017-12-04 14:53 UTC (permalink / raw)
To: edk2-devel@lists.01.org
Cc: Leif Lindholm, Zeng, Star, Wu, Hao A, Tian, Feng, Ard Biesheuvel
On 30 November 2017 at 10:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Many SDHCI implementations exist that are almost spec complicant, and
> could be driven by the generic SD/MMC host controller driver except
> for some minimal necessary init time tweaks.
>
> Adding such tweaks to the generic driver is undesirable. On the other
> hand, forking the driver for every platform that has such a SDHCI
> controller is problematic when it comes to upstreaming and ongoing
> maintenance (which is arguably the point of upstreaming in the first
> place).
>
> So these patches propose a workaround that is minimally invasive on the
> EDK2 side, but gives platforms a lot of leeway when it comes to applying
> SDHCI quirks.
>
> Changes since RFC/v1:
> - add EFI_SD_MMC_PASS_THRU_PROTOCOL* member to override methods
> - use UINT64* not VOID* to pass capability structure (which is alwys 64 bits
> in size)
>
> Ard Biesheuvel (2):
> MdeModulePkg: introduce SD/MMC override protocol
> MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden
>
Hao, Star,
Any comments?
Thanks,
Ard.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] quirks handling for SDHCI controllers
2017-12-04 14:53 ` [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
@ 2017-12-05 2:08 ` Zeng, Star
0 siblings, 0 replies; 17+ messages in thread
From: Zeng, Star @ 2017-12-05 2:08 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org
Cc: Wu, Hao A, Tian, Feng, Leif Lindholm, Ni, Ruiyu,
Kinney, Michael D, Zeng, Star
Include Ruiyu and Mike.
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Monday, December 4, 2017 10:53 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH v2 0/2] quirks handling for SDHCI controllers
On 30 November 2017 at 10:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Many SDHCI implementations exist that are almost spec complicant, and
> could be driven by the generic SD/MMC host controller driver except
> for some minimal necessary init time tweaks.
>
> Adding such tweaks to the generic driver is undesirable. On the other
> hand, forking the driver for every platform that has such a SDHCI
> controller is problematic when it comes to upstreaming and ongoing
> maintenance (which is arguably the point of upstreaming in the first
> place).
>
> So these patches propose a workaround that is minimally invasive on
> the
> EDK2 side, but gives platforms a lot of leeway when it comes to
> applying SDHCI quirks.
>
> Changes since RFC/v1:
> - add EFI_SD_MMC_PASS_THRU_PROTOCOL* member to override methods
> - use UINT64* not VOID* to pass capability structure (which is alwys 64 bits
> in size)
>
> Ard Biesheuvel (2):
> MdeModulePkg: introduce SD/MMC override protocol
> MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden
>
Hao, Star,
Any comments?
Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-11-30 10:11 ` [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol Ard Biesheuvel
@ 2017-12-05 7:08 ` Ni, Ruiyu
2017-12-05 7:20 ` Zeng, Star
2017-12-05 10:12 ` Ni, Ruiyu
1 sibling, 1 reply; 17+ messages in thread
From: Ni, Ruiyu @ 2017-12-05 7:08 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
Cc: "hao.a.wu, feng.tian, star.zeng, leif.lindholm
On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
> Many ARM based SoCs have integrated SDHCI controllers, and often,
> these implementations deviate in subtle ways from the pertinent
> specifications. On the one hand, these deviations are quite easy
> to work around, but on the other hand, having a collection of SoC
> specific workarounds in the generic driver stack is undesirable.
>
> So let's introduce an optional SD/MMC override protocol that we
> can invoke at the appropriate moments in the device initialization.
> That way, the workaround itself remains platform specific, but we
> can still use the generic driver stack on such platforms.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
> MdeModulePkg/MdeModulePkg.dec | 3 +
> 2 files changed, 106 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> new file mode 100644
> index 000000000000..5a5c393896f4
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -0,0 +1,103 @@
> +/** @file
> + Protocol to describe overrides required to support non-standard SDHCI
> + implementations
> +
> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
> +
> + This program and the accompanying materials
> + are licensed and made available under the terms and conditions of the BSD License
> + which accompanies this distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __SD_MMC_OVERRIDE_H__
> +#define __SD_MMC_OVERRIDE_H__
> +
> +#include <Protocol/SdMmcPassThru.h>
> +
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> + { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
> +
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
> +
> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
> +
> +typedef enum {
> + SD_MMC_OVERRIDE_RESET_PRE_HOOK,
> + SD_MMC_OVERRIDE_RESET_POST_HOOK,
> + SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
> + SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
> +} SD_MMC_OVERRIDE_HOOK;
> +
> +/**
> +
> + Override function for SDHCI capability bits
> +
> + @param[in] PassThru A pointer to the
> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
> + @param[in] Slot The 0 based slot index.
> + @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure.
> +
> + @retval EFI_SUCCESS The override function completed successfully.
> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
> + @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
> + IN EFI_HANDLE ControllerHandle,
> + IN UINT8 Slot,
> + IN OUT UINT64 *SdMmcHcSlotCapability
> + );
> +
> +/**
> +
> + Override function for SDHCI controller operations
> +
> + @param[in] PassThru A pointer to the
> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
> + @param[in] Slot The 0 based slot index.
> + @param[in,out] HookType The type of operation and whether the
> + hook is invoked right before (pre) or
> + right after (post)
> +
> + @retval EFI_SUCCESS The override function completed successfully.
> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
> + @retval EFI_INVALID_PARAMETER HookType is invalid
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
> + IN EFI_HANDLE ControllerHandle,
> + IN UINT8 Slot,
> + IN SD_MMC_OVERRIDE_HOOK HookType
> + );
> +
> +struct _SD_MMC_OVERRIDE {
> + //
> + // Protocol version of this implementation
> + //
> + UINTN Version;
> + //
> + // Callback to override SD/MMC host controller capability bits
> + //
> + SD_MMC_OVERRIDE_CAPABILITY OverrideCapability;
> + //
> + // Callback to invoke SD/MMC override hooks
> + //
> + SD_MMC_OVERRIDE_INVOKE_HOOK InvokeHook;
> +};
> +
> +extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
> +
> +#endif
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 856d67aceb21..64ceea029f94 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -562,6 +562,9 @@ [Protocols]
> ## Include/Protocol/SmmMemoryAttribute.h
> gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce, 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
>
> + ## Include/Protocol/SdMmcOverride.h
> + gEdkiiSdMmcOverrideProtocolGuid = { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
> +
> #
> # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> # 0x80000001 | Invalid value provided.
>
Based on your patch #2, I think the protocol is supposed to be installed
by a platform driver (let's call it SdMmcOverridePlatform) on the
controller handle.
There are two ways to produce this Override protocol:
1. SdMmcOverridePlatform registers a protocol notification on PciIo and
find the SDMMC host controller PCI handle, then install SdMmcOverride on
that handle.
2. SdMMcOverridePlatform is written as a driver model driver.
DriverBindingStart() opens PciIo BY_DRIVER and produces SdMmcOverride.
Then SdMmcPciHc driver opens SdMmcOverride BY_DRIVER and produces
SdMmcPassthru.
But since the SdMmcOverride protocol is optional, that makes #2 not
workable.
But #1 cannot handle the "reconnect -r" case. Because there is no
protocol notification on uninstall.
So I am thinking if it's more proper to make this protocol a platform
level singleton instance. I noticed that all APIs exposed by this
protocol already carry the Controller Handle.
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 7:08 ` Ni, Ruiyu
@ 2017-12-05 7:20 ` Zeng, Star
2017-12-05 8:37 ` Ni, Ruiyu
0 siblings, 1 reply; 17+ messages in thread
From: Zeng, Star @ 2017-12-05 7:20 UTC (permalink / raw)
To: Ni, Ruiyu, Ard Biesheuvel, edk2-devel@lists.01.org
Cc: Wu, Hao A, Kinney, Michael D, Tian, Feng,
leif.lindholm@linaro.org, Zeng, Star
If making this protocol a platform level singleton instance, is it so hard to define the interfaces and parameters since different controllers may need different hook points and parameters?
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Tuesday, December 5, 2017 3:09 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
Cc: "hao.a.wu@intel.com; Kinney.d.michael"@intel.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; leif.lindholm@linaro.org
Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
> Many ARM based SoCs have integrated SDHCI controllers, and often,
> these implementations deviate in subtle ways from the pertinent
> specifications. On the one hand, these deviations are quite easy to
> work around, but on the other hand, having a collection of SoC
> specific workarounds in the generic driver stack is undesirable.
>
> So let's introduce an optional SD/MMC override protocol that we can
> invoke at the appropriate moments in the device initialization.
> That way, the workaround itself remains platform specific, but we can
> still use the generic driver stack on such platforms.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
> MdeModulePkg/MdeModulePkg.dec | 3 +
> 2 files changed, 106 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> new file mode 100644
> index 000000000000..5a5c393896f4
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -0,0 +1,103 @@
> +/** @file
> + Protocol to describe overrides required to support non-standard
> +SDHCI
> + implementations
> +
> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
> +
> + This program and the accompanying materials are licensed and made
> + available under the terms and conditions of the BSD License which
> + accompanies this distribution. The full text of the license may be
> + found at http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __SD_MMC_OVERRIDE_H__
> +#define __SD_MMC_OVERRIDE_H__
> +
> +#include <Protocol/SdMmcPassThru.h>
> +
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> + { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83,
> +0x23, 0x23 } }
> +
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
> +
> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
> +
> +typedef enum {
> + SD_MMC_OVERRIDE_RESET_PRE_HOOK,
> + SD_MMC_OVERRIDE_RESET_POST_HOOK,
> + SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
> + SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
> +} SD_MMC_OVERRIDE_HOOK;
> +
> +/**
> +
> + Override function for SDHCI capability bits
> +
> + @param[in] PassThru A pointer to the
> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
> + @param[in] Slot The 0 based slot index.
> + @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure.
> +
> + @retval EFI_SUCCESS The override function completed successfully.
> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
> + @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
> + IN EFI_HANDLE ControllerHandle,
> + IN UINT8 Slot,
> + IN OUT UINT64 *SdMmcHcSlotCapability
> + );
> +
> +/**
> +
> + Override function for SDHCI controller operations
> +
> + @param[in] PassThru A pointer to the
> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
> + @param[in] Slot The 0 based slot index.
> + @param[in,out] HookType The type of operation and whether the
> + hook is invoked right before (pre) or
> + right after (post)
> +
> + @retval EFI_SUCCESS The override function completed successfully.
> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
> + @retval EFI_INVALID_PARAMETER HookType is invalid
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
> + IN EFI_HANDLE ControllerHandle,
> + IN UINT8 Slot,
> + IN SD_MMC_OVERRIDE_HOOK HookType
> + );
> +
> +struct _SD_MMC_OVERRIDE {
> + //
> + // Protocol version of this implementation
> + //
> + UINTN Version;
> + //
> + // Callback to override SD/MMC host controller capability bits
> + //
> + SD_MMC_OVERRIDE_CAPABILITY OverrideCapability;
> + //
> + // Callback to invoke SD/MMC override hooks
> + //
> + SD_MMC_OVERRIDE_INVOKE_HOOK InvokeHook;
> +};
> +
> +extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
> +
> +#endif
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 856d67aceb21..64ceea029f94
> 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -562,6 +562,9 @@ [Protocols]
> ## Include/Protocol/SmmMemoryAttribute.h
> gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce,
> 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
>
> + ## Include/Protocol/SdMmcOverride.h
> + gEdkiiSdMmcOverrideProtocolGuid = { 0xeaf9e3c1, 0xc9cd, 0x46db, {
> + 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
> +
> #
> # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> # 0x80000001 | Invalid value provided.
>
Based on your patch #2, I think the protocol is supposed to be installed by a platform driver (let's call it SdMmcOverridePlatform) on the controller handle.
There are two ways to produce this Override protocol:
1. SdMmcOverridePlatform registers a protocol notification on PciIo and find the SDMMC host controller PCI handle, then install SdMmcOverride on that handle.
2. SdMMcOverridePlatform is written as a driver model driver.
DriverBindingStart() opens PciIo BY_DRIVER and produces SdMmcOverride.
Then SdMmcPciHc driver opens SdMmcOverride BY_DRIVER and produces SdMmcPassthru.
But since the SdMmcOverride protocol is optional, that makes #2 not workable.
But #1 cannot handle the "reconnect -r" case. Because there is no protocol notification on uninstall.
So I am thinking if it's more proper to make this protocol a platform level singleton instance. I noticed that all APIs exposed by this protocol already carry the Controller Handle.
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 7:20 ` Zeng, Star
@ 2017-12-05 8:37 ` Ni, Ruiyu
2017-12-05 8:50 ` Zeng, Star
0 siblings, 1 reply; 17+ messages in thread
From: Ni, Ruiyu @ 2017-12-05 8:37 UTC (permalink / raw)
To: Zeng, Star, Ard Biesheuvel, edk2-devel@lists.01.org
Cc: Wu, Hao A, Kinney, Michael D, Tian, Feng,
leif.lindholm@linaro.org
On 12/5/2017 3:20 PM, Zeng, Star wrote:
> If making this protocol a platform level singleton instance, is it so hard to define the interfaces and parameters since different controllers may need different hook points and parameters?
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, December 5, 2017 3:09 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
> Cc: "hao.a.wu@intel.com; Kinney.d.michael"@intel.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; leif.lindholm@linaro.org
> Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
>
> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>> Many ARM based SoCs have integrated SDHCI controllers, and often,
>> these implementations deviate in subtle ways from the pertinent
>> specifications. On the one hand, these deviations are quite easy to
>> work around, but on the other hand, having a collection of SoC
>> specific workarounds in the generic driver stack is undesirable.
>>
>> So let's introduce an optional SD/MMC override protocol that we can
>> invoke at the appropriate moments in the device initialization.
>> That way, the workaround itself remains platform specific, but we can
>> still use the generic driver stack on such platforms.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
>> MdeModulePkg/MdeModulePkg.dec | 3 +
>> 2 files changed, 106 insertions(+)
>>
>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> new file mode 100644
>> index 000000000000..5a5c393896f4
>> --- /dev/null
>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> @@ -0,0 +1,103 @@
>> +/** @file
>> + Protocol to describe overrides required to support non-standard
>> +SDHCI
>> + implementations
>> +
>> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
>> +
>> + This program and the accompanying materials are licensed and made
>> + available under the terms and conditions of the BSD License which
>> + accompanies this distribution. The full text of the license may be
>> + found at http://opensource.org/licenses/bsd-license.php
>> +
>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __SD_MMC_OVERRIDE_H__
>> +#define __SD_MMC_OVERRIDE_H__
>> +
>> +#include <Protocol/SdMmcPassThru.h>
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>> + { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83,
>> +0x23, 0x23 } }
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
>> +
>> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
>> +
>> +typedef enum {
>> + SD_MMC_OVERRIDE_RESET_PRE_HOOK,
>> + SD_MMC_OVERRIDE_RESET_POST_HOOK,
>> + SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
>> + SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
>> +} SD_MMC_OVERRIDE_HOOK;
>> +
>> +/**
>> +
>> + Override function for SDHCI capability bits
>> +
>> + @param[in] PassThru A pointer to the
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
>> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
>> + @param[in] Slot The 0 based slot index.
>> + @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure.
>> +
>> + @retval EFI_SUCCESS The override function completed successfully.
>> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
>> + @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> + IN EFI_HANDLE ControllerHandle,
>> + IN UINT8 Slot,
>> + IN OUT UINT64 *SdMmcHcSlotCapability
>> + );
>> +
>> +/**
>> +
>> + Override function for SDHCI controller operations
>> +
>> + @param[in] PassThru A pointer to the
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
>> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
>> + @param[in] Slot The 0 based slot index.
>> + @param[in,out] HookType The type of operation and whether the
>> + hook is invoked right before (pre) or
>> + right after (post)
>> +
>> + @retval EFI_SUCCESS The override function completed successfully.
>> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
>> + @retval EFI_INVALID_PARAMETER HookType is invalid
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> + IN EFI_HANDLE ControllerHandle,
>> + IN UINT8 Slot,
>> + IN SD_MMC_OVERRIDE_HOOK HookType
>> + );
>> +
>> +struct _SD_MMC_OVERRIDE {
>> + //
>> + // Protocol version of this implementation
>> + //
>> + UINTN Version;
>> + //
>> + // Callback to override SD/MMC host controller capability bits
>> + //
>> + SD_MMC_OVERRIDE_CAPABILITY OverrideCapability;
>> + //
>> + // Callback to invoke SD/MMC override hooks
>> + //
>> + SD_MMC_OVERRIDE_INVOKE_HOOK InvokeHook;
>> +};
>> +
>> +extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
>> +
>> +#endif
>> diff --git a/MdeModulePkg/MdeModulePkg.dec
>> b/MdeModulePkg/MdeModulePkg.dec index 856d67aceb21..64ceea029f94
>> 100644
>> --- a/MdeModulePkg/MdeModulePkg.dec
>> +++ b/MdeModulePkg/MdeModulePkg.dec
>> @@ -562,6 +562,9 @@ [Protocols]
>> ## Include/Protocol/SmmMemoryAttribute.h
>> gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce,
>> 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
>>
>> + ## Include/Protocol/SdMmcOverride.h
>> + gEdkiiSdMmcOverrideProtocolGuid = { 0xeaf9e3c1, 0xc9cd, 0x46db, {
>> + 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
>> +
>> #
>> # [Error.gEfiMdeModulePkgTokenSpaceGuid]
>> # 0x80000001 | Invalid value provided.
>>
> Based on your patch #2, I think the protocol is supposed to be installed by a platform driver (let's call it SdMmcOverridePlatform) on the controller handle.
>
> There are two ways to produce this Override protocol:
> 1. SdMmcOverridePlatform registers a protocol notification on PciIo and find the SDMMC host controller PCI handle, then install SdMmcOverride on that handle.
> 2. SdMMcOverridePlatform is written as a driver model driver.
> DriverBindingStart() opens PciIo BY_DRIVER and produces SdMmcOverride.
> Then SdMmcPciHc driver opens SdMmcOverride BY_DRIVER and produces SdMmcPassthru.
>
> But since the SdMmcOverride protocol is optional, that makes #2 not workable.
>
> But #1 cannot handle the "reconnect -r" case. Because there is no protocol notification on uninstall.
>
> So I am thinking if it's more proper to make this protocol a platform level singleton instance. I noticed that all APIs exposed by this protocol already carry the Controller Handle.
>
> --
> Thanks,
> Ray
>
Star,
I meant to create a SdMmcPlatform protocol.
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 8:37 ` Ni, Ruiyu
@ 2017-12-05 8:50 ` Zeng, Star
2017-12-05 9:26 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Zeng, Star @ 2017-12-05 8:50 UTC (permalink / raw)
To: Ni, Ruiyu, Ard Biesheuvel, edk2-devel@lists.01.org
Cc: Wu, Hao A, Kinney, Michael D, Tian, Feng,
leif.lindholm@linaro.org, Zeng, Star
Regardless the protocol name SdMmcOverride/SdMmcPlatform, so you mean the protocol should be produced by a DXE driver, but not a UEFI driver, right? I saw the example at https://lists.01.org/pipermail/edk2-devel/2017-November/017672.html shared by Ard installs the protocol in PlatformDxe that is a DXE driver.
And you mean the SD/MMC host controller driver code should use LocateProtocol, but not HandleProtocol to find out the protocol instance, right?
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Tuesday, December 5, 2017 4:37 PM
To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; leif.lindholm@linaro.org
Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
On 12/5/2017 3:20 PM, Zeng, Star wrote:
> If making this protocol a platform level singleton instance, is it so hard to define the interfaces and parameters since different controllers may need different hook points and parameters?
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, December 5, 2017 3:09 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> edk2-devel@lists.01.org
> Cc: "hao.a.wu@intel.com; Kinney.d.michael"@intel.com; Tian, Feng
> <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>;
> leif.lindholm@linaro.org
> Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC
> override protocol
>
> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>> Many ARM based SoCs have integrated SDHCI controllers, and often,
>> these implementations deviate in subtle ways from the pertinent
>> specifications. On the one hand, these deviations are quite easy to
>> work around, but on the other hand, having a collection of SoC
>> specific workarounds in the generic driver stack is undesirable.
>>
>> So let's introduce an optional SD/MMC override protocol that we can
>> invoke at the appropriate moments in the device initialization.
>> That way, the workaround itself remains platform specific, but we can
>> still use the generic driver stack on such platforms.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
>> MdeModulePkg/MdeModulePkg.dec | 3 +
>> 2 files changed, 106 insertions(+)
>>
>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> new file mode 100644
>> index 000000000000..5a5c393896f4
>> --- /dev/null
>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> @@ -0,0 +1,103 @@
>> +/** @file
>> + Protocol to describe overrides required to support non-standard
>> +SDHCI
>> + implementations
>> +
>> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
>> +
>> + This program and the accompanying materials are licensed and made
>> + available under the terms and conditions of the BSD License which
>> + accompanies this distribution. The full text of the license may be
>> + found at http://opensource.org/licenses/bsd-license.php
>> +
>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __SD_MMC_OVERRIDE_H__
>> +#define __SD_MMC_OVERRIDE_H__
>> +
>> +#include <Protocol/SdMmcPassThru.h>
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>> + { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c,
>> +0x83, 0x23, 0x23 } }
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
>> +
>> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
>> +
>> +typedef enum {
>> + SD_MMC_OVERRIDE_RESET_PRE_HOOK,
>> + SD_MMC_OVERRIDE_RESET_POST_HOOK,
>> + SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
>> + SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
>> +} SD_MMC_OVERRIDE_HOOK;
>> +
>> +/**
>> +
>> + Override function for SDHCI capability bits
>> +
>> + @param[in] PassThru A pointer to the
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
>> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
>> + @param[in] Slot The 0 based slot index.
>> + @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure.
>> +
>> + @retval EFI_SUCCESS The override function completed successfully.
>> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
>> + @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> + IN EFI_HANDLE ControllerHandle,
>> + IN UINT8 Slot,
>> + IN OUT UINT64 *SdMmcHcSlotCapability
>> + );
>> +
>> +/**
>> +
>> + Override function for SDHCI controller operations
>> +
>> + @param[in] PassThru A pointer to the
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
>> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
>> + @param[in] Slot The 0 based slot index.
>> + @param[in,out] HookType The type of operation and whether the
>> + hook is invoked right before (pre) or
>> + right after (post)
>> +
>> + @retval EFI_SUCCESS The override function completed successfully.
>> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
>> + @retval EFI_INVALID_PARAMETER HookType is invalid
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> + IN EFI_HANDLE ControllerHandle,
>> + IN UINT8 Slot,
>> + IN SD_MMC_OVERRIDE_HOOK HookType
>> + );
>> +
>> +struct _SD_MMC_OVERRIDE {
>> + //
>> + // Protocol version of this implementation
>> + //
>> + UINTN Version;
>> + //
>> + // Callback to override SD/MMC host controller capability bits
>> + //
>> + SD_MMC_OVERRIDE_CAPABILITY OverrideCapability;
>> + //
>> + // Callback to invoke SD/MMC override hooks
>> + //
>> + SD_MMC_OVERRIDE_INVOKE_HOOK InvokeHook;
>> +};
>> +
>> +extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
>> +
>> +#endif
>> diff --git a/MdeModulePkg/MdeModulePkg.dec
>> b/MdeModulePkg/MdeModulePkg.dec index 856d67aceb21..64ceea029f94
>> 100644
>> --- a/MdeModulePkg/MdeModulePkg.dec
>> +++ b/MdeModulePkg/MdeModulePkg.dec
>> @@ -562,6 +562,9 @@ [Protocols]
>> ## Include/Protocol/SmmMemoryAttribute.h
>> gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce,
>> 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
>>
>> + ## Include/Protocol/SdMmcOverride.h
>> + gEdkiiSdMmcOverrideProtocolGuid = { 0xeaf9e3c1, 0xc9cd, 0x46db, {
>> + 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
>> +
>> #
>> # [Error.gEfiMdeModulePkgTokenSpaceGuid]
>> # 0x80000001 | Invalid value provided.
>>
> Based on your patch #2, I think the protocol is supposed to be installed by a platform driver (let's call it SdMmcOverridePlatform) on the controller handle.
>
> There are two ways to produce this Override protocol:
> 1. SdMmcOverridePlatform registers a protocol notification on PciIo and find the SDMMC host controller PCI handle, then install SdMmcOverride on that handle.
> 2. SdMMcOverridePlatform is written as a driver model driver.
> DriverBindingStart() opens PciIo BY_DRIVER and produces SdMmcOverride.
> Then SdMmcPciHc driver opens SdMmcOverride BY_DRIVER and produces SdMmcPassthru.
>
> But since the SdMmcOverride protocol is optional, that makes #2 not workable.
>
> But #1 cannot handle the "reconnect -r" case. Because there is no protocol notification on uninstall.
>
> So I am thinking if it's more proper to make this protocol a platform level singleton instance. I noticed that all APIs exposed by this protocol already carry the Controller Handle.
>
> --
> Thanks,
> Ray
>
Star,
I meant to create a SdMmcPlatform protocol.
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 8:50 ` Zeng, Star
@ 2017-12-05 9:26 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2017-12-05 9:26 UTC (permalink / raw)
To: Zeng, Star
Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Wu, Hao A, Kinney, Michael D,
Tian, Feng, leif.lindholm@linaro.org
On 5 December 2017 at 08:50, Zeng, Star <star.zeng@intel.com> wrote:
> Regardless the protocol name SdMmcOverride/SdMmcPlatform, so you mean the protocol should be produced by a DXE driver, but not a UEFI driver, right? I saw the example at https://lists.01.org/pipermail/edk2-devel/2017-November/017672.html shared by Ard installs the protocol in PlatformDxe that is a DXE driver.
>
> And you mean the SD/MMC host controller driver code should use LocateProtocol, but not HandleProtocol to find out the protocol instance, right?
>
I think this makes sense, yes. I will respin the second patch accordingly.
--
Ard.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-11-30 10:11 ` [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol Ard Biesheuvel
2017-12-05 7:08 ` Ni, Ruiyu
@ 2017-12-05 10:12 ` Ni, Ruiyu
2017-12-05 10:24 ` Ard Biesheuvel
1 sibling, 1 reply; 17+ messages in thread
From: Ni, Ruiyu @ 2017-12-05 10:12 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel; +Cc: hao.a.wu, feng.tian, star.zeng, leif.lindholm
Some comments re the detailed interfaces embedded in below.
On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
> Many ARM based SoCs have integrated SDHCI controllers, and often,
> these implementations deviate in subtle ways from the pertinent
> specifications. On the one hand, these deviations are quite easy
> to work around, but on the other hand, having a collection of SoC
> specific workarounds in the generic driver stack is undesirable.
>
> So let's introduce an optional SD/MMC override protocol that we
> can invoke at the appropriate moments in the device initialization.
> That way, the workaround itself remains platform specific, but we
> can still use the generic driver stack on such platforms.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
> MdeModulePkg/MdeModulePkg.dec | 3 +
> 2 files changed, 106 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> new file mode 100644
> index 000000000000..5a5c393896f4
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -0,0 +1,103 @@
> +/** @file
> + Protocol to describe overrides required to support non-standard SDHCI
> + implementations
> +
> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
> +
> + This program and the accompanying materials
> + are licensed and made available under the terms and conditions of the BSD License
> + which accompanies this distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __SD_MMC_OVERRIDE_H__
> +#define __SD_MMC_OVERRIDE_H__
> +
> +#include <Protocol/SdMmcPassThru.h>
> +
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> + { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
> +
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
> +
> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
> +
> +typedef enum {
> + SD_MMC_OVERRIDE_RESET_PRE_HOOK,
> + SD_MMC_OVERRIDE_RESET_POST_HOOK,
> + SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
> + SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
How about using name like "SdMmcResetPre"?
Do "ResetPre"/"ResetPost" / "InitHostPre" / "InitHostPost" cover
all potential check points?
> +} SD_MMC_OVERRIDE_HOOK;
How about use "SD_MMC_PHASE_TYPE"?
> +
> +/**
> +
> + Override function for SDHCI capability bits
> +
> + @param[in] PassThru A pointer to the
> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
> + @param[in] Slot The 0 based slot index.
> + @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure.
> +
> + @retval EFI_SUCCESS The override function completed successfully.
> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
> + @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
How about "SD_MMC_CAPABILITY"?
> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
> + IN EFI_HANDLE ControllerHandle,
> + IN UINT8 Slot,
> + IN OUT UINT64 *SdMmcHcSlotCapability
> + );
Is this API too specific?
Besides SlotCapability, is there any other attributes that may need
override as well?
> +
> +/**
> +
> + Override function for SDHCI controller operations
> +
> + @param[in] PassThru A pointer to the
> + EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
> + @param[in] Slot The 0 based slot index.
> + @param[in,out] HookType The type of operation and whether the
> + hook is invoked right before (pre) or
> + right after (post)
> +
> + @retval EFI_SUCCESS The override function completed successfully.
> + @retval EFI_NOT_FOUND The specified controller or slot does not exist.
> + @retval EFI_INVALID_PARAMETER HookType is invalid
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
How about "SD_MMC_NOTIFY_PHASE"?
> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
> + IN EFI_HANDLE ControllerHandle,
> + IN UINT8 Slot,
> + IN SD_MMC_OVERRIDE_HOOK HookType
> + );
> +
> +struct _SD_MMC_OVERRIDE {
> + //
> + // Protocol version of this implementation
> + //
> + UINTN Version;
> + //
> + // Callback to override SD/MMC host controller capability bits
> + //
> + SD_MMC_OVERRIDE_CAPABILITY OverrideCapability;How about "Capability"? So the caller writes code as
"SdMmcOverride->Capability()".
> + //
> + // Callback to invoke SD/MMC override hooks
> + //
> + SD_MMC_OVERRIDE_INVOKE_HOOK InvokeHook;
How about "NotifyPhase"?
> +};
> +
> +extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
> +
> +#endif
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 856d67aceb21..64ceea029f94 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -562,6 +562,9 @@ [Protocols]
> ## Include/Protocol/SmmMemoryAttribute.h
> gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce, 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
>
> + ## Include/Protocol/SdMmcOverride.h
> + gEdkiiSdMmcOverrideProtocolGuid = { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
> +
> #
> # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> # 0x80000001 | Invalid value provided.
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 10:12 ` Ni, Ruiyu
@ 2017-12-05 10:24 ` Ard Biesheuvel
2017-12-05 10:33 ` Zeng, Star
2017-12-05 10:34 ` Ni, Ruiyu
0 siblings, 2 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2017-12-05 10:24 UTC (permalink / raw)
To: Ni, Ruiyu
Cc: edk2-devel@lists.01.org, Wu, Hao A, Tian, Feng, Zeng, Star,
Leif Lindholm
On 5 December 2017 at 10:12, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> Some comments re the detailed interfaces embedded in below.
>
> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>>
>> Many ARM based SoCs have integrated SDHCI controllers, and often,
>> these implementations deviate in subtle ways from the pertinent
>> specifications. On the one hand, these deviations are quite easy
>> to work around, but on the other hand, having a collection of SoC
>> specific workarounds in the generic driver stack is undesirable.
>>
>> So let's introduce an optional SD/MMC override protocol that we
>> can invoke at the appropriate moments in the device initialization.
>> That way, the workaround itself remains platform specific, but we
>> can still use the generic driver stack on such platforms.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
>> MdeModulePkg/MdeModulePkg.dec | 3 +
>> 2 files changed, 106 insertions(+)
>>
>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> new file mode 100644
>> index 000000000000..5a5c393896f4
>> --- /dev/null
>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> @@ -0,0 +1,103 @@
>> +/** @file
>> + Protocol to describe overrides required to support non-standard SDHCI
>> + implementations
>> +
>> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
>> +
>> + This program and the accompanying materials
>> + are licensed and made available under the terms and conditions of the
>> BSD License
>> + which accompanies this distribution. The full text of the license may
>> be found at
>> + http://opensource.org/licenses/bsd-license.php
>> +
>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __SD_MMC_OVERRIDE_H__
>> +#define __SD_MMC_OVERRIDE_H__
>> +
>> +#include <Protocol/SdMmcPassThru.h>
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>> + { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83,
>> 0x23, 0x23 } }
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
>> +
>> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
>> +
>> +typedef enum {
>> + SD_MMC_OVERRIDE_RESET_PRE_HOOK,
>> + SD_MMC_OVERRIDE_RESET_POST_HOOK,
>> + SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
>> + SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
>
>
> How about using name like "SdMmcResetPre"?
Sure
> Do "ResetPre"/"ResetPost" / "InitHostPre" / "InitHostPost" cover
> all potential check points?
>
Perhaps not, but it is hard to predict the future :-) This is why I
added the version field. (I should also update the second patch to
reject the protocol if its version is higher than
EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION)
>> +} SD_MMC_OVERRIDE_HOOK;
>
> How about use "SD_MMC_PHASE_TYPE"?
>
Sure.
>
>> +
>> +/**
>> +
>> + Override function for SDHCI capability bits
>> +
>> + @param[in] PassThru A pointer to the
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL
>> instance.
>> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
>> + @param[in] Slot The 0 based slot index.
>> + @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure.
>> +
>> + @retval EFI_SUCCESS The override function completed
>> successfully.
>> + @retval EFI_NOT_FOUND The specified controller or slot does not
>> exist.
>> + @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
>
>
> How about "SD_MMC_CAPABILITY"?
>
Sure.
>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> + IN EFI_HANDLE ControllerHandle,
>> + IN UINT8 Slot,
>> + IN OUT UINT64 *SdMmcHcSlotCapability
>> + );
>
>
> Is this API too specific?
> Besides SlotCapability, is there any other attributes that may need
> override as well?
>
The capability structure is the root data structure that describes the
SD/MMC host controller. Which other data structures did you have in
mind?
>> +
>> +/**
>> +
>> + Override function for SDHCI controller operations
>> +
>> + @param[in] PassThru A pointer to the
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL
>> instance.
>> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
>> + @param[in] Slot The 0 based slot index.
>> + @param[in,out] HookType The type of operation and whether
>> the
>> + hook is invoked right before
>> (pre) or
>> + right after (post)
>> +
>> + @retval EFI_SUCCESS The override function completed
>> successfully.
>> + @retval EFI_NOT_FOUND The specified controller or slot does not
>> exist.
>> + @retval EFI_INVALID_PARAMETER HookType is invalid
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
>
>
> How about "SD_MMC_NOTIFY_PHASE"?
>
Sure.
>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> + IN EFI_HANDLE ControllerHandle,
>> + IN UINT8 Slot,
>> + IN SD_MMC_OVERRIDE_HOOK HookType
>> + );
>> +
>> +struct _SD_MMC_OVERRIDE {
>> + //
>> + // Protocol version of this implementation
>> + //
>> + UINTN Version;
>> + //
>> + // Callback to override SD/MMC host controller capability bits
>> + //
>> + SD_MMC_OVERRIDE_CAPABILITY OverrideCapability;How about
>> "Capability"? So the caller writes code as
>
> "SdMmcOverride->Capability()".
Sure.
>>
>> + //
>> + // Callback to invoke SD/MMC override hooks
>> + //
>> + SD_MMC_OVERRIDE_INVOKE_HOOK InvokeHook;
>
> How about "NotifyPhase"?
Sure.
>>
>> +};
>> +
>> +extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
>> +
>> +#endif
>> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
>> index 856d67aceb21..64ceea029f94 100644
>> --- a/MdeModulePkg/MdeModulePkg.dec
>> +++ b/MdeModulePkg/MdeModulePkg.dec
>> @@ -562,6 +562,9 @@ [Protocols]
>> ## Include/Protocol/SmmMemoryAttribute.h
>> gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce, 0x402d, {
>> 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
>> + ## Include/Protocol/SdMmcOverride.h
>> + gEdkiiSdMmcOverrideProtocolGuid = { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5,
>> 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
>> +
>> #
>> # [Error.gEfiMdeModulePkgTokenSpaceGuid]
>> # 0x80000001 | Invalid value provided.
>>
>
>
> --
> Thanks,
> Ray
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 10:24 ` Ard Biesheuvel
@ 2017-12-05 10:33 ` Zeng, Star
2017-12-05 10:34 ` Ni, Ruiyu
1 sibling, 0 replies; 17+ messages in thread
From: Zeng, Star @ 2017-12-05 10:33 UTC (permalink / raw)
To: Ard Biesheuvel, Ni, Ruiyu
Cc: Wu, Hao A, edk2-devel@lists.01.org, Leif Lindholm, Tian, Feng,
Zeng, Star
BTW, I prefer all the typedef and structure/field names have EDKII_ prefix, for example MdeModulePkg\Include\Protocol\IoMmu.h and MdeModulePkg\Include\Ppi\SdMmcHostController.h.
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Tuesday, December 5, 2017 6:25 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
On 5 December 2017 at 10:12, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> Some comments re the detailed interfaces embedded in below.
>
> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>>
>> Many ARM based SoCs have integrated SDHCI controllers, and often,
>> these implementations deviate in subtle ways from the pertinent
>> specifications. On the one hand, these deviations are quite easy to
>> work around, but on the other hand, having a collection of SoC
>> specific workarounds in the generic driver stack is undesirable.
>>
>> So let's introduce an optional SD/MMC override protocol that we can
>> invoke at the appropriate moments in the device initialization.
>> That way, the workaround itself remains platform specific, but we can
>> still use the generic driver stack on such platforms.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
>> MdeModulePkg/MdeModulePkg.dec | 3 +
>> 2 files changed, 106 insertions(+)
>>
>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> new file mode 100644
>> index 000000000000..5a5c393896f4
>> --- /dev/null
>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> @@ -0,0 +1,103 @@
>> +/** @file
>> + Protocol to describe overrides required to support non-standard
>> +SDHCI
>> + implementations
>> +
>> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
>> +
>> + This program and the accompanying materials are licensed and made
>> + available under the terms and conditions of the
>> BSD License
>> + which accompanies this distribution. The full text of the license
>> + may
>> be found at
>> + http://opensource.org/licenses/bsd-license.php
>> +
>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>> + EXPRESS OR
>> IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __SD_MMC_OVERRIDE_H__
>> +#define __SD_MMC_OVERRIDE_H__
>> +
>> +#include <Protocol/SdMmcPassThru.h>
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>> + { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c,
>> +0x83,
>> 0x23, 0x23 } }
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
>> +
>> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
>> +
>> +typedef enum {
>> + SD_MMC_OVERRIDE_RESET_PRE_HOOK,
>> + SD_MMC_OVERRIDE_RESET_POST_HOOK,
>> + SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
>> + SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
>
>
> How about using name like "SdMmcResetPre"?
Sure
> Do "ResetPre"/"ResetPost" / "InitHostPre" / "InitHostPost" cover all
> potential check points?
>
Perhaps not, but it is hard to predict the future :-) This is why I added the version field. (I should also update the second patch to reject the protocol if its version is higher than
EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION)
>> +} SD_MMC_OVERRIDE_HOOK;
>
> How about use "SD_MMC_PHASE_TYPE"?
>
Sure.
>
>> +
>> +/**
>> +
>> + Override function for SDHCI capability bits
>> +
>> + @param[in] PassThru A pointer to the
>> +
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL
>> instance.
>> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
>> + @param[in] Slot The 0 based slot index.
>> + @param[in,out] SdMmcHcSlotCapability The SDHCI capability structure.
>> +
>> + @retval EFI_SUCCESS The override function completed
>> successfully.
>> + @retval EFI_NOT_FOUND The specified controller or slot does not
>> exist.
>> + @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
>
>
> How about "SD_MMC_CAPABILITY"?
>
Sure.
>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> + IN EFI_HANDLE ControllerHandle,
>> + IN UINT8 Slot,
>> + IN OUT UINT64 *SdMmcHcSlotCapability
>> + );
>
>
> Is this API too specific?
> Besides SlotCapability, is there any other attributes that may need
> override as well?
>
The capability structure is the root data structure that describes the SD/MMC host controller. Which other data structures did you have in mind?
>> +
>> +/**
>> +
>> + Override function for SDHCI controller operations
>> +
>> + @param[in] PassThru A pointer to the
>> +
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL
>> instance.
>> + @param[in] ControllerHandle The EFI_HANDLE of the controller.
>> + @param[in] Slot The 0 based slot index.
>> + @param[in,out] HookType The type of operation and whether
>> the
>> + hook is invoked right before
>> (pre) or
>> + right after (post)
>> +
>> + @retval EFI_SUCCESS The override function completed
>> successfully.
>> + @retval EFI_NOT_FOUND The specified controller or slot does not
>> exist.
>> + @retval EFI_INVALID_PARAMETER HookType is invalid
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
>
>
> How about "SD_MMC_NOTIFY_PHASE"?
>
Sure.
>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> + IN EFI_HANDLE ControllerHandle,
>> + IN UINT8 Slot,
>> + IN SD_MMC_OVERRIDE_HOOK HookType
>> + );
>> +
>> +struct _SD_MMC_OVERRIDE {
>> + //
>> + // Protocol version of this implementation
>> + //
>> + UINTN Version;
>> + //
>> + // Callback to override SD/MMC host controller capability bits
>> + //
>> + SD_MMC_OVERRIDE_CAPABILITY OverrideCapability;How about
>> "Capability"? So the caller writes code as
>
> "SdMmcOverride->Capability()".
Sure.
>>
>> + //
>> + // Callback to invoke SD/MMC override hooks //
>> + SD_MMC_OVERRIDE_INVOKE_HOOK InvokeHook;
>
> How about "NotifyPhase"?
Sure.
>>
>> +};
>> +
>> +extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
>> +
>> +#endif
>> diff --git a/MdeModulePkg/MdeModulePkg.dec
>> b/MdeModulePkg/MdeModulePkg.dec index 856d67aceb21..64ceea029f94
>> 100644
>> --- a/MdeModulePkg/MdeModulePkg.dec
>> +++ b/MdeModulePkg/MdeModulePkg.dec
>> @@ -562,6 +562,9 @@ [Protocols]
>> ## Include/Protocol/SmmMemoryAttribute.h
>> gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce,
>> 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
>> + ## Include/Protocol/SdMmcOverride.h
>> + gEdkiiSdMmcOverrideProtocolGuid = { 0xeaf9e3c1, 0xc9cd, 0x46db, {
>> + 0xa5,
>> 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
>> +
>> #
>> # [Error.gEfiMdeModulePkgTokenSpaceGuid]
>> # 0x80000001 | Invalid value provided.
>>
>
>
> --
> Thanks,
> Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 10:24 ` Ard Biesheuvel
2017-12-05 10:33 ` Zeng, Star
@ 2017-12-05 10:34 ` Ni, Ruiyu
2017-12-05 13:09 ` Wu, Hao A
1 sibling, 1 reply; 17+ messages in thread
From: Ni, Ruiyu @ 2017-12-05 10:34 UTC (permalink / raw)
To: Ard Biesheuvel, Hao A
Cc: edk2-devel@lists.01.org, Leif Lindholm, Tian, Feng, Zeng, Star
On 12/5/2017 6:24 PM, Ard Biesheuvel wrote:
> On 5 December 2017 at 10:12, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>> Some comments re the detailed interfaces embedded in below.
>>
>> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>>>
>>> Many ARM based SoCs have integrated SDHCI controllers, and often,
>>> these implementations deviate in subtle ways from the pertinent
>>> specifications. On the one hand, these deviations are quite easy
>>> to work around, but on the other hand, having a collection of SoC
>>> specific workarounds in the generic driver stack is undesirable.
>>>
>>> So let's introduce an optional SD/MMC override protocol that we
>>> can invoke at the appropriate moments in the device initialization.
>>> That way, the workaround itself remains platform specific, but we
>>> can still use the generic driver stack on such platforms.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 ++++++++++++++++++++
>>> MdeModulePkg/MdeModulePkg.dec | 3 +
>>> 2 files changed, 106 insertions(+)
>>>
>>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>>> new file mode 100644
>>> index 000000000000..5a5c393896f4
>>> --- /dev/null
>>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>>> @@ -0,0 +1,103 @@
>>> +/** @file
>>> + Protocol to describe overrides required to support non-standard SDHCI
>>> + implementations
>>> ...
>>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>>> + IN EFI_HANDLE ControllerHandle,
>>> + IN UINT8 Slot,
>>> + IN OUT UINT64 *SdMmcHcSlotCapability
>>> + );
>>
>>
>> Is this API too specific?
>> Besides SlotCapability, is there any other attributes that may need
>> override as well?
>>
>
> The capability structure is the root data structure that describes the
> SD/MMC host controller. Which other data structures did you have in
> mind?
>
I do not know either.
Hao, any comments?
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden
2017-11-30 10:11 ` [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden Ard Biesheuvel
@ 2017-12-05 12:46 ` Wu, Hao A
0 siblings, 0 replies; 17+ messages in thread
From: Wu, Hao A @ 2017-12-05 12:46 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org
Cc: leif.lindholm@linaro.org, Zeng, Star, Tian, Feng, Ni, Ruiyu
One small comment below.
Best Regards,
Hao Wu
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, November 30, 2017 6:12 PM
> To: edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org; Zeng, Star; Wu, Hao A; Tian, Feng; Ard Biesheuvel
> Subject: [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities
> to be overridden
>
> 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 | 116
> +++++++++++++++++++-
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 6 +
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf | 2 +
> 3 files changed, 119 insertions(+), 5 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index 0be8828abfcc..61f64285807d 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -60,7 +60,8 @@ SD_MMC_HC_PRIVATE_DATA gSdMmcPciHcTemplate = {
> { // MaxCurrent
> 0,
> },
> - 0 // ControllerVersion
> + 0, // ControllerVersion
> + NULL // Override
> };
>
> SD_DEVICE_PATH mSdDpTemplate = {
> @@ -213,6 +214,92 @@ Done:
> return;
> }
>
Please help to add the function description comments for function:
SdMmcPciHcResetHost() & SdMmcPciHcInitHost()
> +STATIC
> +EFI_STATUS
> +SdMmcPciHcResetHost (
> + IN SD_MMC_HC_PRIVATE_DATA *Private,
> + IN UINT8 Slot
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (Private->Override != NULL &&
> + Private->Override->InvokeHook != NULL) {
> + Status = Private->Override->InvokeHook (
> + &Private->PassThru,
> + Private->ControllerHandle,
> + Slot,
> + SD_MMC_OVERRIDE_RESET_PRE_HOOK);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_WARN, "%a: SD/MMC pre reset hook failed - %r\n",
> + __FUNCTION__, Status));
> + return Status;
> + }
> + }
> +
> + Status = SdMmcHcReset (Private->PciIo, Slot);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if (Private->Override != NULL &&
> + Private->Override->InvokeHook != NULL) {
> + Status = Private->Override->InvokeHook (
> + &Private->PassThru,
> + Private->ControllerHandle,
> + Slot,
> + SD_MMC_OVERRIDE_RESET_POST_HOOK);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_WARN, "%a: SD/MMC post reset hook failed - %r\n",
> + __FUNCTION__, Status));
> + }
> + }
> + return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +SdMmcPciHcInitHost (
> + IN SD_MMC_HC_PRIVATE_DATA *Private,
> + IN UINT8 Slot
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (Private->Override != NULL &&
> + Private->Override->InvokeHook != NULL) {
> + Status = Private->Override->InvokeHook (
> + &Private->PassThru,
> + Private->ControllerHandle,
> + Slot,
> + SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_WARN, "%a: SD/MMC pre init hook failed - %r\n",
> + __FUNCTION__, Status));
> + return Status;
> + }
> + }
> +
> + Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if (Private->Override != NULL &&
> + Private->Override->InvokeHook != NULL) {
> + Status = Private->Override->InvokeHook (
> + &Private->PassThru,
> + Private->ControllerHandle,
> + Slot,
> + SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_WARN, "%a: SD/MMC post init hook failed - %r\n",
> + __FUNCTION__, Status));
> + }
> + }
> + return Status;
> +}
> +
> /**
> Sd removable device enumeration callback function when the timer event is
> signaled.
>
> @@ -281,14 +368,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 +688,12 @@ SdMmcPciHcDriverBindingStart (
> goto Done;
> }
>
> + Status = gBS->HandleProtocol (Controller,
> &gEdkiiSdMmcOverrideProtocolGuid,
> + (VOID **)&Private->Override);
> + if (!EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_INFO, "%a: using SD/MMC override protocol\n",
> __FUNCTION__));
> + }
> +
> Support64BitDma = TRUE;
> for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
> Private->Slot[Slot].Enable = TRUE;
> @@ -609,6 +702,19 @@ SdMmcPciHcDriverBindingStart (
> if (EFI_ERROR (Status)) {
> continue;
> }
> + if (Private->Override != NULL &&
> + Private->Override->OverrideCapability != NULL) {
> + Status = Private->Override->OverrideCapability (
> + &Private->PassThru,
> + Controller,
> + Slot,
> + (VOID *)&Private->Capability[Slot]);
> + 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 +733,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 +748,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..b51e0529f885 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"
> @@ -115,6 +116,11 @@ typedef struct {
> UINT64 MaxCurrent[SD_MMC_HC_MAX_SLOT];
>
> UINT32 ControllerVersion;
> +
> + //
> + // Optional protcol to deal with non-standard SDHCI implementations
> + //
> + SD_MMC_OVERRIDE *Override;
> } SD_MMC_HC_PRIVATE_DATA;
>
> #define SD_MMC_HC_TRB_SIG SIGNATURE_32 ('T', 'R', 'B', 'T')
> 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
> --
> 2.11.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 10:34 ` Ni, Ruiyu
@ 2017-12-05 13:09 ` Wu, Hao A
2017-12-05 17:16 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Wu, Hao A @ 2017-12-05 13:09 UTC (permalink / raw)
To: Ni, Ruiyu, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm, Tian, Feng, Zeng, Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, December 05, 2017 6:35 PM
> To: Ard Biesheuvel; Wu, Hao A
> Cc: edk2-devel@lists.01.org; Leif Lindholm; Tian, Feng; Zeng, Star
> Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override
> protocol
>
> On 12/5/2017 6:24 PM, Ard Biesheuvel wrote:
> > On 5 December 2017 at 10:12, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >> Some comments re the detailed interfaces embedded in below.
> >>
> >> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
> >>>
> >>> Many ARM based SoCs have integrated SDHCI controllers, and often,
> >>> these implementations deviate in subtle ways from the pertinent
> >>> specifications. On the one hand, these deviations are quite easy
> >>> to work around, but on the other hand, having a collection of SoC
> >>> specific workarounds in the generic driver stack is undesirable.
> >>>
> >>> So let's introduce an optional SD/MMC override protocol that we
> >>> can invoke at the appropriate moments in the device initialization.
> >>> That way, the workaround itself remains platform specific, but we
> >>> can still use the generic driver stack on such platforms.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> ---
> >>> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103
> ++++++++++++++++++++
> >>> MdeModulePkg/MdeModulePkg.dec | 3 +
> >>> 2 files changed, 106 insertions(+)
> >>>
> >>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> >>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> >>> new file mode 100644
> >>> index 000000000000..5a5c393896f4
> >>> --- /dev/null
> >>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> >>> @@ -0,0 +1,103 @@
> >>> +/** @file
> >>> + Protocol to describe overrides required to support non-standard SDHCI
> >>> + implementations
> >>> ...
> >>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
> >>> + IN EFI_HANDLE ControllerHandle,
> >>> + IN UINT8 Slot,
> >>> + IN OUT UINT64 *SdMmcHcSlotCapability
> >>> + );
> >>
> >>
> >> Is this API too specific?
> >> Besides SlotCapability, is there any other attributes that may need
> >> override as well?
> >>
> >
> > The capability structure is the root data structure that describes the
> > SD/MMC host controller. Which other data structures did you have in
> > mind?
> >
>
> I do not know either.
> Hao, any comments?
The service is overriding the 'Capability' field of the private data
structure within the HC driver. It's the value read from the 'CAP'
register of the SD/MMC HC.
After a glance of the other fields in 'SD_MMC_HC_PRIVATE_DATA', maybe the
'MaxCurrent' field is another candidate can be overriden. However, I am
not sure about this.
Best Regards,
Hao Wu
>
> --
> Thanks,
> Ray
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
2017-12-05 13:09 ` Wu, Hao A
@ 2017-12-05 17:16 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2017-12-05 17:16 UTC (permalink / raw)
To: Wu, Hao A
Cc: Ni, Ruiyu, edk2-devel@lists.01.org, Leif Lindholm, Tian, Feng,
Zeng, Star
On 5 December 2017 at 13:09, Wu, Hao A <hao.a.wu@intel.com> wrote:
>> -----Original Message-----
>> From: Ni, Ruiyu
>> Sent: Tuesday, December 05, 2017 6:35 PM
>> To: Ard Biesheuvel; Wu, Hao A
>> Cc: edk2-devel@lists.01.org; Leif Lindholm; Tian, Feng; Zeng, Star
>> Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override
>> protocol
>>
>> On 12/5/2017 6:24 PM, Ard Biesheuvel wrote:
>> > On 5 December 2017 at 10:12, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>> >> Some comments re the detailed interfaces embedded in below.
>> >>
>> >> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>> >>>
>> >>> Many ARM based SoCs have integrated SDHCI controllers, and often,
>> >>> these implementations deviate in subtle ways from the pertinent
>> >>> specifications. On the one hand, these deviations are quite easy
>> >>> to work around, but on the other hand, having a collection of SoC
>> >>> specific workarounds in the generic driver stack is undesirable.
>> >>>
>> >>> So let's introduce an optional SD/MMC override protocol that we
>> >>> can invoke at the appropriate moments in the device initialization.
>> >>> That way, the workaround itself remains platform specific, but we
>> >>> can still use the generic driver stack on such platforms.
>> >>>
>> >>> Contributed-under: TianoCore Contribution Agreement 1.1
>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >>> ---
>> >>> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103
>> ++++++++++++++++++++
>> >>> MdeModulePkg/MdeModulePkg.dec | 3 +
>> >>> 2 files changed, 106 insertions(+)
>> >>>
>> >>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> >>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> >>> new file mode 100644
>> >>> index 000000000000..5a5c393896f4
>> >>> --- /dev/null
>> >>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> >>> @@ -0,0 +1,103 @@
>> >>> +/** @file
>> >>> + Protocol to describe overrides required to support non-standard SDHCI
>> >>> + implementations
>> >>> ...
>> >>> + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru,
>> >>> + IN EFI_HANDLE ControllerHandle,
>> >>> + IN UINT8 Slot,
>> >>> + IN OUT UINT64 *SdMmcHcSlotCapability
>> >>> + );
>> >>
>> >>
>> >> Is this API too specific?
>> >> Besides SlotCapability, is there any other attributes that may need
>> >> override as well?
>> >>
>> >
>> > The capability structure is the root data structure that describes the
>> > SD/MMC host controller. Which other data structures did you have in
>> > mind?
>> >
>>
>> I do not know either.
>> Hao, any comments?
>
> The service is overriding the 'Capability' field of the private data
> structure within the HC driver. It's the value read from the 'CAP'
> register of the SD/MMC HC.
>
> After a glance of the other fields in 'SD_MMC_HC_PRIVATE_DATA', maybe the
> 'MaxCurrent' field is another candidate can be overriden. However, I am
> not sure about this.
>
In my experience, trying to cover every imaginable use case upfront
usually fails. That is why I used enum based hooks, which are easily
expanded later if we need to. If we need to override maxcurrent later
on, we can just add it.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-12-05 17:12 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-30 10:11 [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
2017-11-30 10:11 ` [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol Ard Biesheuvel
2017-12-05 7:08 ` Ni, Ruiyu
2017-12-05 7:20 ` Zeng, Star
2017-12-05 8:37 ` Ni, Ruiyu
2017-12-05 8:50 ` Zeng, Star
2017-12-05 9:26 ` Ard Biesheuvel
2017-12-05 10:12 ` Ni, Ruiyu
2017-12-05 10:24 ` Ard Biesheuvel
2017-12-05 10:33 ` Zeng, Star
2017-12-05 10:34 ` Ni, Ruiyu
2017-12-05 13:09 ` Wu, Hao A
2017-12-05 17:16 ` Ard Biesheuvel
2017-11-30 10:11 ` [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden Ard Biesheuvel
2017-12-05 12:46 ` Wu, Hao A
2017-12-04 14:53 ` [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
2017-12-05 2:08 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox