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

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

On 5 December 2017 at 13:09, Wu, Hao A <hao.a.wu@intel.com> wrote:
>> -----Original Message-----
>> From: Ni, Ruiyu
>> Sent: Tuesday, December 05, 2017 6:35 PM
>> To: Ard Biesheuvel; Wu, Hao A
>> Cc: edk2-devel@lists.01.org; Leif Lindholm; Tian, Feng; Zeng, Star
>> Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override
>> protocol
>>
>> On 12/5/2017 6:24 PM, Ard Biesheuvel wrote:
>> > 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
>>  >>> ...
>> >>> +  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?
>> >
>>
>> I do not know either.
>> Hao, any comments?
>
> The service is overriding the 'Capability' field of the private data
> structure within the HC driver. It's the value read from the 'CAP'
> register of the SD/MMC HC.
>
> After a glance of the other fields in 'SD_MMC_HC_PRIVATE_DATA', maybe the
> 'MaxCurrent' field is another candidate can be overriden. However, I am
> not sure about this.
>

In my experience, trying to cover every imaginable use case upfront
usually fails. That is why I used enum based hooks, which are easily
expanded later if we need to. If we need to override maxcurrent later
on, we can just add it.