* [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