[PATCH 0/8] Tegra SDHCI support HS400 on Tegra210 and Tegra186

[PATCH 0/8] Tegra SDHCI support HS400 on Tegra210 and Tegra186

[Aapo Vienamo]

Hi all,
This series implements support for HS400 signaling on Tegra210 and
Tegra186. This includes programming the DQS trimmer values, implementing
enhanced strobe and HS400 delay line calibration.

This series depends on the "Tegra SDHCI add support for HS200 and UHS
signaling" series.

Aapo Vienamo (8):
  dt-bindings: mmc: Add DQS trim value to Tegra SDHCI
  mmc: tegra: Parse and program DQS trim value
  mmc: tegra: Implement HS400 enhanced strobe
  mmc: tegra: Implement HS400 delay line calibration
  arm64: dts: tegra186: Add SDMMC4 DQS trim value
  arm64: dts: tegra210: Add SDMMC4 DQS trim value
  arm64: dts: tegra186: Enable HS400
  arm64: dts: tegra210: Enable HS400

 .../bindings/mmc/nvidia,tegra20-sdhci.txt          |  3 +
 arch/arm64/boot/dts/nvidia/tegra186.dtsi           |  2 +
 arch/arm64/boot/dts/nvidia/tegra210.dtsi           |  2 +
 drivers/mmc/host/sdhci-tegra.c                     | 83 +++++++++++++++++++++-
 4 files changed, 87 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/8] dt-bindings: mmc: Add DQS trim value to Tegra SDHCI

[Aapo Vienamo]

Document HS400 DQS trim value device tree property.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
index 3c7960a..7d294f3 100644
--- a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
@@ -72,6 +72,7 @@ Optional properties for Tegra210 and Tegra186:
   trimmer value for non-tunable modes.
 - nvidia,default-trim : Specify the default outbound clock trimmer
   value.
+- nvidia,dqs-trim : Specify DQS trim value for HS400 timing
 
   Notes on the pad calibration pull up and pulldown offset values:
     - The property values are drive codes which are programmed into the
@@ -88,6 +89,8 @@ Optional properties for Tegra210 and Tegra186:
     - The values are programmed to the Vendor Clock Control Register.
       Please refer to the reference manual of the SoC for correct
       values.
+    - The DQS trim values are only used on controllers which support
+      HS400 timing.
 
 Example:
 sdhci@700b0000 {
-- 
2.7.4

[PATCH 2/8] mmc: tegra: Parse and program DQS trim value

[Aapo Vienamo]

Parse and program the HS400 DQS trim value from dt. Program a fallback
value in case the property is missing.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 7f1ac4a..426f7ea 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -43,6 +43,10 @@
 #define SDHCI_CLOCK_CTRL_PADPIPE_CLKEN_OVERRIDE		BIT(3)
 #define SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE	BIT(2)
 
+#define SDHCI_TEGRA_VENDOR_CAP_OVERRIDES		0x10c
+#define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_MASK		0x00003f00
+#define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_SHIFT	8
+
 #define SDHCI_TEGRA_VENDOR_MISC_CTRL			0x120
 #define SDHCI_MISC_CTRL_ENABLE_SDR104			0x8
 #define SDHCI_MISC_CTRL_ENABLE_SDR50			0x10
@@ -112,6 +116,7 @@ struct sdhci_tegra {
 
 	u32 default_tap;
 	u32 default_trim;
+	u32 dqs_trim;
 };
 
 static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
@@ -500,7 +505,7 @@ static void tegra_sdhci_parse_pad_autocal_dt(struct sdhci_host *host)
 		autocal->pull_down_hs400 = autocal->pull_down_1v8;
 }
 
-static void tegra_sdhci_parse_default_tap_and_trim(struct sdhci_host *host)
+static void tegra_sdhci_parse_tap_and_trim(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
@@ -515,6 +520,11 @@ static void tegra_sdhci_parse_default_tap_and_trim(struct sdhci_host *host)
 				       &tegra_host->default_trim);
 	if (err)
 		tegra_host->default_trim = 0;
+
+	err = device_property_read_u32(host->mmc->parent, "nvidia,dqs-trim",
+				       &tegra_host->dqs_trim);
+	if (err)
+		tegra_host->dqs_trim = 0x11;
 }
 
 static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
@@ -545,20 +555,33 @@ static unsigned int tegra_sdhci_get_max_clock(struct sdhci_host *host)
 	return clk_round_rate(pltfm_host->clk, UINT_MAX);
 }
 
+static void tegra_sdhci_set_dqs_trim(struct sdhci_host *host, u8 val)
+{
+	u32 reg;
+
+	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_CAP_OVERRIDES);
+	reg &= ~SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_MASK;
+	reg |= val<<SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_SHIFT;
+	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_CAP_OVERRIDES);
+}
+
 static void tegra_sdhci_set_uhs_signaling(struct sdhci_host *host,
 					  unsigned timing)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
 	bool set_default_tap = false;
+	bool set_dqs_trim = false;
 
 	switch (timing) {
 	case MMC_TIMING_UHS_SDR50:
 	case MMC_TIMING_UHS_SDR104:
 	case MMC_TIMING_MMC_HS200:
-	case MMC_TIMING_MMC_HS400:
 		/* Don't set default tap on tunable modes. */
 		break;
+	case MMC_TIMING_MMC_HS400:
+		set_dqs_trim = true;
+		break;
 	case MMC_TIMING_MMC_DDR52:
 	case MMC_TIMING_UHS_DDR50:
 		tegra_host->ddr_signaling = true;
@@ -575,6 +598,9 @@ static void tegra_sdhci_set_uhs_signaling(struct sdhci_host *host,
 
 	if (set_default_tap)
 		tegra_sdhci_set_tap(host, tegra_host->default_tap);
+
+	if (set_dqs_trim)
+		tegra_sdhci_set_dqs_trim(host, tegra_host->dqs_trim);
 }
 
 static int tegra_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
@@ -930,7 +956,7 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
 
 	tegra_sdhci_parse_pad_autocal_dt(host);
 
-	tegra_sdhci_parse_default_tap_and_trim(host);
+	tegra_sdhci_parse_tap_and_trim(host);
 
 	tegra_host->power_gpio = devm_gpiod_get_optional(&pdev->dev, "power",
 							 GPIOD_OUT_HIGH);
-- 
2.7.4

[PATCH 3/8] mmc: tegra: Implement HS400 enhanced strobe

[Aapo Vienamo]

Implement HS400 enhanced strobe.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 426f7ea..d81143b 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -43,6 +43,9 @@
 #define SDHCI_CLOCK_CTRL_PADPIPE_CLKEN_OVERRIDE		BIT(3)
 #define SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE	BIT(2)
 
+#define SDHCI_TEGRA_VENDOR_SYS_SW_CTRL			0x104
+#define SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE		BIT(31)
+
 #define SDHCI_TEGRA_VENDOR_CAP_OVERRIDES		0x10c
 #define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_MASK		0x00003f00
 #define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_SHIFT	8
@@ -271,6 +274,22 @@ static void tegra_sdhci_set_tap(struct sdhci_host *host, unsigned int tap)
 	}
 }
 
+static void tegra_sdhci_hs400_enhanced_strobe(struct mmc_host *mmc,
+					      struct mmc_ios *ios)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	u32 reg;
+
+	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL);
+	pr_err("enhanced strobe: %d\n", ios->enhanced_strobe);
+	if (ios->enhanced_strobe)
+		reg |= SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE;
+	else
+		reg &= ~SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE;
+	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL);
+
+}
+
 static void tegra_sdhci_reset(struct sdhci_host *host, u8 mask)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -947,6 +966,9 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
 				sdhci_tegra_start_signal_voltage_switch;
 	}
 
+	host->mmc_host_ops.hs400_enhanced_strobe =
+			tegra_sdhci_hs400_enhanced_strobe;
+
 	rc = mmc_of_parse(host->mmc);
 	if (rc)
 		goto err_parse_dt;
-- 
2.7.4

[PATCH 4/8] mmc: tegra: Implement HS400 delay line calibration

[Aapo Vienamo]

Implement HS400 specific delay line calibration procedure.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index d81143b..d0b68b7 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -56,6 +56,12 @@
 #define SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300		0x20
 #define SDHCI_MISC_CTRL_ENABLE_DDR50			0x200
 
+#define SDHCI_TEGRA_VENDOR_DLLCAL_CFG			0x1b0
+#define SDHCI_TEGRA_DLLCAL_CALIBRATE			BIT(31)
+
+#define SDHCI_TEGRA_VENDOR_DLLCAL_STA			0x1bc
+#define SDHCI_TEGRA_DLLCAL_STA_ACTIVE			BIT(31)
+
 #define SDHCI_VNDR_TUN_CTRL0_0				0x1c0
 #define SDHCI_VNDR_TUN_CTRL0_TUN_HW_TAP			0x20000
 
@@ -584,6 +590,24 @@ static void tegra_sdhci_set_dqs_trim(struct sdhci_host *host, u8 val)
 	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_CAP_OVERRIDES);
 }
 
+static void tegra_sdhci_hs400_dll_cal(struct sdhci_host *host)
+{
+	u32 reg;
+	int err;
+
+	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_DLLCAL_CFG);
+	reg |= SDHCI_TEGRA_DLLCAL_CALIBRATE;
+	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_DLLCAL_CFG);
+
+	/* 1 ms sleep, 5 ms timeout */
+	err = readl_poll_timeout(host->ioaddr + SDHCI_TEGRA_VENDOR_DLLCAL_STA,
+				 reg, !(reg & SDHCI_TEGRA_DLLCAL_STA_ACTIVE),
+				 1000, 5000);
+	if (err)
+		dev_err(mmc_dev(host->mmc),
+			"HS400 delay line calibration timed out\n");
+}
+
 static void tegra_sdhci_set_uhs_signaling(struct sdhci_host *host,
 					  unsigned timing)
 {
@@ -591,6 +615,7 @@ static void tegra_sdhci_set_uhs_signaling(struct sdhci_host *host,
 	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
 	bool set_default_tap = false;
 	bool set_dqs_trim = false;
+	bool do_hs400_dll_cal = false;
 
 	switch (timing) {
 	case MMC_TIMING_UHS_SDR50:
@@ -600,6 +625,7 @@ static void tegra_sdhci_set_uhs_signaling(struct sdhci_host *host,
 		break;
 	case MMC_TIMING_MMC_HS400:
 		set_dqs_trim = true;
+		do_hs400_dll_cal = true;
 		break;
 	case MMC_TIMING_MMC_DDR52:
 	case MMC_TIMING_UHS_DDR50:
@@ -620,6 +646,9 @@ static void tegra_sdhci_set_uhs_signaling(struct sdhci_host *host,
 
 	if (set_dqs_trim)
 		tegra_sdhci_set_dqs_trim(host, tegra_host->dqs_trim);
+
+	if (do_hs400_dll_cal)
+		tegra_sdhci_hs400_dll_cal(host);
 }
 
 static int tegra_sdhci_execute_tuning(struct sdhci_host *host, u32 opcode)
-- 
2.7.4

[PATCH 5/8] arm64: dts: tegra186: Add SDMMC4 DQS trim value

[Aapo Vienamo]

Add the HS400 DQS trim value for Tegra186 SDMMC4.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 6e9ef26..9e07bc6 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -313,6 +313,7 @@
 		nvidia,pad-autocal-pull-down-offset-1v8-timeout = <0x0a>;
 		nvidia,default-tap = <0x5>;
 		nvidia,default-trim = <0x9>;
+		nvidia,dqs-trim = <63>;
 		status = "disabled";
 	};
 
-- 
2.7.4

[PATCH 6/8] arm64: dts: tegra210: Add SDMMC4 DQS trim value

[Aapo Vienamo]

Add the HS400 DQS trim value for Tegra210 SDMMC4.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index 14da98a..f8e5f09 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -1115,6 +1115,7 @@
 		assigned-clocks = <&tegra_car TEGRA210_CLK_SDMMC4>,
 				  <&tegra_car TEGRA210_CLK_PLL_C4_OUT0>;
 		assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_C4_OUT0>;
+		nvidia,dqs-trim = <40>;
 		status = "disabled";
 	};
 
-- 
2.7.4

[PATCH 7/8] arm64: dts: tegra186: Enable HS400

[Aapo Vienamo]

Enable HS400 signaling on Tegra186 SDMMC4 controller.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 9e07bc6..2f3c8e2 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -314,6 +314,7 @@
 		nvidia,default-tap = <0x5>;
 		nvidia,default-trim = <0x9>;
 		nvidia,dqs-trim = <63>;
+		mmc-hs400-1_8v;
 		status = "disabled";
 	};
 
-- 
2.7.4

[PATCH 8/8] arm64: dts: tegra210: Enable HS400

[Aapo Vienamo]

Enable HS400 signaling on Tegra210 SDMMC4 controller.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index f8e5f09..8fe47d6 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -1116,6 +1116,7 @@
 				  <&tegra_car TEGRA210_CLK_PLL_C4_OUT0>;
 		assigned-clock-parents = <&tegra_car TEGRA210_CLK_PLL_C4_OUT0>;
 		nvidia,dqs-trim = <40>;
+		mmc-hs400-1_8v;
 		status = "disabled";
 	};
 
-- 
2.7.4

Re: [PATCH 1/8] dt-bindings: mmc: Add DQS trim value to Tegra SDHCI

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

On Tue, Aug 07, 2018 at 04:59:57PM +0300, Aapo Vienamo wrote:
> Document HS400 DQS trim value device tree property.
> 
> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> ---
>  Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> index 3c7960a..7d294f3 100644
> --- a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> @@ -72,6 +72,7 @@ Optional properties for Tegra210 and Tegra186:
>    trimmer value for non-tunable modes.
>  - nvidia,default-trim : Specify the default outbound clock trimmer
>    value.
> +- nvidia,dqs-trim : Specify DQS trim value for HS400 timing
>  
>    Notes on the pad calibration pull up and pulldown offset values:
>      - The property values are drive codes which are programmed into the
> @@ -88,6 +89,8 @@ Optional properties for Tegra210 and Tegra186:
>      - The values are programmed to the Vendor Clock Control Register.
>        Please refer to the reference manual of the SoC for correct
>        values.
> +    - The DQS trim values are only used on controllers which support
> +      HS400 timing.

One of these additions says "DQS trim values", the other says "DQS trim
value". It is unclear from the above how many values there are. I think
this should be more explicit. Also, I don't see why the note about which
controllers the DQS trim value(s) applies to is in a separate paragraph.
Couldn't it be moved to the property description?

Also, I think the bindings should specify which generations of Tegra do
support HS400. Where else are people supposed to find that information?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/8] mmc: tegra: Parse and program DQS trim value

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]

On Tue, Aug 07, 2018 at 04:59:58PM +0300, Aapo Vienamo wrote:
> Parse and program the HS400 DQS trim value from dt. Program a fallback
> value in case the property is missing.
> 
> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 7f1ac4a..426f7ea 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -43,6 +43,10 @@
>  #define SDHCI_CLOCK_CTRL_PADPIPE_CLKEN_OVERRIDE		BIT(3)
>  #define SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE	BIT(2)
>  
> +#define SDHCI_TEGRA_VENDOR_CAP_OVERRIDES		0x10c
> +#define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_MASK		0x00003f00
> +#define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_SHIFT	8
> +
>  #define SDHCI_TEGRA_VENDOR_MISC_CTRL			0x120
>  #define SDHCI_MISC_CTRL_ENABLE_SDR104			0x8
>  #define SDHCI_MISC_CTRL_ENABLE_SDR50			0x10
> @@ -112,6 +116,7 @@ struct sdhci_tegra {
>  
>  	u32 default_tap;
>  	u32 default_trim;
> +	u32 dqs_trim;
>  };
>  
>  static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
> @@ -500,7 +505,7 @@ static void tegra_sdhci_parse_pad_autocal_dt(struct sdhci_host *host)
>  		autocal->pull_down_hs400 = autocal->pull_down_1v8;
>  }
>  
> -static void tegra_sdhci_parse_default_tap_and_trim(struct sdhci_host *host)
> +static void tegra_sdhci_parse_tap_and_trim(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
> @@ -515,6 +520,11 @@ static void tegra_sdhci_parse_default_tap_and_trim(struct sdhci_host *host)
>  				       &tegra_host->default_trim);
>  	if (err)
>  		tegra_host->default_trim = 0;
> +
> +	err = device_property_read_u32(host->mmc->parent, "nvidia,dqs-trim",
> +				       &tegra_host->dqs_trim);
> +	if (err)
> +		tegra_host->dqs_trim = 0x11;

Okay, so there's only one value. I think that should be clarified in the
bindings documentation. It should mention that a single cell is used for
this. Also, I assume there are lower and upper limits for the valid
range of DQS trim values. Might make sense to specify those in the DT
bindings as well.

>  }
>  
>  static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> @@ -545,20 +555,33 @@ static unsigned int tegra_sdhci_get_max_clock(struct sdhci_host *host)
>  	return clk_round_rate(pltfm_host->clk, UINT_MAX);
>  }
>  
> +static void tegra_sdhci_set_dqs_trim(struct sdhci_host *host, u8 val)
> +{
> +	u32 reg;
> +
> +	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_CAP_OVERRIDES);
> +	reg &= ~SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_MASK;
> +	reg |= val<<SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_SHIFT;
> +	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_CAP_OVERRIDES);
> +}

Nit: I dislike using "reg" as a variable representing a register value
because I keep interpreting it as designating a register offset. Hence
I tend to use more explicit "offset" for actual register offsets and
"value" for register values.

But maybe that's just me.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/8] mmc: tegra: Parse and program DQS trim value

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

Two more comments...

On Tue, Aug 07, 2018 at 04:59:58PM +0300, Aapo Vienamo wrote:
> Parse and program the HS400 DQS trim value from dt. Program a fallback
> value in case the property is missing.

"dt" -> "DT" because it is an abbreviation.

> 
> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
[...]
> @@ -545,20 +555,33 @@ static unsigned int tegra_sdhci_get_max_clock(struct sdhci_host *host)
>  	return clk_round_rate(pltfm_host->clk, UINT_MAX);
>  }
>  
> +static void tegra_sdhci_set_dqs_trim(struct sdhci_host *host, u8 val)
> +{
> +	u32 reg;
> +
> +	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_CAP_OVERRIDES);
> +	reg &= ~SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_MASK;
> +	reg |= val<<SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_SHIFT;

Also, you should add spaces around '<<'.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/8] mmc: tegra: Implement HS400 enhanced strobe

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]

On Tue, Aug 07, 2018 at 04:59:59PM +0300, Aapo Vienamo wrote:
> Implement HS400 enhanced strobe.

Can you provide a little more information about what the impact is of
this? Does this increase throughput? How much?

> 
> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 426f7ea..d81143b 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -43,6 +43,9 @@
>  #define SDHCI_CLOCK_CTRL_PADPIPE_CLKEN_OVERRIDE		BIT(3)
>  #define SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE	BIT(2)
>  
> +#define SDHCI_TEGRA_VENDOR_SYS_SW_CTRL			0x104
> +#define SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE		BIT(31)
> +
>  #define SDHCI_TEGRA_VENDOR_CAP_OVERRIDES		0x10c
>  #define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_MASK		0x00003f00
>  #define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_SHIFT	8
> @@ -271,6 +274,22 @@ static void tegra_sdhci_set_tap(struct sdhci_host *host, unsigned int tap)
>  	}
>  }
>  
> +static void tegra_sdhci_hs400_enhanced_strobe(struct mmc_host *mmc,
> +					      struct mmc_ios *ios)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	u32 reg;
> +
> +	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL);
> +	pr_err("enhanced strobe: %d\n", ios->enhanced_strobe);

Left-over debug error?

> +	if (ios->enhanced_strobe)
> +		reg |= SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE;
> +	else
> +		reg &= ~SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE;
> +	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL);

You might want to add blank lines around the if ... else ... block for
readability.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/8] dt-bindings: mmc: Add DQS trim value to Tegra SDHCI

[Aapo Vienamo]

On Thu, 9 Aug 2018 13:36:09 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Aug 07, 2018 at 04:59:57PM +0300, Aapo Vienamo wrote:
> > Document HS400 DQS trim value device tree property.
> > 
> > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> > ---
> >  Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > index 3c7960a..7d294f3 100644
> > --- a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > +++ b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > @@ -72,6 +72,7 @@ Optional properties for Tegra210 and Tegra186:
> >    trimmer value for non-tunable modes.
> >  - nvidia,default-trim : Specify the default outbound clock trimmer
> >    value.
> > +- nvidia,dqs-trim : Specify DQS trim value for HS400 timing
> >  
> >    Notes on the pad calibration pull up and pulldown offset values:
> >      - The property values are drive codes which are programmed into the
> > @@ -88,6 +89,8 @@ Optional properties for Tegra210 and Tegra186:
> >      - The values are programmed to the Vendor Clock Control Register.
> >        Please refer to the reference manual of the SoC for correct
> >        values.
> > +    - The DQS trim values are only used on controllers which support
> > +      HS400 timing.  
> 
> One of these additions says "DQS trim values", the other says "DQS trim
> value". It is unclear from the above how many values there are. I think
> this should be more explicit. Also, I don't see why the note about which
> controllers the DQS trim value(s) applies to is in a separate paragraph.
> Couldn't it be moved to the property description?

It's a single value. The plural form is a mistake.

> Also, I think the bindings should specify which generations of Tegra do
> support HS400. Where else are people supposed to find that information?

This property is under the "Optional properties for Tegra210 and
Tegra186" section and it only applies for the said generations.

 -Aapo

Re: [PATCH 4/8] mmc: tegra: Implement HS400 delay line calibration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]

On Tue, Aug 07, 2018 at 05:00:00PM +0300, Aapo Vienamo wrote:
> Implement HS400 specific delay line calibration procedure.
> 
> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

Should this be before the previous patch in order to make sure the
calibration is performed as soon as the feature is available. This is
counting beans I guess, but it is technically possible for someone to
get everything up to patch 3/8 and then get the corresponding changes
in the DTS files to enable the mode and then have HS400 enabled without
this calibration.

Thierry

> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index d81143b..d0b68b7 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -56,6 +56,12 @@
>  #define SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300		0x20
>  #define SDHCI_MISC_CTRL_ENABLE_DDR50			0x200
>  
> +#define SDHCI_TEGRA_VENDOR_DLLCAL_CFG			0x1b0
> +#define SDHCI_TEGRA_DLLCAL_CALIBRATE			BIT(31)
> +
> +#define SDHCI_TEGRA_VENDOR_DLLCAL_STA			0x1bc
> +#define SDHCI_TEGRA_DLLCAL_STA_ACTIVE			BIT(31)
> +
>  #define SDHCI_VNDR_TUN_CTRL0_0				0x1c0
>  #define SDHCI_VNDR_TUN_CTRL0_TUN_HW_TAP			0x20000
>  
> @@ -584,6 +590,24 @@ static void tegra_sdhci_set_dqs_trim(struct sdhci_host *host, u8 val)
>  	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_CAP_OVERRIDES);
>  }
>  
> +static void tegra_sdhci_hs400_dll_cal(struct sdhci_host *host)
> +{
> +	u32 reg;
> +	int err;
> +
> +	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_DLLCAL_CFG);
> +	reg |= SDHCI_TEGRA_DLLCAL_CALIBRATE;
> +	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_DLLCAL_CFG);

Is this self-clearing? Or do we need to clear it manually in order for
a subsequent calibration procedure to succeed?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/8] arm64: dts: tegra186: Add SDMMC4 DQS trim value

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

On Tue, Aug 07, 2018 at 05:00:01PM +0300, Aapo Vienamo wrote:
> Add the HS400 DQS trim value for Tegra186 SDMMC4.
> 
> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> index 6e9ef26..9e07bc6 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> @@ -313,6 +313,7 @@
>  		nvidia,pad-autocal-pull-down-offset-1v8-timeout = <0x0a>;
>  		nvidia,default-tap = <0x5>;
>  		nvidia,default-trim = <0x9>;
> +		nvidia,dqs-trim = <63>;
>  		status = "disabled";
>  	};
>  

Isn't this technically dependent on the board layout and as such would
belong in the board DTS file? Or does this value work on all existing
Tegra186 platforms?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/8] arm64: dts: tegra186: Add SDMMC4 DQS trim value

[Aapo Vienamo]

On Thu, 9 Aug 2018 13:49:22 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Aug 07, 2018 at 05:00:01PM +0300, Aapo Vienamo wrote:
> > Add the HS400 DQS trim value for Tegra186 SDMMC4.
> > 
> > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> > ---
> >  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > index 6e9ef26..9e07bc6 100644
> > --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > @@ -313,6 +313,7 @@
> >  		nvidia,pad-autocal-pull-down-offset-1v8-timeout = <0x0a>;
> >  		nvidia,default-tap = <0x5>;
> >  		nvidia,default-trim = <0x9>;
> > +		nvidia,dqs-trim = <63>;
> >  		status = "disabled";
> >  	};
> >    
> 
> Isn't this technically dependent on the board layout and as such would
> belong in the board DTS file? Or does this value work on all existing
> Tegra186 platforms?

This value is specified as part of the controller initialization
sequence in the TRM. I've understood that this (and other tap and trim)
value(s) are used for compensating the propagation delay differences
that are caused by the internal SoC layout.

 -Aapo

Re: [PATCH 3/8] mmc: tegra: Implement HS400 enhanced strobe

[Aapo Vienamo]

On Thu, 9 Aug 2018 13:43:45 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Aug 07, 2018 at 04:59:59PM +0300, Aapo Vienamo wrote:
> > Implement HS400 enhanced strobe.  
> 
> Can you provide a little more information about what the impact is of
> this? Does this increase throughput? How much?

The eMMC enhanced strobe is a mechanism that can be used instead of the
HS400 tuning procedure. Note that the delay line calibration implemented
later on in this series is Tegra specific and has to be performed
regardless of which type of HS400 tuning mechanism is used.

> > 
> > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> > ---
> >  drivers/mmc/host/sdhci-tegra.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> > index 426f7ea..d81143b 100644
> > --- a/drivers/mmc/host/sdhci-tegra.c
> > +++ b/drivers/mmc/host/sdhci-tegra.c
> > @@ -43,6 +43,9 @@
> >  #define SDHCI_CLOCK_CTRL_PADPIPE_CLKEN_OVERRIDE		BIT(3)
> >  #define SDHCI_CLOCK_CTRL_SPI_MODE_CLKEN_OVERRIDE	BIT(2)
> >  
> > +#define SDHCI_TEGRA_VENDOR_SYS_SW_CTRL			0x104
> > +#define SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE		BIT(31)
> > +
> >  #define SDHCI_TEGRA_VENDOR_CAP_OVERRIDES		0x10c
> >  #define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_MASK		0x00003f00
> >  #define SDHCI_TEGRA_CAP_OVERRIDES_DQS_TRIM_SHIFT	8
> > @@ -271,6 +274,22 @@ static void tegra_sdhci_set_tap(struct sdhci_host *host, unsigned int tap)
> >  	}
> >  }
> >  
> > +static void tegra_sdhci_hs400_enhanced_strobe(struct mmc_host *mmc,
> > +					      struct mmc_ios *ios)
> > +{
> > +	struct sdhci_host *host = mmc_priv(mmc);
> > +	u32 reg;
> > +
> > +	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL);
> > +	pr_err("enhanced strobe: %d\n", ios->enhanced_strobe);  
> 
> Left-over debug error?

Yep.

> 
> > +	if (ios->enhanced_strobe)
> > +		reg |= SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE;
> > +	else
> > +		reg &= ~SDHCI_TEGRA_SYS_SW_CTRL_ENHANCED_STROBE;
> > +	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_SYS_SW_CTRL);  
> 
> You might want to add blank lines around the if ... else ... block for
> readability.

True.

 -Aapo

Re: [PATCH 5/8] arm64: dts: tegra186: Add SDMMC4 DQS trim value

[Peter Geis]

On 08/09/2018 08:02 AM, Aapo Vienamo wrote:
> On Thu, 9 Aug 2018 13:49:22 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
>> On Tue, Aug 07, 2018 at 05:00:01PM +0300, Aapo Vienamo wrote:
>>> Add the HS400 DQS trim value for Tegra186 SDMMC4.
>>>
>>> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
>>> ---
>>>   arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>> index 6e9ef26..9e07bc6 100644
>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>> @@ -313,6 +313,7 @@
>>>   		nvidia,pad-autocal-pull-down-offset-1v8-timeout = <0x0a>;
>>>   		nvidia,default-tap = <0x5>;
>>>   		nvidia,default-trim = <0x9>;
>>> +		nvidia,dqs-trim = <63>;
>>>   		status = "disabled";
>>>   	};
>>>     
>>
>> Isn't this technically dependent on the board layout and as such would
>> belong in the board DTS file? Or does this value work on all existing
>> Tegra186 platforms?
> 
> This value is specified as part of the controller initialization
> sequence in the TRM. I've understood that this (and other tap and trim)
> value(s) are used for compensating the propagation delay differences
> that are caused by the internal SoC layout.
> 
>   -Aapo
> --

The Tegra2 and Tegra3 TRMs also specify recommended DQS values, and I am 
working on at least one device that differs in the platform data from 
the default value.
I see that you mentioned this is for the newer devices that support 
HS200/HS400 modes, but does it enable setting DQS on older devices?

Re: [PATCH 4/8] mmc: tegra: Implement HS400 delay line calibration

[Aapo Vienamo]

On Thu, 9 Aug 2018 13:48:05 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Aug 07, 2018 at 05:00:00PM +0300, Aapo Vienamo wrote:
> > Implement HS400 specific delay line calibration procedure.
> > 
> > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> > ---
> >  drivers/mmc/host/sdhci-tegra.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)  
> 
> Should this be before the previous patch in order to make sure the
> calibration is performed as soon as the feature is available. This is
> counting beans I guess, but it is technically possible for someone to
> get everything up to patch 3/8 and then get the corresponding changes
> in the DTS files to enable the mode and then have HS400 enabled without
> this calibration.

True.

> 
> > 
> > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> > index d81143b..d0b68b7 100644
> > --- a/drivers/mmc/host/sdhci-tegra.c
> > +++ b/drivers/mmc/host/sdhci-tegra.c
> > @@ -56,6 +56,12 @@
> >  #define SDHCI_MISC_CTRL_ENABLE_SDHCI_SPEC_300		0x20
> >  #define SDHCI_MISC_CTRL_ENABLE_DDR50			0x200
> >  
> > +#define SDHCI_TEGRA_VENDOR_DLLCAL_CFG			0x1b0
> > +#define SDHCI_TEGRA_DLLCAL_CALIBRATE			BIT(31)
> > +
> > +#define SDHCI_TEGRA_VENDOR_DLLCAL_STA			0x1bc
> > +#define SDHCI_TEGRA_DLLCAL_STA_ACTIVE			BIT(31)
> > +
> >  #define SDHCI_VNDR_TUN_CTRL0_0				0x1c0
> >  #define SDHCI_VNDR_TUN_CTRL0_TUN_HW_TAP			0x20000
> >  
> > @@ -584,6 +590,24 @@ static void tegra_sdhci_set_dqs_trim(struct sdhci_host *host, u8 val)
> >  	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_CAP_OVERRIDES);
> >  }
> >  
> > +static void tegra_sdhci_hs400_dll_cal(struct sdhci_host *host)
> > +{
> > +	u32 reg;
> > +	int err;
> > +
> > +	reg = sdhci_readl(host, SDHCI_TEGRA_VENDOR_DLLCAL_CFG);
> > +	reg |= SDHCI_TEGRA_DLLCAL_CALIBRATE;
> > +	sdhci_writel(host, reg, SDHCI_TEGRA_VENDOR_DLLCAL_CFG);  
> 
> Is this self-clearing? Or do we need to clear it manually in order for
> a subsequent calibration procedure to succeed?

Yes, the TRM states that this bit should not be cleared by software.

 -Aapo

Re: [PATCH 5/8] arm64: dts: tegra186: Add SDMMC4 DQS trim value

[Aapo Vienamo]

On Thu, 9 Aug 2018 08:23:16 -0400
Peter Geis <pgwipeout@gmail.com> wrote:

> On 08/09/2018 08:02 AM, Aapo Vienamo wrote:
> > On Thu, 9 Aug 2018 13:49:22 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >   
> >> On Tue, Aug 07, 2018 at 05:00:01PM +0300, Aapo Vienamo wrote:  
> >>> Add the HS400 DQS trim value for Tegra186 SDMMC4.
> >>>
> >>> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> >>> ---
> >>>   arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>> index 6e9ef26..9e07bc6 100644
> >>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>> @@ -313,6 +313,7 @@
> >>>   		nvidia,pad-autocal-pull-down-offset-1v8-timeout = <0x0a>;
> >>>   		nvidia,default-tap = <0x5>;
> >>>   		nvidia,default-trim = <0x9>;
> >>> +		nvidia,dqs-trim = <63>;
> >>>   		status = "disabled";
> >>>   	};
> >>>       
> >>
> >> Isn't this technically dependent on the board layout and as such would
> >> belong in the board DTS file? Or does this value work on all existing
> >> Tegra186 platforms?  
> > 
> > This value is specified as part of the controller initialization
> > sequence in the TRM. I've understood that this (and other tap and trim)
> > value(s) are used for compensating the propagation delay differences
> > that are caused by the internal SoC layout.
> > 
> >   -Aapo
> > --  
> 
> The Tegra2 and Tegra3 TRMs also specify recommended DQS values, and I am 
> working on at least one device that differs in the platform data from 
> the default value.
> I see that you mentioned this is for the newer devices that support 
> HS200/HS400 modes, but does it enable setting DQS on older devices?

I can't find any mention of _SDMMC_ DQS trimmer on Tegra2, Tegra3 or
Tegra124 TRMs. As far as I can tell, programming the DQS trimmer value
is only required by HS400 signaling on Tegra210 and Tegra186.

 -Aapo

Re: [PATCH 5/8] arm64: dts: tegra186: Add SDMMC4 DQS trim value

[Peter Geis]

On 08/09/2018 08:37 AM, Aapo Vienamo wrote:
> On Thu, 9 Aug 2018 08:23:16 -0400
> Peter Geis <pgwipeout@gmail.com> wrote:
> 
>> On 08/09/2018 08:02 AM, Aapo Vienamo wrote:
>>> On Thu, 9 Aug 2018 13:49:22 +0200
>>> Thierry Reding <thierry.reding@gmail.com> wrote:
>>>    
>>>> On Tue, Aug 07, 2018 at 05:00:01PM +0300, Aapo Vienamo wrote:
>>>>> Add the HS400 DQS trim value for Tegra186 SDMMC4.
>>>>>
>>>>> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> index 6e9ef26..9e07bc6 100644
>>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> @@ -313,6 +313,7 @@
>>>>>    		nvidia,pad-autocal-pull-down-offset-1v8-timeout = <0x0a>;
>>>>>    		nvidia,default-tap = <0x5>;
>>>>>    		nvidia,default-trim = <0x9>;
>>>>> +		nvidia,dqs-trim = <63>;
>>>>>    		status = "disabled";
>>>>>    	};
>>>>>        
>>>>
>>>> Isn't this technically dependent on the board layout and as such would
>>>> belong in the board DTS file? Or does this value work on all existing
>>>> Tegra186 platforms?
>>>
>>> This value is specified as part of the controller initialization
>>> sequence in the TRM. I've understood that this (and other tap and trim)
>>> value(s) are used for compensating the propagation delay differences
>>> that are caused by the internal SoC layout.
>>>
>>>    -Aapo
>>> --
>>
>> The Tegra2 and Tegra3 TRMs also specify recommended DQS values, and I am
>> working on at least one device that differs in the platform data from
>> the default value.
>> I see that you mentioned this is for the newer devices that support
>> HS200/HS400 modes, but does it enable setting DQS on older devices?
> 
> I can't find any mention of _SDMMC_ DQS trimmer on Tegra2, Tegra3 or
> Tegra124 TRMs. As far as I can tell, programming the DQS trimmer value
> is only required by HS400 signaling on Tegra210 and Tegra186.
> 
>   -Aapo
> 
Apologies, I was referring to the default Tap/Trim values.
DQS found it's way into my brain from the memory controller.

Re: [PATCH 1/8] dt-bindings: mmc: Add DQS trim value to Tegra SDHCI

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2550 bytes --]

On Thu, Aug 09, 2018 at 02:45:15PM +0300, Aapo Vienamo wrote:
> On Thu, 9 Aug 2018 13:36:09 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Tue, Aug 07, 2018 at 04:59:57PM +0300, Aapo Vienamo wrote:
> > > Document HS400 DQS trim value device tree property.
> > > 
> > > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> > > ---
> > >  Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > index 3c7960a..7d294f3 100644
> > > --- a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > +++ b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > @@ -72,6 +72,7 @@ Optional properties for Tegra210 and Tegra186:
> > >    trimmer value for non-tunable modes.
> > >  - nvidia,default-trim : Specify the default outbound clock trimmer
> > >    value.
> > > +- nvidia,dqs-trim : Specify DQS trim value for HS400 timing
> > >  
> > >    Notes on the pad calibration pull up and pulldown offset values:
> > >      - The property values are drive codes which are programmed into the
> > > @@ -88,6 +89,8 @@ Optional properties for Tegra210 and Tegra186:
> > >      - The values are programmed to the Vendor Clock Control Register.
> > >        Please refer to the reference manual of the SoC for correct
> > >        values.
> > > +    - The DQS trim values are only used on controllers which support
> > > +      HS400 timing.  
> > 
> > One of these additions says "DQS trim values", the other says "DQS trim
> > value". It is unclear from the above how many values there are. I think
> > this should be more explicit. Also, I don't see why the note about which
> > controllers the DQS trim value(s) applies to is in a separate paragraph.
> > Couldn't it be moved to the property description?
> 
> It's a single value. The plural form is a mistake.
> 
> > Also, I think the bindings should specify which generations of Tegra do
> > support HS400. Where else are people supposed to find that information?
> 
> This property is under the "Optional properties for Tegra210 and
> Tegra186" section and it only applies for the said generations.

What's the point of specifying that they are only used on controllers
which support HS400? Are you saying that only a subset of the SDHCI
controllers on Tegra210 and Tegra186 support HS400?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/8] mmc: tegra: Implement HS400 enhanced strobe

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Thu, Aug 09, 2018 at 03:22:54PM +0300, Aapo Vienamo wrote:
> On Thu, 9 Aug 2018 13:43:45 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Tue, Aug 07, 2018 at 04:59:59PM +0300, Aapo Vienamo wrote:
> > > Implement HS400 enhanced strobe.  
> > 
> > Can you provide a little more information about what the impact is of
> > this? Does this increase throughput? How much?
> 
> The eMMC enhanced strobe is a mechanism that can be used instead of the
> HS400 tuning procedure. Note that the delay line calibration implemented
> later on in this series is Tegra specific and has to be performed
> regardless of which type of HS400 tuning mechanism is used.

Sounds like a variation of that would be good material for the commit
message.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/8] arm64: dts: tegra186: Add SDMMC4 DQS trim value

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]

On Thu, Aug 09, 2018 at 03:02:26PM +0300, Aapo Vienamo wrote:
> On Thu, 9 Aug 2018 13:49:22 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Tue, Aug 07, 2018 at 05:00:01PM +0300, Aapo Vienamo wrote:
> > > Add the HS400 DQS trim value for Tegra186 SDMMC4.
> > > 
> > > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> > > ---
> > >  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > index 6e9ef26..9e07bc6 100644
> > > --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > @@ -313,6 +313,7 @@
> > >  		nvidia,pad-autocal-pull-down-offset-1v8-timeout = <0x0a>;
> > >  		nvidia,default-tap = <0x5>;
> > >  		nvidia,default-trim = <0x9>;
> > > +		nvidia,dqs-trim = <63>;
> > >  		status = "disabled";
> > >  	};
> > >    
> > 
> > Isn't this technically dependent on the board layout and as such would
> > belong in the board DTS file? Or does this value work on all existing
> > Tegra186 platforms?
> 
> This value is specified as part of the controller initialization
> sequence in the TRM. I've understood that this (and other tap and trim)
> value(s) are used for compensating the propagation delay differences
> that are caused by the internal SoC layout.

Hmm... it would seem to me like the routing on a board would have a more
significant impact than the SoC internal routing. But perhaps the board-
specific routing is actually what the automatic calibration is used for?

Anyway, if it ever turns out that we need slightly different values on a
given board, we can always override the value in the board DTS file.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/8] dt-bindings: mmc: Add DQS trim value to Tegra SDHCI

[Aapo Vienamo]

On Thu, 9 Aug 2018 15:46:48 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Thu, Aug 09, 2018 at 02:45:15PM +0300, Aapo Vienamo wrote:
> > On Thu, 9 Aug 2018 13:36:09 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >   
> > > On Tue, Aug 07, 2018 at 04:59:57PM +0300, Aapo Vienamo wrote:  
> > > > Document HS400 DQS trim value device tree property.
> > > > 
> > > > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > > index 3c7960a..7d294f3 100644
> > > > --- a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > > +++ b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > > @@ -72,6 +72,7 @@ Optional properties for Tegra210 and Tegra186:
> > > >    trimmer value for non-tunable modes.
> > > >  - nvidia,default-trim : Specify the default outbound clock trimmer
> > > >    value.
> > > > +- nvidia,dqs-trim : Specify DQS trim value for HS400 timing
> > > >  
> > > >    Notes on the pad calibration pull up and pulldown offset values:
> > > >      - The property values are drive codes which are programmed into the
> > > > @@ -88,6 +89,8 @@ Optional properties for Tegra210 and Tegra186:
> > > >      - The values are programmed to the Vendor Clock Control Register.
> > > >        Please refer to the reference manual of the SoC for correct
> > > >        values.
> > > > +    - The DQS trim values are only used on controllers which support
> > > > +      HS400 timing.    
> > > 
> > > One of these additions says "DQS trim values", the other says "DQS trim
> > > value". It is unclear from the above how many values there are. I think
> > > this should be more explicit. Also, I don't see why the note about which
> > > controllers the DQS trim value(s) applies to is in a separate paragraph.
> > > Couldn't it be moved to the property description?  
> > 
> > It's a single value. The plural form is a mistake.
> >   
> > > Also, I think the bindings should specify which generations of Tegra do
> > > support HS400. Where else are people supposed to find that information?  
> > 
> > This property is under the "Optional properties for Tegra210 and
> > Tegra186" section and it only applies for the said generations.  
> 
> What's the point of specifying that they are only used on controllers
> which support HS400? Are you saying that only a subset of the SDHCI
> controllers on Tegra210 and Tegra186 support HS400?

Yes, on Tegra210 and Tegra186 only SDMMC4 supports HS400.

 -Aapo

Re: [PATCH 1/8] dt-bindings: mmc: Add DQS trim value to Tegra SDHCI

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3060 bytes --]

On Thu, Aug 09, 2018 at 05:06:04PM +0300, Aapo Vienamo wrote:
> On Thu, 9 Aug 2018 15:46:48 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Thu, Aug 09, 2018 at 02:45:15PM +0300, Aapo Vienamo wrote:
> > > On Thu, 9 Aug 2018 13:36:09 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >   
> > > > On Tue, Aug 07, 2018 at 04:59:57PM +0300, Aapo Vienamo wrote:  
> > > > > Document HS400 DQS trim value device tree property.
> > > > > 
> > > > > Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > > > index 3c7960a..7d294f3 100644
> > > > > --- a/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > > > +++ b/Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt
> > > > > @@ -72,6 +72,7 @@ Optional properties for Tegra210 and Tegra186:
> > > > >    trimmer value for non-tunable modes.
> > > > >  - nvidia,default-trim : Specify the default outbound clock trimmer
> > > > >    value.
> > > > > +- nvidia,dqs-trim : Specify DQS trim value for HS400 timing
> > > > >  
> > > > >    Notes on the pad calibration pull up and pulldown offset values:
> > > > >      - The property values are drive codes which are programmed into the
> > > > > @@ -88,6 +89,8 @@ Optional properties for Tegra210 and Tegra186:
> > > > >      - The values are programmed to the Vendor Clock Control Register.
> > > > >        Please refer to the reference manual of the SoC for correct
> > > > >        values.
> > > > > +    - The DQS trim values are only used on controllers which support
> > > > > +      HS400 timing.    
> > > > 
> > > > One of these additions says "DQS trim values", the other says "DQS trim
> > > > value". It is unclear from the above how many values there are. I think
> > > > this should be more explicit. Also, I don't see why the note about which
> > > > controllers the DQS trim value(s) applies to is in a separate paragraph.
> > > > Couldn't it be moved to the property description?  
> > > 
> > > It's a single value. The plural form is a mistake.
> > >   
> > > > Also, I think the bindings should specify which generations of Tegra do
> > > > support HS400. Where else are people supposed to find that information?  
> > > 
> > > This property is under the "Optional properties for Tegra210 and
> > > Tegra186" section and it only applies for the said generations.  
> > 
> > What's the point of specifying that they are only used on controllers
> > which support HS400? Are you saying that only a subset of the SDHCI
> > controllers on Tegra210 and Tegra186 support HS400?
> 
> Yes, on Tegra210 and Tegra186 only SDMMC4 supports HS400.

Good. I think that'd be good to have as part of the DT bindings
documentation.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]