On 8 October 2018 at 15:37, Marcin Wojtas <m...@semihalf.com> wrote:
> pon., 8 paź 2018 o 15:27 Ard Biesheuvel <ard.biesheu...@linaro.org>
> napisał(a):
>>
>> On 8 October 2018 at 15:17, Marcin Wojtas <m...@semihalf.com> wrote:
>> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel <ard.biesheu...@linaro.org>
>> > napisał(a):
>> >>
>> >> On 8 October 2018 at 14:59, Marcin Wojtas <m...@semihalf.com> wrote:
>> >> > Hi Ard,
>> >> >
>> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel <ard.biesheu...@linaro.org>
>> >> > napisał(a):
>> >> >>
>> >> >> (add MdeModulePkg maintainers)
>> >> >>
>> >> >> On 5 October 2018 at 15:25, Marcin Wojtas <m...@semihalf.com> wrote:
>> >> >> > From: Tomasz Michalec <t...@semihalf.com>
>> >> >> >
>> >> >> > Some SD Host Controlers use different values in Host Control 2
>> >> >> > Register
>> >> >> > to select UHS Mode. This patch adds a new UhsSignaling type routine
>> >> >> > to
>> >> >> > the NotifyPhase of the SdMmcOverride protocol.
>> >> >> >
>> >> >> > UHS signaling configuration is moved to a common, default routine
>> >> >> > (SdMmcHcUhsSignaling), which is called when SdMmcOverride does not
>> >> >> > cover this functionality.
>> >> >> >
>> >> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> >> > Signed-off-by: Marcin Wojtas <m...@semihalf.com>
>> >> >> > ---
>> >> >> > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 50 +++++++
>> >> >> > MdeModulePkg/Include/Protocol/SdMmcOverride.h | 2 +
>> >> >> > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 153
>> >> >> > ++++++++++++--------
>> >> >> > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 37 +++--
>> >> >> > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 69 +++++++++
>> >> >> > 5 files changed, 243 insertions(+), 68 deletions(-)
>> >> >> >
>> >> >> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> > index e389d52..a03160d 100644
>> >> >> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
>> >> >> > @@ -63,6 +63,39 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>> >> >> > KIND, EITHER EXPRESS OR IMPLIED.
>> >> >> > #define SD_MMC_HC_CTRL_VER 0xFE
>> >> >> >
>> >> >> > //
>> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
>> >> >> > +//
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x0000
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR25 0x0001
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR50 0x0002
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR104 0x0003
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_DDR50 0x0004
>> >> >> > +#define SD_MMC_HC_CTRL_MMC_DDR52 0x0004
>> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR50 0x0002
>> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR25 0x0001
>> >> >> > +#define SD_MMC_HC_CTRL_MMC_SDR12 0x0000
>> >> >> > +#define SD_MMC_HC_CTRL_HS200 0x0003
>> >> >> > +#define SD_MMC_HC_CTRL_HS400 0x0005
>> >> >> > +
>
> In case we move enums to SdMmcOverride.h, would it be desired, to move
> there register fields values as well? Or should I rather use Xenon
> macros for all of above locally?
>
No, I think the macros should be kept locally.
>> >> >> > +//
>> >> >> > +// Timing modes for uhs
>> >> >> > +//
>> >> >> > +typedef enum {
>> >> >> > + SdMmcUhsSdr12,
>> >> >> > + SdMmcUhsSdr25,
>> >> >> > + SdMmcUhsSdr50,
>> >> >> > + SdMmcUhsSdr104,
>> >> >> > + SdMmcUhsDdr50,
>> >> >> > + SdMmcMmcDdr52,
>> >> >> > + SdMmcMmcSdr50,
>> >> >> > + SdMmcMmcSdr25,
>> >> >> > + SdMmcMmcSdr12,
>> >> >> > + SdMmcMmcHs200,
>> >> >> > + SdMmcMmcHs400,
>> >> >> > +} SD_MMC_UHS_TIMING;
>> >> >> > +
>> >> >>
>> >> >> Here, we end up with two sets of symbolic constants for the same
>> >> >> thing, and I suppose this enum will be duplicated in your
>> >> >> SdMmcOverride implementation?
>> >> >>
>> >> >
>> >> > Why duplicated? Macros are for generic UHS_MODE_SEL field values for
>> >> > SD and MMC in HostControl2Register.
>> >> >
>> >> > SD_MMC_UHS_TIMING is just a timing mode indicator, it can be used not
>> >> > only in UhsSignaling routine (actually the next patch, with
>> >> > SwitchClockFreqPost, use it...).
>> >> >
>> >> > In my SdMmcOverride implementation this enum is not duplicated,
>> >> > because this file (SdMmcPciHci.h) is included via
>> >> > Protocol/SdMmcOverride.h.
>> >> >
>> >>
>> >> Ah ok. Please don't expose internal headers of the SD/MMC driver via
>> >> Protocol/SdMmcOverride.h
>> >>
>> >
>> > OK.
>> >
>> >> I think it should be fine to add the enum definition to
>> >> Protocol/SdMmcOverride.h instead.
>> >>
>> >
>> > OK.
>> >
>> >> But wouldn't it be much easier to have a hook for setting
>> >> HostControl2Register that decodes the value and modifies it according
>> >> to what the platform requires?
>> >>
>> >
>> > Can you please explain, how it will be different from UhsSignaling in
>> > current shape (read required timing value and update UHS_MODE_SEL
>> > field)?
>> >
>>
>> Well, you decode the value, and if, e.g., the SD_MMC_HC_CTRL_HS200
>> bits are set, you substitute them with the appropriate xenon values.
>
> Because values can be same for SD and MMC (e.g. UHS_104 and HS200),
> from the controller driver perspective, how would I know, which mode
> is requested?
>
Good point.
>>
>> Also, how important is it to drive the SD/MMC at its max rated speed
>> at boot time? On Synquacer, I just disable HS200 in the capability
>> struct so I can forget about all this stuff
>
> Some customers want it - a real life scenario from one of them:
> applications, Linux binaries and rootfs stores in the MMC. Each boot a
> couple of hundreds of MB to be loaded. Thanks to HS200 we have huge
> time saving.
>
Do you mean in the initrd? Because otherwise, Linux will use its own
driver and select its own mode.
And btw, does the spec permit using different HC2 values for HS200 / HS400 ?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
