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

["Zeng, Star" <star.zeng@intel.com>]

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