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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

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