From: "Wu, Hao A" <hao.a.wu@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Marcin Wojtas <mw@semihalf.com>, "Ni, Ruiyu" <ruiyu.ni@intel.com>
Cc: "Tian, Feng" <feng.tian@intel.com>,
Tomasz Michalec <tm@semihalf.com>,
"Dong, Eric" <eric.dong@intel.com>,
edk2-devel-01 <edk2-devel@lists.01.org>,
"Gao, Liming" <liming.gao@intel.com>,
Nadav Haklai <nadavh@marvell.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
Date: Tue, 9 Oct 2018 11:22:54 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C829580@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <CAKv+Gu_JtyJgxOcGkLeq3O+J2Jb0uUZLTjuH9-c1+h-FHz3bxg@mail.gmail.com>
> -----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.
Best Regards,
Hao Wu
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
next prev parent reply other threads:[~2018-10-09 11:28 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-05 13:25 [PATCH v2 0/4] SdMmcOverride extension Marcin Wojtas
2018-10-05 13:25 ` [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase Marcin Wojtas
2018-10-08 12:21 ` Ard Biesheuvel
2018-10-05 13:25 ` [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol Marcin Wojtas
2018-10-05 15:12 ` Philippe Mathieu-Daudé
2018-10-05 15:17 ` Marcin Wojtas
2018-10-08 12:41 ` Ard Biesheuvel
2018-10-08 12:59 ` Marcin Wojtas
2018-10-08 13:07 ` Ard Biesheuvel
2018-10-08 13:17 ` Marcin Wojtas
2018-10-08 13:27 ` Ard Biesheuvel
2018-10-08 13:37 ` Marcin Wojtas
2018-10-08 13:43 ` Ard Biesheuvel
2018-10-08 14:52 ` Marcin Wojtas
2018-10-08 15:10 ` Ard Biesheuvel
2018-10-09 11:22 ` Wu, Hao A [this message]
2018-10-09 11:32 ` Marcin Wojtas
2018-10-09 11:45 ` Ard Biesheuvel
2018-10-09 11:51 ` Marcin Wojtas
2018-10-11 15:43 ` Marcin Wojtas
2018-10-12 1:39 ` Wu, Hao A
2018-10-12 5:06 ` Marcin Wojtas
2018-10-12 15:55 ` Ard Biesheuvel
2018-10-12 16:04 ` Marcin Wojtas
2018-10-12 16:24 ` Ard Biesheuvel
2018-10-12 16:49 ` Marcin Wojtas
2018-11-01 7:04 ` Wu, Hao A
2018-11-02 8:21 ` Marcin Wojtas
2018-11-02 12:16 ` Marcin Wojtas
2018-11-03 2:57 ` Wu, Hao A
2018-10-05 13:25 ` [PATCH v2 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride Marcin Wojtas
2018-10-08 12:44 ` Ard Biesheuvel
2018-11-01 7:06 ` Wu, Hao A
2018-11-02 9:39 ` Marcin Wojtas
2018-11-03 3:19 ` Wu, Hao A
2018-10-05 13:25 ` [PATCH v2 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency Marcin Wojtas
2018-10-08 12:49 ` Ard Biesheuvel
2018-11-01 7:11 ` Wu, Hao A
2018-11-02 9:52 ` Marcin Wojtas
2018-10-12 5:24 ` [PATCH v2 0/4] SdMmcOverride extension Wu, Hao A
2018-10-12 5:33 ` Marcin Wojtas
2018-10-12 12:48 ` Wu, Hao A
2018-10-12 12:50 ` Marcin Wojtas
2018-10-25 12:43 ` Marcin Wojtas
2018-10-26 7:22 ` Wu, Hao A
2018-11-01 7:11 ` Wu, Hao A
2018-11-02 10:09 ` Marcin Wojtas
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=B80AF82E9BFB8E4FBD8C89DA810C6A093C829580@SHSMSX104.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