Re: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Marcin Wojtas <mw@semihalf.com>]

wt., 9 paź 2018 o 13:28 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> > Biesheuvel
> > Sent: Monday, October 08, 2018 11:10 PM
> > To: Marcin Wojtas; Ni, Ruiyu; Wu, Hao A
> > Cc: Tian, Feng; Tomasz Michalec; Dong, Eric; edk2-devel-01; Gao, Liming;
> > Nadav Haklai; Kinney, Michael D; Zeng, Star
> > Subject: Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add
> > UhsSignaling to SdMmcOverride protocol
> >
> > On 8 October 2018 at 16:52, Marcin Wojtas <mw@semihalf.com> wrote:
> > > pon., 8 paź 2018 o 15:43 Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > napisał(a):
> > >>
> > >> On 8 October 2018 at 15:37, Marcin Wojtas <mw@semihalf.com> wrote:
> > >> > pon., 8 paź 2018 o 15:27 Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > napisał(a):
> > >> >>
> > >> >> On 8 October 2018 at 15:17, Marcin Wojtas <mw@semihalf.com> wrote:
> > >> >> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > napisał(a):
> > >> >> >>
> > >> >> >> On 8 October 2018 at 14:59, Marcin Wojtas <mw@semihalf.com>
> > wrote:
> > >> >> >> > Hi Ard,
> > >> >> >> >
> > >> >> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > napisał(a):
> > >> >> >> >>
> > >> >> >> >> (add MdeModulePkg maintainers)
> > >> >> >> >>
> > >> >> >> >> On 5 October 2018 at 15:25, Marcin Wojtas <mw@semihalf.com>
> > wrote:
> > >> >> >> >> > From: Tomasz Michalec <tm@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 <mw@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.
> > >
> > > No, I mean loading >300MB images from eMMC to memory in the DXE phase,
> > > before booting anything.
> > >
> >
> > Yikes
> >
> > >>
> > >> And btw, does the spec permit using different HC2 values for HS200 / HS400 ?
> > >
> > > According to SD Host Controller Specification v4.20, UHS_MODE_SEL
> > > values 0x5 and 0x6 are "reserved". According to Linux code, HS400
> > > value is treated as "non-standard" and HS200 should be same as for
> > > SDR104 (0x3). Nothing is written about permiting different values, but
> > > given Linux 'sdhci_set_uhs_signaling' and whole bunch of other quirks
> > > used under drivers/mmc overriding standard behavior is very common.
> > > Please let know your desired way of handling custom UhsSignaling.
> > >
> >
> > I suppose this is defined by the eMMC spec.
> >
> > Ruiyu, Hao, could you clarify? Are the host control 2 register values
> > for HS200/HS400 defined by the eMMC spec?
>
> Hi Ard and Marcin,
>
> As far as I know, the EMMC Electrical Standard Spec 5.1 (latest) does not
> mention on how to set the "UHS Mode Select" field of the Host Control 2
> Register when switching to HS200/HS400. (Actually, the EMMC spec does not
> mention Host Control 2 Register at all)
>
> When it comes to setting the bus mode for EMMC devices, the current
> implementation of the SdMmcPciHcDxe driver does a mapping when setting the
> Host Control 2 Register:
>
> EMMC High Speed SDR - Freq: 0-52 MHz, Data Rate: Single
> matches
> SD   SDR25          - Freq: 0-50 MHz, Data Rate: Single
>
> EMMC High Speed DDR - Freq: 0-52 MHz, Data Rate: Dual
> matches
> SD   DDR50          - Freq: 0-50 MHz, Data Rate: Dual
>
> EMMC HS200          - Freq: 0-200 MHz, Data Rate: Single
> matches
> SD   SDR104         - Freq: 0-208 MHz, Data Rate: Single
>
> EMMC HS400          - Freq: 0-200 MHz, Data Rate: Dual
> matches
> SD   None
>
> And there is no obvious counterpart for the EMMC HS400 mode in the SD
> spec. The driver currently sets the "UHS Mode Select" field to a reserved
> value 0x5.
>

Thank you Hao, above is on par with what the default UhsSignaling
routine does in this patch. IMO especially in case the EMMC standard
is not unequivocal regarding UHS_MODE_SEL, I'd encourage to accept
some way of updating HostControl2 register, depending on the
implementation. What is your opinion Ard?

Best regards,
Marcin