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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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