Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
> -Original Message-
> From: Marcin Wojtas [mailto:m...@semihalf.com]
> Sent: Friday, November 02, 2018 8:17 PM
> To: Wu, Hao A
> Cc: edk2-devel-01; Kinney, Michael D; Gao, Liming; Leif Lindholm; Ard
> Biesheuvel; nad...@marvell.com; j...@semihalf.com; Tomasz Michalec
> Subject: Re: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling
> to SdMmcOverride protocol
>
> pt., 2 lis 2018 o 09:21 Marcin Wojtas napisał(a):
> >
> > Hi Hao,
> >
> > czw., 1 lis 2018 o 08:06 Wu, Hao A napisał(a):
> > >
> > > Hi Marcin,
> > >
> > > > -Original Message-
> > > > From: Marcin Wojtas [mailto:m...@semihalf.com]
> > > > Sent: Friday, October 05, 2018 9:25 PM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Tian, Feng; Kinney, Michael D; Gao, Liming;
> > > > leif.lindh...@linaro.org;
> Wu,
> > > > Hao A; ard.biesheu...@linaro.org; nad...@marvell.com;
> > > > m...@semihalf.com; j...@semihalf.com; t...@semihalf.com
> > > > Subject: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add
> UhsSignaling
> > > > to SdMmcOverride protocol
> > > >
> > > > From: Tomasz Michalec
> > > >
> > > > 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
> > > > ---
> > > > 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_VER0xFE
> > > >
> > > > //
> > > > +// SD Host Controler bits to HOST_CTRL2 register
> > > > +//
> > > > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> > > > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> > > > +#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
> > >
> > > I think SD_MMC_HC_CTRL_MMC_SDR50 is not needed here.
> > >
> > > Since according to the SD Physical Layer Simplified Specification, max
> > > clock
> > > frequency for SD bus mode SDR50 is 100MHz. And there is no eMMC bus
> mode whose
> > > max clock frequency is at 100MHz in Embedded Multi-Media Card Electrical
> > > Standard (5.1).
> >
> > Ok, will drop it.
> >
> > >
> > > > +#define SD_MMC_HC_CTRL_MMC_SDR25 0x0001
> > > > +#define SD_MMC_HC_CTRL_MMC_SDR12 0x
> > > > +#define SD_MMC_HC_CTRL_HS200 0x0003
> > > > +#define SD_MMC_HC_CTRL_HS400 0x0005
> > >
> > > How about the below renames & reorder?
> > >
> > > SD_MMC_HC_CTRL_MMC_LEGACY 0x
> > > SD_MMC_HC_CTRL_MMC_HS_SDR 0x0001
> > > SD_MMC_HC_CTRL_MMC_HS_DDR 0x0004
> > > SD_MMC_HC_CTRL_MMC_HS200 0x0003
> > > SD_MMC_HC_CTRL_MMC_HS400 0x0005
> >
> > Ok.
> >
> > >
> > > > +
> > > > +//
> > > > +// Timing modes for uhs
> > > > +//
> > > > +typedef enum {
> > > > + SdMmcUhsSdr12,
> > > > + SdMmcUhsSdr25,
> > > > + SdMmcUhsSdr50,
> > > > + SdMmcUhsSdr104,
> > > > + SdMmcUhsDdr50,
> > > > + SdMmcMmcDdr52,
> > > > + SdMmcMmcSdr50,
> > > > + SdMmcMmcSdr25,
> > > > + SdMmcMmcSdr12,
> > > > + SdMmcMmcHs200,
> > > > + SdMmcMmcHs400,
> > > > +} SD_MMC_UHS_TIMING;
> > >
> > > Suggest a similar drop of 'SdMmcMmcSdr50' and rename according to the
> above
> > > comments upon HOST_CTRL2 register value definitions. Also, how about a
> rename
> > > for enum to SD_MMC_BUS_MODE?
> >
> > Ok.
> >
> > >
> > > > +
> > > > +//
> > > > // The transfer modes supported by SD Host Controller
> > > > // Simplified Spec 3.0 Table 1-2
> > > > //
> > > > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
> > > >IN UINT8 Slot
> > > >);
> > > >
> > > > +/**
> > > > + Set SD Host Controler control 2 registry according to selected speed.
> > > > +
> > > > + @param[in] PciIo The PCI IO
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
pt., 2 lis 2018 o 09:21 Marcin Wojtas napisał(a):
>
> Hi Hao,
>
> czw., 1 lis 2018 o 08:06 Wu, Hao A napisał(a):
> >
> > Hi Marcin,
> >
> > > -Original Message-
> > > From: Marcin Wojtas [mailto:m...@semihalf.com]
> > > Sent: Friday, October 05, 2018 9:25 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindh...@linaro.org;
> > > Wu,
> > > Hao A; ard.biesheu...@linaro.org; nad...@marvell.com;
> > > m...@semihalf.com; j...@semihalf.com; t...@semihalf.com
> > > Subject: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling
> > > to SdMmcOverride protocol
> > >
> > > From: Tomasz Michalec
> > >
> > > 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
> > > ---
> > > 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_VER0xFE
> > >
> > > //
> > > +// SD Host Controler bits to HOST_CTRL2 register
> > > +//
> > > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> > > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> > > +#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
> >
> > I think SD_MMC_HC_CTRL_MMC_SDR50 is not needed here.
> >
> > Since according to the SD Physical Layer Simplified Specification, max clock
> > frequency for SD bus mode SDR50 is 100MHz. And there is no eMMC bus mode
> > whose
> > max clock frequency is at 100MHz in Embedded Multi-Media Card Electrical
> > Standard (5.1).
>
> Ok, will drop it.
>
> >
> > > +#define SD_MMC_HC_CTRL_MMC_SDR25 0x0001
> > > +#define SD_MMC_HC_CTRL_MMC_SDR12 0x
> > > +#define SD_MMC_HC_CTRL_HS200 0x0003
> > > +#define SD_MMC_HC_CTRL_HS400 0x0005
> >
> > How about the below renames & reorder?
> >
> > SD_MMC_HC_CTRL_MMC_LEGACY 0x
> > SD_MMC_HC_CTRL_MMC_HS_SDR 0x0001
> > SD_MMC_HC_CTRL_MMC_HS_DDR 0x0004
> > SD_MMC_HC_CTRL_MMC_HS200 0x0003
> > SD_MMC_HC_CTRL_MMC_HS400 0x0005
>
> Ok.
>
> >
> > > +
> > > +//
> > > +// Timing modes for uhs
> > > +//
> > > +typedef enum {
> > > + SdMmcUhsSdr12,
> > > + SdMmcUhsSdr25,
> > > + SdMmcUhsSdr50,
> > > + SdMmcUhsSdr104,
> > > + SdMmcUhsDdr50,
> > > + SdMmcMmcDdr52,
> > > + SdMmcMmcSdr50,
> > > + SdMmcMmcSdr25,
> > > + SdMmcMmcSdr12,
> > > + SdMmcMmcHs200,
> > > + SdMmcMmcHs400,
> > > +} SD_MMC_UHS_TIMING;
> >
> > Suggest a similar drop of 'SdMmcMmcSdr50' and rename according to the above
> > comments upon HOST_CTRL2 register value definitions. Also, how about a
> > rename
> > for enum to SD_MMC_BUS_MODE?
>
> Ok.
>
> >
> > > +
> > > +//
> > > // The transfer modes supported by SD Host Controller
> > > // Simplified Spec 3.0 Table 1-2
> > > //
> > > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
> > >IN UINT8 Slot
> > >);
> > >
> > > +/**
> > > + Set SD Host Controler control 2 registry according to selected speed.
> > > +
> > > + @param[in] PciIo The PCI IO protocol instance.
> > > + @param[in] Slot The slot number of the SD card to send the
> > > command to.
> > > + @param[in] Timing The timing to select.
> > > +
> > > + @retval EFI_SUCCESS The timing is set successfully.
> > > + @retval OthersThe timing isn't set successfully.
> > > +**/
> > > +EFI_STATUS
> > > +SdMmcHcUhsSignaling (
> > > + IN EFI_PCI_IO_PROTOCOL*PciIo,
> > > + IN UINT8 Slot,
> > > + IN SD_MMC_UHS_TIMING Timing
> > > + );
> > > +
> > > #endif
> > > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > >
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
Hi Hao,
czw., 1 lis 2018 o 08:06 Wu, Hao A napisał(a):
>
> Hi Marcin,
>
> > -Original Message-
> > From: Marcin Wojtas [mailto:m...@semihalf.com]
> > Sent: Friday, October 05, 2018 9:25 PM
> > To: edk2-devel@lists.01.org
> > Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindh...@linaro.org;
> > Wu,
> > Hao A; ard.biesheu...@linaro.org; nad...@marvell.com;
> > m...@semihalf.com; j...@semihalf.com; t...@semihalf.com
> > Subject: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling
> > to SdMmcOverride protocol
> >
> > From: Tomasz Michalec
> >
> > 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
> > ---
> > 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_VER0xFE
> >
> > //
> > +// SD Host Controler bits to HOST_CTRL2 register
> > +//
> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> > +#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
>
> I think SD_MMC_HC_CTRL_MMC_SDR50 is not needed here.
>
> Since according to the SD Physical Layer Simplified Specification, max clock
> frequency for SD bus mode SDR50 is 100MHz. And there is no eMMC bus mode whose
> max clock frequency is at 100MHz in Embedded Multi-Media Card Electrical
> Standard (5.1).
Ok, will drop it.
>
> > +#define SD_MMC_HC_CTRL_MMC_SDR25 0x0001
> > +#define SD_MMC_HC_CTRL_MMC_SDR12 0x
> > +#define SD_MMC_HC_CTRL_HS200 0x0003
> > +#define SD_MMC_HC_CTRL_HS400 0x0005
>
> How about the below renames & reorder?
>
> SD_MMC_HC_CTRL_MMC_LEGACY 0x
> SD_MMC_HC_CTRL_MMC_HS_SDR 0x0001
> SD_MMC_HC_CTRL_MMC_HS_DDR 0x0004
> SD_MMC_HC_CTRL_MMC_HS200 0x0003
> SD_MMC_HC_CTRL_MMC_HS400 0x0005
Ok.
>
> > +
> > +//
> > +// Timing modes for uhs
> > +//
> > +typedef enum {
> > + SdMmcUhsSdr12,
> > + SdMmcUhsSdr25,
> > + SdMmcUhsSdr50,
> > + SdMmcUhsSdr104,
> > + SdMmcUhsDdr50,
> > + SdMmcMmcDdr52,
> > + SdMmcMmcSdr50,
> > + SdMmcMmcSdr25,
> > + SdMmcMmcSdr12,
> > + SdMmcMmcHs200,
> > + SdMmcMmcHs400,
> > +} SD_MMC_UHS_TIMING;
>
> Suggest a similar drop of 'SdMmcMmcSdr50' and rename according to the above
> comments upon HOST_CTRL2 register value definitions. Also, how about a rename
> for enum to SD_MMC_BUS_MODE?
Ok.
>
> > +
> > +//
> > // The transfer modes supported by SD Host Controller
> > // Simplified Spec 3.0 Table 1-2
> > //
> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
> >IN UINT8 Slot
> >);
> >
> > +/**
> > + Set SD Host Controler control 2 registry according to selected speed.
> > +
> > + @param[in] PciIo The PCI IO protocol instance.
> > + @param[in] Slot The slot number of the SD card to send the
> > command to.
> > + @param[in] Timing The timing to select.
> > +
> > + @retval EFI_SUCCESS The timing is set successfully.
> > + @retval OthersThe timing isn't set successfully.
> > +**/
> > +EFI_STATUS
> > +SdMmcHcUhsSignaling (
> > + IN EFI_PCI_IO_PROTOCOL*PciIo,
> > + IN UINT8 Slot,
> > + IN SD_MMC_UHS_TIMING Timing
> > + );
> > +
> > #endif
> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > index 178945f..25db98a 100644
> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > @@ -17,6 +17,7 @@
> > #ifndef __SD_MMC_OVERRIDE_H__
> > #define __SD_MMC_OVERRIDE_H__
> >
> > +#include
>
> Please do not expose a module private header file here.
>
> One
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
Hi Marcin,
> -Original Message-
> From: Marcin Wojtas [mailto:m...@semihalf.com]
> Sent: Friday, October 05, 2018 9:25 PM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindh...@linaro.org; Wu,
> Hao A; ard.biesheu...@linaro.org; nad...@marvell.com;
> m...@semihalf.com; j...@semihalf.com; t...@semihalf.com
> Subject: [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling
> to SdMmcOverride protocol
>
> From: Tomasz Michalec
>
> 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
> ---
> 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_VER0xFE
>
> //
> +// SD Host Controler bits to HOST_CTRL2 register
> +//
> +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> +#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
I think SD_MMC_HC_CTRL_MMC_SDR50 is not needed here.
Since according to the SD Physical Layer Simplified Specification, max clock
frequency for SD bus mode SDR50 is 100MHz. And there is no eMMC bus mode whose
max clock frequency is at 100MHz in Embedded Multi-Media Card Electrical
Standard (5.1).
> +#define SD_MMC_HC_CTRL_MMC_SDR25 0x0001
> +#define SD_MMC_HC_CTRL_MMC_SDR12 0x
> +#define SD_MMC_HC_CTRL_HS200 0x0003
> +#define SD_MMC_HC_CTRL_HS400 0x0005
How about the below renames & reorder?
SD_MMC_HC_CTRL_MMC_LEGACY 0x
SD_MMC_HC_CTRL_MMC_HS_SDR 0x0001
SD_MMC_HC_CTRL_MMC_HS_DDR 0x0004
SD_MMC_HC_CTRL_MMC_HS200 0x0003
SD_MMC_HC_CTRL_MMC_HS400 0x0005
> +
> +//
> +// Timing modes for uhs
> +//
> +typedef enum {
> + SdMmcUhsSdr12,
> + SdMmcUhsSdr25,
> + SdMmcUhsSdr50,
> + SdMmcUhsSdr104,
> + SdMmcUhsDdr50,
> + SdMmcMmcDdr52,
> + SdMmcMmcSdr50,
> + SdMmcMmcSdr25,
> + SdMmcMmcSdr12,
> + SdMmcMmcHs200,
> + SdMmcMmcHs400,
> +} SD_MMC_UHS_TIMING;
Suggest a similar drop of 'SdMmcMmcSdr50' and rename according to the above
comments upon HOST_CTRL2 register value definitions. Also, how about a rename
for enum to SD_MMC_BUS_MODE?
> +
> +//
> // The transfer modes supported by SD Host Controller
> // Simplified Spec 3.0 Table 1-2
> //
> @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
>IN UINT8 Slot
>);
>
> +/**
> + Set SD Host Controler control 2 registry according to selected speed.
> +
> + @param[in] PciIo The PCI IO protocol instance.
> + @param[in] Slot The slot number of the SD card to send the
> command to.
> + @param[in] Timing The timing to select.
> +
> + @retval EFI_SUCCESS The timing is set successfully.
> + @retval OthersThe timing isn't set successfully.
> +**/
> +EFI_STATUS
> +SdMmcHcUhsSignaling (
> + IN EFI_PCI_IO_PROTOCOL*PciIo,
> + IN UINT8 Slot,
> + IN SD_MMC_UHS_TIMING Timing
> + );
> +
> #endif
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 178945f..25db98a 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -17,6 +17,7 @@
> #ifndef __SD_MMC_OVERRIDE_H__
> #define __SD_MMC_OVERRIDE_H__
>
> +#include
Please do not expose a module private header file here.
One approach comes to me is to keep the SD/MMC bus mode enumeration structure
in this protocol header file. SdMmcPciHcDxe driver and producers of the
Override protocol keep a version of the HOST_CTRL2 register value macros of
their own.
> #include
>
> #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> @@ -31,6 +32,7 @@ typedef enum {
>
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
pt., 12 paź 2018 o 18:24 Ard Biesheuvel napisał(a): > > On Fri 12 Oct 2018 at 18:04, Marcin Wojtas wrote: > > > > pt., 12 paź 2018 o 17:55 Ard Biesheuvel > > napisał(a): > > > > > > On 12 October 2018 at 07:06, Marcin Wojtas wrote: > > > > pt., 12 paź 2018 o 03:41 Wu, Hao A napisał(a): > > > >> > > > >> > -Original Message- > > > >> > From: Marcin Wojtas [mailto:m...@semihalf.com] > > > >> > Sent: Thursday, October 11, 2018 11:43 PM > > > >> > To: Wu, Hao A > > > >> > Cc: Ni, Ruiyu; Ard Biesheuvel; Tian, Feng; Tomasz Michalec; Dong, > > > >> > Eric; edk2- > > > >> > devel-01; Gao, Liming; nad...@marvell.com; Kinney, Michael D; Zeng, > > > >> > Star > > > >> > Subject: Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add > > > >> > UhsSignaling to SdMmcOverride protocol > > > >> > > > > >> > wt., 9 paź 2018 o 13:51 Marcin Wojtas napisał(a): > > > >> > > > > > >> > > wt., 9 paź 2018 o 13:45 Ard Biesheuvel > > > >> > napisał(a): > > > >> > > > > > > >> > > > On 9 October 2018 at 13:32, Marcin Wojtas > > > >> > > > wrote: > > > >> > > > > wt., 9 paź 2018 o 13:28 Wu, Hao A > > > >> > > > > napisał(a): > > > >> > > > >> > > > >> > > > >> > -Original Message- > > > >> > > > >> > From: edk2-devel [mailto:edk2-devel-boun...@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 > > > >> > > > >> > > > > >> > > > ... > > > >> > > > >> > > > > >> > > > >> > 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 > > > >> > > > >> > > > >> > > > >> EMM
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
On Fri 12 Oct 2018 at 18:04, Marcin Wojtas wrote: > > pt., 12 paź 2018 o 17:55 Ard Biesheuvel > napisał(a): > > > > On 12 October 2018 at 07:06, Marcin Wojtas wrote: > > > pt., 12 paź 2018 o 03:41 Wu, Hao A napisał(a): > > >> > > >> > -Original Message- > > >> > From: Marcin Wojtas [mailto:m...@semihalf.com] > > >> > Sent: Thursday, October 11, 2018 11:43 PM > > >> > To: Wu, Hao A > > >> > Cc: Ni, Ruiyu; Ard Biesheuvel; Tian, Feng; Tomasz Michalec; Dong, > > >> > Eric; edk2- > > >> > devel-01; Gao, Liming; nad...@marvell.com; Kinney, Michael D; Zeng, > > >> > Star > > >> > Subject: Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add > > >> > UhsSignaling to SdMmcOverride protocol > > >> > > > >> > wt., 9 paź 2018 o 13:51 Marcin Wojtas napisał(a): > > >> > > > > >> > > wt., 9 paź 2018 o 13:45 Ard Biesheuvel > > >> > napisał(a): > > >> > > > > > >> > > > On 9 October 2018 at 13:32, Marcin Wojtas > > >> > > > wrote: > > >> > > > > wt., 9 paź 2018 o 13:28 Wu, Hao A > > >> > > > > napisał(a): > > >> > > > >> > > >> > > > >> > -Original Message- > > >> > > > >> > From: edk2-devel [mailto:edk2-devel-boun...@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 > > >> > > > >> > > > >> > > > ... > > >> > > > >> > > > >> > > > >> > 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 > > >> > > > >> >
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
pt., 12 paź 2018 o 17:55 Ard Biesheuvel napisał(a): > > On 12 October 2018 at 07:06, Marcin Wojtas wrote: > > pt., 12 paź 2018 o 03:41 Wu, Hao A napisał(a): > >> > >> > -Original Message- > >> > From: Marcin Wojtas [mailto:m...@semihalf.com] > >> > Sent: Thursday, October 11, 2018 11:43 PM > >> > To: Wu, Hao A > >> > Cc: Ni, Ruiyu; Ard Biesheuvel; Tian, Feng; Tomasz Michalec; Dong, Eric; > >> > edk2- > >> > devel-01; Gao, Liming; nad...@marvell.com; Kinney, Michael D; Zeng, Star > >> > Subject: Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add > >> > UhsSignaling to SdMmcOverride protocol > >> > > >> > wt., 9 paź 2018 o 13:51 Marcin Wojtas napisał(a): > >> > > > >> > > wt., 9 paź 2018 o 13:45 Ard Biesheuvel > >> > napisał(a): > >> > > > > >> > > > On 9 October 2018 at 13:32, Marcin Wojtas wrote: > >> > > > > wt., 9 paź 2018 o 13:28 Wu, Hao A napisał(a): > >> > > > >> > >> > > > >> > -Original Message- > >> > > > >> > From: edk2-devel [mailto:edk2-devel-boun...@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 > >> > > > >> > > >> > > > ... > >> > > > >> > > >> > > > >> > 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. > >> > > > &g
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
On 12 October 2018 at 07:06, Marcin Wojtas wrote: > pt., 12 paź 2018 o 03:41 Wu, Hao A napisał(a): >> >> > -Original Message- >> > From: Marcin Wojtas [mailto:m...@semihalf.com] >> > Sent: Thursday, October 11, 2018 11:43 PM >> > To: Wu, Hao A >> > Cc: Ni, Ruiyu; Ard Biesheuvel; Tian, Feng; Tomasz Michalec; Dong, Eric; >> > edk2- >> > devel-01; Gao, Liming; nad...@marvell.com; Kinney, Michael D; Zeng, Star >> > Subject: Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add >> > UhsSignaling to SdMmcOverride protocol >> > >> > wt., 9 paź 2018 o 13:51 Marcin Wojtas napisał(a): >> > > >> > > wt., 9 paź 2018 o 13:45 Ard Biesheuvel >> > napisał(a): >> > > > >> > > > On 9 October 2018 at 13:32, Marcin Wojtas wrote: >> > > > > wt., 9 paź 2018 o 13:28 Wu, Hao A napisał(a): >> > > > >> >> > > > >> > -Original Message- >> > > > >> > From: edk2-devel [mailto:edk2-devel-boun...@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 >> > > > >> > >> > > > ... >> > > > >> > >> > > > >> > 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? >> >
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
pt., 12 paź 2018 o 03:41 Wu, Hao A napisał(a): > > > -Original Message- > > From: Marcin Wojtas [mailto:m...@semihalf.com] > > Sent: Thursday, October 11, 2018 11:43 PM > > To: Wu, Hao A > > Cc: Ni, Ruiyu; Ard Biesheuvel; Tian, Feng; Tomasz Michalec; Dong, Eric; > > edk2- > > devel-01; Gao, Liming; nad...@marvell.com; Kinney, Michael D; Zeng, Star > > Subject: Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add > > UhsSignaling to SdMmcOverride protocol > > > > wt., 9 paź 2018 o 13:51 Marcin Wojtas napisał(a): > > > > > > wt., 9 paź 2018 o 13:45 Ard Biesheuvel > > napisał(a): > > > > > > > > On 9 October 2018 at 13:32, Marcin Wojtas wrote: > > > > > wt., 9 paź 2018 o 13:28 Wu, Hao A napisał(a): > > > > >> > > > > >> > -Original Message- > > > > >> > From: edk2-devel [mailto:edk2-devel-boun...@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 > > > > >> > > > > > ... > > > > >> > > > > > >> > 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? > > > > > > > > > > > > > I would like to know where the current values in SdMmcPciHcDxe come > > > > from if they are not defined in any spec. > > > > > > > > How do we know which ones are the correct ones? > > > > > > Hao, can you justify used values? > > > > > > > Hi Hao, > > > > Can you please take a look at the UHS_MODE_SEL values source for eMMC? > > Hi Marcin, > > Sorry for the delayed response. > > For the current implementation of the SdMmcPciHcDxe driver, the selecting > of "UHS Mode Select" field value of the Host Control 2 Register is based > on a Max Clock Frequency & Data Rate (Single or Dual) matching > relationship between the: > > A. Table 3-6 of the SD Specifications Part 1 Physical Layer Simplified > Specification Version 4.10 > > and > > B. Table 4 of the EMMC Electrical Standard Spec 5.1 > > The matching details was included in my previous reply. The only missing > part is there seems no matching for the EMMC HS400 mode in the SD > specifications. For this case, we are currently using the same approach > with the Linux implementation, that is to set the "UHS Mode Select" to a > value of 0x5 (not standard). > Hao, Thanks a lot for the clarification. Ard, Knowing the numbers details, what is your view of the UhsSignaling handling? Best regards, Marcin ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
> -Original Message- > From: Marcin Wojtas [mailto:m...@semihalf.com] > Sent: Thursday, October 11, 2018 11:43 PM > To: Wu, Hao A > Cc: Ni, Ruiyu; Ard Biesheuvel; Tian, Feng; Tomasz Michalec; Dong, Eric; edk2- > devel-01; Gao, Liming; nad...@marvell.com; Kinney, Michael D; Zeng, Star > Subject: Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add > UhsSignaling to SdMmcOverride protocol > > wt., 9 paź 2018 o 13:51 Marcin Wojtas napisał(a): > > > > wt., 9 paź 2018 o 13:45 Ard Biesheuvel > napisał(a): > > > > > > On 9 October 2018 at 13:32, Marcin Wojtas wrote: > > > > wt., 9 paź 2018 o 13:28 Wu, Hao A napisał(a): > > > >> > > > >> > -Original Message- > > > >> > From: edk2-devel [mailto:edk2-devel-boun...@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 > > > >> > > > > ... > > > >> > > > > >> > 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? > > > > > > > > > > I would like to know where the current values in SdMmcPciHcDxe come > > > from if they are not defined in any spec. > > > > > > How do we know which ones are the correct ones? > > > > Hao, can you justify used values? > > > > Hi Hao, > > Can you please take a look at the UHS_MODE_SEL values source for eMMC? Hi Marcin, Sorry for the delayed response. For the current implementation of the SdMmcPciHcDxe driver, the selecting of "UHS Mode Select" field value of the Host Control 2 Register is based on a Max Clock Frequency & Data Rate (Single or Dual) matching relationship between the: A. Table 3-6 of the SD Specifications Part 1 Physical Layer Simplified Specification Version 4.10 and B. Table 4 of the EMMC Electrical Standard Spec 5.1 The matching details was included in my previous reply. The only missing part is there seems no matching for the EMMC HS400 mode in the SD specifications. For this case, we are currently using the same approach with the Linux implementation, that is to set the "UHS Mode Select" to a value of 0x5 (not standard). Best Regards, Hao Wu > > Thanks, > Marcin ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
wt., 9 paź 2018 o 13:51 Marcin Wojtas napisał(a): > > wt., 9 paź 2018 o 13:45 Ard Biesheuvel napisał(a): > > > > On 9 October 2018 at 13:32, Marcin Wojtas wrote: > > > wt., 9 paź 2018 o 13:28 Wu, Hao A napisał(a): > > >> > > >> > -Original Message- > > >> > From: edk2-devel [mailto:edk2-devel-boun...@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 > > >> > > > ... > > >> > > > >> > 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? > > > > > > > I would like to know where the current values in SdMmcPciHcDxe come > > from if they are not defined in any spec. > > > > How do we know which ones are the correct ones? > > Hao, can you justify used values? > Hi Hao, Can you please take a look at the UHS_MODE_SEL values source for eMMC? Thanks, Marcin ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
wt., 9 paź 2018 o 13:45 Ard Biesheuvel napisał(a): > > On 9 October 2018 at 13:32, Marcin Wojtas wrote: > > wt., 9 paź 2018 o 13:28 Wu, Hao A napisał(a): > >> > >> > -Original Message- > >> > From: edk2-devel [mailto:edk2-devel-boun...@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 > >> > > ... > >> > > >> > 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? > > > > I would like to know where the current values in SdMmcPciHcDxe come > from if they are not defined in any spec. > > How do we know which ones are the correct ones? Hao, can you justify used values? Thanks, Marcin ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
On 9 October 2018 at 13:32, Marcin Wojtas wrote: > wt., 9 paź 2018 o 13:28 Wu, Hao A napisał(a): >> >> > -Original Message- >> > From: edk2-devel [mailto:edk2-devel-boun...@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 >> > ... >> > >> > 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? > I would like to know where the current values in SdMmcPciHcDxe come from if they are not defined in any spec. How do we know which ones are the correct ones? ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
wt., 9 paź 2018 o 13:28 Wu, Hao A napisał(a): > > > -Original Message- > > From: edk2-devel [mailto:edk2-devel-boun...@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 wrote: > > > pon., 8 paź 2018 o 15:43 Ard Biesheuvel > > napisał(a): > > >> > > >> On 8 October 2018 at 15:37, Marcin Wojtas wrote: > > >> > pon., 8 paź 2018 o 15:27 Ard Biesheuvel > > napisał(a): > > >> >> > > >> >> On 8 October 2018 at 15:17, Marcin Wojtas wrote: > > >> >> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel > > napisał(a): > > >> >> >> > > >> >> >> On 8 October 2018 at 14:59, Marcin Wojtas > > wrote: > > >> >> >> > Hi Ard, > > >> >> >> > > > >> >> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel > > >> >> >> > > > napisał(a): > > >> >> >> >> > > >> >> >> >> (add MdeModulePkg maintainers) > > >> >> >> >> > > >> >> >> >> On 5 October 2018 at 15:25, Marcin Wojtas > > wrote: > > >> >> >> >> > From: Tomasz Michalec > > >> >> >> >> > > > >> >> >> >> > 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 > > >> >> >> >> > --- > > >> >> >> >> > 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_VER0xFE > > >> >> >> >> > > > >> >> >> >> > // > > >> >> >> >> > +// SD Host Controler bits to HOST_CTRL2 register > > >> >> >> >> > +// > > >> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007 > > >> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x > > >> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR25 0x0001 > > >> >> >>
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
> -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@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 wrote: > > pon., 8 paź 2018 o 15:43 Ard Biesheuvel > napisał(a): > >> > >> On 8 October 2018 at 15:37, Marcin Wojtas wrote: > >> > pon., 8 paź 2018 o 15:27 Ard Biesheuvel > napisał(a): > >> >> > >> >> On 8 October 2018 at 15:17, Marcin Wojtas wrote: > >> >> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel > napisał(a): > >> >> >> > >> >> >> On 8 October 2018 at 14:59, Marcin Wojtas > wrote: > >> >> >> > Hi Ard, > >> >> >> > > >> >> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel > napisał(a): > >> >> >> >> > >> >> >> >> (add MdeModulePkg maintainers) > >> >> >> >> > >> >> >> >> On 5 October 2018 at 15:25, Marcin Wojtas > wrote: > >> >> >> >> > From: Tomasz Michalec > >> >> >> >> > > >> >> >> >> > 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 > >> >> >> >> > --- > >> >> >> >> > 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_VER0xFE > >> >> >> >> > > >> >> >> >> > // > >> >> >> >> > +// SD Host Controler bits to HOST_CTRL2 register > >> >> >> >> > +// > >> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007 > >> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x > >> >> >> >> > +#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_
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
On 8 October 2018 at 16:52, Marcin Wojtas wrote:
> pon., 8 paź 2018 o 15:43 Ard Biesheuvel
> napisał(a):
>>
>> On 8 October 2018 at 15:37, Marcin Wojtas wrote:
>> > pon., 8 paź 2018 o 15:27 Ard Biesheuvel
>> > napisał(a):
>> >>
>> >> On 8 October 2018 at 15:17, Marcin Wojtas wrote:
>> >> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel
>> >> > napisał(a):
>> >> >>
>> >> >> On 8 October 2018 at 14:59, Marcin Wojtas wrote:
>> >> >> > Hi Ard,
>> >> >> >
>> >> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel
>> >> >> > napisał(a):
>> >> >> >>
>> >> >> >> (add MdeModulePkg maintainers)
>> >> >> >>
>> >> >> >> On 5 October 2018 at 15:25, Marcin Wojtas wrote:
>> >> >> >> > From: Tomasz Michalec
>> >> >> >> >
>> >> >> >> > 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
>> >> >> >> > ---
>> >> >> >> > 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_VER0xFE
>> >> >> >> >
>> >> >> >> > //
>> >> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
>> >> >> >> > +//
>> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
>> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
>> >> >> >> > +#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 0x
>> >> >> >> > +#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.
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
pon., 8 paź 2018 o 15:43 Ard Biesheuvel napisał(a):
>
> On 8 October 2018 at 15:37, Marcin Wojtas wrote:
> > pon., 8 paź 2018 o 15:27 Ard Biesheuvel
> > napisał(a):
> >>
> >> On 8 October 2018 at 15:17, Marcin Wojtas wrote:
> >> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel
> >> > napisał(a):
> >> >>
> >> >> On 8 October 2018 at 14:59, Marcin Wojtas wrote:
> >> >> > Hi Ard,
> >> >> >
> >> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel
> >> >> > napisał(a):
> >> >> >>
> >> >> >> (add MdeModulePkg maintainers)
> >> >> >>
> >> >> >> On 5 October 2018 at 15:25, Marcin Wojtas wrote:
> >> >> >> > From: Tomasz Michalec
> >> >> >> >
> >> >> >> > 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
> >> >> >> > ---
> >> >> >> > 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_VER0xFE
> >> >> >> >
> >> >> >> > //
> >> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
> >> >> >> > +//
> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> >> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> >> >> >> > +#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 0x
> >> >> >> > +#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
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
On 8 October 2018 at 15:37, Marcin Wojtas wrote:
> pon., 8 paź 2018 o 15:27 Ard Biesheuvel
> napisał(a):
>>
>> On 8 October 2018 at 15:17, Marcin Wojtas wrote:
>> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel
>> > napisał(a):
>> >>
>> >> On 8 October 2018 at 14:59, Marcin Wojtas wrote:
>> >> > Hi Ard,
>> >> >
>> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel
>> >> > napisał(a):
>> >> >>
>> >> >> (add MdeModulePkg maintainers)
>> >> >>
>> >> >> On 5 October 2018 at 15:25, Marcin Wojtas wrote:
>> >> >> > From: Tomasz Michalec
>> >> >> >
>> >> >> > 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
>> >> >> > ---
>> >> >> > 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_VER0xFE
>> >> >> >
>> >> >> > //
>> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
>> >> >> > +//
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
>> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
>> >> >> > +#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 0x
>> >> >> > +#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
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
pon., 8 paź 2018 o 15:27 Ard Biesheuvel napisał(a):
>
> On 8 October 2018 at 15:17, Marcin Wojtas wrote:
> > pon., 8 paź 2018 o 15:07 Ard Biesheuvel
> > napisał(a):
> >>
> >> On 8 October 2018 at 14:59, Marcin Wojtas wrote:
> >> > Hi Ard,
> >> >
> >> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel
> >> > napisał(a):
> >> >>
> >> >> (add MdeModulePkg maintainers)
> >> >>
> >> >> On 5 October 2018 at 15:25, Marcin Wojtas wrote:
> >> >> > From: Tomasz Michalec
> >> >> >
> >> >> > 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
> >> >> > ---
> >> >> > 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_VER0xFE
> >> >> >
> >> >> > //
> >> >> > +// SD Host Controler bits to HOST_CTRL2 register
> >> >> > +//
> >> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> >> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> >> >> > +#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 0x
> >> >> > +#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?
> >> >> > +//
> >> >> > +// 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?
>
> Also, how important is it to drive the SD/MMC at
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
On 8 October 2018 at 15:17, Marcin Wojtas wrote:
> pon., 8 paź 2018 o 15:07 Ard Biesheuvel
> napisał(a):
>>
>> On 8 October 2018 at 14:59, Marcin Wojtas wrote:
>> > Hi Ard,
>> >
>> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel
>> > napisał(a):
>> >>
>> >> (add MdeModulePkg maintainers)
>> >>
>> >> On 5 October 2018 at 15:25, Marcin Wojtas wrote:
>> >> > From: Tomasz Michalec
>> >> >
>> >> > 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
>> >> > ---
>> >> > 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_VER0xFE
>> >> >
>> >> > //
>> >> > +// SD Host Controler bits to HOST_CTRL2 register
>> >> > +//
>> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
>> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
>> >> > +#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 0x
>> >> > +#define SD_MMC_HC_CTRL_HS200 0x0003
>> >> > +#define SD_MMC_HC_CTRL_HS400 0x0005
>> >> > +
>> >> > +//
>> >> > +// 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.
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
>>
>>
>> >>
>> >>
>> >> > +//
>> >> > // The transfer modes supported by SD Host Controller
>> >> > // Simplified Spec 3.0 Table 1-2
>> >> > //
>> >> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
>> >> >IN UINT8 Slot
>> >> >);
>> >> >
>> >> > +/**
>> >> > + Set SD Host Controler control 2 registry according to selected speed.
>> >> > +
>> >> > + @param[in] PciIo The PCI IO protocol instance.
>> >> > + @param[in] Slot The slot number of the SD
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
pon., 8 paź 2018 o 15:07 Ard Biesheuvel napisał(a):
>
> On 8 October 2018 at 14:59, Marcin Wojtas wrote:
> > Hi Ard,
> >
> > pon., 8 paź 2018 o 14:41 Ard Biesheuvel
> > napisał(a):
> >>
> >> (add MdeModulePkg maintainers)
> >>
> >> On 5 October 2018 at 15:25, Marcin Wojtas wrote:
> >> > From: Tomasz Michalec
> >> >
> >> > 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
> >> > ---
> >> > 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_VER0xFE
> >> >
> >> > //
> >> > +// SD Host Controler bits to HOST_CTRL2 register
> >> > +//
> >> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> >> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> >> > +#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 0x
> >> > +#define SD_MMC_HC_CTRL_HS200 0x0003
> >> > +#define SD_MMC_HC_CTRL_HS400 0x0005
> >> > +
> >> > +//
> >> > +// 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)?
Thanks,
Marcin
>
>
> >>
> >>
> >> > +//
> >> > // The transfer modes supported by SD Host Controller
> >> > // Simplified Spec 3.0 Table 1-2
> >> > //
> >> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
> >> >IN UINT8 Slot
> >> >);
> >> >
> >> > +/**
> >> > + Set SD Host Controler control 2 registry according to selected speed.
> >> > +
> >> > + @param[in] PciIo The PCI IO protocol instance.
> >> > + @param[in] Slot The slot number of the SD card to send the
> >> > command to.
> >> > + @param[in] Timing The timing to select.
> >> > +
> >> > + @retval EFI_SUCCESS The timing is set successfully.
> >> > + @retval OthersThe timing isn't set successfully.
> >> > +**/
> >> > +EFI_STATUS
> >> > +SdMmcHcUhsSignaling (
> >> > + IN EFI_PCI_IO_PROTOCOL*PciIo,
> >> > + IN UINT8 Slot,
> >> > + IN SD_MMC_UHS_TIMING Timing
> >> > + );
> >> > +
> >> > #endif
> >> > diff --git
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
On 8 October 2018 at 14:59, Marcin Wojtas wrote:
> Hi Ard,
>
> pon., 8 paź 2018 o 14:41 Ard Biesheuvel
> napisał(a):
>>
>> (add MdeModulePkg maintainers)
>>
>> On 5 October 2018 at 15:25, Marcin Wojtas wrote:
>> > From: Tomasz Michalec
>> >
>> > 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
>> > ---
>> > 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_VER0xFE
>> >
>> > //
>> > +// SD Host Controler bits to HOST_CTRL2 register
>> > +//
>> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
>> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
>> > +#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 0x
>> > +#define SD_MMC_HC_CTRL_HS200 0x0003
>> > +#define SD_MMC_HC_CTRL_HS400 0x0005
>> > +
>> > +//
>> > +// 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
I think it should be fine to add the enum definition to
Protocol/SdMmcOverride.h instead.
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?
>>
>>
>> > +//
>> > // The transfer modes supported by SD Host Controller
>> > // Simplified Spec 3.0 Table 1-2
>> > //
>> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
>> >IN UINT8 Slot
>> >);
>> >
>> > +/**
>> > + Set SD Host Controler control 2 registry according to selected speed.
>> > +
>> > + @param[in] PciIo The PCI IO protocol instance.
>> > + @param[in] Slot The slot number of the SD card to send the
>> > command to.
>> > + @param[in] Timing The timing to select.
>> > +
>> > + @retval EFI_SUCCESS The timing is set successfully.
>> > + @retval OthersThe timing isn't set successfully.
>> > +**/
>> > +EFI_STATUS
>> > +SdMmcHcUhsSignaling (
>> > + IN EFI_PCI_IO_PROTOCOL*PciIo,
>> > + IN UINT8 Slot,
>> > + IN SD_MMC_UHS_TIMING Timing
>> > + );
>> > +
>> > #endif
>> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> > index 178945f..25db98a 100644
>> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> > @@ -17,6 +17,7 @@
>> > #ifndef __SD_MMC_OVERRIDE_H__
>> > #define __SD_MMC_OVERRIDE_H__
>> >
>> > +#include
>> > #include
>> >
>> > #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>> > @@ -31,6 +32,7 @@ typedef enum {
>> >
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
Hi Ard,
pon., 8 paź 2018 o 14:41 Ard Biesheuvel napisał(a):
>
> (add MdeModulePkg maintainers)
>
> On 5 October 2018 at 15:25, Marcin Wojtas wrote:
> > From: Tomasz Michalec
> >
> > 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
> > ---
> > 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_VER0xFE
> >
> > //
> > +// SD Host Controler bits to HOST_CTRL2 register
> > +//
> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> > +#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 0x
> > +#define SD_MMC_HC_CTRL_HS200 0x0003
> > +#define SD_MMC_HC_CTRL_HS400 0x0005
> > +
> > +//
> > +// 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.
>
>
> > +//
> > // The transfer modes supported by SD Host Controller
> > // Simplified Spec 3.0 Table 1-2
> > //
> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
> >IN UINT8 Slot
> >);
> >
> > +/**
> > + Set SD Host Controler control 2 registry according to selected speed.
> > +
> > + @param[in] PciIo The PCI IO protocol instance.
> > + @param[in] Slot The slot number of the SD card to send the
> > command to.
> > + @param[in] Timing The timing to select.
> > +
> > + @retval EFI_SUCCESS The timing is set successfully.
> > + @retval OthersThe timing isn't set successfully.
> > +**/
> > +EFI_STATUS
> > +SdMmcHcUhsSignaling (
> > + IN EFI_PCI_IO_PROTOCOL*PciIo,
> > + IN UINT8 Slot,
> > + IN SD_MMC_UHS_TIMING Timing
> > + );
> > +
> > #endif
> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > index 178945f..25db98a 100644
> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > @@ -17,6 +17,7 @@
> > #ifndef __SD_MMC_OVERRIDE_H__
> > #define __SD_MMC_OVERRIDE_H__
> >
> > +#include
> > #include
> >
> > #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> > @@ -31,6 +32,7 @@ typedef enum {
> >EdkiiSdMmcResetPost,
> >EdkiiSdMmcInitHostPre,
> >EdkiiSdMmcInitHostPost,
> > + EdkiiSdMmcUhsSignaling,
> > } EDKII_SD_MMC_PHASE_TYPE;
> >
> > /**
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index c5fd214..05bd4a0 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
> >IN UINT8 BusWidth
> >)
> > {
> > -
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
(add MdeModulePkg maintainers)
On 5 October 2018 at 15:25, Marcin Wojtas wrote:
> From: Tomasz Michalec
>
> 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
> ---
> 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_VER0xFE
>
> //
> +// SD Host Controler bits to HOST_CTRL2 register
> +//
> +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> +#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 0x
> +#define SD_MMC_HC_CTRL_HS200 0x0003
> +#define SD_MMC_HC_CTRL_HS400 0x0005
> +
> +//
> +// 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?
> +//
> // The transfer modes supported by SD Host Controller
> // Simplified Spec 3.0 Table 1-2
> //
> @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
>IN UINT8 Slot
>);
>
> +/**
> + Set SD Host Controler control 2 registry according to selected speed.
> +
> + @param[in] PciIo The PCI IO protocol instance.
> + @param[in] Slot The slot number of the SD card to send the
> command to.
> + @param[in] Timing The timing to select.
> +
> + @retval EFI_SUCCESS The timing is set successfully.
> + @retval OthersThe timing isn't set successfully.
> +**/
> +EFI_STATUS
> +SdMmcHcUhsSignaling (
> + IN EFI_PCI_IO_PROTOCOL*PciIo,
> + IN UINT8 Slot,
> + IN SD_MMC_UHS_TIMING Timing
> + );
> +
> #endif
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 178945f..25db98a 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -17,6 +17,7 @@
> #ifndef __SD_MMC_OVERRIDE_H__
> #define __SD_MMC_OVERRIDE_H__
>
> +#include
> #include
>
> #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> @@ -31,6 +32,7 @@ typedef enum {
>EdkiiSdMmcResetPost,
>EdkiiSdMmcInitHostPre,
>EdkiiSdMmcInitHostPost,
> + EdkiiSdMmcUhsSignaling,
> } EDKII_SD_MMC_PHASE_TYPE;
>
> /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index c5fd214..05bd4a0 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
>IN UINT8 BusWidth
>)
> {
> - EFI_STATUS Status;
> - UINT8 HsTiming;
> - UINT8 HostCtrl1;
> - UINT8 HostCtrl2;
> + EFI_STATUS Status;
> + UINT8 HsTiming;
> + UINT8 HostCtrl1;
> + SD_MMC_UHS_TIMING Timing;
> + SD_MMC_HC_PRIVATE_DATA *Private;
> +
> + Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
>if (EFI_ERROR (Status)) {
> @@ -758,27 +761,37 @@ EmmcSwitchToHighSpeed (
> return Status;
>}
>
> - //
> - // Clean UHS Mode Select field of Host Control 2 reigster before update
> - //
> - HostCtrl2 = (UINT8)~0x7;
> - Status = SdMmcHcAndMmio
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
pt., 5 paź 2018 o 17:12 Philippe Mathieu-Daudé napisał(a):
>
> Hi Marcin, Tomasz.
>
> On 05/10/2018 15:25, Marcin Wojtas wrote:
> > From: Tomasz Michalec
> >
> > Some SD Host Controlers use different values in Host Control 2 Register
>
> My two cents, in various places "Controler" is miswritten, this should
> be "Controller".
Thanks, missed that. It will be corrected in the next revision.
Best regards,
Marcin
>
> > 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
> > ---
> > 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_VER0xFE
> >
> > //
> > +// SD Host Controler bits to HOST_CTRL2 register
> > +//
> > +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> > +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> > +#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 0x
> > +#define SD_MMC_HC_CTRL_HS200 0x0003
> > +#define SD_MMC_HC_CTRL_HS400 0x0005
> > +
> > +//
> > +// Timing modes for uhs
> > +//
> > +typedef enum {
> > + SdMmcUhsSdr12,
> > + SdMmcUhsSdr25,
> > + SdMmcUhsSdr50,
> > + SdMmcUhsSdr104,
> > + SdMmcUhsDdr50,
> > + SdMmcMmcDdr52,
> > + SdMmcMmcSdr50,
> > + SdMmcMmcSdr25,
> > + SdMmcMmcSdr12,
> > + SdMmcMmcHs200,
> > + SdMmcMmcHs400,
> > +} SD_MMC_UHS_TIMING;
> > +
> > +//
> > // The transfer modes supported by SD Host Controller
> > // Simplified Spec 3.0 Table 1-2
> > //
> > @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
> >IN UINT8 Slot
> >);
> >
> > +/**
> > + Set SD Host Controler control 2 registry according to selected speed.
> > +
> > + @param[in] PciIo The PCI IO protocol instance.
> > + @param[in] Slot The slot number of the SD card to send the
> > command to.
> > + @param[in] Timing The timing to select.
> > +
> > + @retval EFI_SUCCESS The timing is set successfully.
> > + @retval OthersThe timing isn't set successfully.
> > +**/
> > +EFI_STATUS
> > +SdMmcHcUhsSignaling (
> > + IN EFI_PCI_IO_PROTOCOL*PciIo,
> > + IN UINT8 Slot,
> > + IN SD_MMC_UHS_TIMING Timing
> > + );
> > +
> > #endif
> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > index 178945f..25db98a 100644
> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > @@ -17,6 +17,7 @@
> > #ifndef __SD_MMC_OVERRIDE_H__
> > #define __SD_MMC_OVERRIDE_H__
> >
> > +#include
> > #include
> >
> > #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> > @@ -31,6 +32,7 @@ typedef enum {
> >EdkiiSdMmcResetPost,
> >EdkiiSdMmcInitHostPre,
> >EdkiiSdMmcInitHostPost,
> > + EdkiiSdMmcUhsSignaling,
> > } EDKII_SD_MMC_PHASE_TYPE;
> >
> > /**
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index c5fd214..05bd4a0 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
> >IN UINT8 BusWidth
> >)
> > {
> > - EFI_STATUS Status;
> > - UINT8 HsTiming;
> > - UINT8 HostCtrl1;
> > - UINT8 HostCtrl2;
> > + EFI_STATUS Status;
> > + UINT8 HsTiming;
> > + UINT8 HostCtrl1;
> > + SD_MMC_UHS_TIMING Timing;
> > + SD_MMC_HC_PRIVATE_DATA *Private;
> > +
> > + Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> >
> >
Re: [edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
Hi Marcin, Tomasz.
On 05/10/2018 15:25, Marcin Wojtas wrote:
> From: Tomasz Michalec
>
> Some SD Host Controlers use different values in Host Control 2 Register
My two cents, in various places "Controler" is miswritten, this should
be "Controller".
> 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
> ---
> 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_VER0xFE
>
> //
> +// SD Host Controler bits to HOST_CTRL2 register
> +//
> +#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
> +#define SD_MMC_HC_CTRL_UHS_SDR12 0x
> +#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 0x
> +#define SD_MMC_HC_CTRL_HS200 0x0003
> +#define SD_MMC_HC_CTRL_HS400 0x0005
> +
> +//
> +// Timing modes for uhs
> +//
> +typedef enum {
> + SdMmcUhsSdr12,
> + SdMmcUhsSdr25,
> + SdMmcUhsSdr50,
> + SdMmcUhsSdr104,
> + SdMmcUhsDdr50,
> + SdMmcMmcDdr52,
> + SdMmcMmcSdr50,
> + SdMmcMmcSdr25,
> + SdMmcMmcSdr12,
> + SdMmcMmcHs200,
> + SdMmcMmcHs400,
> +} SD_MMC_UHS_TIMING;
> +
> +//
> // The transfer modes supported by SD Host Controller
> // Simplified Spec 3.0 Table 1-2
> //
> @@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
>IN UINT8 Slot
>);
>
> +/**
> + Set SD Host Controler control 2 registry according to selected speed.
> +
> + @param[in] PciIo The PCI IO protocol instance.
> + @param[in] Slot The slot number of the SD card to send the
> command to.
> + @param[in] Timing The timing to select.
> +
> + @retval EFI_SUCCESS The timing is set successfully.
> + @retval OthersThe timing isn't set successfully.
> +**/
> +EFI_STATUS
> +SdMmcHcUhsSignaling (
> + IN EFI_PCI_IO_PROTOCOL*PciIo,
> + IN UINT8 Slot,
> + IN SD_MMC_UHS_TIMING Timing
> + );
> +
> #endif
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 178945f..25db98a 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -17,6 +17,7 @@
> #ifndef __SD_MMC_OVERRIDE_H__
> #define __SD_MMC_OVERRIDE_H__
>
> +#include
> #include
>
> #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> @@ -31,6 +32,7 @@ typedef enum {
>EdkiiSdMmcResetPost,
>EdkiiSdMmcInitHostPre,
>EdkiiSdMmcInitHostPost,
> + EdkiiSdMmcUhsSignaling,
> } EDKII_SD_MMC_PHASE_TYPE;
>
> /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index c5fd214..05bd4a0 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
>IN UINT8 BusWidth
>)
> {
> - EFI_STATUS Status;
> - UINT8 HsTiming;
> - UINT8 HostCtrl1;
> - UINT8 HostCtrl2;
> + EFI_STATUS Status;
> + UINT8 HsTiming;
> + UINT8 HostCtrl1;
> + SD_MMC_UHS_TIMING Timing;
> + SD_MMC_HC_PRIVATE_DATA *Private;
> +
> + Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
>if (EFI_ERROR (Status)) {
> @@ -758,27 +761,37 @@ EmmcSwitchToHighSpeed (
> return Status;
>}
>
> - //
> - // Clean UHS Mode Select field of Host Control 2 reigster before update
> - //
> - HostCtrl2 = (UINT8)~0x7;
> - Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof
> (HostCtrl2), );
> - if
[edk2] [PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
From: Tomasz Michalec
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
---
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_VER0xFE
//
+// SD Host Controler bits to HOST_CTRL2 register
+//
+#define SD_MMC_HC_CTRL_UHS_MASK 0x0007
+#define SD_MMC_HC_CTRL_UHS_SDR12 0x
+#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 0x
+#define SD_MMC_HC_CTRL_HS200 0x0003
+#define SD_MMC_HC_CTRL_HS400 0x0005
+
+//
+// Timing modes for uhs
+//
+typedef enum {
+ SdMmcUhsSdr12,
+ SdMmcUhsSdr25,
+ SdMmcUhsSdr50,
+ SdMmcUhsSdr104,
+ SdMmcUhsDdr50,
+ SdMmcMmcDdr52,
+ SdMmcMmcSdr50,
+ SdMmcMmcSdr25,
+ SdMmcMmcSdr12,
+ SdMmcMmcHs200,
+ SdMmcMmcHs400,
+} SD_MMC_UHS_TIMING;
+
+//
// The transfer modes supported by SD Host Controller
// Simplified Spec 3.0 Table 1-2
//
@@ -508,4 +541,21 @@ SdMmcHcInitTimeoutCtrl (
IN UINT8 Slot
);
+/**
+ Set SD Host Controler control 2 registry according to selected speed.
+
+ @param[in] PciIo The PCI IO protocol instance.
+ @param[in] Slot The slot number of the SD card to send the command
to.
+ @param[in] Timing The timing to select.
+
+ @retval EFI_SUCCESS The timing is set successfully.
+ @retval OthersThe timing isn't set successfully.
+**/
+EFI_STATUS
+SdMmcHcUhsSignaling (
+ IN EFI_PCI_IO_PROTOCOL*PciIo,
+ IN UINT8 Slot,
+ IN SD_MMC_UHS_TIMING Timing
+ );
+
#endif
diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index 178945f..25db98a 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -17,6 +17,7 @@
#ifndef __SD_MMC_OVERRIDE_H__
#define __SD_MMC_OVERRIDE_H__
+#include
#include
#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
@@ -31,6 +32,7 @@ typedef enum {
EdkiiSdMmcResetPost,
EdkiiSdMmcInitHostPre,
EdkiiSdMmcInitHostPost,
+ EdkiiSdMmcUhsSignaling,
} EDKII_SD_MMC_PHASE_TYPE;
/**
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index c5fd214..05bd4a0 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
IN UINT8 BusWidth
)
{
- EFI_STATUS Status;
- UINT8 HsTiming;
- UINT8 HostCtrl1;
- UINT8 HostCtrl2;
+ EFI_STATUS Status;
+ UINT8 HsTiming;
+ UINT8 HostCtrl1;
+ SD_MMC_UHS_TIMING Timing;
+ SD_MMC_HC_PRIVATE_DATA *Private;
+
+ Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
if (EFI_ERROR (Status)) {
@@ -758,27 +761,37 @@ EmmcSwitchToHighSpeed (
return Status;
}
- //
- // Clean UHS Mode Select field of Host Control 2 reigster before update
- //
- HostCtrl2 = (UINT8)~0x7;
- Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof
(HostCtrl2), );
- if (EFI_ERROR (Status)) {
-return Status;
- }
- //
- // Set UHS Mode Select field of Host Control 2 reigster to SDR12/25/50
- //
if (IsDdr) {
-HostCtrl2 = BIT2;
+Timing = SdMmcMmcDdr52;
} else if (ClockFreq == 52) {
-HostCtrl2 = BIT0;
+Timing = SdMmcMmcSdr50;
+ } else if (ClockFreq == 26) {
+Timing = SdMmcMmcSdr25;
} else {
-HostCtrl2 = 0;
+Timing = SdMmcMmcSdr12;
}
- Status =
