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

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

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