public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support
@ 2018-01-30 10:32 Ard Biesheuvel
  2018-01-30 11:00 ` Leif Lindholm
  0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2018-01-30 10:32 UTC (permalink / raw)
  To: edk2-devel; +Cc: leif.lindholm, Ard Biesheuvel

Implement support for the SynQuacer eMMC controller. This involves an
implementation of the SD/MMC override protocol to handle a couple of
quirks that would otherwise prevent this IP from being driven by the
generic SDHCI driver.

Also, add a HII page to the PlatformDxe driver that allows eMMC support
to be enabled, and wire it up for both DeveloperBox and EVB.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Now that the core support for the SD/MMC override protocol is finally
merged, resubmit this again. I dropped Leif's R-b given that I have
now added DeveloperBox, as well as a HII option to enable eMMC.

 Platform/Socionext/DeveloperBox/DeveloperBox.dsc                                    |   8 +
 Platform/Socionext/DeveloperBox/DeveloperBox.fdf                                    |   7 +
 Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc                        |   8 +
 Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf                        |   7 +
 Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                               |   1 -
 Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts                       |   4 -
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c                              | 203 ++++++++++++++++++++
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c                       |   5 +
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h                       |   9 +
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf                     |   4 +
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni                  |   6 +
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr                  |   8 +
 Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h                             |   6 +-
 Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c   |  23 ++-
 Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf |   1 +
 15 files changed, 287 insertions(+), 13 deletions(-)

diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
index 2d46b4515749..7e69eaba9b70 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
@@ -179,6 +179,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
@@ -549,6 +550,13 @@ [Components.common]
   MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/DeveloperBox/DeveloperBox.fdf b/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
index 8443986fc3e7..b668f42c7962 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
@@ -148,6 +148,13 @@ [FV.FvMain]
   INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
index 6241b5bbd0b2..e35c17f0bcb7 100644
--- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
+++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
@@ -179,6 +179,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
@@ -549,6 +550,13 @@ [Components.common]
   MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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 cc61cf42ccd4..ba2f32328c2b 100644
--- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
+++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
@@ -150,6 +150,13 @@ [FV.FvMain]
   INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
index fae3f033f98f..f1daac74973f 100644
--- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
+++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
@@ -567,6 +567,5 @@
         clocks = <&clk_alw_c_0 &clk_alw_b_0>;
         clock-names = "core", "iface";
         dma-coherent;
-        status = "disabled";
     };
 };
diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
index 97fddfedcb46..0c6826f52c35 100644
--- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
+++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
@@ -31,10 +31,6 @@
                       "PEC-PD28",   "PEC-PD29",   "PEC-PD30",     "PEC-PD31";
 };
 
-&sdhci {
-    status = "okay";
-};
-
 &mdio_netsec {
     phy_netsec: ethernet-phy@1 {
         compatible = "ethernet-phy-ieee802.3-c22";
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
new file mode 100644
index 000000000000..29a3ebb369fb
--- /dev/null
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
@@ -0,0 +1,203 @@
+ /** @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]      PassThru              A pointer to the
+                                        EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @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
+SynQuacerSdMmcCapability (
+  IN      EFI_HANDLE                      ControllerHandle,
+  IN      UINT8                           Slot,
+  IN  OUT VOID                            *SdMmcHcSlotCapability
+  )
+{
+  UINT64 Capability;
+
+  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
+    return EFI_SUCCESS;
+  }
+
+  //
+  // Clear the SDR104 capability bit. This avoids the need for a HS200 tuning
+  // quirk that is difficult to support using the generic driver.
+  //
+  Capability = ReadUnaligned64 (SdMmcHcSlotCapability);
+  Capability &= ~(UINT64)SD_HC_CAP_SDR104;
+  WriteUnaligned64 (SdMmcHcSlotCapability, Capability);
+
+  return EFI_SUCCESS;
+}
+
+/**
+
+  Override function for SDHCI controller operations
+
+  @param[in]      ControllerHandle      The EFI_HANDLE of the controller.
+  @param[in]      Slot                  The 0 based slot index.
+  @param[in]      PhaseType             The type of operation and whether the
+                                        hook is invoked right before (pre) or
+                                        right after (post)
+
+  @retval EFI_SUCCESS           The override function completed successfully.
+  @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
+  @retval EFI_INVALID_PARAMETER PhaseType is invalid
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+SynQuacerSdMmcNotifyPhase (
+  IN      EFI_HANDLE                      ControllerHandle,
+  IN      UINT8                           Slot,
+  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
+  )
+{
+  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
+    return EFI_SUCCESS;
+  }
+
+  switch (PhaseType) {
+  case EdkiiSdMmcResetPre:
+    // Soft reset does not complete unless the clock is already enabled.
+    MmioWrite16 (SYNQUACER_EMMC_BASE + SD_HC_CLOCK_CTRL,
+                 SYNQUACER_CLOCK_CTRL_VAL);
+    break;
+
+  case EdkiiSdMmcInitHostPre:
+    // init vendor specific regs
+    MmioAnd16 (SYNQUACER_EMMC_BASE + F_SDH30_AHB_CONFIG,
+      ~(F_SDH30_AHB_BIGED | F_SDH30_BUSLOCK_EN));
+
+    MmioOr16 (SYNQUACER_EMMC_BASE + F_SDH30_AHB_CONFIG,
+      F_SDH30_SIN | F_SDH30_AHB_INCR_16 | F_SDH30_AHB_INCR_8 |
+      F_SDH30_AHB_INCR_4);
+
+    MmioAnd32 (SYNQUACER_EMMC_BASE + F_SDH30_ESD_CONTROL, ~F_SDH30_EMMC_RST);
+    MemoryFence ();
+    gBS->Stall (ESD_CONTROL_RESET_DELAY);
+
+    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_ESD_CONTROL,
+      F_SDH30_EMMC_RST | F_SDH30_CMD_DAT_DELAY | F_SDH30_EMMC_HS200);
+
+    gBS->Stall (IO_CONTROL2_SETTLE_US);
+    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, F_SDH30_CRES_O_DN);
+    MemoryFence ();
+    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, F_SDH30_MSEL_O_1_8);
+    MemoryFence ();
+    MmioAnd32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, ~F_SDH30_CRES_O_DN);
+    MemoryFence ();
+    gBS->Stall (IO_CONTROL2_SETTLE_US);
+
+    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_TUNING_SETTING,
+      F_SDH30_CMD_CHK_DIS);
+    break;
+
+  default:
+    break;
+  }
+  return EFI_SUCCESS;
+}
+
+STATIC EDKII_SD_MMC_OVERRIDE mSdMmcOverride = {
+  EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION,
+  SynQuacerSdMmcCapability,
+  SynQuacerSdMmcNotifyPhase,
+};
+
+EFI_STATUS
+EFIAPI
+RegisterEmmc (
+  VOID
+  )
+{
+  EFI_STATUS                      Status;
+  EFI_HANDLE                      Handle;
+
+  Status = RegisterNonDiscoverableMmioDevice (
+             NonDiscoverableDeviceTypeSdhci,
+             NonDiscoverableDeviceDmaTypeCoherent,
+             NULL,
+             &mSdMmcControllerHandle,
+             1,
+             SYNQUACER_EMMC_BASE, SYNQUACER_EMMC_BASE_SZ);
+  ASSERT_EFI_ERROR (Status);
+
+  Handle = NULL;
+  Status = gBS->InstallProtocolInterface (&Handle,
+                  &gEdkiiSdMmcOverrideProtocolGuid,
+                  EFI_NATIVE_INTERFACE, (VOID **)&mSdMmcOverride);
+  ASSERT_EFI_ERROR (Status);
+
+  return EFI_SUCCESS;
+}
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
index b9394aa19f1a..aab830dc3a5a 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
@@ -322,5 +322,10 @@ PlatformDxeEntryPoint (
   Status = EnableSettingsForm ();
   ASSERT_EFI_ERROR (Status);
 
+  if (mHiiSettings->EnableEmmc == EMMC_ENABLED) {
+    Status = RegisterEmmc ();
+    ASSERT_EFI_ERROR (Status);
+  }
+
   return EFI_SUCCESS;
 }
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h
index 5fb1437757b9..a391d2f67c29 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h
@@ -19,6 +19,7 @@
 #include <Guid/SynQuacerPlatformFormSet.h>
 #include <IndustryStandard/Pci.h>
 #include <Library/ArmGenericTimerCounterLib.h>
+#include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
@@ -26,6 +27,7 @@
 #include <Library/HiiLib.h>
 #include <Library/IoLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/NonDiscoverableDeviceRegistrationLib.h>
 #include <Library/PcdLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
@@ -35,6 +37,7 @@
 #include <Platform/VarStore.h>
 #include <Protocol/NonDiscoverableDevice.h>
 #include <Protocol/PciIo.h>
+#include <Protocol/SdMmcOverride.h>
 
 extern UINT8                             PlatformDxeHiiBin[];
 extern UINT8                             PlatformDxeStrings[];
@@ -48,4 +51,10 @@ RegisterPcieNotifier (
   VOID
   );
 
+EFI_STATUS
+EFIAPI
+RegisterEmmc (
+  VOID
+  );
+
 #endif
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
index 40e42a4d1864..49d9deee57ea 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
@@ -23,6 +23,7 @@ [Defines]
   ENTRY_POINT                    = PlatformDxeEntryPoint
 
 [Sources]
+  Emmc.c
   Pci.c
   PlatformDxe.c
   PlatformDxeHii.uni
@@ -39,6 +40,7 @@ [Packages]
 
 [LibraryClasses]
   ArmGenericTimerCounterLib
+  BaseLib
   BaseMemoryLib
   DebugLib
   DevicePathLib
@@ -46,6 +48,7 @@ [LibraryClasses]
   HiiLib
   IoLib
   MemoryAllocationLib
+  NonDiscoverableDeviceRegistrationLib
   PcdLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
@@ -62,6 +65,7 @@ [Guids]
 
 [Protocols]
   gEdkiiNonDiscoverableDeviceProtocolGuid         ## PRODUCES
+  gEdkiiSdMmcOverrideProtocolGuid                 ## PRODUCES
   gEfiPciIoProtocolGuid                           ## CONSUMES
   gPcf8563RealTimeClockLibI2cMasterProtocolGuid   ## PRODUCES
 
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
index b274d12ed2c6..2eca8bbba8c3 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
@@ -27,3 +27,9 @@
 
 #string STR_PCIE_MAX_SPEED_UNLIMITED   #language en-US "Unlimited"
 #string STR_PCIE_MAX_SPEED_GEN1        #language en-US "Gen1 (2.5 GT/s)"
+
+#string STR_EMMC_ENABLE_PROMPT         #language en-US "Enable on-board eMMC"
+#string STR_EMMC_ENABLE_HELP           #language en-US "Enable the on-board eMMC for booting and for use by the OS."
+
+#string STR_EMMC_DISABLED              #language en-US "Disabled"
+#string STR_EMMC_ENABLED               #language en-US "Enabled"
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr
index 52f554b61e3c..ea35e902b2d7 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr
@@ -62,6 +62,14 @@ formset
         option text = STRING_TOKEN(STR_PCIE_MAX_SPEED_GEN1), value = PCIE_MAX_SPEED_GEN1, flags = 0;
     endoneof;
 
+    oneof varid = SynQuacerPlatformSettings.EnableEmmc,
+        prompt      = STRING_TOKEN(STR_EMMC_ENABLE_PROMPT),
+        help        = STRING_TOKEN(STR_EMMC_ENABLE_HELP),
+        flags       = NUMERIC_SIZE_1 | INTERACTIVE | RESET_REQUIRED,
+        option text = STRING_TOKEN(STR_EMMC_DISABLED), value = EMMC_DISABLED, flags = DEFAULT;
+        option text = STRING_TOKEN(STR_EMMC_ENABLED), value = EMMC_ENABLED, flags = 0;
+    endoneof;
+
     subtitle text = STRING_TOKEN(STR_NULL_STRING);
 
   endform;
diff --git a/Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h b/Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h
index 5944613e7465..fbbcbd7d3eec 100644
--- a/Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h
+++ b/Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h
@@ -16,14 +16,18 @@
 
 #define SYNQUACER_PLATFORM_VARIABLE_NAME  L"SynQuacerPlatformSettings"
 
+#define EMMC_DISABLED                     0x0
+#define EMMC_ENABLED                      0x1
+
 #define PCIE_MAX_SPEED_UNLIMITED          0x0
 #define PCIE_MAX_SPEED_GEN1               0x1
 
 typedef struct {
+  UINT8         EnableEmmc;
   UINT8         PcieSlot0MaxSpeed;
   UINT8         PcieSlot1MaxSpeed;
   UINT8         PcieSlot2MaxSpeed;
-  UINT8         Reserved[5];
+  UINT8         Reserved[4];
 } SYNQUACER_PLATFORM_VARSTORE_DATA;
 
 #endif
diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
index cebf30c9b6ee..897d06743708 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
@@ -19,8 +19,9 @@
 #include <Library/DebugLib.h>
 #include <Library/DxeServicesLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Platform/VarStore.h>
 
-// add enough space for two instances of 'status = "disabled"'
+// add enough space for three instances of 'status = "disabled"'
 #define DTB_PADDING               64
 
 STATIC
@@ -65,12 +66,14 @@ DtPlatformLoadDtb (
   OUT   UINTN       *DtbSize
   )
 {
-  EFI_STATUS      Status;
-  VOID            *OrigDtb;
-  VOID            *CopyDtb;
-  UINTN           OrigDtbSize;
-  UINTN           CopyDtbSize;
-  INT32           Rc;
+  EFI_STATUS                        Status;
+  VOID                              *OrigDtb;
+  VOID                              *CopyDtb;
+  UINTN                             OrigDtbSize;
+  UINTN                             CopyDtbSize;
+  INT32                             Rc;
+  UINT64                            SettingsVal;
+  SYNQUACER_PLATFORM_VARSTORE_DATA  *Settings;
 
   Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
              EFI_SECTION_RAW, 0, &OrigDtb, &OrigDtbSize);
@@ -98,6 +101,12 @@ DtPlatformLoadDtb (
     DisableDtNode (CopyDtb, "/pcie@70000000");
   }
 
+  SettingsVal = PcdGet64 (PcdPlatformSettings);
+  Settings = (SYNQUACER_PLATFORM_VARSTORE_DATA *)&SettingsVal;
+  if (Settings->EnableEmmc == EMMC_DISABLED) {
+    DisableDtNode (CopyDtb, "/sdhci@52300000");
+  }
+
   *Dtb = CopyDtb;
   *DtbSize = CopyDtbSize;
 
diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
index e1f564f73078..548d62fd5c0a 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
@@ -37,6 +37,7 @@ [LibraryClasses]
 
 [Pcd]
   gSynQuacerTokenSpaceGuid.PcdPcieEnableMask
+  gSynQuacerTokenSpaceGuid.PcdPlatformSettings
 
 [Guids]
   gDtPlatformDefaultDtbFileGuid
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support
  2018-01-30 10:32 [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support Ard Biesheuvel
@ 2018-01-30 11:00 ` Leif Lindholm
  2018-01-30 11:14   ` Ard Biesheuvel
  0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2018-01-30 11:00 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel

On Tue, Jan 30, 2018 at 10:32:40AM +0000, Ard Biesheuvel wrote:
> Implement support for the SynQuacer eMMC controller. This involves an
> implementation of the SD/MMC override protocol to handle a couple of
> quirks that would otherwise prevent this IP from being driven by the
> generic SDHCI driver.
> 
> Also, add a HII page to the PlatformDxe driver that allows eMMC support
> to be enabled, and wire it up for both DeveloperBox and EVB.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Now that the core support for the SD/MMC override protocol is finally
> merged, resubmit this again. I dropped Leif's R-b given that I have
> now added DeveloperBox, as well as a HII option to enable eMMC.

Couple of minor comments/suggestions below and a question.

>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc                                    |   8 +
>  Platform/Socionext/DeveloperBox/DeveloperBox.fdf                                    |   7 +
>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc                        |   8 +
>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf                        |   7 +
>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                               |   1 -
>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts                       |   4 -
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c                              | 203 ++++++++++++++++++++
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c                       |   5 +
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h                       |   9 +
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf                     |   4 +
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni                  |   6 +
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr                  |   8 +
>  Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h                             |   6 +-
>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c   |  23 ++-
>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf |   1 +
>  15 files changed, 287 insertions(+), 13 deletions(-)
> 

> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> index 2d46b4515749..7e69eaba9b70 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> @@ -179,6 +179,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
> @@ -549,6 +550,13 @@ [Components.common]
>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/DeveloperBox/DeveloperBox.fdf b/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
> index 8443986fc3e7..b668f42c7962 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
> @@ -148,6 +148,13 @@ [FV.FvMain]
>    INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> index 6241b5bbd0b2..e35c17f0bcb7 100644
> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> @@ -179,6 +179,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
> @@ -549,6 +550,13 @@ [Components.common]
>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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 cc61cf42ccd4..ba2f32328c2b 100644
> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
> @@ -150,6 +150,13 @@ [FV.FvMain]
>    INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
> index fae3f033f98f..f1daac74973f 100644
> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
> @@ -567,6 +567,5 @@
>          clocks = <&clk_alw_c_0 &clk_alw_b_0>;
>          clock-names = "core", "iface";
>          dma-coherent;
> -        status = "disabled";
>      };
>  };
> diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
> index 97fddfedcb46..0c6826f52c35 100644
> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
> @@ -31,10 +31,6 @@
>                        "PEC-PD28",   "PEC-PD29",   "PEC-PD30",     "PEC-PD31";
>  };
>  
> -&sdhci {
> -    status = "okay";
> -};
> -
>  &mdio_netsec {
>      phy_netsec: ethernet-phy@1 {
>          compatible = "ethernet-phy-ieee802.3-c22";
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> new file mode 100644
> index 000000000000..29a3ebb369fb
> --- /dev/null
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
> @@ -0,0 +1,203 @@
> + /** @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]      PassThru              A pointer to the
> +                                        EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
> +  @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
> +SynQuacerSdMmcCapability (
> +  IN      EFI_HANDLE                      ControllerHandle,
> +  IN      UINT8                           Slot,
> +  IN  OUT VOID                            *SdMmcHcSlotCapability
> +  )
> +{
> +  UINT64 Capability;
> +
> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {

This test pattern repeats below, does it suggest a macro?

> +    return EFI_SUCCESS;
> +  }
> +
> +  //
> +  // Clear the SDR104 capability bit. This avoids the need for a HS200 tuning
> +  // quirk that is difficult to support using the generic driver.
> +  //
> +  Capability = ReadUnaligned64 (SdMmcHcSlotCapability);
> +  Capability &= ~(UINT64)SD_HC_CAP_SDR104;
> +  WriteUnaligned64 (SdMmcHcSlotCapability, Capability);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +
> +  Override function for SDHCI controller operations
> +
> +  @param[in]      ControllerHandle      The EFI_HANDLE of the controller.
> +  @param[in]      Slot                  The 0 based slot index.
> +  @param[in]      PhaseType             The type of operation and whether the
> +                                        hook is invoked right before (pre) or
> +                                        right after (post)
> +
> +  @retval EFI_SUCCESS           The override function completed successfully.
> +  @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
> +  @retval EFI_INVALID_PARAMETER PhaseType is invalid
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +SynQuacerSdMmcNotifyPhase (
> +  IN      EFI_HANDLE                      ControllerHandle,
> +  IN      UINT8                           Slot,
> +  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
> +  )
> +{
> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  switch (PhaseType) {
> +  case EdkiiSdMmcResetPre:
> +    // Soft reset does not complete unless the clock is already enabled.
> +    MmioWrite16 (SYNQUACER_EMMC_BASE + SD_HC_CLOCK_CTRL,
> +                 SYNQUACER_CLOCK_CTRL_VAL);

This function varies between this ^ type of wrapped indentation of args...

> +    break;
> +
> +  case EdkiiSdMmcInitHostPre:
> +    // init vendor specific regs
> +    MmioAnd16 (SYNQUACER_EMMC_BASE + F_SDH30_AHB_CONFIG,
> +      ~(F_SDH30_AHB_BIGED | F_SDH30_BUSLOCK_EN));

... and this ^ type of wrapped indentation of args.

I don't really mind which, but I would prefer consistency.

> +
> +    MmioOr16 (SYNQUACER_EMMC_BASE + F_SDH30_AHB_CONFIG,
> +      F_SDH30_SIN | F_SDH30_AHB_INCR_16 | F_SDH30_AHB_INCR_8 |
> +      F_SDH30_AHB_INCR_4);
> +
> +    MmioAnd32 (SYNQUACER_EMMC_BASE + F_SDH30_ESD_CONTROL, ~F_SDH30_EMMC_RST);
> +    MemoryFence ();
> +    gBS->Stall (ESD_CONTROL_RESET_DELAY);
> +
> +    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_ESD_CONTROL,
> +      F_SDH30_EMMC_RST | F_SDH30_CMD_DAT_DELAY | F_SDH30_EMMC_HS200);
> +
> +    gBS->Stall (IO_CONTROL2_SETTLE_US);
> +    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, F_SDH30_CRES_O_DN);
> +    MemoryFence ();
> +    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, F_SDH30_MSEL_O_1_8);
> +    MemoryFence ();
> +    MmioAnd32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, ~F_SDH30_CRES_O_DN);
> +    MemoryFence ();
> +    gBS->Stall (IO_CONTROL2_SETTLE_US);
> +
> +    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_TUNING_SETTING,
> +      F_SDH30_CMD_CHK_DIS);
> +    break;
> +
> +  default:
> +    break;
> +  }
> +  return EFI_SUCCESS;
> +}

> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> index 40e42a4d1864..49d9deee57ea 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -23,6 +23,7 @@ [Defines]
>    ENTRY_POINT                    = PlatformDxeEntryPoint
>  
>  [Sources]
> +  Emmc.c
>    Pci.c
>    PlatformDxe.c
>    PlatformDxeHii.uni
> @@ -39,6 +40,7 @@ [Packages]
>  
>  [LibraryClasses]
>    ArmGenericTimerCounterLib
> +  BaseLib
>    BaseMemoryLib
>    DebugLib
>    DevicePathLib
> @@ -46,6 +48,7 @@ [LibraryClasses]
>    HiiLib
>    IoLib
>    MemoryAllocationLib
> +  NonDiscoverableDeviceRegistrationLib
>    PcdLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> @@ -62,6 +65,7 @@ [Guids]
>  
>  [Protocols]
>    gEdkiiNonDiscoverableDeviceProtocolGuid         ## PRODUCES
> +  gEdkiiSdMmcOverrideProtocolGuid                 ## PRODUCES
>    gEfiPciIoProtocolGuid                           ## CONSUMES
>    gPcf8563RealTimeClockLibI2cMasterProtocolGuid   ## PRODUCES
>  
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
> index b274d12ed2c6..2eca8bbba8c3 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
> @@ -27,3 +27,9 @@
>  
>  #string STR_PCIE_MAX_SPEED_UNLIMITED   #language en-US "Unlimited"
>  #string STR_PCIE_MAX_SPEED_GEN1        #language en-US "Gen1 (2.5 GT/s)"
> +
> +#string STR_EMMC_ENABLE_PROMPT         #language en-US "Enable on-board eMMC"
> +#string STR_EMMC_ENABLE_HELP           #language en-US "Enable the on-board eMMC for booting and for use by the OS."
> +
> +#string STR_EMMC_DISABLED              #language en-US "Disabled"
> +#string STR_EMMC_ENABLED               #language en-US "Enabled"

Perhaps a random question, but ...
Why am I seeing this in cleartext in the patch? Is it really a unicode file?

/
    Leif


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support
  2018-01-30 11:00 ` Leif Lindholm
@ 2018-01-30 11:14   ` Ard Biesheuvel
  2018-01-30 11:47     ` Leif Lindholm
  0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2018-01-30 11:14 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: edk2-devel@lists.01.org

On 30 January 2018 at 11:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jan 30, 2018 at 10:32:40AM +0000, Ard Biesheuvel wrote:
>> Implement support for the SynQuacer eMMC controller. This involves an
>> implementation of the SD/MMC override protocol to handle a couple of
>> quirks that would otherwise prevent this IP from being driven by the
>> generic SDHCI driver.
>>
>> Also, add a HII page to the PlatformDxe driver that allows eMMC support
>> to be enabled, and wire it up for both DeveloperBox and EVB.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> Now that the core support for the SD/MMC override protocol is finally
>> merged, resubmit this again. I dropped Leif's R-b given that I have
>> now added DeveloperBox, as well as a HII option to enable eMMC.
>
> Couple of minor comments/suggestions below and a question.
>
>>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc                                    |   8 +
>>  Platform/Socionext/DeveloperBox/DeveloperBox.fdf                                    |   7 +
>>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc                        |   8 +
>>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf                        |   7 +
>>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                               |   1 -
>>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts                       |   4 -
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c                              | 203 ++++++++++++++++++++
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c                       |   5 +
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h                       |   9 +
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf                     |   4 +
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni                  |   6 +
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr                  |   8 +
>>  Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h                             |   6 +-
>>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c   |  23 ++-
>>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf |   1 +
>>  15 files changed, 287 insertions(+), 13 deletions(-)
>>
>
>> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
>> index 2d46b4515749..7e69eaba9b70 100644
>> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
>> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
>> @@ -179,6 +179,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
>> @@ -549,6 +550,13 @@ [Components.common]
>>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/DeveloperBox/DeveloperBox.fdf b/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
>> index 8443986fc3e7..b668f42c7962 100644
>> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
>> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.fdf
>> @@ -148,6 +148,13 @@ [FV.FvMain]
>>    INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
>> index 6241b5bbd0b2..e35c17f0bcb7 100644
>> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
>> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
>> @@ -179,6 +179,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
>> @@ -549,6 +550,13 @@ [Components.common]
>>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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 cc61cf42ccd4..ba2f32328c2b 100644
>> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
>> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf
>> @@ -150,6 +150,13 @@ [FV.FvMain]
>>    INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.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/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
>> index fae3f033f98f..f1daac74973f 100644
>> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
>> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
>> @@ -567,6 +567,5 @@
>>          clocks = <&clk_alw_c_0 &clk_alw_b_0>;
>>          clock-names = "core", "iface";
>>          dma-coherent;
>> -        status = "disabled";
>>      };
>>  };
>> diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
>> index 97fddfedcb46..0c6826f52c35 100644
>> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
>> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts
>> @@ -31,10 +31,6 @@
>>                        "PEC-PD28",   "PEC-PD29",   "PEC-PD30",     "PEC-PD31";
>>  };
>>
>> -&sdhci {
>> -    status = "okay";
>> -};
>> -
>>  &mdio_netsec {
>>      phy_netsec: ethernet-phy@1 {
>>          compatible = "ethernet-phy-ieee802.3-c22";
>> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
>> new file mode 100644
>> index 000000000000..29a3ebb369fb
>> --- /dev/null
>> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c
>> @@ -0,0 +1,203 @@
>> + /** @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]      PassThru              A pointer to the
>> +                                        EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
>> +  @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
>> +SynQuacerSdMmcCapability (
>> +  IN      EFI_HANDLE                      ControllerHandle,
>> +  IN      UINT8                           Slot,
>> +  IN  OUT VOID                            *SdMmcHcSlotCapability
>> +  )
>> +{
>> +  UINT64 Capability;
>> +
>> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
>
> This test pattern repeats below, does it suggest a macro?
>

I don't see how that would clear things up tbh, and the pattern occurs
only twice

#define IS_OUR_QUIRKY_SDMMC_CONTROLLER(Handle, Slot) \
  ((Handle) == mSdMmcControllerHandle && (Slot) == 0)

if (!IS_OUR_QUIRKY_SDMMC_CONTROLLER(ControllerHandle, Slot) {
  return EFI_SUCCESS;
}

I can change it if you want, or add a comment if the condition is not
self-explanatory enough.


>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  //
>> +  // Clear the SDR104 capability bit. This avoids the need for a HS200 tuning
>> +  // quirk that is difficult to support using the generic driver.
>> +  //
>> +  Capability = ReadUnaligned64 (SdMmcHcSlotCapability);
>> +  Capability &= ~(UINT64)SD_HC_CAP_SDR104;
>> +  WriteUnaligned64 (SdMmcHcSlotCapability, Capability);
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +
>> +  Override function for SDHCI controller operations
>> +
>> +  @param[in]      ControllerHandle      The EFI_HANDLE of the controller.
>> +  @param[in]      Slot                  The 0 based slot index.
>> +  @param[in]      PhaseType             The type of operation and whether the
>> +                                        hook is invoked right before (pre) or
>> +                                        right after (post)
>> +
>> +  @retval EFI_SUCCESS           The override function completed successfully.
>> +  @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
>> +  @retval EFI_INVALID_PARAMETER PhaseType is invalid
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +SynQuacerSdMmcNotifyPhase (
>> +  IN      EFI_HANDLE                      ControllerHandle,
>> +  IN      UINT8                           Slot,
>> +  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
>> +  )
>> +{
>> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  switch (PhaseType) {
>> +  case EdkiiSdMmcResetPre:
>> +    // Soft reset does not complete unless the clock is already enabled.
>> +    MmioWrite16 (SYNQUACER_EMMC_BASE + SD_HC_CLOCK_CTRL,
>> +                 SYNQUACER_CLOCK_CTRL_VAL);
>
> This function varies between this ^ type of wrapped indentation of args...
>
>> +    break;
>> +
>> +  case EdkiiSdMmcInitHostPre:
>> +    // init vendor specific regs
>> +    MmioAnd16 (SYNQUACER_EMMC_BASE + F_SDH30_AHB_CONFIG,
>> +      ~(F_SDH30_AHB_BIGED | F_SDH30_BUSLOCK_EN));
>
> ... and this ^ type of wrapped indentation of args.
>
> I don't really mind which, but I would prefer consistency.
>

Will fix

>> +
>> +    MmioOr16 (SYNQUACER_EMMC_BASE + F_SDH30_AHB_CONFIG,
>> +      F_SDH30_SIN | F_SDH30_AHB_INCR_16 | F_SDH30_AHB_INCR_8 |
>> +      F_SDH30_AHB_INCR_4);
>> +
>> +    MmioAnd32 (SYNQUACER_EMMC_BASE + F_SDH30_ESD_CONTROL, ~F_SDH30_EMMC_RST);
>> +    MemoryFence ();
>> +    gBS->Stall (ESD_CONTROL_RESET_DELAY);
>> +
>> +    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_ESD_CONTROL,
>> +      F_SDH30_EMMC_RST | F_SDH30_CMD_DAT_DELAY | F_SDH30_EMMC_HS200);
>> +
>> +    gBS->Stall (IO_CONTROL2_SETTLE_US);
>> +    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, F_SDH30_CRES_O_DN);
>> +    MemoryFence ();
>> +    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, F_SDH30_MSEL_O_1_8);
>> +    MemoryFence ();
>> +    MmioAnd32 (SYNQUACER_EMMC_BASE + F_SDH30_IO_CONTROL2, ~F_SDH30_CRES_O_DN);
>> +    MemoryFence ();
>> +    gBS->Stall (IO_CONTROL2_SETTLE_US);
>> +
>> +    MmioOr32 (SYNQUACER_EMMC_BASE + F_SDH30_TUNING_SETTING,
>> +      F_SDH30_CMD_CHK_DIS);
>> +    break;
>> +
>> +  default:
>> +    break;
>> +  }
>> +  return EFI_SUCCESS;
>> +}
>
>> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
>> index 40e42a4d1864..49d9deee57ea 100644
>> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
>> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
>> @@ -23,6 +23,7 @@ [Defines]
>>    ENTRY_POINT                    = PlatformDxeEntryPoint
>>
>>  [Sources]
>> +  Emmc.c
>>    Pci.c
>>    PlatformDxe.c
>>    PlatformDxeHii.uni
>> @@ -39,6 +40,7 @@ [Packages]
>>
>>  [LibraryClasses]
>>    ArmGenericTimerCounterLib
>> +  BaseLib
>>    BaseMemoryLib
>>    DebugLib
>>    DevicePathLib
>> @@ -46,6 +48,7 @@ [LibraryClasses]
>>    HiiLib
>>    IoLib
>>    MemoryAllocationLib
>> +  NonDiscoverableDeviceRegistrationLib
>>    PcdLib
>>    UefiBootServicesTableLib
>>    UefiDriverEntryPoint
>> @@ -62,6 +65,7 @@ [Guids]
>>
>>  [Protocols]
>>    gEdkiiNonDiscoverableDeviceProtocolGuid         ## PRODUCES
>> +  gEdkiiSdMmcOverrideProtocolGuid                 ## PRODUCES
>>    gEfiPciIoProtocolGuid                           ## CONSUMES
>>    gPcf8563RealTimeClockLibI2cMasterProtocolGuid   ## PRODUCES
>>
>> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
>> index b274d12ed2c6..2eca8bbba8c3 100644
>> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
>> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
>> @@ -27,3 +27,9 @@
>>
>>  #string STR_PCIE_MAX_SPEED_UNLIMITED   #language en-US "Unlimited"
>>  #string STR_PCIE_MAX_SPEED_GEN1        #language en-US "Gen1 (2.5 GT/s)"
>> +
>> +#string STR_EMMC_ENABLE_PROMPT         #language en-US "Enable on-board eMMC"
>> +#string STR_EMMC_ENABLE_HELP           #language en-US "Enable the on-board eMMC for booting and for use by the OS."
>> +
>> +#string STR_EMMC_DISABLED              #language en-US "Disabled"
>> +#string STR_EMMC_ENABLED               #language en-US "Enabled"
>
> Perhaps a random question, but ...
> Why am I seeing this in cleartext in the patch? Is it really a unicode file?
>

Given that we support UTF-8 in .uni files these days, and the fact
that all characters used are in the 7-bit ASCII range, it doesn't
really make a difference, I guess.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support
  2018-01-30 11:14   ` Ard Biesheuvel
@ 2018-01-30 11:47     ` Leif Lindholm
  2018-01-30 11:52       ` Ard Biesheuvel
  0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2018-01-30 11:47 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org

On Tue, Jan 30, 2018 at 11:14:31AM +0000, Ard Biesheuvel wrote:
> On 30 January 2018 at 11:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Tue, Jan 30, 2018 at 10:32:40AM +0000, Ard Biesheuvel wrote:
> >> Implement support for the SynQuacer eMMC controller. This involves an
> >> implementation of the SD/MMC override protocol to handle a couple of
> >> quirks that would otherwise prevent this IP from being driven by the
> >> generic SDHCI driver.
> >>
> >> Also, add a HII page to the PlatformDxe driver that allows eMMC support
> >> to be enabled, and wire it up for both DeveloperBox and EVB.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> Now that the core support for the SD/MMC override protocol is finally
> >> merged, resubmit this again. I dropped Leif's R-b given that I have
> >> now added DeveloperBox, as well as a HII option to enable eMMC.
> >
> > Couple of minor comments/suggestions below and a question.
> >
> >>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc                                    |   8 +
> >>  Platform/Socionext/DeveloperBox/DeveloperBox.fdf                                    |   7 +
> >>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc                        |   8 +
> >>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf                        |   7 +
> >>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                               |   1 -
> >>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts                       |   4 -
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c                              | 203 ++++++++++++++++++++
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c                       |   5 +
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h                       |   9 +
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf                     |   4 +
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni                  |   6 +
> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr                  |   8 +
> >>  Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h                             |   6 +-
> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c   |  23 ++-
> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf |   1 +
> >>  15 files changed, 287 insertions(+), 13 deletions(-)
> >>
> >
> >> +/**
> >> +
> >> +  Override function for SDHCI capability bits
> >> +
> >> +  @param[in]      PassThru              A pointer to the
> >> +                                        EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
> >> +  @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
> >> +SynQuacerSdMmcCapability (
> >> +  IN      EFI_HANDLE                      ControllerHandle,
> >> +  IN      UINT8                           Slot,
> >> +  IN  OUT VOID                            *SdMmcHcSlotCapability
> >> +  )
> >> +{
> >> +  UINT64 Capability;
> >> +
> >> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
> >
> > This test pattern repeats below, does it suggest a macro?
> >
> 
> I don't see how that would clear things up tbh, and the pattern occurs
> only twice
> 
> #define IS_OUR_QUIRKY_SDMMC_CONTROLLER(Handle, Slot) \
>   ((Handle) == mSdMmcControllerHandle && (Slot) == 0)
> 
> if (!IS_OUR_QUIRKY_SDMMC_CONTROLLER(ControllerHandle, Slot) {
>   return EFI_SUCCESS;
> }
> 
> I can change it if you want, or add a comment if the condition is not
> self-explanatory enough.

It's just an awful lot of logical operations on a single line.
'ControllerHandle != mSdMmcControllerHandle' is reasonably easy to
figure out, but '|| Slot != 0' looks a bit random there.

A comment would be sufficient.

Another option would be to reduce the number of logical operations by
two by doing
  if (ControllerHandle == mSdMmcControllerHandle && Slot == 0) {
and doing the body inside the if-statement.

That's a bit uglier in the next function, but I would expect it
follows the paradigm of "handle most likely case first"?

> >> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
> >> index b274d12ed2c6..2eca8bbba8c3 100644
> >> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
> >> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
> >> @@ -27,3 +27,9 @@
> >>
> >>  #string STR_PCIE_MAX_SPEED_UNLIMITED   #language en-US "Unlimited"
> >>  #string STR_PCIE_MAX_SPEED_GEN1        #language en-US "Gen1 (2.5 GT/s)"
> >> +
> >> +#string STR_EMMC_ENABLE_PROMPT         #language en-US "Enable on-board eMMC"
> >> +#string STR_EMMC_ENABLE_HELP           #language en-US "Enable the on-board eMMC for booting and for use by the OS."
> >> +
> >> +#string STR_EMMC_DISABLED              #language en-US "Disabled"
> >> +#string STR_EMMC_ENABLED               #language en-US "Enabled"
> >
> > Perhaps a random question, but ...
> > Why am I seeing this in cleartext in the patch? Is it really a unicode file?
> 
> Given that we support UTF-8 in .uni files these days, and the fact
> that all characters used are in the 7-bit ASCII range, it doesn't
> really make a difference, I guess.

Fair enough.

/
    Leif


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support
  2018-01-30 11:47     ` Leif Lindholm
@ 2018-01-30 11:52       ` Ard Biesheuvel
  2018-01-30 12:52         ` Leif Lindholm
  0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2018-01-30 11:52 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: edk2-devel@lists.01.org

On 30 January 2018 at 11:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jan 30, 2018 at 11:14:31AM +0000, Ard Biesheuvel wrote:
>> On 30 January 2018 at 11:00, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Tue, Jan 30, 2018 at 10:32:40AM +0000, Ard Biesheuvel wrote:
>> >> Implement support for the SynQuacer eMMC controller. This involves an
>> >> implementation of the SD/MMC override protocol to handle a couple of
>> >> quirks that would otherwise prevent this IP from being driven by the
>> >> generic SDHCI driver.
>> >>
>> >> Also, add a HII page to the PlatformDxe driver that allows eMMC support
>> >> to be enabled, and wire it up for both DeveloperBox and EVB.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >> Now that the core support for the SD/MMC override protocol is finally
>> >> merged, resubmit this again. I dropped Leif's R-b given that I have
>> >> now added DeveloperBox, as well as a HII option to enable eMMC.
>> >
>> > Couple of minor comments/suggestions below and a question.
>> >
>> >>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc                                    |   8 +
>> >>  Platform/Socionext/DeveloperBox/DeveloperBox.fdf                                    |   7 +
>> >>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc                        |   8 +
>> >>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf                        |   7 +
>> >>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                               |   1 -
>> >>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacerEvalBoard.dts                       |   4 -
>> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Emmc.c                              | 203 ++++++++++++++++++++
>> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c                       |   5 +
>> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h                       |   9 +
>> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf                     |   4 +
>> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni                  |   6 +
>> >>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr                  |   8 +
>> >>  Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h                             |   6 +-
>> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c   |  23 ++-
>> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf |   1 +
>> >>  15 files changed, 287 insertions(+), 13 deletions(-)
>> >>
>> >
>> >> +/**
>> >> +
>> >> +  Override function for SDHCI capability bits
>> >> +
>> >> +  @param[in]      PassThru              A pointer to the
>> >> +                                        EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
>> >> +  @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
>> >> +SynQuacerSdMmcCapability (
>> >> +  IN      EFI_HANDLE                      ControllerHandle,
>> >> +  IN      UINT8                           Slot,
>> >> +  IN  OUT VOID                            *SdMmcHcSlotCapability
>> >> +  )
>> >> +{
>> >> +  UINT64 Capability;
>> >> +
>> >> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
>> >
>> > This test pattern repeats below, does it suggest a macro?
>> >
>>
>> I don't see how that would clear things up tbh, and the pattern occurs
>> only twice
>>
>> #define IS_OUR_QUIRKY_SDMMC_CONTROLLER(Handle, Slot) \
>>   ((Handle) == mSdMmcControllerHandle && (Slot) == 0)
>>
>> if (!IS_OUR_QUIRKY_SDMMC_CONTROLLER(ControllerHandle, Slot) {
>>   return EFI_SUCCESS;
>> }
>>
>> I can change it if you want, or add a comment if the condition is not
>> self-explanatory enough.
>
> It's just an awful lot of logical operations on a single line.
> 'ControllerHandle != mSdMmcControllerHandle' is reasonably easy to
> figure out, but '|| Slot != 0' looks a bit random there.
>
> A comment would be sufficient.
>
> Another option would be to reduce the number of logical operations by
> two by doing
>   if (ControllerHandle == mSdMmcControllerHandle && Slot == 0) {
> and doing the body inside the if-statement.
>
> That's a bit uglier in the next function, but I would expect it
> follows the paradigm of "handle most likely case first"?
>

Actually, come to think of it, the slot number is completely
redundant. The non-discoverable SDHCI controller we expose only
implements a single slot, so something is terribly wrong if
ControllerHandle == mSdMmcControllerHandle && Slot != 0.

So I will reduce the sequence to

if (ControllerHandle != mSdMmcControllerHandle) {
  return EFI_SUCCESS;
}

ASSERT (Slot == 0);

instead. Ok?

>> >> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
>> >> index b274d12ed2c6..2eca8bbba8c3 100644
>> >> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
>> >> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni
>> >> @@ -27,3 +27,9 @@
>> >>
>> >>  #string STR_PCIE_MAX_SPEED_UNLIMITED   #language en-US "Unlimited"
>> >>  #string STR_PCIE_MAX_SPEED_GEN1        #language en-US "Gen1 (2.5 GT/s)"
>> >> +
>> >> +#string STR_EMMC_ENABLE_PROMPT         #language en-US "Enable on-board eMMC"
>> >> +#string STR_EMMC_ENABLE_HELP           #language en-US "Enable the on-board eMMC for booting and for use by the OS."
>> >> +
>> >> +#string STR_EMMC_DISABLED              #language en-US "Disabled"
>> >> +#string STR_EMMC_ENABLED               #language en-US "Enabled"
>> >
>> > Perhaps a random question, but ...
>> > Why am I seeing this in cleartext in the patch? Is it really a unicode file?
>>
>> Given that we support UTF-8 in .uni files these days, and the fact
>> that all characters used are in the 7-bit ASCII range, it doesn't
>> really make a difference, I guess.
>
> Fair enough.
>
> /
>     Leif


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support
  2018-01-30 11:52       ` Ard Biesheuvel
@ 2018-01-30 12:52         ` Leif Lindholm
  2018-01-30 13:04           ` Ard Biesheuvel
  0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2018-01-30 12:52 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org

On Tue, Jan 30, 2018 at 11:52:41AM +0000, Ard Biesheuvel wrote:
> >> >> +STATIC
> >> >> +EFI_STATUS
> >> >> +EFIAPI
> >> >> +SynQuacerSdMmcCapability (
> >> >> +  IN      EFI_HANDLE                      ControllerHandle,
> >> >> +  IN      UINT8                           Slot,
> >> >> +  IN  OUT VOID                            *SdMmcHcSlotCapability
> >> >> +  )
> >> >> +{
> >> >> +  UINT64 Capability;
> >> >> +
> >> >> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
> >> >
> >> > This test pattern repeats below, does it suggest a macro?
> >> >
> >>
> >> I don't see how that would clear things up tbh, and the pattern occurs
> >> only twice
> >>
> >> #define IS_OUR_QUIRKY_SDMMC_CONTROLLER(Handle, Slot) \
> >>   ((Handle) == mSdMmcControllerHandle && (Slot) == 0)
> >>
> >> if (!IS_OUR_QUIRKY_SDMMC_CONTROLLER(ControllerHandle, Slot) {
> >>   return EFI_SUCCESS;
> >> }
> >>
> >> I can change it if you want, or add a comment if the condition is not
> >> self-explanatory enough.
> >
> > It's just an awful lot of logical operations on a single line.
> > 'ControllerHandle != mSdMmcControllerHandle' is reasonably easy to
> > figure out, but '|| Slot != 0' looks a bit random there.
> >
> > A comment would be sufficient.
> >
> > Another option would be to reduce the number of logical operations by
> > two by doing
> >   if (ControllerHandle == mSdMmcControllerHandle && Slot == 0) {
> > and doing the body inside the if-statement.
> >
> > That's a bit uglier in the next function, but I would expect it
> > follows the paradigm of "handle most likely case first"?
> >
> 
> Actually, come to think of it, the slot number is completely
> redundant. The non-discoverable SDHCI controller we expose only
> implements a single slot, so something is terribly wrong if
> ControllerHandle == mSdMmcControllerHandle && Slot != 0.
> 
> So I will reduce the sequence to
> 
> if (ControllerHandle != mSdMmcControllerHandle) {
>   return EFI_SUCCESS;
> }
> 
> ASSERT (Slot == 0);
> 
> instead. Ok?

Sure, that works for me.
With that (in both places), and the indentation fix:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support
  2018-01-30 12:52         ` Leif Lindholm
@ 2018-01-30 13:04           ` Ard Biesheuvel
  0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2018-01-30 13:04 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: edk2-devel@lists.01.org

On 30 January 2018 at 12:52, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jan 30, 2018 at 11:52:41AM +0000, Ard Biesheuvel wrote:
>> >> >> +STATIC
>> >> >> +EFI_STATUS
>> >> >> +EFIAPI
>> >> >> +SynQuacerSdMmcCapability (
>> >> >> +  IN      EFI_HANDLE                      ControllerHandle,
>> >> >> +  IN      UINT8                           Slot,
>> >> >> +  IN  OUT VOID                            *SdMmcHcSlotCapability
>> >> >> +  )
>> >> >> +{
>> >> >> +  UINT64 Capability;
>> >> >> +
>> >> >> +  if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) {
>> >> >
>> >> > This test pattern repeats below, does it suggest a macro?
>> >> >
>> >>
>> >> I don't see how that would clear things up tbh, and the pattern occurs
>> >> only twice
>> >>
>> >> #define IS_OUR_QUIRKY_SDMMC_CONTROLLER(Handle, Slot) \
>> >>   ((Handle) == mSdMmcControllerHandle && (Slot) == 0)
>> >>
>> >> if (!IS_OUR_QUIRKY_SDMMC_CONTROLLER(ControllerHandle, Slot) {
>> >>   return EFI_SUCCESS;
>> >> }
>> >>
>> >> I can change it if you want, or add a comment if the condition is not
>> >> self-explanatory enough.
>> >
>> > It's just an awful lot of logical operations on a single line.
>> > 'ControllerHandle != mSdMmcControllerHandle' is reasonably easy to
>> > figure out, but '|| Slot != 0' looks a bit random there.
>> >
>> > A comment would be sufficient.
>> >
>> > Another option would be to reduce the number of logical operations by
>> > two by doing
>> >   if (ControllerHandle == mSdMmcControllerHandle && Slot == 0) {
>> > and doing the body inside the if-statement.
>> >
>> > That's a bit uglier in the next function, but I would expect it
>> > follows the paradigm of "handle most likely case first"?
>> >
>>
>> Actually, come to think of it, the slot number is completely
>> redundant. The non-discoverable SDHCI controller we expose only
>> implements a single slot, so something is terribly wrong if
>> ControllerHandle == mSdMmcControllerHandle && Slot != 0.
>>
>> So I will reduce the sequence to
>>
>> if (ControllerHandle != mSdMmcControllerHandle) {
>>   return EFI_SUCCESS;
>> }
>>
>> ASSERT (Slot == 0);
>>
>> instead. Ok?
>
> Sure, that works for me.
> With that (in both places), and the indentation fix:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Thanks. Pushed as c733b7ef291f


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-01-30 12:58 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-30 10:32 [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support Ard Biesheuvel
2018-01-30 11:00 ` Leif Lindholm
2018-01-30 11:14   ` Ard Biesheuvel
2018-01-30 11:47     ` Leif Lindholm
2018-01-30 11:52       ` Ard Biesheuvel
2018-01-30 12:52         ` Leif Lindholm
2018-01-30 13:04           ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox