public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Zeng, Star" <star.zeng@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	"Ni, Ruiyu" <ruiyu.ni@intel.com>
Cc: "Wu, Hao A" <hao.a.wu@intel.com>,
	"edk2-devel@lists.01.org" <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: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
Date: Tue, 5 Dec 2017 10:33:52 +0000	[thread overview]
Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B9BFC6D@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <CAKv+Gu_x6EV58auKLMeUghWoPnf8qxRkpgGa4bnnAagg-CceSw@mail.gmail.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


  reply	other threads:[~2017-12-05 10:29 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-30 10:11 [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
2017-11-30 10:11 ` [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol Ard Biesheuvel
2017-12-05  7:08   ` Ni, Ruiyu
2017-12-05  7:20     ` Zeng, Star
2017-12-05  8:37       ` Ni, Ruiyu
2017-12-05  8:50         ` Zeng, Star
2017-12-05  9:26           ` Ard Biesheuvel
2017-12-05 10:12   ` Ni, Ruiyu
2017-12-05 10:24     ` Ard Biesheuvel
2017-12-05 10:33       ` Zeng, Star [this message]
2017-12-05 10:34       ` Ni, Ruiyu
2017-12-05 13:09         ` Wu, Hao A
2017-12-05 17:16           ` Ard Biesheuvel
2017-11-30 10:11 ` [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden Ard Biesheuvel
2017-12-05 12:46   ` Wu, Hao A
2017-12-04 14:53 ` [PATCH v2 0/2] quirks handling for SDHCI controllers Ard Biesheuvel
2017-12-05  2:08   ` Zeng, Star

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0C09AFA07DD0434D9E2A0C6AEB0483103B9BFC6D@shsmsx102.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox