[PATCH v3 0/6] mmc: imx: a few fixes and new feature

[PATCH v3 0/6] mmc: imx: a few fixes and new feature

[Haibo Chen]

Changes for v3:
-Add property describe in binding doc.

Haibo Chen (6):
  mmc: sdhci-esdhc-imx: add imx7d support and support HS400
  mmc: sdhci-esdhc-imx: add tuning-step seting support
  ARM: dts: imx7d-sdb: add eMMC5.0 support
  mmc: sdhci-esdhc-imx: add compatible string in bingding doc
  mmc: sdhci-esdhc-imx: config watermark level and burst length
  mmc: sdhci-esdhc-imx: set back the burst_length_enable bit to 1

 .../devicetree/bindings/mmc/fsl-imx-esdhc.txt      |  2 +
 arch/arm/boot/dts/imx7d-sdb.dts                    | 13 +++
 drivers/mmc/host/sdhci-esdhc-imx.c                 | 97 +++++++++++++++++++++-
 include/linux/platform_data/mmc-esdhc-imx.h        |  1 +
 4 files changed, 112 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support and support HS400

[Haibo Chen]

The imx7d usdhc is derived from imx6sx, the difference is that
imx7d support HS400.

So introduce a new compatible string for imx7d and add HS400
support for imx7d usdhc.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 66 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index c6b9f64..b441eed 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -44,6 +44,7 @@
 #define  ESDHC_MIX_CTRL_EXE_TUNE	(1 << 22)
 #define  ESDHC_MIX_CTRL_SMPCLK_SEL	(1 << 23)
 #define  ESDHC_MIX_CTRL_FBCLK_SEL	(1 << 25)
+#define  ESDHC_MIX_CTRL_HS400_EN	(1 << 26)
 /* Bits 3 and 6 are not SDHCI standard definitions */
 #define  ESDHC_MIX_CTRL_SDHCI_MASK	0xb7
 /* Tuning bits */
@@ -60,6 +61,16 @@
 #define  ESDHC_TUNE_CTRL_MIN		0
 #define  ESDHC_TUNE_CTRL_MAX		((1 << 7) - 1)
 
+/* strobe dll register */
+#define ESDHC_STROBE_DLL_CTRL		0x70
+#define ESDHC_STROBE_DLL_CTRL_ENABLE	(1 << 0)
+#define ESDHC_STROBE_DLL_CTRL_RESET	(1 << 1)
+#define ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT	3
+
+#define ESDHC_STROBE_DLL_STATUS		0x74
+#define ESDHC_STROBE_DLL_STS_REF_LOCK	(1 << 1)
+#define ESDHC_STROBE_DLL_STS_SLV_LOCK	0x1
+
 #define ESDHC_TUNING_CTRL		0xcc
 #define ESDHC_STD_TUNING_EN		(1 << 24)
 /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */
@@ -120,6 +131,8 @@
 #define ESDHC_FLAG_ERR004536		BIT(7)
 /* The IP supports HS200 mode */
 #define ESDHC_FLAG_HS200		BIT(8)
+/* The IP supports HS400 mode */
+#define ESDHC_FLAG_SUP_HS400		BIT(9)
 
 struct esdhc_soc_data {
 	u32 flags;
@@ -156,6 +169,12 @@ static struct esdhc_soc_data usdhc_imx6sx_data = {
 			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200,
 };
 
+static struct esdhc_soc_data usdhc_imx7d_data = {
+	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
+			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
+			| ESDHC_FLAG_SUP_HS400,
+};
+
 struct pltfm_imx_data {
 	u32 scratchpad;
 	struct pinctrl *pinctrl;
@@ -199,6 +218,7 @@ static const struct of_device_id imx_esdhc_dt_ids[] = {
 	{ .compatible = "fsl,imx6sx-usdhc", .data = &usdhc_imx6sx_data, },
 	{ .compatible = "fsl,imx6sl-usdhc", .data = &usdhc_imx6sl_data, },
 	{ .compatible = "fsl,imx6q-usdhc", .data = &usdhc_imx6q_data, },
+	{ .compatible = "fsl,imx7d-usdhc", .data = &usdhc_imx7d_data, },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids);
@@ -274,6 +294,10 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 				val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
 					| SDHCI_SUPPORT_SDR50
 					| SDHCI_USE_SDR50_TUNING;
+
+			/* imx7d does not have a support_hs400 register, fake one */
+			if (imx_data->socdata->flags & ESDHC_FLAG_SUP_HS400)
+				val |= SDHCI_SUPPORT_HS400;
 		}
 	}
 
@@ -774,6 +798,7 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
 		break;
 	case MMC_TIMING_UHS_SDR104:
 	case MMC_TIMING_MMC_HS200:
+	case MMC_TIMING_MMC_HS400:
 		pinctrl = imx_data->pins_200mhz;
 		break;
 	default:
@@ -784,6 +809,30 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
 	return pinctrl_select_state(imx_data->pinctrl, pinctrl);
 }
 
+static void esdhc_set_strobe_dll(struct sdhci_host *host)
+{
+	u32 v;
+
+	/* force a reset on strobe dll */
+	writel(ESDHC_STROBE_DLL_CTRL_RESET, host->ioaddr + ESDHC_STROBE_DLL_CTRL);
+	/*
+	 * enable strobe dll ctrl and adjust the delay target
+	 * for the uSDHC loopback read clock
+	 */
+	v = ESDHC_STROBE_DLL_CTRL_ENABLE |
+		(7 << ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT);
+	writel(v, host->ioaddr + ESDHC_STROBE_DLL_CTRL);
+	/* wait 1us to make sure strobe dll status register stable */
+	udelay(1);
+	v = readl(host->ioaddr + ESDHC_STROBE_DLL_STATUS);
+	if (!(v & ESDHC_STROBE_DLL_STS_REF_LOCK))
+		dev_warn(mmc_dev(host->mmc),
+			"warning! HS400 strobe DLL status REF not lock!\n");
+	if (!(v & ESDHC_STROBE_DLL_STS_SLV_LOCK))
+		dev_warn(mmc_dev(host->mmc),
+			"warning! HS400 strobe DLL status SLV not lock!\n");
+}
+
 static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -795,7 +844,13 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 	case MMC_TIMING_UHS_SDR25:
 	case MMC_TIMING_UHS_SDR50:
 	case MMC_TIMING_UHS_SDR104:
+		break;
 	case MMC_TIMING_MMC_HS200:
+		/* disable ddr mode and disable HS400 mode */
+		writel(readl(host->ioaddr + ESDHC_MIX_CTRL) &
+			~(ESDHC_MIX_CTRL_DDREN | ESDHC_MIX_CTRL_HS400_EN),
+			host->ioaddr + ESDHC_MIX_CTRL);
+		imx_data->is_ddr = 0;
 		break;
 	case MMC_TIMING_UHS_DDR50:
 	case MMC_TIMING_MMC_DDR52:
@@ -813,6 +868,14 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 			writel(v, host->ioaddr + ESDHC_DLL_CTRL);
 		}
 		break;
+	case MMC_TIMING_MMC_HS400:
+		writel(readl(host->ioaddr + ESDHC_MIX_CTRL) |
+				ESDHC_MIX_CTRL_DDREN | ESDHC_MIX_CTRL_HS400_EN,
+				host->ioaddr + ESDHC_MIX_CTRL);
+		imx_data->is_ddr = 1;
+		if (host->clock == 200000000)
+			esdhc_set_strobe_dll(host);
+		break;
 	}
 
 	esdhc_change_pinstate(host, timing);
@@ -1100,6 +1163,9 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	if (imx_data->socdata->flags & ESDHC_FLAG_ERR004536)
 		host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
 
+	if (imx_data->socdata->flags & ESDHC_FLAG_SUP_HS400)
+		host->quirks2 |= SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400;
+
 	if (of_id)
 		err = sdhci_esdhc_imx_probe_dt(pdev, host, imx_data);
 	else
-- 
1.9.1

[PATCH v3 2/6] mmc: sdhci-esdhc-imx: add tuning-step seting support

[Haibo Chen]

tuning-step is the delay cell steps in tuning procedure. The default value
of tuning-step is 1. For imx6 series usdhc, tuning procedure can be passed
when the tuning-step value is 1. But imx7d usdhc need the tuning-step value
as 2, otherwise it can't pass the tuning procedure.

So this patch add the tuning-step setting in driver, so that user can set
the tuning-step value in dts.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c          | 9 +++++++++
 include/linux/platform_data/mmc-esdhc-imx.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index b441eed..158f93b 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -75,6 +75,7 @@
 #define ESDHC_STD_TUNING_EN		(1 << 24)
 /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */
 #define ESDHC_TUNING_START_TAP		0x1
+#define ESDHC_TUNING_STEP_SHIFT		16
 
 /* pinctrl state */
 #define ESDHC_PINCTRL_STATE_100MHZ	"state_100mhz"
@@ -472,6 +473,7 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
 		} else if (imx_data->socdata->flags & ESDHC_FLAG_STD_TUNING) {
 			u32 v = readl(host->ioaddr + SDHCI_ACMD12_ERR);
 			u32 m = readl(host->ioaddr + ESDHC_MIX_CTRL);
+			u32 tuning_ctrl;
 			if (val & SDHCI_CTRL_TUNED_CLK) {
 				v |= ESDHC_MIX_CTRL_SMPCLK_SEL;
 			} else {
@@ -482,6 +484,11 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
 			if (val & SDHCI_CTRL_EXEC_TUNING) {
 				v |= ESDHC_MIX_CTRL_EXE_TUNE;
 				m |= ESDHC_MIX_CTRL_FBCLK_SEL;
+				tuning_ctrl = readl(host->ioaddr + ESDHC_TUNING_CTRL);
+				tuning_ctrl |= ESDHC_STD_TUNING_EN | ESDHC_TUNING_START_TAP;
+				if (imx_data->boarddata.tuning_step)
+					tuning_ctrl |= imx_data->boarddata.tuning_step << ESDHC_TUNING_STEP_SHIFT;
+					writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
 			} else {
 				v &= ~ESDHC_MIX_CTRL_EXE_TUNE;
 			}
@@ -949,6 +956,8 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 	if (gpio_is_valid(boarddata->wp_gpio))
 		boarddata->wp_type = ESDHC_WP_GPIO;
 
+	of_property_read_u32(np, "fsl,tuning-step", &boarddata->tuning_step);
+
 	if (of_find_property(np, "no-1-8-v", NULL))
 		boarddata->support_vsel = false;
 	else
diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
index e1571ef..95ccab3 100644
--- a/include/linux/platform_data/mmc-esdhc-imx.h
+++ b/include/linux/platform_data/mmc-esdhc-imx.h
@@ -45,5 +45,6 @@ struct esdhc_platform_data {
 	int max_bus_width;
 	bool support_vsel;
 	unsigned int delay_line;
+	unsigned int tuning_step;       /* The delay cell steps in tuning procedure */
 };
 #endif /* __ASM_ARCH_IMX_ESDHC_H */
-- 
1.9.1

[PATCH v3 3/6] ARM: dts: imx7d-sdb: add eMMC5.0 support

[Haibo Chen]

imx7d-sdb board has a eMMC5.0 on usdhc3. This eMMC support HS400.
This patch add usdhc3 support for HS400

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 arch/arm/boot/dts/imx7d-sdb.dts | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d-sdb.dts b/arch/arm/boot/dts/imx7d-sdb.dts
index fdd1d7c..8059458 100644
--- a/arch/arm/boot/dts/imx7d-sdb.dts
+++ b/arch/arm/boot/dts/imx7d-sdb.dts
@@ -241,6 +241,19 @@
 	status = "okay";
 };
 
+&usdhc3 {
+	pinctrl-names = "default", "state_100mhz", "state_200mhz";
+	pinctrl-0 = <&pinctrl_usdhc3>;
+	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
+	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
+	assigned-clocks = <&clks IMX7D_USDHC3_ROOT_CLK>;
+	assigned-clock-rates = <400000000>;
+	bus-width = <8>;
+	fsl,tuning-step = <2>;
+	non-removable;
+	status = "okay";
+};
+
 &iomuxc {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_hog>;
-- 
1.9.1

[PATCH v3 4/6] mmc: sdhci-esdhc-imx: add compatible string in bingding doc

[Haibo Chen]

Add a required property "fsl,imx7d-usdhc" in binding doc.
Add an optional property "fsl,tuning-step" in binding doc.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
index 211e778..c6624bc 100644
--- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
+++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
@@ -15,6 +15,7 @@ Required properties:
 	       "fsl,imx6q-usdhc"
 	       "fsl,imx6sl-usdhc"
 	       "fsl,imx6sx-usdhc"
+	       "fsl,imx7d-usdhc"
 
 Optional properties:
 - fsl,wp-controller : Indicate to use controller internal write protection
@@ -27,6 +28,7 @@ Optional properties:
   transparent level shifters on the outputs of the controller. Two cells are
   required, first cell specifies minimum slot voltage (mV), second cell
   specifies maximum slot voltage (mV). Several ranges could be specified.
+- fsl,tuning-step: Specify the increasing delay cell steps in tuning procedure.
 
 Examples:
 
-- 
1.9.1

[PATCH v3 5/6] mmc: sdhci-esdhc-imx: config watermark level and burst length

[Haibo Chen]

i.MX7D support eMMC HS400 mode, this mode can run in 8 bit,200MHZ
DDR mode. So the I/O speed improve a lot compare to SD3.0

The default burst length is 8, if we don't change this value, in
HS400 mode, when we do eMMC read operation, we can find that the
clock signal will stop for a period of time. This means the speed
of data moving on AHB bus is slower than I/O speed. So we should
improve the speed of data moving on AHB bus.

For imx7d usdhc, this patch set the burst length as 16, and set
watermark level as 64. The test result is the clock signal has
no stop during the eMMC HS400 operation. For other imx usdhc, remain
the default value: burst length as 8, watermark level as 16.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 158f93b..37d0095 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -239,6 +239,11 @@ static inline int is_imx6q_usdhc(struct pltfm_imx_data *data)
 	return data->socdata == &usdhc_imx6q_data;
 }
 
+static inline int is_imx7d_usdhc(struct pltfm_imx_data *data)
+{
+	return data->socdata == &usdhc_imx7d_data;
+}
+
 static inline int esdhc_is_usdhc(struct pltfm_imx_data *data)
 {
 	return !!(data->socdata->flags & ESDHC_FLAG_USDHC);
@@ -1145,7 +1150,11 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	 * to something insane.  Change it back here.
 	 */
 	if (esdhc_is_usdhc(imx_data)) {
-		writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
+		if (is_imx7d_usdhc(imx_data))
+			writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
+		else
+			writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
+
 		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
 		host->mmc->caps |= MMC_CAP_1_8V_DDR;
 
-- 
1.9.1

[PATCH v3 6/6] mmc: sdhci-esdhc-imx: set back the burst_length_enable bit to 1

[Haibo Chen]

Currently we find that if a usdhc is choosed to boot system, then ROM
code will set the burst length enable bit of this usdhc as 0.

This will make performance drop a lot if this usdhc's burst length is
16. So this patch set back the burst_length_enable bit as 1, which is
the default value, and means burst length is enabled for INCR.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 37d0095..dd945e5 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -32,6 +32,7 @@
 #include "sdhci-esdhc.h"
 
 #define	ESDHC_CTRL_D3CD			0x08
+#define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
 /* VENDOR SPEC register */
 #define ESDHC_VENDOR_SPEC		0xc0
 #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
@@ -1158,6 +1159,16 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
 		host->mmc->caps |= MMC_CAP_1_8V_DDR;
 
+		/*
+		 * ROM code will change the burst_length_enable setting to
+		 * zero if this usdhc is choosed to boot system. Change it
+		 * back here, otherwise it will impact the performance a
+		 * lot if the burst length is 16.
+		 */
+		writel(readl(host->ioaddr + SDHCI_HOST_CONTROL)
+			| ESDHC_BURST_LEN_EN_INCR,
+			host->ioaddr + SDHCI_HOST_CONTROL);
+
 		if (!(imx_data->socdata->flags & ESDHC_FLAG_HS200))
 			host->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200;
 
-- 
1.9.1

Re: [PATCH v3 0/6] mmc: imx: a few fixes and new feature

[Dong Aisheng]

On Wed, Jul 29, 2015 at 05:03:51PM +0800, Haibo Chen wrote:
> Changes for v3:
> -Add property describe in binding doc.
> 
> Haibo Chen (6):
>   mmc: sdhci-esdhc-imx: add imx7d support and support HS400
>   mmc: sdhci-esdhc-imx: add tuning-step seting support
>   ARM: dts: imx7d-sdb: add eMMC5.0 support
>   mmc: sdhci-esdhc-imx: add compatible string in bingding doc
>   mmc: sdhci-esdhc-imx: config watermark level and burst length
>   mmc: sdhci-esdhc-imx: set back the burst_length_enable bit to 1
> 

Hi Haibo,

I'm a little busy these days.
Will help review it ASAP, maybe can do it tomorrow.

Regards
Dong Aisheng

>  .../devicetree/bindings/mmc/fsl-imx-esdhc.txt      |  2 +
>  arch/arm/boot/dts/imx7d-sdb.dts                    | 13 +++
>  drivers/mmc/host/sdhci-esdhc-imx.c                 | 97 +++++++++++++++++++++-
>  include/linux/platform_data/mmc-esdhc-imx.h        |  1 +
>  4 files changed, 112 insertions(+), 1 deletion(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH v3 0/6] mmc: imx: a few fixes and new feature

[Holger Schurig]

Can you also fix the driver to NOT use mdelay(1) while in spinlock irqsafe?

A driver with such a design should have gotten a NAK in the first place ...

Re: [PATCH v3 2/6] mmc: sdhci-esdhc-imx: add tuning-step seting support

[Jan Lübbe]

On Mi, 2015-07-29 at 17:03 +0800, Haibo Chen wrote:
> tuning-step is the delay cell steps in tuning procedure. The default
> value of tuning-step is 1. For imx6 series usdhc, tuning procedure can
> be passed when the tuning-step value is 1. But imx7d usdhc need the
> tuning-step value as 2, otherwise it can't pass the tuning procedure.
> 
> So this patch add the tuning-step setting in driver, so that user can
> set the tuning-step value in dts.

>From your description, the correct tuning-step value only depends on the
SoC. Why not derive it from the compatible string?

Regards,
Jan Lübbe
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 2/6] mmc: sdhci-esdhc-imx: add tuning-step seting support

[Dong Aisheng]

On Thu, Jul 30, 2015 at 06:25:06PM +0200, Jan Lübbe wrote:
> On Mi, 2015-07-29 at 17:03 +0800, Haibo Chen wrote:
> > tuning-step is the delay cell steps in tuning procedure. The default
> > value of tuning-step is 1. For imx6 series usdhc, tuning procedure can
> > be passed when the tuning-step value is 1. But imx7d usdhc need the
> > tuning-step value as 2, otherwise it can't pass the tuning procedure.
> > 
> > So this patch add the tuning-step setting in driver, so that user can
> > set the tuning-step value in dts.
> 
> From your description, the correct tuning-step value only depends on the
> SoC. Why not derive it from the compatible string?
> 

'tuning-step' actually depends on board and card.
The commit message should be reformed a bit.

Regards
Dong Aisheng

> Regards,
> Jan Lübbe
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

Re: [PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support and support HS400

[Dong Aisheng]

On Wed, Jul 29, 2015 at 05:03:52PM +0800, Haibo Chen wrote:
> The imx7d usdhc is derived from imx6sx, the difference is that
> imx7d support HS400.
> 
> So introduce a new compatible string for imx7d and add HS400
> support for imx7d usdhc.
> 
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 66 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index c6b9f64..b441eed 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -44,6 +44,7 @@
>  #define  ESDHC_MIX_CTRL_EXE_TUNE	(1 << 22)
>  #define  ESDHC_MIX_CTRL_SMPCLK_SEL	(1 << 23)
>  #define  ESDHC_MIX_CTRL_FBCLK_SEL	(1 << 25)
> +#define  ESDHC_MIX_CTRL_HS400_EN	(1 << 26)
>  /* Bits 3 and 6 are not SDHCI standard definitions */
>  #define  ESDHC_MIX_CTRL_SDHCI_MASK	0xb7
>  /* Tuning bits */
> @@ -60,6 +61,16 @@
>  #define  ESDHC_TUNE_CTRL_MIN		0
>  #define  ESDHC_TUNE_CTRL_MAX		((1 << 7) - 1)
>  
> +/* strobe dll register */
> +#define ESDHC_STROBE_DLL_CTRL		0x70
> +#define ESDHC_STROBE_DLL_CTRL_ENABLE	(1 << 0)
> +#define ESDHC_STROBE_DLL_CTRL_RESET	(1 << 1)
> +#define ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT	3
> +
> +#define ESDHC_STROBE_DLL_STATUS		0x74
> +#define ESDHC_STROBE_DLL_STS_REF_LOCK	(1 << 1)
> +#define ESDHC_STROBE_DLL_STS_SLV_LOCK	0x1
> +
>  #define ESDHC_TUNING_CTRL		0xcc
>  #define ESDHC_STD_TUNING_EN		(1 << 24)
>  /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */
> @@ -120,6 +131,8 @@
>  #define ESDHC_FLAG_ERR004536		BIT(7)
>  /* The IP supports HS200 mode */
>  #define ESDHC_FLAG_HS200		BIT(8)
> +/* The IP supports HS400 mode */
> +#define ESDHC_FLAG_SUP_HS400		BIT(9)
>  
>  struct esdhc_soc_data {
>  	u32 flags;
> @@ -156,6 +169,12 @@ static struct esdhc_soc_data usdhc_imx6sx_data = {
>  			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200,
>  };
>  
> +static struct esdhc_soc_data usdhc_imx7d_data = {
> +	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> +			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> +			| ESDHC_FLAG_SUP_HS400,
> +};
> +
>  struct pltfm_imx_data {
>  	u32 scratchpad;
>  	struct pinctrl *pinctrl;
> @@ -199,6 +218,7 @@ static const struct of_device_id imx_esdhc_dt_ids[] = {
>  	{ .compatible = "fsl,imx6sx-usdhc", .data = &usdhc_imx6sx_data, },
>  	{ .compatible = "fsl,imx6sl-usdhc", .data = &usdhc_imx6sl_data, },
>  	{ .compatible = "fsl,imx6q-usdhc", .data = &usdhc_imx6q_data, },
> +	{ .compatible = "fsl,imx7d-usdhc", .data = &usdhc_imx7d_data, },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids);
> @@ -274,6 +294,10 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>  				val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
>  					| SDHCI_SUPPORT_SDR50
>  					| SDHCI_USE_SDR50_TUNING;
> +
> +			/* imx7d does not have a support_hs400 register, fake one */

You could remove this line.
It's bit, not register and i think no need such comment.

> +			if (imx_data->socdata->flags & ESDHC_FLAG_SUP_HS400)
> +				val |= SDHCI_SUPPORT_HS400;
>  		}
>  	}
>  
> @@ -774,6 +798,7 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
>  		break;
>  	case MMC_TIMING_UHS_SDR104:
>  	case MMC_TIMING_MMC_HS200:
> +	case MMC_TIMING_MMC_HS400:
>  		pinctrl = imx_data->pins_200mhz;
>  		break;
>  	default:
> @@ -784,6 +809,30 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
>  	return pinctrl_select_state(imx_data->pinctrl, pinctrl);
>  }
>  
> +static void esdhc_set_strobe_dll(struct sdhci_host *host)

It would be good if we can add some comments for this function for better
understand.

> +{
> +	u32 v;
> +
> +	/* force a reset on strobe dll */
> +	writel(ESDHC_STROBE_DLL_CTRL_RESET, host->ioaddr + ESDHC_STROBE_DLL_CTRL);
> +	/*
> +	 * enable strobe dll ctrl and adjust the delay target
> +	 * for the uSDHC loopback read clock
> +	 */
> +	v = ESDHC_STROBE_DLL_CTRL_ENABLE |
> +		(7 << ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT);
> +	writel(v, host->ioaddr + ESDHC_STROBE_DLL_CTRL);
> +	/* wait 1us to make sure strobe dll status register stable */
> +	udelay(1);
> +	v = readl(host->ioaddr + ESDHC_STROBE_DLL_STATUS);
> +	if (!(v & ESDHC_STROBE_DLL_STS_REF_LOCK))
> +		dev_warn(mmc_dev(host->mmc),
> +			"warning! HS400 strobe DLL status REF not lock!\n");
> +	if (!(v & ESDHC_STROBE_DLL_STS_SLV_LOCK))
> +		dev_warn(mmc_dev(host->mmc),
> +			"warning! HS400 strobe DLL status SLV not lock!\n");
> +}
> +
>  static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -795,7 +844,13 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>  	case MMC_TIMING_UHS_SDR25:
>  	case MMC_TIMING_UHS_SDR50:
>  	case MMC_TIMING_UHS_SDR104:
> +		break;
>  	case MMC_TIMING_MMC_HS200:
> +		/* disable ddr mode and disable HS400 mode */
> +		writel(readl(host->ioaddr + ESDHC_MIX_CTRL) &
> +			~(ESDHC_MIX_CTRL_DDREN | ESDHC_MIX_CTRL_HS400_EN),
> +			host->ioaddr + ESDHC_MIX_CTRL);
> +		imx_data->is_ddr = 0;
>  		break;
>  	case MMC_TIMING_UHS_DDR50:
>  	case MMC_TIMING_MMC_DDR52:
> @@ -813,6 +868,14 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>  			writel(v, host->ioaddr + ESDHC_DLL_CTRL);
>  		}
>  		break;
> +	case MMC_TIMING_MMC_HS400:
> +		writel(readl(host->ioaddr + ESDHC_MIX_CTRL) |
> +				ESDHC_MIX_CTRL_DDREN | ESDHC_MIX_CTRL_HS400_EN,
> +				host->ioaddr + ESDHC_MIX_CTRL);
> +		imx_data->is_ddr = 1;
> +		if (host->clock == 200000000)

I can't remember, but could this be a range required by SoC?

> +			esdhc_set_strobe_dll(host);
> +		break;
>  	}
>  
>  	esdhc_change_pinstate(host, timing);
> @@ -1100,6 +1163,9 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	if (imx_data->socdata->flags & ESDHC_FLAG_ERR004536)
>  		host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
>  
> +	if (imx_data->socdata->flags & ESDHC_FLAG_SUP_HS400)
> +		host->quirks2 |= SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400;
> +
>  	if (of_id)
>  		err = sdhci_esdhc_imx_probe_dt(pdev, host, imx_data);
>  	else
> -- 
> 1.9.1
> 

Regards
Dong Aisheng

Re: [PATCH v3 4/6] mmc: sdhci-esdhc-imx: add compatible string in bingding doc

[Dong Aisheng]

On Wed, Jul 29, 2015 at 05:03:55PM +0800, Haibo Chen wrote:
> Add a required property "fsl,imx7d-usdhc" in binding doc.
> Add an optional property "fsl,tuning-step" in binding doc.
> 

Better change to:
mmc: sdhci-esdhc-imx: add imx7d support in bingding doc

> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
>  Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> index 211e778..c6624bc 100644
> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> @@ -15,6 +15,7 @@ Required properties:
>  	       "fsl,imx6q-usdhc"
>  	       "fsl,imx6sl-usdhc"
>  	       "fsl,imx6sx-usdhc"
> +	       "fsl,imx7d-usdhc"
>  
>  Optional properties:
>  - fsl,wp-controller : Indicate to use controller internal write protection
> @@ -27,6 +28,7 @@ Optional properties:
>    transparent level shifters on the outputs of the controller. Two cells are
>    required, first cell specifies minimum slot voltage (mV), second cell
>    specifies maximum slot voltage (mV). Several ranges could be specified.
> +- fsl,tuning-step: Specify the increasing delay cell steps in tuning procedure.

we could add more explain about this property for better understanding:
e.g. The uSDHC is using one delay cell as default increasing step to do
tuning process. This property allows user to change the tuning step to more
than one delay cells which is useful for some special boards or cards when
the default tuning step can't find the proper delay window within limited
tuning reties.

>  
>  Examples:
>  
> -- 
> 1.9.1
> 

Regards
Dong Aisheng

Re: [PATCH v3 3/6] ARM: dts: imx7d-sdb: add eMMC5.0 support

[Dong Aisheng]

On Wed, Jul 29, 2015 at 05:03:54PM +0800, Haibo Chen wrote:
> imx7d-sdb board has a eMMC5.0 on usdhc3. This eMMC support HS400.
> This patch add usdhc3 support for HS400
> 

It seems this patch should be after
[PATCH v3 4/6] mmc: sdhci-esdhc-imx: add compatible string in bingding doc

Regards
Dong Aisheng

> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
>  arch/arm/boot/dts/imx7d-sdb.dts | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx7d-sdb.dts b/arch/arm/boot/dts/imx7d-sdb.dts
> index fdd1d7c..8059458 100644
> --- a/arch/arm/boot/dts/imx7d-sdb.dts
> +++ b/arch/arm/boot/dts/imx7d-sdb.dts
> @@ -241,6 +241,19 @@
>  	status = "okay";
>  };
>  
> +&usdhc3 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> +	pinctrl-0 = <&pinctrl_usdhc3>;
> +	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> +	assigned-clocks = <&clks IMX7D_USDHC3_ROOT_CLK>;
> +	assigned-clock-rates = <400000000>;
> +	bus-width = <8>;
> +	fsl,tuning-step = <2>;
> +	non-removable;
> +	status = "okay";
> +};
> +
>  &iomuxc {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_hog>;
> -- 
> 1.9.1
> 

Re: [PATCH v3 5/6] mmc: sdhci-esdhc-imx: config watermark level and burst length

[Dong Aisheng]

On Wed, Jul 29, 2015 at 05:03:56PM +0800, Haibo Chen wrote:
> i.MX7D support eMMC HS400 mode, this mode can run in 8 bit,200MHZ
> DDR mode. So the I/O speed improve a lot compare to SD3.0
> 
> The default burst length is 8, if we don't change this value, in
> HS400 mode, when we do eMMC read operation, we can find that the
> clock signal will stop for a period of time. This means the speed
> of data moving on AHB bus is slower than I/O speed. So we should
> improve the speed of data moving on AHB bus.
> 
> For imx7d usdhc, this patch set the burst length as 16, and set
> watermark level as 64. The test result is the clock signal has
> no stop during the eMMC HS400 operation. For other imx usdhc, remain
> the default value: burst length as 8, watermark level as 16.
> 
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 158f93b..37d0095 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -239,6 +239,11 @@ static inline int is_imx6q_usdhc(struct pltfm_imx_data *data)
>  	return data->socdata == &usdhc_imx6q_data;
>  }
>  
> +static inline int is_imx7d_usdhc(struct pltfm_imx_data *data)
> +{
> +	return data->socdata == &usdhc_imx7d_data;
> +}

Can we using flag to check instead of adding more is_imx_usdhc()?

> +
>  static inline int esdhc_is_usdhc(struct pltfm_imx_data *data)
>  {
>  	return !!(data->socdata->flags & ESDHC_FLAG_USDHC);
> @@ -1145,7 +1150,11 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	 * to something insane.  Change it back here.
>  	 */
>  	if (esdhc_is_usdhc(imx_data)) {
> -		writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
> +		if (is_imx7d_usdhc(imx_data))
> +			writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
> +		else
> +			writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
> +
>  		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
>  		host->mmc->caps |= MMC_CAP_1_8V_DDR;
>  

Regards
Dong Aisheng

> -- 
> 1.9.1
> 

Re: [PATCH v3 5/6] mmc: sdhci-esdhc-imx: config watermark level and burst length

[Dong Aisheng]

On Wed, Jul 29, 2015 at 05:03:56PM +0800, Haibo Chen wrote:
> i.MX7D support eMMC HS400 mode, this mode can run in 8 bit,200MHZ
> DDR mode. So the I/O speed improve a lot compare to SD3.0
> 
> The default burst length is 8, if we don't change this value, in
> HS400 mode, when we do eMMC read operation, we can find that the
> clock signal will stop for a period of time. This means the speed
> of data moving on AHB bus is slower than I/O speed. So we should
> improve the speed of data moving on AHB bus.
> 
> For imx7d usdhc, this patch set the burst length as 16, and set
> watermark level as 64. The test result is the clock signal has
> no stop during the eMMC HS400 operation. For other imx usdhc, remain
> the default value: burst length as 8, watermark level as 16.
> 

Add please change patch title a bit since this patch change is actually
for mx7d:

mmc: sdhci-esdhc-imx: change watermark level and burst length for imx7d

Regards
Dong Aisheng

> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 158f93b..37d0095 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -239,6 +239,11 @@ static inline int is_imx6q_usdhc(struct pltfm_imx_data *data)
>  	return data->socdata == &usdhc_imx6q_data;
>  }
>  
> +static inline int is_imx7d_usdhc(struct pltfm_imx_data *data)
> +{
> +	return data->socdata == &usdhc_imx7d_data;
> +}
> +
>  static inline int esdhc_is_usdhc(struct pltfm_imx_data *data)
>  {
>  	return !!(data->socdata->flags & ESDHC_FLAG_USDHC);
> @@ -1145,7 +1150,11 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	 * to something insane.  Change it back here.
>  	 */
>  	if (esdhc_is_usdhc(imx_data)) {
> -		writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
> +		if (is_imx7d_usdhc(imx_data))
> +			writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
> +		else
> +			writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
> +
>  		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
>  		host->mmc->caps |= MMC_CAP_1_8V_DDR;
>  
> -- 
> 1.9.1
> 

Re: [PATCH v3 6/6] mmc: sdhci-esdhc-imx: set back the burst_length_enable bit to 1

[Dong Aisheng]

On Wed, Jul 29, 2015 at 05:03:57PM +0800, Haibo Chen wrote:
> Currently we find that if a usdhc is choosed to boot system, then ROM
> code will set the burst length enable bit of this usdhc as 0.
> 
> This will make performance drop a lot if this usdhc's burst length is
> 16. So this patch set back the burst_length_enable bit as 1, which is
> the default value, and means burst length is enabled for INCR.
> 
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 37d0095..dd945e5 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -32,6 +32,7 @@
>  #include "sdhci-esdhc.h"
>  
>  #define	ESDHC_CTRL_D3CD			0x08
> +#define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
>  /* VENDOR SPEC register */
>  #define ESDHC_VENDOR_SPEC		0xc0
>  #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
> @@ -1158,6 +1159,16 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
>  		host->mmc->caps |= MMC_CAP_1_8V_DDR;
>  
> +		/*
> +		 * ROM code will change the burst_length_enable setting to
> +		 * zero if this usdhc is choosed to boot system. Change it
> +		 * back here, otherwise it will impact the performance a
> +		 * lot if the burst length is 16.

Can you clarify a bit more on why performance drops a lot if burst
length is 16?
Caused by the burst length setting did not work due to ROM disabled it?

Regards
Dong Aisheng

> +		 */
> +		writel(readl(host->ioaddr + SDHCI_HOST_CONTROL)
> +			| ESDHC_BURST_LEN_EN_INCR,
> +			host->ioaddr + SDHCI_HOST_CONTROL);
> +
>  		if (!(imx_data->socdata->flags & ESDHC_FLAG_HS200))
>  			host->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200;
>  
> -- 
> 1.9.1
> 

Re: [PATCH v3 5/6] mmc: sdhci-esdhc-imx: config watermark level and burst length

[Russell King - ARM Linux]

On Fri, Jul 31, 2015 at 10:51:41PM +0800, Dong Aisheng wrote:
> On Wed, Jul 29, 2015 at 05:03:56PM +0800, Haibo Chen wrote:
> > i.MX7D support eMMC HS400 mode, this mode can run in 8 bit,200MHZ
> > DDR mode. So the I/O speed improve a lot compare to SD3.0
> > 
> > The default burst length is 8, if we don't change this value, in
> > HS400 mode, when we do eMMC read operation, we can find that the
> > clock signal will stop for a period of time. This means the speed
> > of data moving on AHB bus is slower than I/O speed. So we should
> > improve the speed of data moving on AHB bus.
> > 
> > For imx7d usdhc, this patch set the burst length as 16, and set
> > watermark level as 64. The test result is the clock signal has
> > no stop during the eMMC HS400 operation. For other imx usdhc, remain
> > the default value: burst length as 8, watermark level as 16.
> > 
> > Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index 158f93b..37d0095 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -239,6 +239,11 @@ static inline int is_imx6q_usdhc(struct pltfm_imx_data *data)
> >  	return data->socdata == &usdhc_imx6q_data;
> >  }
> >  
> > +static inline int is_imx7d_usdhc(struct pltfm_imx_data *data)
> > +{
> > +	return data->socdata == &usdhc_imx7d_data;
> > +}
> 
> Can we using flag to check instead of adding more is_imx_usdhc()?

No, not more flags.  Do the job properly and parameterise the differences.

> >  static inline int esdhc_is_usdhc(struct pltfm_imx_data *data)
> >  {
> >  	return !!(data->socdata->flags & ESDHC_FLAG_USDHC);
> > @@ -1145,7 +1150,11 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
> >  	 * to something insane.  Change it back here.
> >  	 */
> >  	if (esdhc_is_usdhc(imx_data)) {
> > -		writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
> > +		if (is_imx7d_usdhc(imx_data))
> > +			writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
> > +		else
> > +			writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);

So the value to be written to this register should come from the
driver data, which is already used as a struct esdhc_soc_data to
extrapolate out the differences.

Going down the flag path will lead you to an even more of a stinking
shitpile than sdhci already is - if another version of the SoC requires
a different value there, what are you going to do?  Add yet another
flag for the next value?  What are you going to do when you have 16
different values?  Use 16 different flags?  Clearly that path is insane.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 5/6] mmc: sdhci-esdhc-imx: config watermark level and burst length

[Dong Aisheng]

On Fri, Jul 31, 2015 at 04:13:45PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 31, 2015 at 10:51:41PM +0800, Dong Aisheng wrote:
> > On Wed, Jul 29, 2015 at 05:03:56PM +0800, Haibo Chen wrote:
> > > i.MX7D support eMMC HS400 mode, this mode can run in 8 bit,200MHZ
> > > DDR mode. So the I/O speed improve a lot compare to SD3.0
> > > 
> > > The default burst length is 8, if we don't change this value, in
> > > HS400 mode, when we do eMMC read operation, we can find that the
> > > clock signal will stop for a period of time. This means the speed
> > > of data moving on AHB bus is slower than I/O speed. So we should
> > > improve the speed of data moving on AHB bus.
> > > 
> > > For imx7d usdhc, this patch set the burst length as 16, and set
> > > watermark level as 64. The test result is the clock signal has
> > > no stop during the eMMC HS400 operation. For other imx usdhc, remain
> > > the default value: burst length as 8, watermark level as 16.
> > > 
> > > Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> > > ---
> > >  drivers/mmc/host/sdhci-esdhc-imx.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > index 158f93b..37d0095 100644
> > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > @@ -239,6 +239,11 @@ static inline int is_imx6q_usdhc(struct pltfm_imx_data *data)
> > >  	return data->socdata == &usdhc_imx6q_data;
> > >  }
> > >  
> > > +static inline int is_imx7d_usdhc(struct pltfm_imx_data *data)
> > > +{
> > > +	return data->socdata == &usdhc_imx7d_data;
> > > +}
> > 
> > Can we using flag to check instead of adding more is_imx_usdhc()?
> 
> No, not more flags.  Do the job properly and parameterise the differences.
> 

Hi Russell,

Thanks for the review.

I mean using the exist flag to do like:
if (imx_data->socdata->flags & ESDHC_FLAG_SUP_HS400)
        writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
else    
        writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
Not adding another new flag.

ESDHC_FLAG_SUP_HS400 represents the new feature support of HS400 which is
already added in patch
[PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support and support HS400

> > >  static inline int esdhc_is_usdhc(struct pltfm_imx_data *data)
> > >  {
> > >  	return !!(data->socdata->flags & ESDHC_FLAG_USDHC);
> > > @@ -1145,7 +1150,11 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
> > >  	 * to something insane.  Change it back here.
> > >  	 */
> > >  	if (esdhc_is_usdhc(imx_data)) {
> > > -		writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
> > > +		if (is_imx7d_usdhc(imx_data))
> > > +			writel(0x10401040, host->ioaddr + ESDHC_WTMK_LVL);
> > > +		else
> > > +			writel(0x08100810, host->ioaddr + ESDHC_WTMK_LVL);
> 
> So the value to be written to this register should come from the
> driver data, which is already used as a struct esdhc_soc_data to
> extrapolate out the differences.
> 
> Going down the flag path will lead you to an even more of a stinking
> shitpile than sdhci already is - if another version of the SoC requires
> a different value there, what are you going to do?  Add yet another
> flag for the next value?  What are you going to do when you have 16
> different values?  Use 16 different flags?  Clearly that path is insane.
> 

I understand your concern for the potential bad situation.

Here the watermark level setting of mx7d is dependant on the new feature
of HS400, so it seems make senese to use that flag to set value and does
not need adding new flag.

Actually that is the current approach for uSDHC driver to distinguish
the difference between register offset/settings by checking feature flags.
e.g. ESDHC_FLAG_USDHC, ESDHC_FLAG_STD_TUNING, ESDHC_FLAG_HS200 and etc.
This approach can save us a lot more flags for SoC checking for common
feature part. And up till now, things seem good.

If really new SoC comes while it requires a different value, if it's feature
dependant, we may still use feature flag for it.
If not, probably a quirk may be needed if it's IP limitation.

If it's normal situation and such situation happened too many in the future,
we may choose to parameterise all these normal feature independent settings
into esdhc_soc_data to avoid adding meaningless flags.
e.g.
static struct esdhc_soc_data usdhc_imx6sx_data = {
        .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
                        | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200,
        .wml = 32,
};

static struct esdhc_soc_data usdhc_imx7d_data = {
        .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
                        | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200,
        .wml = 64,
};

But currently i still did not see such urgent requirement to do it for only
water mark level settings.

Regards
Dong Aisheng

> -- 
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.

Re: [PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support and support HS400

[Dong Aisheng]

On Sun, Aug 02, 2015 at 04:59:04PM +0800, Chen Haibo-B51421 wrote:
> 
> 
> > -----Original Message-----
> > From: Dong Aisheng [mailto:aisheng.dong@freescale.com]
> > Sent: Friday, July 31, 2015 10:15 PM
> > To: Chen Haibo-B51421
> > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; shawnguo@kernel.org;
> > kernel@pengutronix.de; linux@arm.linux.org.uk; ulf.hansson@linaro.org;
> > johan.derycke@barco.com; Estevam Fabio-R49496; Dong Aisheng-B29396;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-mmc@vger.kernel.org
> > Subject: Re: [PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support and
> > support HS400
> > 
> > On Wed, Jul 29, 2015 at 05:03:52PM +0800, Haibo Chen wrote:
> > > The imx7d usdhc is derived from imx6sx, the difference is that imx7d
> > > support HS400.
> > >
> > > So introduce a new compatible string for imx7d and add HS400 support
> > > for imx7d usdhc.
> > >
> > > Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> > > ---
> > >  drivers/mmc/host/sdhci-esdhc-imx.c | 66
> > > ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > index c6b9f64..b441eed 100644
> > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > @@ -44,6 +44,7 @@
> > >  #define  ESDHC_MIX_CTRL_EXE_TUNE	(1 << 22)
> > >  #define  ESDHC_MIX_CTRL_SMPCLK_SEL	(1 << 23)
> > >  #define  ESDHC_MIX_CTRL_FBCLK_SEL	(1 << 25)
> > > +#define  ESDHC_MIX_CTRL_HS400_EN	(1 << 26)
> > >  /* Bits 3 and 6 are not SDHCI standard definitions */
> > >  #define  ESDHC_MIX_CTRL_SDHCI_MASK	0xb7
> > >  /* Tuning bits */
> > > @@ -60,6 +61,16 @@
> > >  #define  ESDHC_TUNE_CTRL_MIN		0
> > >  #define  ESDHC_TUNE_CTRL_MAX		((1 << 7) - 1)
> > >
> > > +/* strobe dll register */
> > > +#define ESDHC_STROBE_DLL_CTRL		0x70
> > > +#define ESDHC_STROBE_DLL_CTRL_ENABLE	(1 << 0)
> > > +#define ESDHC_STROBE_DLL_CTRL_RESET	(1 << 1)
> > > +#define ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT	3
> > > +
> > > +#define ESDHC_STROBE_DLL_STATUS		0x74
> > > +#define ESDHC_STROBE_DLL_STS_REF_LOCK	(1 << 1)
> > > +#define ESDHC_STROBE_DLL_STS_SLV_LOCK	0x1
> > > +
> > >  #define ESDHC_TUNING_CTRL		0xcc
> > >  #define ESDHC_STD_TUNING_EN		(1 << 24)
> > >  /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */ @@
> > > -120,6 +131,8 @@
> > >  #define ESDHC_FLAG_ERR004536		BIT(7)
> > >  /* The IP supports HS200 mode */
> > >  #define ESDHC_FLAG_HS200		BIT(8)
> > > +/* The IP supports HS400 mode */
> > > +#define ESDHC_FLAG_SUP_HS400		BIT(9)
> > >
> > >  struct esdhc_soc_data {
> > >  	u32 flags;
> > > @@ -156,6 +169,12 @@ static struct esdhc_soc_data usdhc_imx6sx_data = {
> > >  			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200,  };
> > >
> > > +static struct esdhc_soc_data usdhc_imx7d_data = {
> > > +	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> > > +			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> > > +			| ESDHC_FLAG_SUP_HS400,
> > > +};
> > > +
> > >  struct pltfm_imx_data {
> > >  	u32 scratchpad;
> > >  	struct pinctrl *pinctrl;
> > > @@ -199,6 +218,7 @@ static const struct of_device_id imx_esdhc_dt_ids[]
> > = {
> > >  	{ .compatible = "fsl,imx6sx-usdhc", .data = &usdhc_imx6sx_data, },
> > >  	{ .compatible = "fsl,imx6sl-usdhc", .data = &usdhc_imx6sl_data, },
> > >  	{ .compatible = "fsl,imx6q-usdhc", .data = &usdhc_imx6q_data, },
> > > +	{ .compatible = "fsl,imx7d-usdhc", .data = &usdhc_imx7d_data, },
> > >  	{ /* sentinel */ }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids); @@ -274,6 +294,10 @@
> > > static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
> > >  				val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
> > >  					| SDHCI_SUPPORT_SDR50
> > >  					| SDHCI_USE_SDR50_TUNING;
> > > +
> > > +			/* imx7d does not have a support_hs400 register, fake
> > one */
> > 
> > You could remove this line.
> > It's bit, not register and i think no need such comment.
> > 
> > > +			if (imx_data->socdata->flags & ESDHC_FLAG_SUP_HS400)
> > > +				val |= SDHCI_SUPPORT_HS400;
> > >  		}
> > >  	}
> > >
> > > @@ -774,6 +798,7 @@ static int esdhc_change_pinstate(struct sdhci_host
> > *host,
> > >  		break;
> > >  	case MMC_TIMING_UHS_SDR104:
> > >  	case MMC_TIMING_MMC_HS200:
> > > +	case MMC_TIMING_MMC_HS400:
> > >  		pinctrl = imx_data->pins_200mhz;
> > >  		break;
> > >  	default:
> > > @@ -784,6 +809,30 @@ static int esdhc_change_pinstate(struct sdhci_host
> > *host,
> > >  	return pinctrl_select_state(imx_data->pinctrl, pinctrl);  }
> > >
> > > +static void esdhc_set_strobe_dll(struct sdhci_host *host)
> > 
> > It would be good if we can add some comments for this function for better
> > understand.
> > 
> 
> [haibo] okay, I will add comments here. 
> 
> > > +{
> > > +	u32 v;
> > > +
> > > +	/* force a reset on strobe dll */
> > > +	writel(ESDHC_STROBE_DLL_CTRL_RESET, host->ioaddr +
> > ESDHC_STROBE_DLL_CTRL);
> > > +	/*
> > > +	 * enable strobe dll ctrl and adjust the delay target
> > > +	 * for the uSDHC loopback read clock
> > > +	 */
> > > +	v = ESDHC_STROBE_DLL_CTRL_ENABLE |
> > > +		(7 << ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT);
> > > +	writel(v, host->ioaddr + ESDHC_STROBE_DLL_CTRL);
> > > +	/* wait 1us to make sure strobe dll status register stable */
> > > +	udelay(1);
> > > +	v = readl(host->ioaddr + ESDHC_STROBE_DLL_STATUS);
> > > +	if (!(v & ESDHC_STROBE_DLL_STS_REF_LOCK))
> > > +		dev_warn(mmc_dev(host->mmc),
> > > +			"warning! HS400 strobe DLL status REF not lock!\n");
> > > +	if (!(v & ESDHC_STROBE_DLL_STS_SLV_LOCK))
> > > +		dev_warn(mmc_dev(host->mmc),
> > > +			"warning! HS400 strobe DLL status SLV not lock!\n"); }
> > > +
> > >  static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned
> > > timing)  {
> > >  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -795,7
> > > +844,13 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host,
> > unsigned timing)
> > >  	case MMC_TIMING_UHS_SDR25:
> > >  	case MMC_TIMING_UHS_SDR50:
> > >  	case MMC_TIMING_UHS_SDR104:
> > > +		break;
> > >  	case MMC_TIMING_MMC_HS200:
> > > +		/* disable ddr mode and disable HS400 mode */
> > > +		writel(readl(host->ioaddr + ESDHC_MIX_CTRL) &
> > > +			~(ESDHC_MIX_CTRL_DDREN | ESDHC_MIX_CTRL_HS400_EN),
> > > +			host->ioaddr + ESDHC_MIX_CTRL);
> > > +		imx_data->is_ddr = 0;
> > >  		break;
> > >  	case MMC_TIMING_UHS_DDR50:
> > >  	case MMC_TIMING_MMC_DDR52:
> > > @@ -813,6 +868,14 @@ static void esdhc_set_uhs_signaling(struct
> > sdhci_host *host, unsigned timing)
> > >  			writel(v, host->ioaddr + ESDHC_DLL_CTRL);
> > >  		}
> > >  		break;
> > > +	case MMC_TIMING_MMC_HS400:
> > > +		writel(readl(host->ioaddr + ESDHC_MIX_CTRL) |
> > > +				ESDHC_MIX_CTRL_DDREN | ESDHC_MIX_CTRL_HS400_EN,
> > > +				host->ioaddr + ESDHC_MIX_CTRL);
> > > +		imx_data->is_ddr = 1;
> > > +		if (host->clock == 200000000)
> > 
> > I can't remember, but could this be a range required by SoC?
> 
> [haibo]we should set strobe_dll only when the host clock is 200MHz. For other condition, no need to do this.
> 

It is not quite make sense for only 200Mhz clock here.

First, shall we check host->mmc->actual_clock instead of host->clock?
That is the real clock frequency the controller is working on.


Second, even MMC core requests to set 200Mhz clock for HS400, the real
clock controller working on may be less than 200Mhz which depends on the
uSDHC root clock capability.
e.g. it may be 192Mhz if the parent is 384Mhz or less.
If that, do we need call strobe_dll too?

Can you check above two concerns?

Regards
Dong Aisheng

> > 
> > > +			esdhc_set_strobe_dll(host);
> > > +		break;
> > >  	}
> > >
> > >  	esdhc_change_pinstate(host, timing); @@ -1100,6 +1163,9 @@ static
> > > int sdhci_esdhc_imx_probe(struct platform_device *pdev)
> > >  	if (imx_data->socdata->flags & ESDHC_FLAG_ERR004536)
> > >  		host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
> > >
> > > +	if (imx_data->socdata->flags & ESDHC_FLAG_SUP_HS400)
> > > +		host->quirks2 |= SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400;
> > > +
> > >  	if (of_id)
> > >  		err = sdhci_esdhc_imx_probe_dt(pdev, host, imx_data);
> > >  	else
> > > --
> > > 1.9.1
> > >
> > 
> > Regards
> > Dong Aisheng

Re: [PATCH v3 6/6] mmc: sdhci-esdhc-imx: set back the burst_length_enable bit to 1

[Dong Aisheng]

On Mon, Aug 03, 2015 at 09:08:28AM +0800, Chen Haibo-B51421 wrote:
> 
> 
> > -----Original Message-----
> > From: Dong Aisheng [mailto:aisheng.dong@freescale.com]
> > Sent: Friday, July 31, 2015 10:58 PM
> > To: Chen Haibo-B51421
> > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; shawnguo@kernel.org;
> > kernel@pengutronix.de; linux@arm.linux.org.uk; ulf.hansson@linaro.org;
> > johan.derycke@barco.com; Estevam Fabio-R49496; Dong Aisheng-B29396;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-mmc@vger.kernel.org
> > Subject: Re: [PATCH v3 6/6] mmc: sdhci-esdhc-imx: set back the
> > burst_length_enable bit to 1
> > 
> > On Wed, Jul 29, 2015 at 05:03:57PM +0800, Haibo Chen wrote:
> > > Currently we find that if a usdhc is choosed to boot system, then ROM
> > > code will set the burst length enable bit of this usdhc as 0.
> > >
> > > This will make performance drop a lot if this usdhc's burst length is
> > > 16. So this patch set back the burst_length_enable bit as 1, which is
> > > the default value, and means burst length is enabled for INCR.
> > >
> > > Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> > > ---
> > >  drivers/mmc/host/sdhci-esdhc-imx.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > index 37d0095..dd945e5 100644
> > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > @@ -32,6 +32,7 @@
> > >  #include "sdhci-esdhc.h"
> > >
> > >  #define	ESDHC_CTRL_D3CD			0x08
> > > +#define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
> > >  /* VENDOR SPEC register */
> > >  #define ESDHC_VENDOR_SPEC		0xc0
> > >  #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
> > > @@ -1158,6 +1159,16 @@ static int sdhci_esdhc_imx_probe(struct
> > platform_device *pdev)
> > >  		host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> > >  		host->mmc->caps |= MMC_CAP_1_8V_DDR;
> > >
> > > +		/*
> > > +		 * ROM code will change the burst_length_enable setting to
> > > +		 * zero if this usdhc is choosed to boot system. Change it
> > > +		 * back here, otherwise it will impact the performance a
> > > +		 * lot if the burst length is 16.
> > 
> > Can you clarify a bit more on why performance drops a lot if burst length
> > is 16?
> > Caused by the burst length setting did not work due to ROM disabled it?
> 
> 
> [haibo] this bit is used to enable/disable the burst length for the external AHB2AXI bridge, 
> It's useful especially for INCR transfer because without burst length indicator, the AHB2AXI 
> bridge doesn't know the burst length in advance. And without burst length indicator, AHB INCR 
> transfers can only be converted to singles on the AXI side.
> 
> Seting this bit means burst length enabled for INCR.
> If this bit is not set, performance will drop a lot when burst length is 8 or 16. I will add 
> this in the commit log.
>  

Thanks for clarify.
One question: with this patch, can we set the default watermark level to 64 by default for all
SoC types?

If yes, we may not need patch 5 anymore.
[PATCH v3 5/6] mmc: sdhci-esdhc-imx: config watermark level and burst length

Regards
Dong Aisheng

> > 
> > Regards
> > Dong Aisheng
> > 
> > > +		 */
> > > +		writel(readl(host->ioaddr + SDHCI_HOST_CONTROL)
> > > +			| ESDHC_BURST_LEN_EN_INCR,
> > > +			host->ioaddr + SDHCI_HOST_CONTROL);
> > > +
> > >  		if (!(imx_data->socdata->flags & ESDHC_FLAG_HS200))
> > >  			host->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200;
> > >
> > > --
> > > 1.9.1
> > >

Re: [PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support and support HS400

[Dong Aisheng]

On Wed, Jul 29, 2015 at 05:03:52PM +0800, Haibo Chen wrote:
> The imx7d usdhc is derived from imx6sx, the difference is that
> imx7d support HS400.
> 
> So introduce a new compatible string for imx7d and add HS400
> support for imx7d usdhc.
> 
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 66 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index c6b9f64..b441eed 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -44,6 +44,7 @@
>  #define  ESDHC_MIX_CTRL_EXE_TUNE	(1 << 22)
>  #define  ESDHC_MIX_CTRL_SMPCLK_SEL	(1 << 23)
>  #define  ESDHC_MIX_CTRL_FBCLK_SEL	(1 << 25)
> +#define  ESDHC_MIX_CTRL_HS400_EN	(1 << 26)
>  /* Bits 3 and 6 are not SDHCI standard definitions */
>  #define  ESDHC_MIX_CTRL_SDHCI_MASK	0xb7
>  /* Tuning bits */
> @@ -60,6 +61,16 @@
>  #define  ESDHC_TUNE_CTRL_MIN		0
>  #define  ESDHC_TUNE_CTRL_MAX		((1 << 7) - 1)
>  
> +/* strobe dll register */
> +#define ESDHC_STROBE_DLL_CTRL		0x70
> +#define ESDHC_STROBE_DLL_CTRL_ENABLE	(1 << 0)
> +#define ESDHC_STROBE_DLL_CTRL_RESET	(1 << 1)
> +#define ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT	3
> +
> +#define ESDHC_STROBE_DLL_STATUS		0x74
> +#define ESDHC_STROBE_DLL_STS_REF_LOCK	(1 << 1)
> +#define ESDHC_STROBE_DLL_STS_SLV_LOCK	0x1
> +
>  #define ESDHC_TUNING_CTRL		0xcc
>  #define ESDHC_STD_TUNING_EN		(1 << 24)
>  /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */
> @@ -120,6 +131,8 @@
>  #define ESDHC_FLAG_ERR004536		BIT(7)
>  /* The IP supports HS200 mode */
>  #define ESDHC_FLAG_HS200		BIT(8)
> +/* The IP supports HS400 mode */
> +#define ESDHC_FLAG_SUP_HS400		BIT(9)
>  
>  struct esdhc_soc_data {
>  	u32 flags;
> @@ -156,6 +169,12 @@ static struct esdhc_soc_data usdhc_imx6sx_data = {
>  			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200,
>  };
>  
> +static struct esdhc_soc_data usdhc_imx7d_data = {
> +	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> +			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> +			| ESDHC_FLAG_SUP_HS400,

Better to use ESDHC_FLAG_HS400 to keep align with exist ESDHC_FLAG_HS200.

Regards
Dong Aisheng

Re: [PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support and support HS400

[Dong Aisheng]

On Wed, Aug 05, 2015 at 03:39:29PM +0800, Chen Haibo-B51421 wrote:
> 
> 
> > -----Original Message-----
> > From: Dong Aisheng [mailto:aisheng.dong@freescale.com]
> > Sent: Monday, August 03, 2015 8:10 PM
> > To: Chen Haibo-B51421
> > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; shawnguo@kernel.org;
> > kernel@pengutronix.de; linux@arm.linux.org.uk; ulf.hansson@linaro.org;
> > johan.derycke@barco.com; Estevam Fabio-R49496; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-mmc@vger.kernel.org
> > Subject: Re: [PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support and
> > support HS400
> > 
> > On Sun, Aug 02, 2015 at 04:59:04PM +0800, Chen Haibo-B51421 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Dong Aisheng [mailto:aisheng.dong@freescale.com]
> > > > Sent: Friday, July 31, 2015 10:15 PM
> > > > To: Chen Haibo-B51421
> > > > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> > > > ijc+devicetree@hellion.org.uk; galak@codeaurora.org;
> > > > ijc+shawnguo@kernel.org;
> > > > kernel@pengutronix.de; linux@arm.linux.org.uk;
> > > > ulf.hansson@linaro.org; johan.derycke@barco.com; Estevam
> > > > Fabio-R49496; Dong Aisheng-B29396; devicetree@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
> > > > linux-mmc@vger.kernel.org
> > > > Subject: Re: [PATCH v3 1/6] mmc: sdhci-esdhc-imx: add imx7d support
> > > > and support HS400
> > > >
> > > > On Wed, Jul 29, 2015 at 05:03:52PM +0800, Haibo Chen wrote:
> > > > > The imx7d usdhc is derived from imx6sx, the difference is that
> > > > > imx7d support HS400.
> > > > >
> > > > > So introduce a new compatible string for imx7d and add HS400
> > > > > support for imx7d usdhc.
> > > > >
> > > > > Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> > > > > ---
> > > > >  drivers/mmc/host/sdhci-esdhc-imx.c | 66
> > > > > ++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 66 insertions(+)
> > > > >
> > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > index c6b9f64..b441eed 100644
> > > > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > @@ -44,6 +44,7 @@
> > > > >  #define  ESDHC_MIX_CTRL_EXE_TUNE	(1 << 22)
> > > > >  #define  ESDHC_MIX_CTRL_SMPCLK_SEL	(1 << 23)
> > > > >  #define  ESDHC_MIX_CTRL_FBCLK_SEL	(1 << 25)
> > > > > +#define  ESDHC_MIX_CTRL_HS400_EN	(1 << 26)
> > > > >  /* Bits 3 and 6 are not SDHCI standard definitions */
> > > > >  #define  ESDHC_MIX_CTRL_SDHCI_MASK	0xb7
> > > > >  /* Tuning bits */
> > > > > @@ -60,6 +61,16 @@
> > > > >  #define  ESDHC_TUNE_CTRL_MIN		0
> > > > >  #define  ESDHC_TUNE_CTRL_MAX		((1 << 7) - 1)
> > > > >
> > > > > +/* strobe dll register */
> > > > > +#define ESDHC_STROBE_DLL_CTRL		0x70
> > > > > +#define ESDHC_STROBE_DLL_CTRL_ENABLE	(1 << 0)
> > > > > +#define ESDHC_STROBE_DLL_CTRL_RESET	(1 << 1)
> > > > > +#define ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT	3
> > > > > +
> > > > > +#define ESDHC_STROBE_DLL_STATUS		0x74
> > > > > +#define ESDHC_STROBE_DLL_STS_REF_LOCK	(1 << 1)
> > > > > +#define ESDHC_STROBE_DLL_STS_SLV_LOCK	0x1
> > > > > +
> > > > >  #define ESDHC_TUNING_CTRL		0xcc
> > > > >  #define ESDHC_STD_TUNING_EN		(1 << 24)
> > > > >  /* NOTE: the minimum valid tuning start tap for mx6sl is 1 */ @@
> > > > > -120,6 +131,8 @@
> > > > >  #define ESDHC_FLAG_ERR004536		BIT(7)
> > > > >  /* The IP supports HS200 mode */
> > > > >  #define ESDHC_FLAG_HS200		BIT(8)
> > > > > +/* The IP supports HS400 mode */
> > > > > +#define ESDHC_FLAG_SUP_HS400		BIT(9)
> > > > >
> > > > >  struct esdhc_soc_data {
> > > > >  	u32 flags;
> > > > > @@ -156,6 +169,12 @@ static struct esdhc_soc_data usdhc_imx6sx_data
> > = {
> > > > >  			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200,  };
> > > > >
> > > > > +static struct esdhc_soc_data usdhc_imx7d_data = {
> > > > > +	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> > > > > +			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> > > > > +			| ESDHC_FLAG_SUP_HS400,
> > > > > +};
> > > > > +
> > > > >  struct pltfm_imx_data {
> > > > >  	u32 scratchpad;
> > > > >  	struct pinctrl *pinctrl;
> > > > > @@ -199,6 +218,7 @@ static const struct of_device_id
> > > > > imx_esdhc_dt_ids[]
> > > > = {
> > > > >  	{ .compatible = "fsl,imx6sx-usdhc", .data =
> > &usdhc_imx6sx_data, },
> > > > >  	{ .compatible = "fsl,imx6sl-usdhc", .data =
> > &usdhc_imx6sl_data, },
> > > > >  	{ .compatible = "fsl,imx6q-usdhc", .data =
> > &usdhc_imx6q_data, },
> > > > > +	{ .compatible = "fsl,imx7d-usdhc", .data =
> > &usdhc_imx7d_data, },
> > > > >  	{ /* sentinel */ }
> > > > >  };
> > > > >  MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids); @@ -274,6 +294,10 @@
> > > > > static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
> > > > >  				val = SDHCI_SUPPORT_DDR50 |
> > SDHCI_SUPPORT_SDR104
> > > > >  					| SDHCI_SUPPORT_SDR50
> > > > >  					| SDHCI_USE_SDR50_TUNING;
> > > > > +
> > > > > +			/* imx7d does not have a support_hs400 register,
> > fake
> > > > one */
> > > >
> > > > You could remove this line.
> > > > It's bit, not register and i think no need such comment.
> > > >
> > > > > +			if (imx_data->socdata->flags &
> > ESDHC_FLAG_SUP_HS400)
> > > > > +				val |= SDHCI_SUPPORT_HS400;
> > > > >  		}
> > > > >  	}
> > > > >
> > > > > @@ -774,6 +798,7 @@ static int esdhc_change_pinstate(struct
> > > > > sdhci_host
> > > > *host,
> > > > >  		break;
> > > > >  	case MMC_TIMING_UHS_SDR104:
> > > > >  	case MMC_TIMING_MMC_HS200:
> > > > > +	case MMC_TIMING_MMC_HS400:
> > > > >  		pinctrl = imx_data->pins_200mhz;
> > > > >  		break;
> > > > >  	default:
> > > > > @@ -784,6 +809,30 @@ static int esdhc_change_pinstate(struct
> > > > > sdhci_host
> > > > *host,
> > > > >  	return pinctrl_select_state(imx_data->pinctrl, pinctrl);  }
> > > > >
> > > > > +static void esdhc_set_strobe_dll(struct sdhci_host *host)
> > > >
> > > > It would be good if we can add some comments for this function for
> > > > better understand.
> > > >
> > >
> > > [haibo] okay, I will add comments here.
> > >
> > > > > +{
> > > > > +	u32 v;
> > > > > +
> > > > > +	/* force a reset on strobe dll */
> > > > > +	writel(ESDHC_STROBE_DLL_CTRL_RESET, host->ioaddr +
> > > > ESDHC_STROBE_DLL_CTRL);
> > > > > +	/*
> > > > > +	 * enable strobe dll ctrl and adjust the delay target
> > > > > +	 * for the uSDHC loopback read clock
> > > > > +	 */
> > > > > +	v = ESDHC_STROBE_DLL_CTRL_ENABLE |
> > > > > +		(7 << ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT);
> > > > > +	writel(v, host->ioaddr + ESDHC_STROBE_DLL_CTRL);
> > > > > +	/* wait 1us to make sure strobe dll status register stable */
> > > > > +	udelay(1);
> > > > > +	v = readl(host->ioaddr + ESDHC_STROBE_DLL_STATUS);
> > > > > +	if (!(v & ESDHC_STROBE_DLL_STS_REF_LOCK))
> > > > > +		dev_warn(mmc_dev(host->mmc),
> > > > > +			"warning! HS400 strobe DLL status REF not
> > lock!\n");
> > > > > +	if (!(v & ESDHC_STROBE_DLL_STS_SLV_LOCK))
> > > > > +		dev_warn(mmc_dev(host->mmc),
> > > > > +			"warning! HS400 strobe DLL status SLV not
> > lock!\n"); }
> > > > > +
> > > > >  static void esdhc_set_uhs_signaling(struct sdhci_host *host,
> > > > > unsigned
> > > > > timing)  {
> > > > >  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@
> > > > > -795,7
> > > > > +844,13 @@ static void esdhc_set_uhs_signaling(struct sdhci_host
> > > > > +*host,
> > > > unsigned timing)
> > > > >  	case MMC_TIMING_UHS_SDR25:
> > > > >  	case MMC_TIMING_UHS_SDR50:
> > > > >  	case MMC_TIMING_UHS_SDR104:
> > > > > +		break;
> > > > >  	case MMC_TIMING_MMC_HS200:
> > > > > +		/* disable ddr mode and disable HS400 mode */
> > > > > +		writel(readl(host->ioaddr + ESDHC_MIX_CTRL) &
> > > > > +			~(ESDHC_MIX_CTRL_DDREN | ESDHC_MIX_CTRL_HS400_EN),
> > > > > +			host->ioaddr + ESDHC_MIX_CTRL);
> > > > > +		imx_data->is_ddr = 0;
> > > > >  		break;
> > > > >  	case MMC_TIMING_UHS_DDR50:
> > > > >  	case MMC_TIMING_MMC_DDR52:
> > > > > @@ -813,6 +868,14 @@ static void esdhc_set_uhs_signaling(struct
> > > > sdhci_host *host, unsigned timing)
> > > > >  			writel(v, host->ioaddr + ESDHC_DLL_CTRL);
> > > > >  		}
> > > > >  		break;
> > > > > +	case MMC_TIMING_MMC_HS400:
> > > > > +		writel(readl(host->ioaddr + ESDHC_MIX_CTRL) |
> > > > > +				ESDHC_MIX_CTRL_DDREN |
> > ESDHC_MIX_CTRL_HS400_EN,
> > > > > +				host->ioaddr + ESDHC_MIX_CTRL);
> > > > > +		imx_data->is_ddr = 1;
> > > > > +		if (host->clock == 200000000)
> > > >
> > > > I can't remember, but could this be a range required by SoC?
> > >
> > > [haibo]we should set strobe_dll only when the host clock is 200MHz. For
> > other condition, no need to do this.
> > >
> > 
> > It is not quite make sense for only 200Mhz clock here.
> > 
> > First, shall we check host->mmc->actual_clock instead of host->clock?
> > That is the real clock frequency the controller is working on.
> > 
> > 
> > Second, even MMC core requests to set 200Mhz clock for HS400, the real
> > clock controller working on may be less than 200Mhz which depends on the
> > uSDHC root clock capability.
> > e.g. it may be 192Mhz if the parent is 384Mhz or less.
> > If that, do we need call strobe_dll too?
> > 
> 
> [haibo] according to IC team feedback, when we use HS400 mode in high frequency, like:
> close to 200Mhz, including 192Mhz, we should call strobe_dll. When the eMMC clock is around
> 100Mhz, like 98Mhz, no need to call strobe_dll, I have test this, eMMC can read normally.
> 
> When the card clock become higher, each clock cycle becomes shorter. When each clock cycle is short
> enough, the time delay between CLK and strobe_data_clock maybe larger than one clock cycle, so the CLk
> and strobe_data_clock misaligned,  then read error shows up. (here CLK means the clock host supply to card,
> and strobe_data_clock means the clock generated by device which feedback to host for data read in HS400 mode).
> 
> So can we call strobe_dll when the card clock is over 100Mhz? 
> 

Yes, i'm okay with it.
You could define a macro for it like:
/* a higher clock frequency than this rate requires strobe dll control */
#define ESDHC_STROBE_DLL_CLK_FREQ	100000000
if (host->mmc->actual_clock > ESDHC_STROBE_DLL_CLK_FREQ)
	strobe_dll();

You can also get this as a separate function call which seems better.

Regards
Dong Aisheng

> 
> > Can you check above two concerns?
> > 
> > Regards
> > Dong Aisheng
> > 
> > > >
> > > > > +			esdhc_set_strobe_dll(host);
> > > > > +		break;
> > > > >  	}
> > > > >
> > > > >  	esdhc_change_pinstate(host, timing); @@ -1100,6 +1163,9 @@
> > > > > static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
> > > > >  	if (imx_data->socdata->flags & ESDHC_FLAG_ERR004536)
> > > > >  		host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
> > > > >
> > > > > +	if (imx_data->socdata->flags & ESDHC_FLAG_SUP_HS400)
> > > > > +		host->quirks2 |= SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400;
> > > > > +
> > > > >  	if (of_id)
> > > > >  		err = sdhci_esdhc_imx_probe_dt(pdev, host, imx_data);
> > > > >  	else
> > > > > --
> > > > > 1.9.1
> > > > >
> > > >
> > > > Regards
> > > > Dong Aisheng