public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Wu, Hao A" <hao.a.wu@intel.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>,
	"Tian, Feng" <feng.tian@intel.com>,
	"leif.lindholm@linaro.org" <leif.lindholm@linaro.org>
Subject: Re: [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol
Date: Tue, 5 Dec 2017 16:37:06 +0800	[thread overview]
Message-ID: <9c301a27-024d-3ba8-65f5-260303d2c9ac@Intel.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9BFA78@shsmsx102.ccr.corp.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


  reply	other threads:[~2017-12-05  8:32 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 [this message]
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
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=9c301a27-024d-3ba8-65f5-260303d2c9ac@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