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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 17 November 2017 at 17:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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?
>

I guess a UINT64* would be more appropriate. Care to add a comment to
that effect to the EDK2 patches that propose the protocol? :-)