Re: [PATCH edk2-platforms v4 34/34] Platform/SynQuacerEvalBoard: add eMMC driver stack

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Nov 17, 2017 at 05:25:45PM +0000, Ard Biesheuvel wrote:
> On 17 November 2017 at 17:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, Nov 10, 2017 at 02:21:27PM +0000, Ard Biesheuvel wrote:
> >> Add the drivers required to use the onboard eMMC on the SynQuacer
> >> Evaluation Board.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> NOTE: this depends on an upstream EDK2 change that is currently still under
> >>       discussion
> >>
> >>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc                |   1 +
> >>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc    |   8 +
> >>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf    |   7 +
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c          | 204 ++++++++++++++++++++
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c   |  12 +-
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h   |  37 ++++
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf |   5 +
> >>  7 files changed, 266 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> >> index cd4eb79b35bf..1a09f727b31f 100644
> >> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> >> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> >> @@ -177,6 +177,7 @@ [LibraryClasses.common.DXE_DRIVER]
> >>    #
> >>    PciSegmentLib|Silicon/Socionext/SynQuacer/Library/SynQuacerPciSegmentLib/SynQuacerPciSegmentLib.inf
> >>    PciHostBridgeLib|Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLib.inf
> >> +  NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
> >>
> >>  [LibraryClasses.common.UEFI_APPLICATION]
> >>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
> >> diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> >> index 4034bcfe82c5..20b719794d06 100644
> >> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> >> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> >> @@ -176,6 +176,7 @@ [LibraryClasses.common.DXE_DRIVER]
> >>    #
> >>    PciSegmentLib|Silicon/Socionext/SynQuacer/Library/SynQuacerPciSegmentLib/SynQuacerPciSegmentLib.inf
> >>    PciHostBridgeLib|Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLib.inf
> >> +  NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
> >>
> >>  [LibraryClasses.common.UEFI_APPLICATION]
> >>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
> >> @@ -529,6 +530,13 @@ [Components.common]
> >>    MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
> >>
> >>    #
> >> +  # eMMC support
> >> +  #
> >> +  MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
> >> +  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> >> +  MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf
> >> +
> >> +  #
> >>    # AHCI Support
> >>    #
> >>    MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
> >> diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
> >> index 4577bd316a1f..1599f8953008 100644
> >> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
> >> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
> >> @@ -149,6 +149,13 @@ [FV.FvMain]
> >>    INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
> >>
> >>    #
> >> +  # eMMC support
> >> +  #
> >> +  INF MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
> >> +  INF MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
> >> +  INF MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf
> >> +
> >> +  #
> >>    # AHCI Support
> >>    #
> >>    INF MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
> >> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> >> new file mode 100644
> >> index 000000000000..4c7713d78fa3
> >> --- /dev/null
> >> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> >> @@ -0,0 +1,204 @@
> >> + /** @file
> >> +  SynQuacer DXE platform driver - eMMC support
> >> +
> >> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
> >> +
> >> +  This program and the accompanying materials are licensed and made available
> >> +  under the terms and conditions of the BSD License which accompanies this
> >> +  distribution.  The full text of the license may be found at
> >> +  http://opensource.org/licenses/bsd-license.php
> >> +
> >> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> +**/
> >> +
> >> +#include "PlatformDxe.h"
> >> +
> >> +// F_SDH30 extended Controller registers
> >> +#define F_SDH30_AHB_CONFIG        0x100
> >> +#define  F_SDH30_AHB_BIGED        BIT6
> >> +#define  F_SDH30_BUSLOCK_DMA      BIT5
> >> +#define  F_SDH30_BUSLOCK_EN       BIT4
> >> +#define  F_SDH30_SIN              BIT3
> >> +#define  F_SDH30_AHB_INCR_16      BIT2
> >> +#define  F_SDH30_AHB_INCR_8       BIT1
> >> +#define  F_SDH30_AHB_INCR_4       BIT0
> >> +
> >> +#define F_SDH30_TUNING_SETTING    0x108
> >> +#define  F_SDH30_CMD_CHK_DIS      BIT16
> >> +
> >> +#define F_SDH30_IO_CONTROL2       0x114
> >> +#define  F_SDH30_MSEL_O_1_8       BIT18
> >> +#define  F_SDH30_CRES_O_DN        BIT19
> >> +
> >> +#define F_SDH30_ESD_CONTROL       0x124
> >> +#define  F_SDH30_EMMC_RST         BIT1
> >> +#define  F_SDH30_EMMC_HS200       BIT24
> >> +#define  F_SDH30_CMD_DAT_DELAY    BIT9
> >> +
> >> +#define F_SDH30_TUNING_SETTING    0x108
> >> +#define  F_SDH30_CMD_CHK_DIS      BIT16
> >> +
> >> +#define F_SDH30_IO_CONTROL2       0x114
> >> +#define  F_SDH30_MSEL_O_1_8       BIT18
> >> +#define  F_SDH30_CRES_O_DN        BIT19
> >> +
> >> +#define F_SDH30_ESD_CONTROL       0x124
> >> +#define  F_SDH30_EMMC_RST         BIT1
> >> +#define  F_SDH30_EMMC_HS200       BIT24
> >> +#define  F_SDH30_CMD_DAT_DELAY    BIT9
> >> +
> >> +#define SD_HC_CLOCK_CTRL          0x2C
> >> +#define SYNQUACER_CLOCK_CTRL_VAL  0xBC01
> >> +
> >> +#define SD_HC_CAP_SDR104          BIT33
> >> +
> >> +#define ESD_CONTROL_RESET_DELAY   (20 * 1000)
> >> +#define IO_CONTROL2_SETTLE_US     3000
> >> +
> >> +STATIC EFI_HANDLE mSdMmcControllerHandle;
> >> +
> >> +/**
> >> +
> >> +  Override function for SDHCI capability bits
> >> +
> >> +  @param[in]      ControllerHandle          The EFI_HANDLE of the controller.
> >> +  @param[in]      Slot                      The 0 based slot index.
> >> +  @param[in,out]  SdMmcHcSlotCapability     The SDHCI capability structure.
> >> +
> >> +  @retval EFI_SUCCESS           The override function completed successfully.
> >> +  @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
> >> +  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
> >> +
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +EFIAPI
> >> +SynQuacerSdMmcOverrideCapability (
> >> +  IN      EFI_HANDLE              ControllerHandle,
> >> +  IN      UINT8                   Slot,
> >> +  IN  OUT VOID                    *SdMmcHcSlotCapability
> >> +  )
> >> +{
> >> +  UINT64    *Caps;
> >> +
> >> +  ASSERT (ControllerHandle == mSdMmcControllerHandle);
> >> +  ASSERT (Slot == 0);
> >> +
> >> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
> >> +    return EFI_NOT_FOUND;
> >> +  }
> >> +
> >> +  //
> >> +  // Clear the SDR104 capability bit. This avoids the need for a HS200 tuning
> >> +  // quirk that is difficult to support using the generic driver.
> >> +  //
> >> +  Caps = SdMmcHcSlotCapability;
> >> +  *Caps &= ~(UINT64)SD_HC_CAP_SDR104;
> >
> > MmioAnd64 and drop the temporary Caps variable?
> > (I only suggest it because it initially confused me to think
> > something more clever was going on.)
> >
> 
> This is memory not MMIO. (The SD/MMC driver reads the capability
> register and passes the value by reference to this function)

Hmm, OK, but in that case - why is it a VOID * rather than a UINT64 *?
Is the size of the register (and its location in some struct) not
standardised across implementations?

/
    Leif