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

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

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