[PATCH v3 0/2] quirks handling for SDHCI controllers

[PATCH v3 0/2] quirks handling for SDHCI controllers

[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 v2:
- use a singleton instance of the SD/MMC protocol rather than one per
  controller; this is needed to support 'reconnect -r', as pointed out
  by Ray
- use EDKII prefixes for all types defined by the protocol
- replace 'hook' with 'notify', and tweak some other identifiers
- add missing function comment headers for factored out functions

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 always 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   | 134 +++++++++++++++++++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   |   1 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |   2 +
 MdeModulePkg/Include/Protocol/SdMmcOverride.h        | 103 +++++++++++++++
 MdeModulePkg/MdeModulePkg.dec                        |   3 +
 5 files changed, 239 insertions(+), 4 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/SdMmcOverride.h

-- 
2.11.0

[PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

[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..af57988f5625
--- /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 _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
+
+typedef enum {
+  EdkiiSdMmcResetPre,
+  EdkiiSdMmcResetPost,
+  EdkiiSdMmcInitHostPre,
+  EdkiiSdMmcInitHostPost,
+} EDKII_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 * EDKII_SD_MMC_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]  PhaseType             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 * EDKII_SD_MMC_NOTIFY_PHASE) (
+  IN      EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN      EFI_HANDLE                      ControllerHandle,
+  IN      UINT8                           Slot,
+  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
+  );
+
+struct _EDKII_SD_MMC_OVERRIDE {
+  //
+  // Protocol version of this implementation
+  //
+  UINTN                         Version;
+  //
+  // Callback to override SD/MMC host controller capability bits
+  //
+  EDKII_SD_MMC_CAPABILITY       Capability;
+  //
+  // Callback to invoke SD/MMC override hooks
+  //
+  EDKII_SD_MMC_NOTIFY_PHASE     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.
-- 
2.11.0

Re: [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

[Ni, Ruiyu]

On 12/6/2017 2:01 AM, 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..af57988f5625
> --- /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 _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
> +
> +typedef enum {
> +  EdkiiSdMmcResetPre,
> +  EdkiiSdMmcResetPost,
> +  EdkiiSdMmcInitHostPre,
> +  EdkiiSdMmcInitHostPost,
> +} EDKII_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 * EDKII_SD_MMC_CAPABILITY) (
> +  IN      EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
By looking a bit deeper, I get confused about this parameter.
SdMmcOverride protocol is supposed to be used by SdMmcHostController
driver to produce SdMmcPassthru protocol.
But how does SdMmcOverride uses the not-yet-produced SdMmcPassthru 
protocol? It's like a chicken-egg problem.

> +  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]  PhaseType             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 * EDKII_SD_MMC_NOTIFY_PHASE) (
> +  IN      EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
> +  IN      EFI_HANDLE                      ControllerHandle,
> +  IN      UINT8                           Slot,
> +  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
> +  );
> +
> +struct _EDKII_SD_MMC_OVERRIDE {
> +  //
> +  // Protocol version of this implementation
> +  //
> +  UINTN                         Version;
> +  //
> +  // Callback to override SD/MMC host controller capability bits
> +  //
> +  EDKII_SD_MMC_CAPABILITY       Capability;
> +  //
> +  // Callback to invoke SD/MMC override hooks
> +  //
> +  EDKII_SD_MMC_NOTIFY_PHASE     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

Re: [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

[Zeng, Star]

It should be not needed for Capability, but may be needed for NotifyPhase.
Hao can explain more.


Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu 
Sent: Wednesday, December 6, 2017 11:22 AM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
Cc: leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; Tian, Feng <feng.tian@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: Re: [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

On 12/6/2017 2:01 AM, 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..af57988f5625
> --- /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 _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
> +
> +typedef enum {
> +  EdkiiSdMmcResetPre,
> +  EdkiiSdMmcResetPost,
> +  EdkiiSdMmcInitHostPre,
> +  EdkiiSdMmcInitHostPost,
> +} EDKII_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 * EDKII_SD_MMC_CAPABILITY) (
> +  IN      EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
By looking a bit deeper, I get confused about this parameter.
SdMmcOverride protocol is supposed to be used by SdMmcHostController driver to produce SdMmcPassthru protocol.
But how does SdMmcOverride uses the not-yet-produced SdMmcPassthru protocol? It's like a chicken-egg problem.

> +  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]  PhaseType             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 * EDKII_SD_MMC_NOTIFY_PHASE) (
> +  IN      EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
> +  IN      EFI_HANDLE                      ControllerHandle,
> +  IN      UINT8                           Slot,
> +  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
> +  );
> +
> +struct _EDKII_SD_MMC_OVERRIDE {
> +  //
> +  // Protocol version of this implementation
> +  //
> +  UINTN                         Version;
> +  //
> +  // Callback to override SD/MMC host controller capability bits
> +  //
> +  EDKII_SD_MMC_CAPABILITY       Capability;
> +  //
> +  // Callback to invoke SD/MMC override hooks
> +  //
> +  EDKII_SD_MMC_NOTIFY_PHASE     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

Re: [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

[Ni, Ruiyu]

On 12/6/2017 11:25 AM, Zeng, Star wrote:
> It should be not needed for Capability, but may be needed for NotifyPhase.
> Hao can explain more.

When the SdMmcPassthru protocol is installed by gBS->InstallProtocol(),
that's the indication of readiness of this protocol.

Before that, it's not guaranteed that every interface of SdMmcPassthru
is safe to call, e.g.: GetNextSlot or ResetDevice may not work because
the device enumeration depends on the SdMmcOverride.

Even for PassThru interface, is might be possible that some private
data needed by PassThru to work hasn't been initialized, or
future implementation might choose to defer some private data
initialization to just before protocol installation.

I think after Hao's evaluation, if PassThru is only needed and
must-use interface for some usage case, we could just pass the PassThru
function pointer to SdMmcOverride.

> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, December 6, 2017 11:22 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; Tian, Feng <feng.tian@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: Re: [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol
> 
> On 12/6/2017 2:01 AM, 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..af57988f5625
>> --- /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 _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
>> +
>> +typedef enum {
>> +  EdkiiSdMmcResetPre,
>> +  EdkiiSdMmcResetPost,
>> +  EdkiiSdMmcInitHostPre,
>> +  EdkiiSdMmcInitHostPost,
>> +} EDKII_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 * EDKII_SD_MMC_CAPABILITY) (
>> +  IN      EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
> By looking a bit deeper, I get confused about this parameter.
> SdMmcOverride protocol is supposed to be used by SdMmcHostController driver to produce SdMmcPassthru protocol.
> But how does SdMmcOverride uses the not-yet-produced SdMmcPassthru protocol? It's like a chicken-egg problem.
> 
>> +  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]  PhaseType             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 * EDKII_SD_MMC_NOTIFY_PHASE) (
>> +  IN      EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
>> +  IN      EFI_HANDLE                      ControllerHandle,
>> +  IN      UINT8                           Slot,
>> +  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
>> +  );
>> +
>> +struct _EDKII_SD_MMC_OVERRIDE {
>> +  //
>> +  // Protocol version of this implementation
>> +  //
>> +  UINTN                         Version;
>> +  //
>> +  // Callback to override SD/MMC host controller capability bits
>> +  //
>> +  EDKII_SD_MMC_CAPABILITY       Capability;
>> +  //
>> +  // Callback to invoke SD/MMC override hooks
>> +  //
>> +  EDKII_SD_MMC_NOTIFY_PHASE     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
> 


-- 
Thanks,
Ray

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

[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   | 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
+  )
+{
+  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);
+    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]);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n",
+          __FUNCTION__, Status));
+        continue;
+      }
+    }
     DumpCapabilityReg (Slot, &Private->Capability[Slot]);
 
     Support64BitDma &= Private->Capability[Slot].SysBus64;
@@ -627,7 +753,7 @@ SdMmcPciHcDriverBindingStart (
     //
     // Reset the specified slot of the SD/MMC Pci Host Controller
     //
-    Status = SdMmcHcReset (PciIo, Slot);
+    Status = SdMmcPciHcResetHost (Private, Slot);
     if (EFI_ERROR (Status)) {
       continue;
     }
@@ -642,7 +768,7 @@ SdMmcPciHcDriverBindingStart (
       continue;
     }
 
-    Status = SdMmcHcInitHost (PciIo, Slot, Private->Capability[Slot]);
+    Status = SdMmcPciHcInitHost (Private, Slot);
     if (EFI_ERROR (Status)) {
       continue;
     }
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index 6a2a27969936..8830cd437edd 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -35,6 +35,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Protocol/DriverBinding.h>
 #include <Protocol/ComponentName.h>
 #include <Protocol/ComponentName2.h>
+#include <Protocol/SdMmcOverride.h>
 #include <Protocol/SdMmcPassThru.h>
 
 #include "SdMmcPciHci.h"
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
index e26e6a098c17..154ce45d8223 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
@@ -48,6 +48,7 @@ [Sources]
 
 [Packages]
   MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
 
 [LibraryClasses]
   DevicePathLib
@@ -61,6 +62,7 @@ [LibraryClasses]
   DebugLib
 
 [Protocols]
+  gEdkiiSdMmcOverrideProtocolGuid               ## SOMETIMES_CONSUMES
   gEfiDevicePathProtocolGuid                    ## TO_START
   gEfiPciIoProtocolGuid                         ## TO_START
   gEfiSdMmcPassThruProtocolGuid                 ## BY_START
-- 
2.11.0

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

[Ni, Ruiyu]

Ard,
I should have provided some of them in the last version.
Sorry about that.

We just found an internal/private SdMmcPciHc implementation
developed by other teams. We are evaluating whether your
proposed SdMmcOverride can be used to retire that private
implementation.


On 12/6/2017 2:01 AM, Ard Biesheuvel wrote:
> Invoke the newly introduced SD/MMC override protocol to override
> the capabilities register after reading it from the device registers,
> and to call the pre/post host init and reset hooks at the appropriate
> times.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c   | 134 +++++++++++++++++++-
>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   |   1 +
>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |   2 +
>   3 files changed, 133 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index 0be8828abfcc..f1ea78de809e 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -17,6 +17,8 @@
>   
>   #include "SdMmcPciHcDxe.h"
>   
> +STATIC EDKII_SD_MMC_OVERRIDE           *mOverride;
> +
>   //
>   // Driver Global Variables
>   //
> @@ -214,6 +216,104 @@ Done:
>   }
>   
>   /**
> +  Execute a SD/MMC host controller reset
> +
> +  @param[in] Private      A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
> +  @param[in] Slot         The slot number of the host controller to reset.
> +**/
> +STATIC
> +EFI_STATUS
> +SdMmcPciHcResetHost (
> +  IN  SD_MMC_HC_PRIVATE_DATA          *Private,
> +  IN  UINT8                           Slot
> +  )
I checked the implementation of SdMmcHcReset(). It's quite simple.
So how about we do not introduce this new function SdMmcPciHcResetHost,
but just put the two NotifyPhase call inside SdMmcHcReset().

Because the names of the two functions (SdMmcPciHcResetHost and
SdMmcHcReset) are quite similar. After not a long period, maintainer
may get confused about which is which.

I agree parameters of SdMmcHcReset need to change.

> +{
> +  EFI_STATUS    Status;
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          &Private->PassThru,
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcResetPre);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: SD/MMC pre reset notifier callback failed - %r\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +  }
> +
> +  Status = SdMmcHcReset (Private->PciIo, Slot);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          &Private->PassThru,
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcResetPost);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: SD/MMC post reset notifier callback failed - %r\n",
> +        __FUNCTION__, Status));
> +    }
> +  }
> +  return Status;
> +}
> +
> +/**
> +  Initialize a SD/MMC host controller
> +
> +  @param[in] Private      A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
> +  @param[in] Slot         The slot number of the host controller to initialize.
> +**/
> +STATIC
> +EFI_STATUS
> +SdMmcPciHcInitHost (
> +  IN  SD_MMC_HC_PRIVATE_DATA          *Private,
> +  IN  UINT8                           Slot
> +  )
> +{
> +  EFI_STATUS    Status;
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          &Private->PassThru,
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcInitHostPre);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: SD/MMC pre init notifier callback failed - %r\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +  }
> +
> +  Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          &Private->PassThru,
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcInitHostPost);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: SD/MMC post init notifier callback failed - %r\n",
> +        __FUNCTION__, Status));
> +    }
> +  }
> +  return Status;
> +}
> +
> +/**
>     Sd removable device enumeration callback function when the timer event is signaled.
>   
>     @param[in]  Event     The Event this notify function registered to.
> @@ -281,14 +381,14 @@ SdMmcPciHcEnumerateDevice (
>           //
>           // Reset the specified slot of the SD/MMC Pci Host Controller
>           //
> -        Status = SdMmcHcReset (Private->PciIo, Slot);
> +        Status = SdMmcPciHcResetHost (Private, Slot);
>           if (EFI_ERROR (Status)) {
>             continue;
>           }
>           //
>           // Reinitialize slot and restart identification process for the new attached device
>           //
> -        Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
> +        Status = SdMmcPciHcInitHost (Private, Slot);
>           if (EFI_ERROR (Status)) {
>             continue;
>           }
> @@ -601,6 +701,20 @@ SdMmcPciHcDriverBindingStart (
>       goto Done;
>     }
>   
> +  //
> +  // Attempt to locate the singleton instance of the SD/MMC override protocol,
> +  // which implements platform specific workarounds for non-standard SDHCI
> +  // implementations.
> +  //
> +  if (mOverride == NULL) {
> +    Status = gBS->HandleProtocol (Controller, &gEdkiiSdMmcOverrideProtocolGuid,
> +                    (VOID **)&mOverride);

HandleProtocol() on the Controller cannot work. HandleProtocol() looks
for the SdMmcOverride instance on the specific handle.

You should use LocateProtocol() to look for the SdMmcOverride instance
regardless the handle.

> +    if (!EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_INFO, "%a: found SD/MMC override protocol\n",
> +        __FUNCTION__));
> +    }
> +  }
> +
>     Support64BitDma = TRUE;
>     for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
>       Private->Slot[Slot].Enable = TRUE;
> @@ -609,6 +723,18 @@ SdMmcPciHcDriverBindingStart (
>       if (EFI_ERROR (Status)) {
>         continue;
>       }
> +    if (mOverride != NULL && mOverride->Capability != NULL) {
> +      Status = mOverride->Capability (
> +                            &Private->PassThru,
> +                            Controller,
> +                            Slot,
> +                            (VOID *)&Private->Capability[Slot]);
(VOID *) can be removed?

> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n",
> +          __FUNCTION__, Status));
> +        continue;
> +      }
> +    }
>       DumpCapabilityReg (Slot, &Private->Capability[Slot]);
>   
>       Support64BitDma &= Private->Capability[Slot].SysBus64;
> @@ -627,7 +753,7 @@ SdMmcPciHcDriverBindingStart (
>       //
>       // Reset the specified slot of the SD/MMC Pci Host Controller
>       //
> -    Status = SdMmcHcReset (PciIo, Slot);
> +    Status = SdMmcPciHcResetHost (Private, Slot);
>       if (EFI_ERROR (Status)) {
>         continue;
>       }
> @@ -642,7 +768,7 @@ SdMmcPciHcDriverBindingStart (
>         continue;
>       }
>   
> -    Status = SdMmcHcInitHost (PciIo, Slot, Private->Capability[Slot]);
> +    Status = SdMmcPciHcInitHost (Private, Slot);
>       if (EFI_ERROR (Status)) {
>         continue;
>       }
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index 6a2a27969936..8830cd437edd 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -35,6 +35,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>   #include <Protocol/DriverBinding.h>
>   #include <Protocol/ComponentName.h>
>   #include <Protocol/ComponentName2.h>
> +#include <Protocol/SdMmcOverride.h>
>   #include <Protocol/SdMmcPassThru.h>
>   
>   #include "SdMmcPciHci.h"
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> index e26e6a098c17..154ce45d8223 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> @@ -48,6 +48,7 @@ [Sources]
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>   
>   [LibraryClasses]
>     DevicePathLib
> @@ -61,6 +62,7 @@ [LibraryClasses]
>     DebugLib
>   
>   [Protocols]
> +  gEdkiiSdMmcOverrideProtocolGuid               ## SOMETIMES_CONSUMES
>     gEfiDevicePathProtocolGuid                    ## TO_START
>     gEfiPciIoProtocolGuid                         ## TO_START
>     gEfiSdMmcPassThruProtocolGuid                 ## BY_START
> 


-- 
Thanks,
Ray

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

[Ard Biesheuvel]

On 6 December 2017 at 03:33, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> Ard,
> I should have provided some of them in the last version.
> Sorry about that.
>
> We just found an internal/private SdMmcPciHc implementation
> developed by other teams. We are evaluating whether your
> proposed SdMmcOverride can be used to retire that private
> implementation.
>

OK, good to know.

>
>
> On 12/6/2017 2:01 AM, Ard Biesheuvel wrote:
>>
>> Invoke the newly introduced SD/MMC override protocol to override
>> the capabilities register after reading it from the device registers,
>> and to call the pre/post host init and reset hooks at the appropriate
>> times.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c   | 134
>> +++++++++++++++++++-
>>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   |   1 +
>>   MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |   2 +
>>   3 files changed, 133 insertions(+), 4 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
>> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
>> index 0be8828abfcc..f1ea78de809e 100644
>> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
>> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
>> @@ -17,6 +17,8 @@
>>     #include "SdMmcPciHcDxe.h"
>>   +STATIC EDKII_SD_MMC_OVERRIDE           *mOverride;
>> +
>>   //
>>   // Driver Global Variables
>>   //
>> @@ -214,6 +216,104 @@ Done:
>>   }
>>     /**
>> +  Execute a SD/MMC host controller reset
>> +
>> +  @param[in] Private      A pointer to the SD_MMC_HC_PRIVATE_DATA
>> instance.
>> +  @param[in] Slot         The slot number of the host controller to
>> reset.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +SdMmcPciHcResetHost (
>> +  IN  SD_MMC_HC_PRIVATE_DATA          *Private,
>> +  IN  UINT8                           Slot
>> +  )
>
> I checked the implementation of SdMmcHcReset(). It's quite simple.
> So how about we do not introduce this new function SdMmcPciHcResetHost,
> but just put the two NotifyPhase call inside SdMmcHcReset().
>
> Because the names of the two functions (SdMmcPciHcResetHost and
> SdMmcHcReset) are quite similar. After not a long period, maintainer
> may get confused about which is which.
>
> I agree parameters of SdMmcHcReset need to change.
>

OK, that works for me.

>
>> +{
>> +  EFI_STATUS    Status;
>> +
>> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
>> +    Status = mOverride->NotifyPhase (
>> +                          &Private->PassThru,
>> +                          Private->ControllerHandle,
>> +                          Slot,
>> +                          EdkiiSdMmcResetPre);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_WARN,
>> +        "%a: SD/MMC pre reset notifier callback failed - %r\n",
>> +        __FUNCTION__, Status));
>> +      return Status;
>> +    }
>> +  }
>> +
>> +  Status = SdMmcHcReset (Private->PciIo, Slot);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
>> +    Status = mOverride->NotifyPhase (
>> +                          &Private->PassThru,
>> +                          Private->ControllerHandle,
>> +                          Slot,
>> +                          EdkiiSdMmcResetPost);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_WARN,
>> +        "%a: SD/MMC post reset notifier callback failed - %r\n",
>> +        __FUNCTION__, Status));
>> +    }
>> +  }
>> +  return Status;
>> +}
>> +
>> +/**
>> +  Initialize a SD/MMC host controller
>> +
>> +  @param[in] Private      A pointer to the SD_MMC_HC_PRIVATE_DATA
>> instance.
>> +  @param[in] Slot         The slot number of the host controller to
>> initialize.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +SdMmcPciHcInitHost (
>> +  IN  SD_MMC_HC_PRIVATE_DATA          *Private,
>> +  IN  UINT8                           Slot
>> +  )
>> +{
>> +  EFI_STATUS    Status;
>> +
>> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
>> +    Status = mOverride->NotifyPhase (
>> +                          &Private->PassThru,
>> +                          Private->ControllerHandle,
>> +                          Slot,
>> +                          EdkiiSdMmcInitHostPre);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_WARN,
>> +        "%a: SD/MMC pre init notifier callback failed - %r\n",
>> +        __FUNCTION__, Status));
>> +      return Status;
>> +    }
>> +  }
>> +
>> +  Status = SdMmcHcInitHost (Private->PciIo, Slot,
>> Private->Capability[Slot]);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
>> +    Status = mOverride->NotifyPhase (
>> +                          &Private->PassThru,
>> +                          Private->ControllerHandle,
>> +                          Slot,
>> +                          EdkiiSdMmcInitHostPost);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_WARN,
>> +        "%a: SD/MMC post init notifier callback failed - %r\n",
>> +        __FUNCTION__, Status));
>> +    }
>> +  }
>> +  return Status;
>> +}
>> +
>> +/**
>>     Sd removable device enumeration callback function when the timer event
>> is signaled.
>>       @param[in]  Event     The Event this notify function registered to.
>> @@ -281,14 +381,14 @@ SdMmcPciHcEnumerateDevice (
>>           //
>>           // Reset the specified slot of the SD/MMC Pci Host Controller
>>           //
>> -        Status = SdMmcHcReset (Private->PciIo, Slot);
>> +        Status = SdMmcPciHcResetHost (Private, Slot);
>>           if (EFI_ERROR (Status)) {
>>             continue;
>>           }
>>           //
>>           // Reinitialize slot and restart identification process for the
>> new attached device
>>           //
>> -        Status = SdMmcHcInitHost (Private->PciIo, Slot,
>> Private->Capability[Slot]);
>> +        Status = SdMmcPciHcInitHost (Private, Slot);
>>           if (EFI_ERROR (Status)) {
>>             continue;
>>           }
>> @@ -601,6 +701,20 @@ SdMmcPciHcDriverBindingStart (
>>       goto Done;
>>     }
>>   +  //
>> +  // Attempt to locate the singleton instance of the SD/MMC override
>> protocol,
>> +  // which implements platform specific workarounds for non-standard
>> SDHCI
>> +  // implementations.
>> +  //
>> +  if (mOverride == NULL) {
>> +    Status = gBS->HandleProtocol (Controller,
>> &gEdkiiSdMmcOverrideProtocolGuid,
>> +                    (VOID **)&mOverride);
>
>
> HandleProtocol() on the Controller cannot work. HandleProtocol() looks
> for the SdMmcOverride instance on the specific handle.
>
> You should use LocateProtocol() to look for the SdMmcOverride instance
> regardless the handle.
>

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

>> +    if (!EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_INFO, "%a: found SD/MMC override protocol\n",
>> +        __FUNCTION__));
>> +    }
>> +  }
>> +
>>     Support64BitDma = TRUE;
>>     for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
>>       Private->Slot[Slot].Enable = TRUE;
>> @@ -609,6 +723,18 @@ SdMmcPciHcDriverBindingStart (
>>       if (EFI_ERROR (Status)) {
>>         continue;
>>       }
>> +    if (mOverride != NULL && mOverride->Capability != NULL) {
>> +      Status = mOverride->Capability (
>> +                            &Private->PassThru,
>> +                            Controller,
>> +                            Slot,
>> +                            (VOID *)&Private->Capability[Slot]);
>
> (VOID *) can be removed?
>

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

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

>
>> +      if (EFI_ERROR (Status)) {
>> +        DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n",
>> +          __FUNCTION__, Status));
>> +        continue;
>> +      }
>> +    }
>>       DumpCapabilityReg (Slot, &Private->Capability[Slot]);
>>         Support64BitDma &= Private->Capability[Slot].SysBus64;
>> @@ -627,7 +753,7 @@ SdMmcPciHcDriverBindingStart (
>>       //
>>       // Reset the specified slot of the SD/MMC Pci Host Controller
>>       //
>> -    Status = SdMmcHcReset (PciIo, Slot);
>> +    Status = SdMmcPciHcResetHost (Private, Slot);
>>       if (EFI_ERROR (Status)) {
>>         continue;
>>       }
>> @@ -642,7 +768,7 @@ SdMmcPciHcDriverBindingStart (
>>         continue;
>>       }
>>   -    Status = SdMmcHcInitHost (PciIo, Slot, Private->Capability[Slot]);
>> +    Status = SdMmcPciHcInitHost (Private, Slot);
>>       if (EFI_ERROR (Status)) {
>>         continue;
>>       }
>> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
>> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
>> index 6a2a27969936..8830cd437edd 100644
>> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
>> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
>> @@ -35,6 +35,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER EXPRESS OR IMPLIED.
>>   #include <Protocol/DriverBinding.h>
>>   #include <Protocol/ComponentName.h>
>>   #include <Protocol/ComponentName2.h>
>> +#include <Protocol/SdMmcOverride.h>
>>   #include <Protocol/SdMmcPassThru.h>
>>     #include "SdMmcPciHci.h"
>> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
>> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
>> index e26e6a098c17..154ce45d8223 100644
>> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
>> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
>> @@ -48,6 +48,7 @@ [Sources]
>>     [Packages]
>>     MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>>     [LibraryClasses]
>>     DevicePathLib
>> @@ -61,6 +62,7 @@ [LibraryClasses]
>>     DebugLib
>>     [Protocols]
>> +  gEdkiiSdMmcOverrideProtocolGuid               ## SOMETIMES_CONSUMES
>>     gEfiDevicePathProtocolGuid                    ## TO_START
>>     gEfiPciIoProtocolGuid                         ## TO_START
>>     gEfiSdMmcPassThruProtocolGuid                 ## BY_START
>>
>
>
> --
> Thanks,
> Ray

Re: [PATCH v3 0/2] quirks handling for SDHCI controllers

[Ard Biesheuvel]

On 5 December 2017 at 18:01, 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 v2:
> - use a singleton instance of the SD/MMC protocol rather than one per
>   controller; this is needed to support 'reconnect -r', as pointed out
>   by Ray
> - use EDKII prefixes for all types defined by the protocol
> - replace 'hook' with 'notify', and tweak some other identifiers
> - add missing function comment headers for factored out functions
>
> 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 always 64 bits
>   in size)
>

Comments anyone?

Re: [PATCH v3 0/2] quirks handling for SDHCI controllers

[Ard Biesheuvel]

On 11 December 2017 at 23:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 5 December 2017 at 18:01, 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 v2:
>> - use a singleton instance of the SD/MMC protocol rather than one per
>>   controller; this is needed to support 'reconnect -r', as pointed out
>>   by Ray
>> - use EDKII prefixes for all types defined by the protocol
>> - replace 'hook' with 'notify', and tweak some other identifiers
>> - add missing function comment headers for factored out functions
>>
>> 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 always 64 bits
>>   in size)
>>
>
> Comments anyone?

Please ignore - I thought I had sent out my v4 already, but apparently not.