[PATCH 0/8] mmc: sdhci-esdhc-imx: add SD3.0 support

[PATCH 0/8] mmc: sdhci-esdhc-imx: add SD3.0 support

[Dong Aisheng]

This patch series add SD3.0 support for i.MX6Q/DL.
Since freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
the standard tuning process defined in host controller spec v3.0.
So we add a hook to allow execute platform specific tuning instead of
standard host controller tuning.

The main difference are:
1) not only generate Buffer Read Ready interrupt when tuning is performing.
It generates all other DATA interrupts like the normal data command.
2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
instead it's controlled by SW.
3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
it's controlled by SW.
4) the clock delay for every tuning is set by SW.

Tested on i.MX6Q Sabreauto board.

The series is based on latest Linus tree.

Dong Aisheng (8):
  mmc: sdhci: add hooks for platform specific tuning
  mmc: sdhci: allow platform access of sdhci_send_command
  sdhci: sdhci-esdhc-imx: support real clock on and off for imx6q
  sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support
  sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode
  mmc: sdhci-esdhc: correct pre_div for imx6q
  mmc: sdhci-esdhc: set actual_clock in clock setting
  ARM: dts: imx6qdl: add uhs pinctrl state for usdhc3

 arch/arm/boot/dts/imx6dl.dtsi               |   33 +++
 arch/arm/boot/dts/imx6q.dtsi                |   33 +++
 arch/arm/boot/dts/imx6qdl-sabreauto.dtsi    |    4 +-
 drivers/mmc/host/sdhci-esdhc-imx.c          |  307 ++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci-esdhc.h              |   35 +++-
 drivers/mmc/host/sdhci.c                    |   12 +-
 drivers/mmc/host/sdhci.h                    |    3 +
 include/linux/platform_data/mmc-esdhc-imx.h |    4 +
 8 files changed, 419 insertions(+), 12 deletions(-)

[PATCH 1/8] mmc: sdhci: add hooks for platform specific tuning

[Dong Aisheng]

The tuning of some platforms may not follow the standard host control
spec v3.0, e.g. Freescale uSDHC on i.MX6Q/DL.
Add a hook here to allow execute platform specific tuning instead of
standard host controller tuning.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci.c |    8 ++++++++
 drivers/mmc/host/sdhci.h |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index dd2c083..2890cfd 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1875,6 +1875,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 		return 0;
 	}
 
+	if (host->ops->platform_execute_tuning) {
+		spin_unlock(&host->lock);
+		enable_irq(host->irq);
+		err = host->ops->platform_execute_tuning(host, opcode);
+		sdhci_runtime_pm_put(host);
+		return err;
+	}
+
 	sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 	/*
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index b037f18..976c14b 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -288,6 +288,7 @@ struct sdhci_ops {
 	unsigned int    (*get_ro)(struct sdhci_host *host);
 	void	(*platform_reset_enter)(struct sdhci_host *host, u8 mask);
 	void	(*platform_reset_exit)(struct sdhci_host *host, u8 mask);
+	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
 	int	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
 	void	(*hw_reset)(struct sdhci_host *host);
 	void	(*platform_suspend)(struct sdhci_host *host);
-- 
1.7.1

Re: [PATCH 1/8] mmc: sdhci: add hooks for platform specific tuning

[Shawn Guo]

On Wed, Sep 04, 2013 at 08:54:10PM +0800, Dong Aisheng wrote:
> The tuning of some platforms may not follow the standard host control
> spec v3.0, e.g. Freescale uSDHC on i.MX6Q/DL.
> Add a hook here to allow execute platform specific tuning instead of
> standard host controller tuning.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci.c |    8 ++++++++
>  drivers/mmc/host/sdhci.h |    1 +
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index dd2c083..2890cfd 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1875,6 +1875,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  		return 0;
>  	}
>  
> +	if (host->ops->platform_execute_tuning) {
> +		spin_unlock(&host->lock);
> +		enable_irq(host->irq);

Hmm, you want these two functions be called before
platform_execute_tuning()?  That probably means you do not need to call
spin_lock() and disable_irq() for platform_execute_tuning()?  In that
case, can we not just put this platform hook at the beginning of the
function, something like below?

	host = mmc_priv(mmc);

	if (host->ops->platform_execute_tuning) {
		sdhci_runtime_pm_get(host);
		err = host->ops->platform_execute_tuning(host, opcode);
		sdhci_runtime_pm_put(host);
	}

I guess that's more clear to tell that platform_execute_tuning hook is
there to replace sdhci_execute_tuning() completely.  Is it the case?

Shawn

> +		err = host->ops->platform_execute_tuning(host, opcode);
> +		sdhci_runtime_pm_put(host);
> +		return err;
> +	}
> +
>  	sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>  
>  	/*
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index b037f18..976c14b 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -288,6 +288,7 @@ struct sdhci_ops {
>  	unsigned int    (*get_ro)(struct sdhci_host *host);
>  	void	(*platform_reset_enter)(struct sdhci_host *host, u8 mask);
>  	void	(*platform_reset_exit)(struct sdhci_host *host, u8 mask);
> +	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
>  	int	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
>  	void	(*hw_reset)(struct sdhci_host *host);
>  	void	(*platform_suspend)(struct sdhci_host *host);
> -- 
> 1.7.1
> 
> 

Re: [PATCH 1/8] mmc: sdhci: add hooks for platform specific tuning

[Dong Aisheng]

On Thu, Sep 5, 2013 at 11:14 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Sep 04, 2013 at 08:54:10PM +0800, Dong Aisheng wrote:
>> The tuning of some platforms may not follow the standard host control
>> spec v3.0, e.g. Freescale uSDHC on i.MX6Q/DL.
>> Add a hook here to allow execute platform specific tuning instead of
>> standard host controller tuning.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/mmc/host/sdhci.c |    8 ++++++++
>>  drivers/mmc/host/sdhci.h |    1 +
>>  2 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index dd2c083..2890cfd 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1875,6 +1875,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>               return 0;
>>       }
>>
>> +     if (host->ops->platform_execute_tuning) {
>> +             spin_unlock(&host->lock);
>> +             enable_irq(host->irq);
>
> Hmm, you want these two functions be called before
> platform_execute_tuning()?  That probably means you do not need to call
> spin_lock() and disable_irq() for platform_execute_tuning()?  In that

Platform_execute_tuning may send commands and can not execute with
lock all the time.
Since the lock is acquired in upper layer(sdhci_execute_tuning), so we
just release it at the same layer for more clear code.
In platform_execute_tuning, it may still need acquire lock according
to specific implemenation if it wants to access host.
However, not need to handle sdhci_runtime_pm_get/put things since it's
executed with
runtime_pm_get already.
I could add more description in commit message about this API definition.

> case, can we not just put this platform hook at the beginning of the
> function, something like below?
>
>         host = mmc_priv(mmc);
>
>         if (host->ops->platform_execute_tuning) {
>                 sdhci_runtime_pm_get(host);
>                 err = host->ops->platform_execute_tuning(host, opcode);
>                 sdhci_runtime_pm_put(host);
>         }
>
> I guess that's more clear to tell that platform_execute_tuning hook is
> there to replace sdhci_execute_tuning() completely.  Is it the case?
>

The sdhci_execute_tuning includes two parts: checking whehter need tuning
and the tuning execution process.
The first part is common for all other platforms.
So i just put the platform tuning under it, only replace later tuning
execution process.

Regards
Dong Aisheng

>
>> +             err = host->ops->platform_execute_tuning(host, opcode);
>> +             sdhci_runtime_pm_put(host);
>> +             return err;
>> +     }
>> +
>>       sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>
>>       /*
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index b037f18..976c14b 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -288,6 +288,7 @@ struct sdhci_ops {
>>       unsigned int    (*get_ro)(struct sdhci_host *host);
>>       void    (*platform_reset_enter)(struct sdhci_host *host, u8 mask);
>>       void    (*platform_reset_exit)(struct sdhci_host *host, u8 mask);
>> +     int     (*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
>>       int     (*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
>>       void    (*hw_reset)(struct sdhci_host *host);
>>       void    (*platform_suspend)(struct sdhci_host *host);
>> --
>> 1.7.1
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] mmc: sdhci: allow platform access of sdhci_send_command

[Dong Aisheng]

It helps for platform code to use it send tuning commands.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci.c |    4 ++--
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2890cfd..ae8200a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -49,7 +49,6 @@ static unsigned int debug_quirks2;
 
 static void sdhci_finish_data(struct sdhci_host *);
 
-static void sdhci_send_command(struct sdhci_host *, struct mmc_command *);
 static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
 static void sdhci_tuning_timer(unsigned long data);
@@ -981,7 +980,7 @@ static void sdhci_finish_data(struct sdhci_host *host)
 		tasklet_schedule(&host->finish_tasklet);
 }
 
-static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
+void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	int flags;
 	u32 mask;
@@ -1053,6 +1052,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 
 	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
+EXPORT_SYMBOL_GPL(sdhci_send_command);
 
 static void sdhci_finish_command(struct sdhci_host *host)
 {
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 976c14b..0a3ed01 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -394,6 +394,8 @@ static inline void *sdhci_priv(struct sdhci_host *host)
 extern void sdhci_card_detect(struct sdhci_host *host);
 extern int sdhci_add_host(struct sdhci_host *host);
 extern void sdhci_remove_host(struct sdhci_host *host, int dead);
+extern void sdhci_send_command(struct sdhci_host *host,
+				struct mmc_command *cmd);
 
 #ifdef CONFIG_PM
 extern int sdhci_suspend_host(struct sdhci_host *host);
-- 
1.7.1

[PATCH 3/8] sdhci: sdhci-esdhc-imx: support real clock on and off for imx6q

[Dong Aisheng]

The signal voltage switch follow requires to shutdown and output
clock in a specific sequence according to standard host controller
v3.0 spec. In that timing, the card must really receive clock or not.

However, for i.MX6Q, the uSDHC will not output clock even the clock
is enabled until there is command or data in transfer on the bus,
which will then cause singal voltage switch always to fail.

For i.MX6Q, we clear ESDHC_VENDOR_SPEC_FRC_SDCLK_ON bit to let
controller to gate off clock automatically and set that bit
to force clock output if clock is on.

This is required by SD3.0 support.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |    3 ---
 drivers/mmc/host/sdhci-esdhc.h     |   29 ++++++++++++++++++++++++++---
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 1dd5ba8..3118a82 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -31,9 +31,6 @@
 #include "sdhci-esdhc.h"
 
 #define	ESDHC_CTRL_D3CD			0x08
-/* VENDOR SPEC register */
-#define ESDHC_VENDOR_SPEC		0xc0
-#define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
 #define ESDHC_WTMK_LVL			0x44
 #define ESDHC_MIX_CTRL			0x48
 #define  ESDHC_MIX_CTRL_AC23EN		(1 << 7)
diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index a2a0642..86fcd5b 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -14,6 +14,8 @@
 #ifndef _DRIVERS_MMC_SDHCI_ESDHC_H
 #define _DRIVERS_MMC_SDHCI_ESDHC_H
 
+#include <linux/of.h>
+
 /*
  * Ops and quirks for the Freescale eSDHC controller.
  */
@@ -33,6 +35,12 @@
 #define ESDHC_CLOCK_HCKEN	0x00000002
 #define ESDHC_CLOCK_IPGEN	0x00000001
 
+/* VENDOR SPEC register */
+#define ESDHC_VENDOR_SPEC		0xc0
+#define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
+#define  ESDHC_VENDOR_SPEC_VSELECT	(1 << 1)
+#define  ESDHC_VENDOR_SPEC_FRC_SDCLK_ON (1 << 8)
+
 /* pltfm-specific */
 #define ESDHC_HOST_CONTROL_LE	0x20
 
@@ -52,12 +60,20 @@
 static inline void esdhc_set_clock(struct sdhci_host *host, unsigned int clock,
 				   unsigned int host_clock)
 {
+	struct device *dev;
 	int pre_div = 2;
 	int div = 1;
-	u32 temp;
-
-	if (clock == 0)
+	u32 temp, val;
+
+	dev = mmc_dev(host->mmc);
+	if (clock == 0) {
+		if (of_device_is_compatible(dev->of_node, "fsl,imx6q-usdhc")) {
+			val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
+			writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
+				host->ioaddr + ESDHC_VENDOR_SPEC);
+		}
 		goto out;
+	}
 
 	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
 	temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
@@ -81,6 +97,13 @@ static inline void esdhc_set_clock(struct sdhci_host *host, unsigned int clock,
 		| (div << ESDHC_DIVIDER_SHIFT)
 		| (pre_div << ESDHC_PREDIV_SHIFT));
 	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
+
+	if (of_device_is_compatible(dev->of_node, "fsl,imx6q-usdhc")) {
+		val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
+		writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
+			host->ioaddr + ESDHC_VENDOR_SPEC);
+	}
+
 	mdelay(1);
 out:
 	host->clock = clock;
-- 
1.7.1

Re: [PATCH 3/8] sdhci: sdhci-esdhc-imx: support real clock on and off for imx6q

[Shawn Guo]

On Wed, Sep 04, 2013 at 08:54:12PM +0800, Dong Aisheng wrote:
> The signal voltage switch follow requires to shutdown and output

s/follow/flow

> clock in a specific sequence according to standard host controller
> v3.0 spec. In that timing, the card must really receive clock or not.
> 
> However, for i.MX6Q, the uSDHC will not output clock even the clock
> is enabled until there is command or data in transfer on the bus,
> which will then cause singal voltage switch always to fail.
> 
> For i.MX6Q, we clear ESDHC_VENDOR_SPEC_FRC_SDCLK_ON bit to let
> controller to gate off clock automatically and set that bit
> to force clock output if clock is on.
> 
> This is required by SD3.0 support.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |    3 ---
>  drivers/mmc/host/sdhci-esdhc.h     |   29 ++++++++++++++++++++++++++---
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 1dd5ba8..3118a82 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -31,9 +31,6 @@
>  #include "sdhci-esdhc.h"
>  
>  #define	ESDHC_CTRL_D3CD			0x08
> -/* VENDOR SPEC register */
> -#define ESDHC_VENDOR_SPEC		0xc0
> -#define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
>  #define ESDHC_WTMK_LVL			0x44
>  #define ESDHC_MIX_CTRL			0x48
>  #define  ESDHC_MIX_CTRL_AC23EN		(1 << 7)
> diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
> index a2a0642..86fcd5b 100644
> --- a/drivers/mmc/host/sdhci-esdhc.h
> +++ b/drivers/mmc/host/sdhci-esdhc.h
> @@ -14,6 +14,8 @@
>  #ifndef _DRIVERS_MMC_SDHCI_ESDHC_H
>  #define _DRIVERS_MMC_SDHCI_ESDHC_H
>  
> +#include <linux/of.h>
> +
>  /*
>   * Ops and quirks for the Freescale eSDHC controller.
>   */
> @@ -33,6 +35,12 @@
>  #define ESDHC_CLOCK_HCKEN	0x00000002
>  #define ESDHC_CLOCK_IPGEN	0x00000001
>  
> +/* VENDOR SPEC register */
> +#define ESDHC_VENDOR_SPEC		0xc0
> +#define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
> +#define  ESDHC_VENDOR_SPEC_VSELECT	(1 << 1)

It would be better to introduce the macro only when it's actually being
used.

> +#define  ESDHC_VENDOR_SPEC_FRC_SDCLK_ON (1 << 8)
> +
>  /* pltfm-specific */
>  #define ESDHC_HOST_CONTROL_LE	0x20
>  
> @@ -52,12 +60,20 @@
>  static inline void esdhc_set_clock(struct sdhci_host *host, unsigned int clock,
>  				   unsigned int host_clock)
>  {
> +	struct device *dev;
>  	int pre_div = 2;
>  	int div = 1;
> -	u32 temp;
> -
> -	if (clock == 0)
> +	u32 temp, val;
> +
> +	dev = mmc_dev(host->mmc);
> +	if (clock == 0) {
> +		if (of_device_is_compatible(dev->of_node, "fsl,imx6q-usdhc")) {
> +			val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> +			writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> +				host->ioaddr + ESDHC_VENDOR_SPEC);
> +		}

To me, maintaining this esdhc_set_clock() for both imx and powerpc makes
no sense now.  I think it might be the time to have different versions
of the function for imx and powerpc, just in esdhc_pltfm_set_clock() and
esdhc_of_set_clock() respectively.

Shawn

>  		goto out;
> +	}
>  
>  	temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
>  	temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
> @@ -81,6 +97,13 @@ static inline void esdhc_set_clock(struct sdhci_host *host, unsigned int clock,
>  		| (div << ESDHC_DIVIDER_SHIFT)
>  		| (pre_div << ESDHC_PREDIV_SHIFT));
>  	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
> +
> +	if (of_device_is_compatible(dev->of_node, "fsl,imx6q-usdhc")) {
> +		val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> +		writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> +			host->ioaddr + ESDHC_VENDOR_SPEC);
> +	}
> +
>  	mdelay(1);
>  out:
>  	host->clock = clock;
> -- 
> 1.7.1
> 
> 

Re: [PATCH 3/8] sdhci: sdhci-esdhc-imx: support real clock on and off for imx6q

[Dong Aisheng]

On Thu, Sep 5, 2013 at 12:32 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Wed, Sep 04, 2013 at 08:54:12PM +0800, Dong Aisheng wrote:
> > The signal voltage switch follow requires to shutdown and output
>
> s/follow/flow
>
> > clock in a specific sequence according to standard host controller
> > v3.0 spec. In that timing, the card must really receive clock or not.
> >
> > However, for i.MX6Q, the uSDHC will not output clock even the clock
> > is enabled until there is command or data in transfer on the bus,
> > which will then cause singal voltage switch always to fail.
> >
> > For i.MX6Q, we clear ESDHC_VENDOR_SPEC_FRC_SDCLK_ON bit to let
> > controller to gate off clock automatically and set that bit
> > to force clock output if clock is on.
> >
> > This is required by SD3.0 support.
> >
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c |    3 ---
> >  drivers/mmc/host/sdhci-esdhc.h     |   29 ++++++++++++++++++++++++++---
> >  2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index 1dd5ba8..3118a82 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -31,9 +31,6 @@
> >  #include "sdhci-esdhc.h"
> >
> >  #define      ESDHC_CTRL_D3CD                 0x08
> > -/* VENDOR SPEC register */
> > -#define ESDHC_VENDOR_SPEC            0xc0
> > -#define  ESDHC_VENDOR_SPEC_SDIO_QUIRK        (1 << 1)
> >  #define ESDHC_WTMK_LVL                       0x44
> >  #define ESDHC_MIX_CTRL                       0x48
> >  #define  ESDHC_MIX_CTRL_AC23EN               (1 << 7)
> > diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
> > index a2a0642..86fcd5b 100644
> > --- a/drivers/mmc/host/sdhci-esdhc.h
> > +++ b/drivers/mmc/host/sdhci-esdhc.h
> > @@ -14,6 +14,8 @@
> >  #ifndef _DRIVERS_MMC_SDHCI_ESDHC_H
> >  #define _DRIVERS_MMC_SDHCI_ESDHC_H
> >
> > +#include <linux/of.h>
> > +
> >  /*
> >   * Ops and quirks for the Freescale eSDHC controller.
> >   */
> > @@ -33,6 +35,12 @@
> >  #define ESDHC_CLOCK_HCKEN    0x00000002
> >  #define ESDHC_CLOCK_IPGEN    0x00000001
> >
> > +/* VENDOR SPEC register */
> > +#define ESDHC_VENDOR_SPEC            0xc0
> > +#define  ESDHC_VENDOR_SPEC_SDIO_QUIRK        (1 << 1)
> > +#define  ESDHC_VENDOR_SPEC_VSELECT   (1 << 1)
>
> It would be better to introduce the macro only when it's actually being
> used.
>

Correct.

> > +#define  ESDHC_VENDOR_SPEC_FRC_SDCLK_ON (1 << 8)
> > +
> >  /* pltfm-specific */
> >  #define ESDHC_HOST_CONTROL_LE        0x20
> >
> > @@ -52,12 +60,20 @@
> >  static inline void esdhc_set_clock(struct sdhci_host *host, unsigned int clock,
> >                                  unsigned int host_clock)
> >  {
> > +     struct device *dev;
> >       int pre_div = 2;
> >       int div = 1;
> > -     u32 temp;
> > -
> > -     if (clock == 0)
> > +     u32 temp, val;
> > +
> > +     dev = mmc_dev(host->mmc);
> > +     if (clock == 0) {
> > +             if (of_device_is_compatible(dev->of_node, "fsl,imx6q-usdhc")) {
> > +                     val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> > +                     writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> > +                             host->ioaddr + ESDHC_VENDOR_SPEC);
> > +             }
>
> To me, maintaining this esdhc_set_clock() for both imx and powerpc makes
> no sense now.  I think it might be the time to have different versions
> of the function for imx and powerpc, just in esdhc_pltfm_set_clock() and
> esdhc_of_set_clock() respectively.

I agree with you.
Since later i will need add a few more imx6q specific things in this
common headfile
which seems not good.
So it would be better to end this dependency ASAP.
I could move that code into esdhc_pltfm_set_clock in next version
if no objections from others people.

Regards
Dong Aisheng

>
> Shawn
>
> >               goto out;
> > +     }
> >
> >       temp = sdhci_readl(host, ESDHC_SYSTEM_CONTROL);
> >       temp &= ~(ESDHC_CLOCK_IPGEN | ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN
> > @@ -81,6 +97,13 @@ static inline void esdhc_set_clock(struct sdhci_host *host, unsigned int clock,
> >               | (div << ESDHC_DIVIDER_SHIFT)
> >               | (pre_div << ESDHC_PREDIV_SHIFT));
> >       sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
> > +
> > +     if (of_device_is_compatible(dev->of_node, "fsl,imx6q-usdhc")) {
> > +             val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> > +             writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
> > +                     host->ioaddr + ESDHC_VENDOR_SPEC);
> > +     }
> > +
> >       mdelay(1);
> >  out:
> >       host->clock = clock;
> > --
> > 1.7.1
> >
> >
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support

[Dong Aisheng]

Freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
the standard tuning process defined in host controller spec v3.0.
Thus we use platform_execute_tuning instead of standard sdhci tuning.

The main difference are:
1) not only generate Buffer Read Ready interrupt when tuning is performing.
It generates all other DATA interrupts like the normal data command.
2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
instead it's controlled by SW.
3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
it's controlled by SW.
4) the clock delay for every tuning is set by SW.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c |  194 +++++++++++++++++++++++++++++++++++-
 1 files changed, 193 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 3118a82..36b9f63 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -34,9 +34,21 @@
 #define ESDHC_WTMK_LVL			0x44
 #define ESDHC_MIX_CTRL			0x48
 #define  ESDHC_MIX_CTRL_AC23EN		(1 << 7)
+#define  ESDHC_MIX_CTRL_EXE_TUNE	(1 << 22)
+#define  ESDHC_MIX_CTRL_SMPCLK_SEL	(1 << 23)
+#define  ESDHC_MIX_CTRL_AUTO_TUNE	(1 << 24)
+#define  ESDHC_MIX_CTRL_FBCLK_SEL	(1 << 25)
 /* Bits 3 and 6 are not SDHCI standard definitions */
 #define  ESDHC_MIX_CTRL_SDHCI_MASK	0xb7
 
+/* tune control register */
+#define ESDHC_TUNE_CTRL_STATUS		0x68
+#define  ESDHC_TUNE_CTRL_STEP		1
+#define  ESDHC_TUNE_CTRL_MIN		0
+#define  ESDHC_TUNE_CTRL_MAX		((1 << 7) - 1)
+
+#define ESDHC_TUNING_BLOCK_PATTERN_LEN	64
+
 /*
  * Our interpretation of the SDHCI_HOST_CONTROL register
  */
@@ -87,7 +99,7 @@ struct pltfm_imx_data {
 		MULTIBLK_IN_PROCESS, /* exact multiblock cmd in process */
 		WAIT_FOR_INT,        /* sent CMD12, waiting for response INT */
 	} multiblock_status;
-
+	u32 uhs_mode;
 };
 
 static struct platform_device_id imx_esdhc_devtype[] = {
@@ -161,6 +173,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
 	u32 val = readl(host->ioaddr + reg);
 
+	if (unlikely(reg == SDHCI_PRESENT_STATE)) {
+		u32 fsl_prss = val;
+		val = 0;
+		/* save the least 20 bits */
+		val |= fsl_prss & 0x000FFFFF;
+		/* move dat[0-3] bits */
+		val |= (fsl_prss & 0x0F000000) >> 4;
+		/* move cmd line bit */
+		val |= (fsl_prss & 0x00800000) << 1;
+	}
+
 	if (unlikely(reg == SDHCI_CAPABILITIES)) {
 		/* In FSL esdhc IC module, only bit20 is used to indicate the
 		 * ADMA2 capability of esdhc, but this bit is messed up on
@@ -175,6 +198,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 		}
 	}
 
+	if (unlikely(reg == SDHCI_CAPABILITIES_1) && is_imx6q_usdhc(imx_data))
+		val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
+				| SDHCI_SUPPORT_SDR50;
+
+	if (unlikely(reg == SDHCI_MAX_CURRENT) && is_imx6q_usdhc(imx_data)) {
+		val = 0;
+		val |= 0xFF << SDHCI_MAX_CURRENT_330_SHIFT;
+		val |= 0xFF << SDHCI_MAX_CURRENT_300_SHIFT;
+		val |= 0xFF << SDHCI_MAX_CURRENT_180_SHIFT;
+	}
+
 	if (unlikely(reg == SDHCI_INT_STATUS)) {
 		if (val & ESDHC_INT_VENDOR_SPEC_DMA_ERR) {
 			val &= ~ESDHC_INT_VENDOR_SPEC_DMA_ERR;
@@ -253,6 +287,8 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+	u16 ret = 0;
+	u32 val;
 
 	if (unlikely(reg == SDHCI_HOST_VERSION)) {
 		reg ^= 2;
@@ -265,6 +301,25 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
 		}
 	}
 
+	if (unlikely(reg == SDHCI_HOST_CONTROL2)) {
+		val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
+		if (val & ESDHC_VENDOR_SPEC_VSELECT)
+			ret |= SDHCI_CTRL_VDD_180;
+
+		if (is_imx6q_usdhc(imx_data)) {
+			val = readl(host->ioaddr + ESDHC_MIX_CTRL);
+			if (val & ESDHC_MIX_CTRL_EXE_TUNE)
+				ret |= SDHCI_CTRL_EXEC_TUNING;
+			if (val & ESDHC_MIX_CTRL_SMPCLK_SEL)
+				ret |= SDHCI_CTRL_TUNED_CLK;
+		}
+
+		ret |= (imx_data->uhs_mode & SDHCI_CTRL_UHS_MASK);
+		ret &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
+
+		return ret;
+	}
+
 	return readw(host->ioaddr + reg);
 }
 
@@ -272,8 +327,32 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+	u32 new_val = 0;
 
 	switch (reg) {
+	case SDHCI_CLOCK_CONTROL:
+		new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
+		if (val & SDHCI_CLOCK_CARD_EN)
+			new_val |= ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
+		else
+			new_val &= ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
+			writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
+		return;
+	case SDHCI_HOST_CONTROL2:
+		new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
+		if (val & SDHCI_CTRL_VDD_180)
+			new_val |= ESDHC_VENDOR_SPEC_VSELECT;
+		else
+			new_val &= ~ESDHC_VENDOR_SPEC_VSELECT;
+		writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
+		imx_data->uhs_mode = val & SDHCI_CTRL_UHS_MASK;
+		new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
+		if (val & SDHCI_CTRL_TUNED_CLK)
+			new_val |= ESDHC_MIX_CTRL_SMPCLK_SEL;
+		else
+			new_val &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
+		writel(new_val , host->ioaddr + ESDHC_MIX_CTRL);
+		return;
 	case SDHCI_TRANSFER_MODE:
 		if ((imx_data->flags & ESDHC_FLAG_MULTIBLK_NO_INT)
 				&& (host->cmd->opcode == SD_IO_RW_EXTENDED)
@@ -451,6 +530,118 @@ static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
 	return 0;
 }
 
+static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
+{
+	u32 reg;
+
+	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
+	reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
+			ESDHC_MIX_CTRL_FBCLK_SEL;
+	writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
+	writel((val << 8), host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
+	dev_dbg(mmc_dev(host->mmc),
+		"tunning with delay 0x%x ESDHC_TUNE_CTRL_STATUS 0x%x\n",
+			val, readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS));
+}
+
+static void request_done(struct mmc_request *mrq)
+{
+	complete(&mrq->completion);
+}
+
+static int esdhc_send_tuning_cmd(struct sdhci_host *host, u32 opcode)
+{
+	struct mmc_command cmd = {0};
+	struct mmc_request mrq = {0};
+	struct mmc_data data = {0};
+	struct scatterlist sg;
+	char tuning_pattern[ESDHC_TUNING_BLOCK_PATTERN_LEN];
+
+	cmd.opcode = opcode;
+	cmd.arg = 0;
+	cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
+
+	data.blksz = ESDHC_TUNING_BLOCK_PATTERN_LEN;
+	data.blocks = 1;
+	data.flags = MMC_DATA_READ;
+	data.sg = &sg;
+	data.sg_len = 1;
+
+	sg_init_one(&sg, tuning_pattern, sizeof(tuning_pattern));
+
+	mrq.cmd = &cmd;
+	mrq.cmd->mrq = &mrq;
+	mrq.data = &data;
+	mrq.data->mrq = &mrq;
+	mrq.cmd->data = mrq.data;
+
+	mrq.done = request_done;
+	init_completion(&(mrq.completion));
+
+	disable_irq(host->irq);
+	spin_lock(&host->lock);
+	host->mrq = &mrq;
+
+	sdhci_send_command(host, mrq.cmd);
+
+	spin_unlock(&host->lock);
+	enable_irq(host->irq);
+
+	wait_for_completion(&mrq.completion);
+
+	if (cmd.error)
+		return cmd.error;
+	if (data.error)
+		return data.error;
+
+	return 0;
+}
+
+static void esdhc_post_tuning(struct sdhci_host *host)
+{
+	u32 reg;
+
+	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
+	reg &= ~ESDHC_MIX_CTRL_EXE_TUNE;
+	writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
+}
+
+static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
+{
+	int min, max, avg, ret;
+
+	/* find the mininum delay first which can pass tuning*/
+	min = ESDHC_TUNE_CTRL_MIN;
+	while (min < ESDHC_TUNE_CTRL_MAX) {
+		esdhc_prepare_tuning(host, min);
+		if (!esdhc_send_tuning_cmd(host, opcode))
+			break;
+		min += ESDHC_TUNE_CTRL_STEP;
+	}
+
+	/* find the maxinum delay which can not pass tuning*/
+	max = min + ESDHC_TUNE_CTRL_STEP;
+	while (max < ESDHC_TUNE_CTRL_MAX) {
+		esdhc_prepare_tuning(host, max);
+		if (esdhc_send_tuning_cmd(host, opcode)) {
+			max -= ESDHC_TUNE_CTRL_STEP;
+			break;
+		}
+		max += ESDHC_TUNE_CTRL_STEP;
+	}
+
+	/* use average delay to get the best timing */
+	avg = (min + max) / 2;
+	esdhc_prepare_tuning(host, avg);
+	ret = esdhc_send_tuning_cmd(host, opcode);
+	esdhc_post_tuning(host);
+
+	dev_dbg(mmc_dev(host->mmc), "tunning %s at 0x%x ret %d\n",
+		ret ? "failed" : "passed", avg, ret);
+
+	return ret;
+}
+
 static const struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl_le,
 	.read_w = esdhc_readw_le,
@@ -462,6 +653,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
 	.get_min_clock = esdhc_pltfm_get_min_clock,
 	.get_ro = esdhc_pltfm_get_ro,
 	.platform_bus_width = esdhc_pltfm_bus_width,
+	.platform_execute_tuning = esdhc_executing_tuning,
 };
 
 static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
-- 
1.7.1

Re: [PATCH 4/8] sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support

[Shawn Guo]

Nothing major, only a few nitpicks.

On Wed, Sep 04, 2013 at 08:54:13PM +0800, Dong Aisheng wrote:
> Freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
> the standard tuning process defined in host controller spec v3.0.
> Thus we use platform_execute_tuning instead of standard sdhci tuning.
> 
> The main difference are:
> 1) not only generate Buffer Read Ready interrupt when tuning is performing.
> It generates all other DATA interrupts like the normal data command.
> 2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
> instead it's controlled by SW.
> 3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
> it's controlled by SW.
> 4) the clock delay for every tuning is set by SW.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |  194 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 193 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 3118a82..36b9f63 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -34,9 +34,21 @@
>  #define ESDHC_WTMK_LVL			0x44
>  #define ESDHC_MIX_CTRL			0x48
>  #define  ESDHC_MIX_CTRL_AC23EN		(1 << 7)
> +#define  ESDHC_MIX_CTRL_EXE_TUNE	(1 << 22)
> +#define  ESDHC_MIX_CTRL_SMPCLK_SEL	(1 << 23)
> +#define  ESDHC_MIX_CTRL_AUTO_TUNE	(1 << 24)

It seems unused?

> +#define  ESDHC_MIX_CTRL_FBCLK_SEL	(1 << 25)
>  /* Bits 3 and 6 are not SDHCI standard definitions */
>  #define  ESDHC_MIX_CTRL_SDHCI_MASK	0xb7
>  
> +/* tune control register */
> +#define ESDHC_TUNE_CTRL_STATUS		0x68
> +#define  ESDHC_TUNE_CTRL_STEP		1
> +#define  ESDHC_TUNE_CTRL_MIN		0
> +#define  ESDHC_TUNE_CTRL_MAX		((1 << 7) - 1)
> +
> +#define ESDHC_TUNING_BLOCK_PATTERN_LEN	64
> +
>  /*
>   * Our interpretation of the SDHCI_HOST_CONTROL register
>   */
> @@ -87,7 +99,7 @@ struct pltfm_imx_data {
>  		MULTIBLK_IN_PROCESS, /* exact multiblock cmd in process */
>  		WAIT_FOR_INT,        /* sent CMD12, waiting for response INT */
>  	} multiblock_status;
> -
> +	u32 uhs_mode;
>  };
>  
>  static struct platform_device_id imx_esdhc_devtype[] = {
> @@ -161,6 +173,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
>  	u32 val = readl(host->ioaddr + reg);
>  
> +	if (unlikely(reg == SDHCI_PRESENT_STATE)) {
> +		u32 fsl_prss = val;
> +		val = 0;
> +		/* save the least 20 bits */
> +		val |= fsl_prss & 0x000FFFFF;

Nit: you can do the following to save one assignment, right?

		val = fsl_prss & 0x000FFFFF;

> +		/* move dat[0-3] bits */
> +		val |= (fsl_prss & 0x0F000000) >> 4;
> +		/* move cmd line bit */
> +		val |= (fsl_prss & 0x00800000) << 1;
> +	}
> +
>  	if (unlikely(reg == SDHCI_CAPABILITIES)) {
>  		/* In FSL esdhc IC module, only bit20 is used to indicate the
>  		 * ADMA2 capability of esdhc, but this bit is messed up on
> @@ -175,6 +198,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>  		}
>  	}
>  
> +	if (unlikely(reg == SDHCI_CAPABILITIES_1) && is_imx6q_usdhc(imx_data))
> +		val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
> +				| SDHCI_SUPPORT_SDR50;
> +
> +	if (unlikely(reg == SDHCI_MAX_CURRENT) && is_imx6q_usdhc(imx_data)) {
> +		val = 0;
> +		val |= 0xFF << SDHCI_MAX_CURRENT_330_SHIFT;
> +		val |= 0xFF << SDHCI_MAX_CURRENT_300_SHIFT;
> +		val |= 0xFF << SDHCI_MAX_CURRENT_180_SHIFT;
> +	}
> +
>  	if (unlikely(reg == SDHCI_INT_STATUS)) {
>  		if (val & ESDHC_INT_VENDOR_SPEC_DMA_ERR) {
>  			val &= ~ESDHC_INT_VENDOR_SPEC_DMA_ERR;
> @@ -253,6 +287,8 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +	u16 ret = 0;
> +	u32 val;
>  
>  	if (unlikely(reg == SDHCI_HOST_VERSION)) {
>  		reg ^= 2;
> @@ -265,6 +301,25 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
>  		}
>  	}
>  
> +	if (unlikely(reg == SDHCI_HOST_CONTROL2)) {
> +		val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> +		if (val & ESDHC_VENDOR_SPEC_VSELECT)
> +			ret |= SDHCI_CTRL_VDD_180;
> +
> +		if (is_imx6q_usdhc(imx_data)) {
> +			val = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +			if (val & ESDHC_MIX_CTRL_EXE_TUNE)
> +				ret |= SDHCI_CTRL_EXEC_TUNING;
> +			if (val & ESDHC_MIX_CTRL_SMPCLK_SEL)
> +				ret |= SDHCI_CTRL_TUNED_CLK;
> +		}
> +
> +		ret |= (imx_data->uhs_mode & SDHCI_CTRL_UHS_MASK);
> +		ret &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
> +
> +		return ret;
> +	}
> +
>  	return readw(host->ioaddr + reg);
>  }
>  
> @@ -272,8 +327,32 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +	u32 new_val = 0;
>  
>  	switch (reg) {
> +	case SDHCI_CLOCK_CONTROL:
> +		new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> +		if (val & SDHCI_CLOCK_CARD_EN)
> +			new_val |= ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
> +		else
> +			new_val &= ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
> +			writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
> +		return;
> +	case SDHCI_HOST_CONTROL2:
> +		new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> +		if (val & SDHCI_CTRL_VDD_180)
> +			new_val |= ESDHC_VENDOR_SPEC_VSELECT;
> +		else
> +			new_val &= ~ESDHC_VENDOR_SPEC_VSELECT;
> +		writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
> +		imx_data->uhs_mode = val & SDHCI_CTRL_UHS_MASK;
> +		new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +		if (val & SDHCI_CTRL_TUNED_CLK)
> +			new_val |= ESDHC_MIX_CTRL_SMPCLK_SEL;
> +		else
> +			new_val &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
> +		writel(new_val , host->ioaddr + ESDHC_MIX_CTRL);
> +		return;
>  	case SDHCI_TRANSFER_MODE:
>  		if ((imx_data->flags & ESDHC_FLAG_MULTIBLK_NO_INT)
>  				&& (host->cmd->opcode == SD_IO_RW_EXTENDED)
> @@ -451,6 +530,118 @@ static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
>  	return 0;
>  }
>  
> +static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
> +{
> +	u32 reg;
> +
> +	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +	reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
> +			ESDHC_MIX_CTRL_FBCLK_SEL;
> +	writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
> +	writel((val << 8), host->ioaddr + ESDHC_TUNE_CTRL_STATUS);

Nit: unnecessary parentheses around val << 8

> +	dev_dbg(mmc_dev(host->mmc),
> +		"tunning with delay 0x%x ESDHC_TUNE_CTRL_STATUS 0x%x\n",
> +			val, readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS));
> +}
> +
> +static void request_done(struct mmc_request *mrq)

s/request_done/esdhc_request_done to have proper namespace.

> +{
> +	complete(&mrq->completion);
> +}
> +
> +static int esdhc_send_tuning_cmd(struct sdhci_host *host, u32 opcode)
> +{
> +	struct mmc_command cmd = {0};
> +	struct mmc_request mrq = {0};
> +	struct mmc_data data = {0};
> +	struct scatterlist sg;
> +	char tuning_pattern[ESDHC_TUNING_BLOCK_PATTERN_LEN];
> +
> +	cmd.opcode = opcode;
> +	cmd.arg = 0;
> +	cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +	data.blksz = ESDHC_TUNING_BLOCK_PATTERN_LEN;
> +	data.blocks = 1;
> +	data.flags = MMC_DATA_READ;
> +	data.sg = &sg;
> +	data.sg_len = 1;
> +
> +	sg_init_one(&sg, tuning_pattern, sizeof(tuning_pattern));
> +
> +	mrq.cmd = &cmd;
> +	mrq.cmd->mrq = &mrq;
> +	mrq.data = &data;
> +	mrq.data->mrq = &mrq;
> +	mrq.cmd->data = mrq.data;
> +
> +	mrq.done = request_done;
> +	init_completion(&(mrq.completion));
> +
> +	disable_irq(host->irq);
> +	spin_lock(&host->lock);
> +	host->mrq = &mrq;
> +
> +	sdhci_send_command(host, mrq.cmd);
> +
> +	spin_unlock(&host->lock);
> +	enable_irq(host->irq);
> +
> +	wait_for_completion(&mrq.completion);
> +
> +	if (cmd.error)
> +		return cmd.error;
> +	if (data.error)
> +		return data.error;
> +
> +	return 0;
> +}
> +
> +static void esdhc_post_tuning(struct sdhci_host *host)
> +{
> +	u32 reg;
> +
> +	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +	reg &= ~ESDHC_MIX_CTRL_EXE_TUNE;
> +	writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
> +}
> +
> +static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
> +{
> +	int min, max, avg, ret;
> +
> +	/* find the mininum delay first which can pass tuning*/

Nit: put a space before */

> +	min = ESDHC_TUNE_CTRL_MIN;
> +	while (min < ESDHC_TUNE_CTRL_MAX) {
> +		esdhc_prepare_tuning(host, min);
> +		if (!esdhc_send_tuning_cmd(host, opcode))
> +			break;
> +		min += ESDHC_TUNE_CTRL_STEP;
> +	}
> +
> +	/* find the maxinum delay which can not pass tuning*/

Ditto

Shawn

> +	max = min + ESDHC_TUNE_CTRL_STEP;
> +	while (max < ESDHC_TUNE_CTRL_MAX) {
> +		esdhc_prepare_tuning(host, max);
> +		if (esdhc_send_tuning_cmd(host, opcode)) {
> +			max -= ESDHC_TUNE_CTRL_STEP;
> +			break;
> +		}
> +		max += ESDHC_TUNE_CTRL_STEP;
> +	}
> +
> +	/* use average delay to get the best timing */
> +	avg = (min + max) / 2;
> +	esdhc_prepare_tuning(host, avg);
> +	ret = esdhc_send_tuning_cmd(host, opcode);
> +	esdhc_post_tuning(host);
> +
> +	dev_dbg(mmc_dev(host->mmc), "tunning %s at 0x%x ret %d\n",
> +		ret ? "failed" : "passed", avg, ret);
> +
> +	return ret;
> +}
> +
>  static const struct sdhci_ops sdhci_esdhc_ops = {
>  	.read_l = esdhc_readl_le,
>  	.read_w = esdhc_readw_le,
> @@ -462,6 +653,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>  	.get_min_clock = esdhc_pltfm_get_min_clock,
>  	.get_ro = esdhc_pltfm_get_ro,
>  	.platform_bus_width = esdhc_pltfm_bus_width,
> +	.platform_execute_tuning = esdhc_executing_tuning,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> -- 
> 1.7.1
> 
> 

Re: [PATCH 4/8] sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support

[Dong Aisheng]

On Thu, Sep 5, 2013 at 2:00 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> Nothing major, only a few nitpicks.
>

Thanks for the careful review.
Will address them all in next version.

Regards
Dong Aisheng

> On Wed, Sep 04, 2013 at 08:54:13PM +0800, Dong Aisheng wrote:
> > Freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
> > the standard tuning process defined in host controller spec v3.0.
> > Thus we use platform_execute_tuning instead of standard sdhci tuning.
> >
> > The main difference are:
> > 1) not only generate Buffer Read Ready interrupt when tuning is performing.
> > It generates all other DATA interrupts like the normal data command.
> > 2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
> > instead it's controlled by SW.
> > 3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
> > it's controlled by SW.
> > 4) the clock delay for every tuning is set by SW.
> >
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c |  194 +++++++++++++++++++++++++++++++++++-
> >  1 files changed, 193 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index 3118a82..36b9f63 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -34,9 +34,21 @@
> >  #define ESDHC_WTMK_LVL                       0x44
> >  #define ESDHC_MIX_CTRL                       0x48
> >  #define  ESDHC_MIX_CTRL_AC23EN               (1 << 7)
> > +#define  ESDHC_MIX_CTRL_EXE_TUNE     (1 << 22)
> > +#define  ESDHC_MIX_CTRL_SMPCLK_SEL   (1 << 23)
> > +#define  ESDHC_MIX_CTRL_AUTO_TUNE    (1 << 24)
>
> It seems unused?
>
> > +#define  ESDHC_MIX_CTRL_FBCLK_SEL    (1 << 25)
> >  /* Bits 3 and 6 are not SDHCI standard definitions */
> >  #define  ESDHC_MIX_CTRL_SDHCI_MASK   0xb7
> >
> > +/* tune control register */
> > +#define ESDHC_TUNE_CTRL_STATUS               0x68
> > +#define  ESDHC_TUNE_CTRL_STEP                1
> > +#define  ESDHC_TUNE_CTRL_MIN         0
> > +#define  ESDHC_TUNE_CTRL_MAX         ((1 << 7) - 1)
> > +
> > +#define ESDHC_TUNING_BLOCK_PATTERN_LEN       64
> > +
> >  /*
> >   * Our interpretation of the SDHCI_HOST_CONTROL register
> >   */
> > @@ -87,7 +99,7 @@ struct pltfm_imx_data {
> >               MULTIBLK_IN_PROCESS, /* exact multiblock cmd in process */
> >               WAIT_FOR_INT,        /* sent CMD12, waiting for response INT */
> >       } multiblock_status;
> > -
> > +     u32 uhs_mode;
> >  };
> >
> >  static struct platform_device_id imx_esdhc_devtype[] = {
> > @@ -161,6 +173,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
> >       struct pltfm_imx_data *imx_data = pltfm_host->priv;
> >       u32 val = readl(host->ioaddr + reg);
> >
> > +     if (unlikely(reg == SDHCI_PRESENT_STATE)) {
> > +             u32 fsl_prss = val;
> > +             val = 0;
> > +             /* save the least 20 bits */
> > +             val |= fsl_prss & 0x000FFFFF;
>
> Nit: you can do the following to save one assignment, right?
>
>                 val = fsl_prss & 0x000FFFFF;
>
> > +             /* move dat[0-3] bits */
> > +             val |= (fsl_prss & 0x0F000000) >> 4;
> > +             /* move cmd line bit */
> > +             val |= (fsl_prss & 0x00800000) << 1;
> > +     }
> > +
> >       if (unlikely(reg == SDHCI_CAPABILITIES)) {
> >               /* In FSL esdhc IC module, only bit20 is used to indicate the
> >                * ADMA2 capability of esdhc, but this bit is messed up on
> > @@ -175,6 +198,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
> >               }
> >       }
> >
> > +     if (unlikely(reg == SDHCI_CAPABILITIES_1) && is_imx6q_usdhc(imx_data))
> > +             val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
> > +                             | SDHCI_SUPPORT_SDR50;
> > +
> > +     if (unlikely(reg == SDHCI_MAX_CURRENT) && is_imx6q_usdhc(imx_data)) {
> > +             val = 0;
> > +             val |= 0xFF << SDHCI_MAX_CURRENT_330_SHIFT;
> > +             val |= 0xFF << SDHCI_MAX_CURRENT_300_SHIFT;
> > +             val |= 0xFF << SDHCI_MAX_CURRENT_180_SHIFT;
> > +     }
> > +
> >       if (unlikely(reg == SDHCI_INT_STATUS)) {
> >               if (val & ESDHC_INT_VENDOR_SPEC_DMA_ERR) {
> >                       val &= ~ESDHC_INT_VENDOR_SPEC_DMA_ERR;
> > @@ -253,6 +287,8 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
> >  {
> >       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >       struct pltfm_imx_data *imx_data = pltfm_host->priv;
> > +     u16 ret = 0;
> > +     u32 val;
> >
> >       if (unlikely(reg == SDHCI_HOST_VERSION)) {
> >               reg ^= 2;
> > @@ -265,6 +301,25 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
> >               }
> >       }
> >
> > +     if (unlikely(reg == SDHCI_HOST_CONTROL2)) {
> > +             val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> > +             if (val & ESDHC_VENDOR_SPEC_VSELECT)
> > +                     ret |= SDHCI_CTRL_VDD_180;
> > +
> > +             if (is_imx6q_usdhc(imx_data)) {
> > +                     val = readl(host->ioaddr + ESDHC_MIX_CTRL);
> > +                     if (val & ESDHC_MIX_CTRL_EXE_TUNE)
> > +                             ret |= SDHCI_CTRL_EXEC_TUNING;
> > +                     if (val & ESDHC_MIX_CTRL_SMPCLK_SEL)
> > +                             ret |= SDHCI_CTRL_TUNED_CLK;
> > +             }
> > +
> > +             ret |= (imx_data->uhs_mode & SDHCI_CTRL_UHS_MASK);
> > +             ret &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
> > +
> > +             return ret;
> > +     }
> > +
> >       return readw(host->ioaddr + reg);
> >  }
> >
> > @@ -272,8 +327,32 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
> >  {
> >       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >       struct pltfm_imx_data *imx_data = pltfm_host->priv;
> > +     u32 new_val = 0;
> >
> >       switch (reg) {
> > +     case SDHCI_CLOCK_CONTROL:
> > +             new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> > +             if (val & SDHCI_CLOCK_CARD_EN)
> > +                     new_val |= ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
> > +             else
> > +                     new_val &= ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
> > +                     writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
> > +             return;
> > +     case SDHCI_HOST_CONTROL2:
> > +             new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> > +             if (val & SDHCI_CTRL_VDD_180)
> > +                     new_val |= ESDHC_VENDOR_SPEC_VSELECT;
> > +             else
> > +                     new_val &= ~ESDHC_VENDOR_SPEC_VSELECT;
> > +             writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
> > +             imx_data->uhs_mode = val & SDHCI_CTRL_UHS_MASK;
> > +             new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
> > +             if (val & SDHCI_CTRL_TUNED_CLK)
> > +                     new_val |= ESDHC_MIX_CTRL_SMPCLK_SEL;
> > +             else
> > +                     new_val &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
> > +             writel(new_val , host->ioaddr + ESDHC_MIX_CTRL);
> > +             return;
> >       case SDHCI_TRANSFER_MODE:
> >               if ((imx_data->flags & ESDHC_FLAG_MULTIBLK_NO_INT)
> >                               && (host->cmd->opcode == SD_IO_RW_EXTENDED)
> > @@ -451,6 +530,118 @@ static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
> >       return 0;
> >  }
> >
> > +static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
> > +{
> > +     u32 reg;
> > +
> > +     reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> > +     reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
> > +                     ESDHC_MIX_CTRL_FBCLK_SEL;
> > +     writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
> > +     writel((val << 8), host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
>
> Nit: unnecessary parentheses around val << 8
>
> > +     dev_dbg(mmc_dev(host->mmc),
> > +             "tunning with delay 0x%x ESDHC_TUNE_CTRL_STATUS 0x%x\n",
> > +                     val, readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS));
> > +}
> > +
> > +static void request_done(struct mmc_request *mrq)
>
> s/request_done/esdhc_request_done to have proper namespace.
>
> > +{
> > +     complete(&mrq->completion);
> > +}
> > +
> > +static int esdhc_send_tuning_cmd(struct sdhci_host *host, u32 opcode)
> > +{
> > +     struct mmc_command cmd = {0};
> > +     struct mmc_request mrq = {0};
> > +     struct mmc_data data = {0};
> > +     struct scatterlist sg;
> > +     char tuning_pattern[ESDHC_TUNING_BLOCK_PATTERN_LEN];
> > +
> > +     cmd.opcode = opcode;
> > +     cmd.arg = 0;
> > +     cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> > +
> > +     data.blksz = ESDHC_TUNING_BLOCK_PATTERN_LEN;
> > +     data.blocks = 1;
> > +     data.flags = MMC_DATA_READ;
> > +     data.sg = &sg;
> > +     data.sg_len = 1;
> > +
> > +     sg_init_one(&sg, tuning_pattern, sizeof(tuning_pattern));
> > +
> > +     mrq.cmd = &cmd;
> > +     mrq.cmd->mrq = &mrq;
> > +     mrq.data = &data;
> > +     mrq.data->mrq = &mrq;
> > +     mrq.cmd->data = mrq.data;
> > +
> > +     mrq.done = request_done;
> > +     init_completion(&(mrq.completion));
> > +
> > +     disable_irq(host->irq);
> > +     spin_lock(&host->lock);
> > +     host->mrq = &mrq;
> > +
> > +     sdhci_send_command(host, mrq.cmd);
> > +
> > +     spin_unlock(&host->lock);
> > +     enable_irq(host->irq);
> > +
> > +     wait_for_completion(&mrq.completion);
> > +
> > +     if (cmd.error)
> > +             return cmd.error;
> > +     if (data.error)
> > +             return data.error;
> > +
> > +     return 0;
> > +}
> > +
> > +static void esdhc_post_tuning(struct sdhci_host *host)
> > +{
> > +     u32 reg;
> > +
> > +     reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> > +     reg &= ~ESDHC_MIX_CTRL_EXE_TUNE;
> > +     writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
> > +}
> > +
> > +static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
> > +{
> > +     int min, max, avg, ret;
> > +
> > +     /* find the mininum delay first which can pass tuning*/
>
> Nit: put a space before */
>
> > +     min = ESDHC_TUNE_CTRL_MIN;
> > +     while (min < ESDHC_TUNE_CTRL_MAX) {
> > +             esdhc_prepare_tuning(host, min);
> > +             if (!esdhc_send_tuning_cmd(host, opcode))
> > +                     break;
> > +             min += ESDHC_TUNE_CTRL_STEP;
> > +     }
> > +
> > +     /* find the maxinum delay which can not pass tuning*/
>
> Ditto
>
> Shawn
>
> > +     max = min + ESDHC_TUNE_CTRL_STEP;
> > +     while (max < ESDHC_TUNE_CTRL_MAX) {
> > +             esdhc_prepare_tuning(host, max);
> > +             if (esdhc_send_tuning_cmd(host, opcode)) {
> > +                     max -= ESDHC_TUNE_CTRL_STEP;
> > +                     break;
> > +             }
> > +             max += ESDHC_TUNE_CTRL_STEP;
> > +     }
> > +
> > +     /* use average delay to get the best timing */
> > +     avg = (min + max) / 2;
> > +     esdhc_prepare_tuning(host, avg);
> > +     ret = esdhc_send_tuning_cmd(host, opcode);
> > +     esdhc_post_tuning(host);
> > +
> > +     dev_dbg(mmc_dev(host->mmc), "tunning %s at 0x%x ret %d\n",
> > +             ret ? "failed" : "passed", avg, ret);
> > +
> > +     return ret;
> > +}
> > +
> >  static const struct sdhci_ops sdhci_esdhc_ops = {
> >       .read_l = esdhc_readl_le,
> >       .read_w = esdhc_readw_le,
> > @@ -462,6 +653,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
> >       .get_min_clock = esdhc_pltfm_get_min_clock,
> >       .get_ro = esdhc_pltfm_get_ro,
> >       .platform_bus_width = esdhc_pltfm_bus_width,
> > +     .platform_execute_tuning = esdhc_executing_tuning,
> >  };
> >
> >  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> > --
> > 1.7.1
> >
> >
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support

[Ulf Hansson]

On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
> Freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
> the standard tuning process defined in host controller spec v3.0.
> Thus we use platform_execute_tuning instead of standard sdhci tuning.
>
> The main difference are:
> 1) not only generate Buffer Read Ready interrupt when tuning is performing.
> It generates all other DATA interrupts like the normal data command.
> 2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
> instead it's controlled by SW.
> 3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
> it's controlled by SW.
> 4) the clock delay for every tuning is set by SW.
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |  194 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 193 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 3118a82..36b9f63 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -34,9 +34,21 @@
>  #define ESDHC_WTMK_LVL                 0x44
>  #define ESDHC_MIX_CTRL                 0x48
>  #define  ESDHC_MIX_CTRL_AC23EN         (1 << 7)
> +#define  ESDHC_MIX_CTRL_EXE_TUNE       (1 << 22)
> +#define  ESDHC_MIX_CTRL_SMPCLK_SEL     (1 << 23)
> +#define  ESDHC_MIX_CTRL_AUTO_TUNE      (1 << 24)
> +#define  ESDHC_MIX_CTRL_FBCLK_SEL      (1 << 25)
>  /* Bits 3 and 6 are not SDHCI standard definitions */
>  #define  ESDHC_MIX_CTRL_SDHCI_MASK     0xb7
>
> +/* tune control register */
> +#define ESDHC_TUNE_CTRL_STATUS         0x68
> +#define  ESDHC_TUNE_CTRL_STEP          1
> +#define  ESDHC_TUNE_CTRL_MIN           0
> +#define  ESDHC_TUNE_CTRL_MAX           ((1 << 7) - 1)
> +
> +#define ESDHC_TUNING_BLOCK_PATTERN_LEN 64
> +
>  /*
>   * Our interpretation of the SDHCI_HOST_CONTROL register
>   */
> @@ -87,7 +99,7 @@ struct pltfm_imx_data {
>                 MULTIBLK_IN_PROCESS, /* exact multiblock cmd in process */
>                 WAIT_FOR_INT,        /* sent CMD12, waiting for response INT */
>         } multiblock_status;
> -
> +       u32 uhs_mode;
>  };
>
>  static struct platform_device_id imx_esdhc_devtype[] = {
> @@ -161,6 +173,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
>         u32 val = readl(host->ioaddr + reg);
>
> +       if (unlikely(reg == SDHCI_PRESENT_STATE)) {
> +               u32 fsl_prss = val;
> +               val = 0;
> +               /* save the least 20 bits */
> +               val |= fsl_prss & 0x000FFFFF;
> +               /* move dat[0-3] bits */
> +               val |= (fsl_prss & 0x0F000000) >> 4;
> +               /* move cmd line bit */
> +               val |= (fsl_prss & 0x00800000) << 1;
> +       }
> +
>         if (unlikely(reg == SDHCI_CAPABILITIES)) {
>                 /* In FSL esdhc IC module, only bit20 is used to indicate the
>                  * ADMA2 capability of esdhc, but this bit is messed up on
> @@ -175,6 +198,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>                 }
>         }
>
> +       if (unlikely(reg == SDHCI_CAPABILITIES_1) && is_imx6q_usdhc(imx_data))
> +               val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
> +                               | SDHCI_SUPPORT_SDR50;
> +
> +       if (unlikely(reg == SDHCI_MAX_CURRENT) && is_imx6q_usdhc(imx_data)) {
> +               val = 0;
> +               val |= 0xFF << SDHCI_MAX_CURRENT_330_SHIFT;
> +               val |= 0xFF << SDHCI_MAX_CURRENT_300_SHIFT;
> +               val |= 0xFF << SDHCI_MAX_CURRENT_180_SHIFT;
> +       }
> +
>         if (unlikely(reg == SDHCI_INT_STATUS)) {
>                 if (val & ESDHC_INT_VENDOR_SPEC_DMA_ERR) {
>                         val &= ~ESDHC_INT_VENDOR_SPEC_DMA_ERR;
> @@ -253,6 +287,8 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
>  {
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +       u16 ret = 0;
> +       u32 val;
>
>         if (unlikely(reg == SDHCI_HOST_VERSION)) {
>                 reg ^= 2;
> @@ -265,6 +301,25 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
>                 }
>         }
>
> +       if (unlikely(reg == SDHCI_HOST_CONTROL2)) {
> +               val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> +               if (val & ESDHC_VENDOR_SPEC_VSELECT)
> +                       ret |= SDHCI_CTRL_VDD_180;
> +
> +               if (is_imx6q_usdhc(imx_data)) {
> +                       val = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +                       if (val & ESDHC_MIX_CTRL_EXE_TUNE)
> +                               ret |= SDHCI_CTRL_EXEC_TUNING;
> +                       if (val & ESDHC_MIX_CTRL_SMPCLK_SEL)
> +                               ret |= SDHCI_CTRL_TUNED_CLK;
> +               }
> +
> +               ret |= (imx_data->uhs_mode & SDHCI_CTRL_UHS_MASK);
> +               ret &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
> +
> +               return ret;
> +       }
> +
>         return readw(host->ioaddr + reg);
>  }
>
> @@ -272,8 +327,32 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>  {
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +       u32 new_val = 0;
>
>         switch (reg) {
> +       case SDHCI_CLOCK_CONTROL:
> +               new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> +               if (val & SDHCI_CLOCK_CARD_EN)
> +                       new_val |= ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
> +               else
> +                       new_val &= ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
> +                       writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
> +               return;
> +       case SDHCI_HOST_CONTROL2:
> +               new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
> +               if (val & SDHCI_CTRL_VDD_180)
> +                       new_val |= ESDHC_VENDOR_SPEC_VSELECT;
> +               else
> +                       new_val &= ~ESDHC_VENDOR_SPEC_VSELECT;
> +               writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
> +               imx_data->uhs_mode = val & SDHCI_CTRL_UHS_MASK;
> +               new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +               if (val & SDHCI_CTRL_TUNED_CLK)
> +                       new_val |= ESDHC_MIX_CTRL_SMPCLK_SEL;
> +               else
> +                       new_val &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
> +               writel(new_val , host->ioaddr + ESDHC_MIX_CTRL);
> +               return;
>         case SDHCI_TRANSFER_MODE:
>                 if ((imx_data->flags & ESDHC_FLAG_MULTIBLK_NO_INT)
>                                 && (host->cmd->opcode == SD_IO_RW_EXTENDED)
> @@ -451,6 +530,118 @@ static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
>         return 0;
>  }
>
> +static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
> +{
> +       u32 reg;
> +
> +       reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +       reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
> +                       ESDHC_MIX_CTRL_FBCLK_SEL;
> +       writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
> +       writel((val << 8), host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
> +       dev_dbg(mmc_dev(host->mmc),
> +               "tunning with delay 0x%x ESDHC_TUNE_CTRL_STATUS 0x%x\n",
> +                       val, readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS));
> +}
> +
> +static void request_done(struct mmc_request *mrq)
> +{
> +       complete(&mrq->completion);
> +}
> +
> +static int esdhc_send_tuning_cmd(struct sdhci_host *host, u32 opcode)

This function implements protocol related code. It should not reside
in a host driver but instead as an API in the mmc protocol layer which
host drivers shall call.

I realize that there are already other host drivers implementing
similar protocol code as here. Those should move to use the new API
once it is available.

Kind regards
Ulf Hansson

> +{
> +       struct mmc_command cmd = {0};
> +       struct mmc_request mrq = {0};
> +       struct mmc_data data = {0};
> +       struct scatterlist sg;
> +       char tuning_pattern[ESDHC_TUNING_BLOCK_PATTERN_LEN];
> +
> +       cmd.opcode = opcode;
> +       cmd.arg = 0;
> +       cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +       data.blksz = ESDHC_TUNING_BLOCK_PATTERN_LEN;
> +       data.blocks = 1;
> +       data.flags = MMC_DATA_READ;
> +       data.sg = &sg;
> +       data.sg_len = 1;
> +
> +       sg_init_one(&sg, tuning_pattern, sizeof(tuning_pattern));
> +
> +       mrq.cmd = &cmd;
> +       mrq.cmd->mrq = &mrq;
> +       mrq.data = &data;
> +       mrq.data->mrq = &mrq;
> +       mrq.cmd->data = mrq.data;
> +
> +       mrq.done = request_done;
> +       init_completion(&(mrq.completion));
> +
> +       disable_irq(host->irq);
> +       spin_lock(&host->lock);
> +       host->mrq = &mrq;
> +
> +       sdhci_send_command(host, mrq.cmd);
> +
> +       spin_unlock(&host->lock);
> +       enable_irq(host->irq);
> +
> +       wait_for_completion(&mrq.completion);
> +
> +       if (cmd.error)
> +               return cmd.error;
> +       if (data.error)
> +               return data.error;
> +
> +       return 0;
> +}
> +
> +static void esdhc_post_tuning(struct sdhci_host *host)
> +{
> +       u32 reg;
> +
> +       reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> +       reg &= ~ESDHC_MIX_CTRL_EXE_TUNE;
> +       writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
> +}
> +
> +static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
> +{
> +       int min, max, avg, ret;
> +
> +       /* find the mininum delay first which can pass tuning*/
> +       min = ESDHC_TUNE_CTRL_MIN;
> +       while (min < ESDHC_TUNE_CTRL_MAX) {
> +               esdhc_prepare_tuning(host, min);
> +               if (!esdhc_send_tuning_cmd(host, opcode))
> +                       break;
> +               min += ESDHC_TUNE_CTRL_STEP;
> +       }
> +
> +       /* find the maxinum delay which can not pass tuning*/
> +       max = min + ESDHC_TUNE_CTRL_STEP;
> +       while (max < ESDHC_TUNE_CTRL_MAX) {
> +               esdhc_prepare_tuning(host, max);
> +               if (esdhc_send_tuning_cmd(host, opcode)) {
> +                       max -= ESDHC_TUNE_CTRL_STEP;
> +                       break;
> +               }
> +               max += ESDHC_TUNE_CTRL_STEP;
> +       }
> +
> +       /* use average delay to get the best timing */
> +       avg = (min + max) / 2;
> +       esdhc_prepare_tuning(host, avg);
> +       ret = esdhc_send_tuning_cmd(host, opcode);
> +       esdhc_post_tuning(host);
> +
> +       dev_dbg(mmc_dev(host->mmc), "tunning %s at 0x%x ret %d\n",
> +               ret ? "failed" : "passed", avg, ret);
> +
> +       return ret;
> +}
> +
>  static const struct sdhci_ops sdhci_esdhc_ops = {
>         .read_l = esdhc_readl_le,
>         .read_w = esdhc_readw_le,
> @@ -462,6 +653,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>         .get_min_clock = esdhc_pltfm_get_min_clock,
>         .get_ro = esdhc_pltfm_get_ro,
>         .platform_bus_width = esdhc_pltfm_bus_width,
> +       .platform_execute_tuning = esdhc_executing_tuning,
>  };
>
>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> --
> 1.7.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support

[Dong Aisheng]

On Thu, Sep 5, 2013 at 3:33 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
>> Freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
>> the standard tuning process defined in host controller spec v3.0.
>> Thus we use platform_execute_tuning instead of standard sdhci tuning.
>>
>> The main difference are:
>> 1) not only generate Buffer Read Ready interrupt when tuning is performing.
>> It generates all other DATA interrupts like the normal data command.
>> 2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
>> instead it's controlled by SW.
>> 3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
>> it's controlled by SW.
>> 4) the clock delay for every tuning is set by SW.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/mmc/host/sdhci-esdhc-imx.c |  194 +++++++++++++++++++++++++++++++++++-
>>  1 files changed, 193 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 3118a82..36b9f63 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -34,9 +34,21 @@
>>  #define ESDHC_WTMK_LVL                 0x44
>>  #define ESDHC_MIX_CTRL                 0x48
>>  #define  ESDHC_MIX_CTRL_AC23EN         (1 << 7)
>> +#define  ESDHC_MIX_CTRL_EXE_TUNE       (1 << 22)
>> +#define  ESDHC_MIX_CTRL_SMPCLK_SEL     (1 << 23)
>> +#define  ESDHC_MIX_CTRL_AUTO_TUNE      (1 << 24)
>> +#define  ESDHC_MIX_CTRL_FBCLK_SEL      (1 << 25)
>>  /* Bits 3 and 6 are not SDHCI standard definitions */
>>  #define  ESDHC_MIX_CTRL_SDHCI_MASK     0xb7
>>
>> +/* tune control register */
>> +#define ESDHC_TUNE_CTRL_STATUS         0x68
>> +#define  ESDHC_TUNE_CTRL_STEP          1
>> +#define  ESDHC_TUNE_CTRL_MIN           0
>> +#define  ESDHC_TUNE_CTRL_MAX           ((1 << 7) - 1)
>> +
>> +#define ESDHC_TUNING_BLOCK_PATTERN_LEN 64
>> +
>>  /*
>>   * Our interpretation of the SDHCI_HOST_CONTROL register
>>   */
>> @@ -87,7 +99,7 @@ struct pltfm_imx_data {
>>                 MULTIBLK_IN_PROCESS, /* exact multiblock cmd in process */
>>                 WAIT_FOR_INT,        /* sent CMD12, waiting for response INT */
>>         } multiblock_status;
>> -
>> +       u32 uhs_mode;
>>  };
>>
>>  static struct platform_device_id imx_esdhc_devtype[] = {
>> @@ -161,6 +173,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>         u32 val = readl(host->ioaddr + reg);
>>
>> +       if (unlikely(reg == SDHCI_PRESENT_STATE)) {
>> +               u32 fsl_prss = val;
>> +               val = 0;
>> +               /* save the least 20 bits */
>> +               val |= fsl_prss & 0x000FFFFF;
>> +               /* move dat[0-3] bits */
>> +               val |= (fsl_prss & 0x0F000000) >> 4;
>> +               /* move cmd line bit */
>> +               val |= (fsl_prss & 0x00800000) << 1;
>> +       }
>> +
>>         if (unlikely(reg == SDHCI_CAPABILITIES)) {
>>                 /* In FSL esdhc IC module, only bit20 is used to indicate the
>>                  * ADMA2 capability of esdhc, but this bit is messed up on
>> @@ -175,6 +198,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>>                 }
>>         }
>>
>> +       if (unlikely(reg == SDHCI_CAPABILITIES_1) && is_imx6q_usdhc(imx_data))
>> +               val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
>> +                               | SDHCI_SUPPORT_SDR50;
>> +
>> +       if (unlikely(reg == SDHCI_MAX_CURRENT) && is_imx6q_usdhc(imx_data)) {
>> +               val = 0;
>> +               val |= 0xFF << SDHCI_MAX_CURRENT_330_SHIFT;
>> +               val |= 0xFF << SDHCI_MAX_CURRENT_300_SHIFT;
>> +               val |= 0xFF << SDHCI_MAX_CURRENT_180_SHIFT;
>> +       }
>> +
>>         if (unlikely(reg == SDHCI_INT_STATUS)) {
>>                 if (val & ESDHC_INT_VENDOR_SPEC_DMA_ERR) {
>>                         val &= ~ESDHC_INT_VENDOR_SPEC_DMA_ERR;
>> @@ -253,6 +287,8 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
>>  {
>>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
>> +       u16 ret = 0;
>> +       u32 val;
>>
>>         if (unlikely(reg == SDHCI_HOST_VERSION)) {
>>                 reg ^= 2;
>> @@ -265,6 +301,25 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
>>                 }
>>         }
>>
>> +       if (unlikely(reg == SDHCI_HOST_CONTROL2)) {
>> +               val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> +               if (val & ESDHC_VENDOR_SPEC_VSELECT)
>> +                       ret |= SDHCI_CTRL_VDD_180;
>> +
>> +               if (is_imx6q_usdhc(imx_data)) {
>> +                       val = readl(host->ioaddr + ESDHC_MIX_CTRL);
>> +                       if (val & ESDHC_MIX_CTRL_EXE_TUNE)
>> +                               ret |= SDHCI_CTRL_EXEC_TUNING;
>> +                       if (val & ESDHC_MIX_CTRL_SMPCLK_SEL)
>> +                               ret |= SDHCI_CTRL_TUNED_CLK;
>> +               }
>> +
>> +               ret |= (imx_data->uhs_mode & SDHCI_CTRL_UHS_MASK);
>> +               ret &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
>> +
>> +               return ret;
>> +       }
>> +
>>         return readw(host->ioaddr + reg);
>>  }
>>
>> @@ -272,8 +327,32 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>>  {
>>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
>> +       u32 new_val = 0;
>>
>>         switch (reg) {
>> +       case SDHCI_CLOCK_CONTROL:
>> +               new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> +               if (val & SDHCI_CLOCK_CARD_EN)
>> +                       new_val |= ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
>> +               else
>> +                       new_val &= ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
>> +                       writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
>> +               return;
>> +       case SDHCI_HOST_CONTROL2:
>> +               new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>> +               if (val & SDHCI_CTRL_VDD_180)
>> +                       new_val |= ESDHC_VENDOR_SPEC_VSELECT;
>> +               else
>> +                       new_val &= ~ESDHC_VENDOR_SPEC_VSELECT;
>> +               writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
>> +               imx_data->uhs_mode = val & SDHCI_CTRL_UHS_MASK;
>> +               new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
>> +               if (val & SDHCI_CTRL_TUNED_CLK)
>> +                       new_val |= ESDHC_MIX_CTRL_SMPCLK_SEL;
>> +               else
>> +                       new_val &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
>> +               writel(new_val , host->ioaddr + ESDHC_MIX_CTRL);
>> +               return;
>>         case SDHCI_TRANSFER_MODE:
>>                 if ((imx_data->flags & ESDHC_FLAG_MULTIBLK_NO_INT)
>>                                 && (host->cmd->opcode == SD_IO_RW_EXTENDED)
>> @@ -451,6 +530,118 @@ static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
>>         return 0;
>>  }
>>
>> +static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
>> +{
>> +       u32 reg;
>> +
>> +       reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
>> +       reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
>> +                       ESDHC_MIX_CTRL_FBCLK_SEL;
>> +       writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
>> +       writel((val << 8), host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
>> +       dev_dbg(mmc_dev(host->mmc),
>> +               "tunning with delay 0x%x ESDHC_TUNE_CTRL_STATUS 0x%x\n",
>> +                       val, readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS));
>> +}
>> +
>> +static void request_done(struct mmc_request *mrq)
>> +{
>> +       complete(&mrq->completion);
>> +}
>> +
>> +static int esdhc_send_tuning_cmd(struct sdhci_host *host, u32 opcode)
>
> This function implements protocol related code. It should not reside
> in a host driver but instead as an API in the mmc protocol layer which
> host drivers shall call.
>
> I realize that there are already other host drivers implementing
> similar protocol code as here. Those should move to use the new API
> once it is available.

That sounds like a good idea.
The tuning comand sending process seems could be implemented in protocal layer
since it's standard.
One problem is that since the tuning command handling is tightly
related to host controller.
It may be a big rework on current code if implement such API and
switch all the drivers to call it.
And it may also increase the host driver complication since it needs
to treat tuning
command differently in driver and do specfic settings at specific
places, e.g sdhci.
Not sure it's good enough.

For imx, since it's based on sdhci, it may be less impact even once there's
such an API for it to switch to.
So i may would like to keep the using as sdhci currently if could,
which also will block
my following eMMC4.5 and SDIO3.0 work.

Regards
Dong Aisheng

>
> Kind regards
> Ulf Hansson
>
>> +{
>> +       struct mmc_command cmd = {0};
>> +       struct mmc_request mrq = {0};
>> +       struct mmc_data data = {0};
>> +       struct scatterlist sg;
>> +       char tuning_pattern[ESDHC_TUNING_BLOCK_PATTERN_LEN];
>> +
>> +       cmd.opcode = opcode;
>> +       cmd.arg = 0;
>> +       cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
>> +
>> +       data.blksz = ESDHC_TUNING_BLOCK_PATTERN_LEN;
>> +       data.blocks = 1;
>> +       data.flags = MMC_DATA_READ;
>> +       data.sg = &sg;
>> +       data.sg_len = 1;
>> +
>> +       sg_init_one(&sg, tuning_pattern, sizeof(tuning_pattern));
>> +
>> +       mrq.cmd = &cmd;
>> +       mrq.cmd->mrq = &mrq;
>> +       mrq.data = &data;
>> +       mrq.data->mrq = &mrq;
>> +       mrq.cmd->data = mrq.data;
>> +
>> +       mrq.done = request_done;
>> +       init_completion(&(mrq.completion));
>> +
>> +       disable_irq(host->irq);
>> +       spin_lock(&host->lock);
>> +       host->mrq = &mrq;
>> +
>> +       sdhci_send_command(host, mrq.cmd);
>> +
>> +       spin_unlock(&host->lock);
>> +       enable_irq(host->irq);
>> +
>> +       wait_for_completion(&mrq.completion);
>> +
>> +       if (cmd.error)
>> +               return cmd.error;
>> +       if (data.error)
>> +               return data.error;
>> +
>> +       return 0;
>> +}
>> +
>> +static void esdhc_post_tuning(struct sdhci_host *host)
>> +{
>> +       u32 reg;
>> +
>> +       reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
>> +       reg &= ~ESDHC_MIX_CTRL_EXE_TUNE;
>> +       writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
>> +}
>> +
>> +static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
>> +{
>> +       int min, max, avg, ret;
>> +
>> +       /* find the mininum delay first which can pass tuning*/
>> +       min = ESDHC_TUNE_CTRL_MIN;
>> +       while (min < ESDHC_TUNE_CTRL_MAX) {
>> +               esdhc_prepare_tuning(host, min);
>> +               if (!esdhc_send_tuning_cmd(host, opcode))
>> +                       break;
>> +               min += ESDHC_TUNE_CTRL_STEP;
>> +       }
>> +
>> +       /* find the maxinum delay which can not pass tuning*/
>> +       max = min + ESDHC_TUNE_CTRL_STEP;
>> +       while (max < ESDHC_TUNE_CTRL_MAX) {
>> +               esdhc_prepare_tuning(host, max);
>> +               if (esdhc_send_tuning_cmd(host, opcode)) {
>> +                       max -= ESDHC_TUNE_CTRL_STEP;
>> +                       break;
>> +               }
>> +               max += ESDHC_TUNE_CTRL_STEP;
>> +       }
>> +
>> +       /* use average delay to get the best timing */
>> +       avg = (min + max) / 2;
>> +       esdhc_prepare_tuning(host, avg);
>> +       ret = esdhc_send_tuning_cmd(host, opcode);
>> +       esdhc_post_tuning(host);
>> +
>> +       dev_dbg(mmc_dev(host->mmc), "tunning %s at 0x%x ret %d\n",
>> +               ret ? "failed" : "passed", avg, ret);
>> +
>> +       return ret;
>> +}
>> +
>>  static const struct sdhci_ops sdhci_esdhc_ops = {
>>         .read_l = esdhc_readl_le,
>>         .read_w = esdhc_readw_le,
>> @@ -462,6 +653,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>>         .get_min_clock = esdhc_pltfm_get_min_clock,
>>         .get_ro = esdhc_pltfm_get_ro,
>>         .platform_bus_width = esdhc_pltfm_bus_width,
>> +       .platform_execute_tuning = esdhc_executing_tuning,
>>  };
>>
>>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
>> --
>> 1.7.1
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support

[Ulf Hansson]

On 5 September 2013 19:52, Dong Aisheng <dongas86@gmail.com> wrote:
> On Thu, Sep 5, 2013 at 3:33 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
>>> Freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
>>> the standard tuning process defined in host controller spec v3.0.
>>> Thus we use platform_execute_tuning instead of standard sdhci tuning.
>>>
>>> The main difference are:
>>> 1) not only generate Buffer Read Ready interrupt when tuning is performing.
>>> It generates all other DATA interrupts like the normal data command.
>>> 2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
>>> instead it's controlled by SW.
>>> 3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
>>> it's controlled by SW.
>>> 4) the clock delay for every tuning is set by SW.
>>>
>>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>>> ---
>>>  drivers/mmc/host/sdhci-esdhc-imx.c |  194 +++++++++++++++++++++++++++++++++++-
>>>  1 files changed, 193 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> index 3118a82..36b9f63 100644
>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> @@ -34,9 +34,21 @@
>>>  #define ESDHC_WTMK_LVL                 0x44
>>>  #define ESDHC_MIX_CTRL                 0x48
>>>  #define  ESDHC_MIX_CTRL_AC23EN         (1 << 7)
>>> +#define  ESDHC_MIX_CTRL_EXE_TUNE       (1 << 22)
>>> +#define  ESDHC_MIX_CTRL_SMPCLK_SEL     (1 << 23)
>>> +#define  ESDHC_MIX_CTRL_AUTO_TUNE      (1 << 24)
>>> +#define  ESDHC_MIX_CTRL_FBCLK_SEL      (1 << 25)
>>>  /* Bits 3 and 6 are not SDHCI standard definitions */
>>>  #define  ESDHC_MIX_CTRL_SDHCI_MASK     0xb7
>>>
>>> +/* tune control register */
>>> +#define ESDHC_TUNE_CTRL_STATUS         0x68
>>> +#define  ESDHC_TUNE_CTRL_STEP          1
>>> +#define  ESDHC_TUNE_CTRL_MIN           0
>>> +#define  ESDHC_TUNE_CTRL_MAX           ((1 << 7) - 1)
>>> +
>>> +#define ESDHC_TUNING_BLOCK_PATTERN_LEN 64
>>> +
>>>  /*
>>>   * Our interpretation of the SDHCI_HOST_CONTROL register
>>>   */
>>> @@ -87,7 +99,7 @@ struct pltfm_imx_data {
>>>                 MULTIBLK_IN_PROCESS, /* exact multiblock cmd in process */
>>>                 WAIT_FOR_INT,        /* sent CMD12, waiting for response INT */
>>>         } multiblock_status;
>>> -
>>> +       u32 uhs_mode;
>>>  };
>>>
>>>  static struct platform_device_id imx_esdhc_devtype[] = {
>>> @@ -161,6 +173,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>>>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>>         u32 val = readl(host->ioaddr + reg);
>>>
>>> +       if (unlikely(reg == SDHCI_PRESENT_STATE)) {
>>> +               u32 fsl_prss = val;
>>> +               val = 0;
>>> +               /* save the least 20 bits */
>>> +               val |= fsl_prss & 0x000FFFFF;
>>> +               /* move dat[0-3] bits */
>>> +               val |= (fsl_prss & 0x0F000000) >> 4;
>>> +               /* move cmd line bit */
>>> +               val |= (fsl_prss & 0x00800000) << 1;
>>> +       }
>>> +
>>>         if (unlikely(reg == SDHCI_CAPABILITIES)) {
>>>                 /* In FSL esdhc IC module, only bit20 is used to indicate the
>>>                  * ADMA2 capability of esdhc, but this bit is messed up on
>>> @@ -175,6 +198,17 @@ static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
>>>                 }
>>>         }
>>>
>>> +       if (unlikely(reg == SDHCI_CAPABILITIES_1) && is_imx6q_usdhc(imx_data))
>>> +               val = SDHCI_SUPPORT_DDR50 | SDHCI_SUPPORT_SDR104
>>> +                               | SDHCI_SUPPORT_SDR50;
>>> +
>>> +       if (unlikely(reg == SDHCI_MAX_CURRENT) && is_imx6q_usdhc(imx_data)) {
>>> +               val = 0;
>>> +               val |= 0xFF << SDHCI_MAX_CURRENT_330_SHIFT;
>>> +               val |= 0xFF << SDHCI_MAX_CURRENT_300_SHIFT;
>>> +               val |= 0xFF << SDHCI_MAX_CURRENT_180_SHIFT;
>>> +       }
>>> +
>>>         if (unlikely(reg == SDHCI_INT_STATUS)) {
>>>                 if (val & ESDHC_INT_VENDOR_SPEC_DMA_ERR) {
>>>                         val &= ~ESDHC_INT_VENDOR_SPEC_DMA_ERR;
>>> @@ -253,6 +287,8 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
>>>  {
>>>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>> +       u16 ret = 0;
>>> +       u32 val;
>>>
>>>         if (unlikely(reg == SDHCI_HOST_VERSION)) {
>>>                 reg ^= 2;
>>> @@ -265,6 +301,25 @@ static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
>>>                 }
>>>         }
>>>
>>> +       if (unlikely(reg == SDHCI_HOST_CONTROL2)) {
>>> +               val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>>> +               if (val & ESDHC_VENDOR_SPEC_VSELECT)
>>> +                       ret |= SDHCI_CTRL_VDD_180;
>>> +
>>> +               if (is_imx6q_usdhc(imx_data)) {
>>> +                       val = readl(host->ioaddr + ESDHC_MIX_CTRL);
>>> +                       if (val & ESDHC_MIX_CTRL_EXE_TUNE)
>>> +                               ret |= SDHCI_CTRL_EXEC_TUNING;
>>> +                       if (val & ESDHC_MIX_CTRL_SMPCLK_SEL)
>>> +                               ret |= SDHCI_CTRL_TUNED_CLK;
>>> +               }
>>> +
>>> +               ret |= (imx_data->uhs_mode & SDHCI_CTRL_UHS_MASK);
>>> +               ret &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
>>> +
>>> +               return ret;
>>> +       }
>>> +
>>>         return readw(host->ioaddr + reg);
>>>  }
>>>
>>> @@ -272,8 +327,32 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
>>>  {
>>>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>         struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>> +       u32 new_val = 0;
>>>
>>>         switch (reg) {
>>> +       case SDHCI_CLOCK_CONTROL:
>>> +               new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>>> +               if (val & SDHCI_CLOCK_CARD_EN)
>>> +                       new_val |= ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
>>> +               else
>>> +                       new_val &= ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
>>> +                       writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
>>> +               return;
>>> +       case SDHCI_HOST_CONTROL2:
>>> +               new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
>>> +               if (val & SDHCI_CTRL_VDD_180)
>>> +                       new_val |= ESDHC_VENDOR_SPEC_VSELECT;
>>> +               else
>>> +                       new_val &= ~ESDHC_VENDOR_SPEC_VSELECT;
>>> +               writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
>>> +               imx_data->uhs_mode = val & SDHCI_CTRL_UHS_MASK;
>>> +               new_val = readl(host->ioaddr + ESDHC_MIX_CTRL);
>>> +               if (val & SDHCI_CTRL_TUNED_CLK)
>>> +                       new_val |= ESDHC_MIX_CTRL_SMPCLK_SEL;
>>> +               else
>>> +                       new_val &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
>>> +               writel(new_val , host->ioaddr + ESDHC_MIX_CTRL);
>>> +               return;
>>>         case SDHCI_TRANSFER_MODE:
>>>                 if ((imx_data->flags & ESDHC_FLAG_MULTIBLK_NO_INT)
>>>                                 && (host->cmd->opcode == SD_IO_RW_EXTENDED)
>>> @@ -451,6 +530,118 @@ static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
>>>         return 0;
>>>  }
>>>
>>> +static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
>>> +{
>>> +       u32 reg;
>>> +
>>> +       reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
>>> +       reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
>>> +                       ESDHC_MIX_CTRL_FBCLK_SEL;
>>> +       writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
>>> +       writel((val << 8), host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
>>> +       dev_dbg(mmc_dev(host->mmc),
>>> +               "tunning with delay 0x%x ESDHC_TUNE_CTRL_STATUS 0x%x\n",
>>> +                       val, readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS));
>>> +}
>>> +
>>> +static void request_done(struct mmc_request *mrq)
>>> +{
>>> +       complete(&mrq->completion);
>>> +}
>>> +
>>> +static int esdhc_send_tuning_cmd(struct sdhci_host *host, u32 opcode)
>>
>> This function implements protocol related code. It should not reside
>> in a host driver but instead as an API in the mmc protocol layer which
>> host drivers shall call.
>>
>> I realize that there are already other host drivers implementing
>> similar protocol code as here. Those should move to use the new API
>> once it is available.
>
> That sounds like a good idea.
> The tuning comand sending process seems could be implemented in protocal layer
> since it's standard.
> One problem is that since the tuning command handling is tightly
> related to host controller.
> It may be a big rework on current code if implement such API and
> switch all the drivers to call it.
> And it may also increase the host driver complication since it needs
> to treat tuning
> command differently in driver and do specfic settings at specific
> places, e.g sdhci.
> Not sure it's good enough.

Hi Dong,

I am not proposing to move the entire tuning sequence to the protocol
layer, only the part that is related to send the actual tuning
command.
Typically, "esdhc_send_tuning_cmd" could be an API that host driver's
can call. I think this could prevent us from having a lot of
duplicated code in host drivers.

Not directly related to this patch; regarding periodic re-tuning; this
kind of feature should also be implemented in the protocol layer.
Otherwise each an every driver will re-implement similar
timer/work-queue and maybe even block statistics to trigger re-tuning.
Sdhci has already done this, which to me feels plain wrong for host
driver to do.

So before this goes out of control, we need to be aligned on what code
should reside the protocol layer, because that is in the end what this
discussion boils done to.

Kind regards
Ulf Hansson

>
> For imx, since it's based on sdhci, it may be less impact even once there's
> such an API for it to switch to.
> So i may would like to keep the using as sdhci currently if could,
> which also will block
> my following eMMC4.5 and SDIO3.0 work.
>
> Regards
> Dong Aisheng
>
>>
>> Kind regards
>> Ulf Hansson
>>
>>> +{
>>> +       struct mmc_command cmd = {0};
>>> +       struct mmc_request mrq = {0};
>>> +       struct mmc_data data = {0};
>>> +       struct scatterlist sg;
>>> +       char tuning_pattern[ESDHC_TUNING_BLOCK_PATTERN_LEN];
>>> +
>>> +       cmd.opcode = opcode;
>>> +       cmd.arg = 0;
>>> +       cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
>>> +
>>> +       data.blksz = ESDHC_TUNING_BLOCK_PATTERN_LEN;
>>> +       data.blocks = 1;
>>> +       data.flags = MMC_DATA_READ;
>>> +       data.sg = &sg;
>>> +       data.sg_len = 1;
>>> +
>>> +       sg_init_one(&sg, tuning_pattern, sizeof(tuning_pattern));
>>> +
>>> +       mrq.cmd = &cmd;
>>> +       mrq.cmd->mrq = &mrq;
>>> +       mrq.data = &data;
>>> +       mrq.data->mrq = &mrq;
>>> +       mrq.cmd->data = mrq.data;
>>> +
>>> +       mrq.done = request_done;
>>> +       init_completion(&(mrq.completion));
>>> +
>>> +       disable_irq(host->irq);
>>> +       spin_lock(&host->lock);
>>> +       host->mrq = &mrq;
>>> +
>>> +       sdhci_send_command(host, mrq.cmd);
>>> +
>>> +       spin_unlock(&host->lock);
>>> +       enable_irq(host->irq);
>>> +
>>> +       wait_for_completion(&mrq.completion);
>>> +
>>> +       if (cmd.error)
>>> +               return cmd.error;
>>> +       if (data.error)
>>> +               return data.error;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static void esdhc_post_tuning(struct sdhci_host *host)
>>> +{
>>> +       u32 reg;
>>> +
>>> +       reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
>>> +       reg &= ~ESDHC_MIX_CTRL_EXE_TUNE;
>>> +       writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
>>> +}
>>> +
>>> +static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
>>> +{
>>> +       int min, max, avg, ret;
>>> +
>>> +       /* find the mininum delay first which can pass tuning*/
>>> +       min = ESDHC_TUNE_CTRL_MIN;
>>> +       while (min < ESDHC_TUNE_CTRL_MAX) {
>>> +               esdhc_prepare_tuning(host, min);
>>> +               if (!esdhc_send_tuning_cmd(host, opcode))
>>> +                       break;
>>> +               min += ESDHC_TUNE_CTRL_STEP;
>>> +       }
>>> +
>>> +       /* find the maxinum delay which can not pass tuning*/
>>> +       max = min + ESDHC_TUNE_CTRL_STEP;
>>> +       while (max < ESDHC_TUNE_CTRL_MAX) {
>>> +               esdhc_prepare_tuning(host, max);
>>> +               if (esdhc_send_tuning_cmd(host, opcode)) {
>>> +                       max -= ESDHC_TUNE_CTRL_STEP;
>>> +                       break;
>>> +               }
>>> +               max += ESDHC_TUNE_CTRL_STEP;
>>> +       }
>>> +
>>> +       /* use average delay to get the best timing */
>>> +       avg = (min + max) / 2;
>>> +       esdhc_prepare_tuning(host, avg);
>>> +       ret = esdhc_send_tuning_cmd(host, opcode);
>>> +       esdhc_post_tuning(host);
>>> +
>>> +       dev_dbg(mmc_dev(host->mmc), "tunning %s at 0x%x ret %d\n",
>>> +               ret ? "failed" : "passed", avg, ret);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>  static const struct sdhci_ops sdhci_esdhc_ops = {
>>>         .read_l = esdhc_readl_le,
>>>         .read_w = esdhc_readw_le,
>>> @@ -462,6 +653,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>>>         .get_min_clock = esdhc_pltfm_get_min_clock,
>>>         .get_ro = esdhc_pltfm_get_ro,
>>>         .platform_bus_width = esdhc_pltfm_bus_width,
>>> +       .platform_execute_tuning = esdhc_executing_tuning,
>>>  };
>>>
>>>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
>>> --
>>> 1.7.1
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Dong Aisheng]

Without proper pinctrl state, the card may not be able to work
on high speed stablely. e.g. SDR104.

This patch add pinctrl state switch code according to different
uhs mode include 100mhz sate, 200mhz sate and normal state
(50Mhz and below).

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c          |  110 ++++++++++++++++++++++++++-
 include/linux/platform_data/mmc-esdhc-imx.h |    4 +
 2 files changed, 113 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 36b9f63..3e89863 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -49,6 +49,10 @@
 
 #define ESDHC_TUNING_BLOCK_PATTERN_LEN	64
 
+/* pinctrl state */
+#define ESDHC_PINCTRL_STATE_100MHZ	"state_100mhz"
+#define ESDHC_PINCTRL_STATE_200MHZ	"state_200mhz"
+
 /*
  * Our interpretation of the SDHCI_HOST_CONTROL register
  */
@@ -90,6 +94,10 @@ struct pltfm_imx_data {
 	u32 scratchpad;
 	enum imx_esdhc_type devtype;
 	struct pinctrl *pinctrl;
+	struct pinctrl_state *pins_current;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_100mhz;
+	struct pinctrl_state *pins_200mhz;
 	struct esdhc_platform_data boarddata;
 	struct clk *clk_ipg;
 	struct clk *clk_ahb;
@@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
 	return ret;
 }
 
+static int esdhc_change_pinstate(struct sdhci_host *host,
+						unsigned int uhs)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+	struct pinctrl_state *pinctrl;
+	int ret;
+
+	dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
+
+	if (IS_ERR(imx_data->pinctrl) ||
+		IS_ERR(imx_data->pins_default) ||
+		IS_ERR(imx_data->pins_100mhz) ||
+		IS_ERR(imx_data->pins_200mhz))
+		return -EINVAL;
+
+	switch (uhs) {
+	case MMC_TIMING_UHS_SDR12:
+	case MMC_TIMING_UHS_SDR25:
+	case MMC_TIMING_UHS_DDR50:
+		pinctrl = imx_data->pins_default;
+		break;
+	case MMC_TIMING_UHS_SDR50:
+		pinctrl = imx_data->pins_100mhz;
+		break;
+	case MMC_TIMING_UHS_SDR104:
+		pinctrl = imx_data->pins_200mhz;
+		break;
+	default:
+		/* back to default state for other legacy timing */
+		pinctrl = imx_data->pins_default;
+	}
+
+	if (pinctrl == imx_data->pins_current)
+		return 0;
+
+	ret = pinctrl_select_state(imx_data->pinctrl, pinctrl);
+	if (!ret)
+		imx_data->pins_current = pinctrl;
+
+	return ret;
+}
+
+static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = pltfm_host->priv;
+
+	switch (uhs) {
+	case MMC_TIMING_UHS_SDR12:
+		imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12;
+		break;
+	case MMC_TIMING_UHS_SDR25:
+		imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25;
+		break;
+	case MMC_TIMING_UHS_SDR50:
+		imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50;
+		break;
+	case MMC_TIMING_UHS_SDR104:
+		imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104;
+		break;
+	case MMC_TIMING_UHS_DDR50:
+		imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50;
+		break;
+	}
+
+	return esdhc_change_pinstate(host, uhs);
+}
+
 static const struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl_le,
 	.read_w = esdhc_readw_le,
@@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
 	.get_min_clock = esdhc_pltfm_get_min_clock,
 	.get_ro = esdhc_pltfm_get_ro,
 	.platform_bus_width = esdhc_pltfm_bus_width,
+	.set_uhs_signaling = esdhc_set_uhs_signaling,
 	.platform_execute_tuning = esdhc_executing_tuning,
 };
 
@@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 
 	of_property_read_u32(np, "max-frequency", &boarddata->f_max);
 
+	if (of_find_property(np, "no-1-8-v", NULL))
+		boarddata->support_vsel = false;
+	else
+		boarddata->support_vsel = true;
+
 	return 0;
 }
 #else
@@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	clk_prepare_enable(imx_data->clk_ipg);
 	clk_prepare_enable(imx_data->clk_ahb);
 
-	imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	imx_data->pinctrl = devm_pinctrl_get(&pdev->dev);
 	if (IS_ERR(imx_data->pinctrl)) {
 		err = PTR_ERR(imx_data->pinctrl);
 		goto disable_clk;
 	}
 
+	imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl,
+						PINCTRL_STATE_DEFAULT);
+	if (IS_ERR(imx_data->pins_default)) {
+		err = PTR_ERR(imx_data->pins_default);
+		dev_err(mmc_dev(host->mmc), "could not get default state\n");
+		goto disable_clk;
+	}
+
 	host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
 	if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
@@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 		break;
 	}
 
+	/* sdr50 and sdr104 needs work on 1.8v signal voltage */
+	if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) {
+		imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
+						ESDHC_PINCTRL_STATE_100MHZ);
+		imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
+						ESDHC_PINCTRL_STATE_200MHZ);
+		if (IS_ERR(imx_data->pins_100mhz) ||
+				IS_ERR(imx_data->pins_200mhz)) {
+			dev_warn(mmc_dev(host->mmc),
+				"could not get ultra high speed state, work on normal mode\n");
+			/* fall back to not support uhs by specify no 1.8v quirk */
+			host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
+		}
+	} else {
+		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
+	}
+
 	err = sdhci_add_host(host);
 	if (err)
 		goto disable_clk;
diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
index d44912d..a0f5a8f 100644
--- a/include/linux/platform_data/mmc-esdhc-imx.h
+++ b/include/linux/platform_data/mmc-esdhc-imx.h
@@ -10,6 +10,8 @@
 #ifndef __ASM_ARCH_IMX_ESDHC_H
 #define __ASM_ARCH_IMX_ESDHC_H
 
+#include <linux/types.h>
+
 enum wp_types {
 	ESDHC_WP_NONE,		/* no WP, neither controller nor gpio */
 	ESDHC_WP_CONTROLLER,	/* mmc controller internal WP */
@@ -32,6 +34,7 @@ enum cd_types {
  * @cd_gpio:	gpio for card_detect interrupt
  * @wp_type:	type of write_protect method (see wp_types enum above)
  * @cd_type:	type of card_detect method (see cd_types enum above)
+ * @support_vsel:  indicate it supports 1.8v switching
  */
 
 struct esdhc_platform_data {
@@ -41,5 +44,6 @@ struct esdhc_platform_data {
 	enum cd_types cd_type;
 	int max_bus_width;
 	unsigned int f_max;
+	bool support_vsel;
 };
 #endif /* __ASM_ARCH_IMX_ESDHC_H */
-- 
1.7.1

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Shawn Guo]

On Wed, Sep 04, 2013 at 08:54:14PM +0800, Dong Aisheng wrote:
> Without proper pinctrl state, the card may not be able to work
> on high speed stablely. e.g. SDR104.
> 
> This patch add pinctrl state switch code according to different
> uhs mode include 100mhz sate, 200mhz sate and normal state
> (50Mhz and below).
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c          |  110 ++++++++++++++++++++++++++-
>  include/linux/platform_data/mmc-esdhc-imx.h |    4 +
>  2 files changed, 113 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 36b9f63..3e89863 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -49,6 +49,10 @@
>  
>  #define ESDHC_TUNING_BLOCK_PATTERN_LEN	64
>  
> +/* pinctrl state */
> +#define ESDHC_PINCTRL_STATE_100MHZ	"state_100mhz"
> +#define ESDHC_PINCTRL_STATE_200MHZ	"state_200mhz"
> +
>  /*
>   * Our interpretation of the SDHCI_HOST_CONTROL register
>   */
> @@ -90,6 +94,10 @@ struct pltfm_imx_data {
>  	u32 scratchpad;
>  	enum imx_esdhc_type devtype;
>  	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pins_current;
> +	struct pinctrl_state *pins_default;
> +	struct pinctrl_state *pins_100mhz;
> +	struct pinctrl_state *pins_200mhz;
>  	struct esdhc_platform_data boarddata;
>  	struct clk *clk_ipg;
>  	struct clk *clk_ahb;
> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
>  	return ret;
>  }
>  
> +static int esdhc_change_pinstate(struct sdhci_host *host,
> +						unsigned int uhs)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +	struct pinctrl_state *pinctrl;
> +	int ret;
> +
> +	dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
> +
> +	if (IS_ERR(imx_data->pinctrl) ||
> +		IS_ERR(imx_data->pins_default) ||
> +		IS_ERR(imx_data->pins_100mhz) ||
> +		IS_ERR(imx_data->pins_200mhz))
> +		return -EINVAL;
> +
> +	switch (uhs) {
> +	case MMC_TIMING_UHS_SDR12:
> +	case MMC_TIMING_UHS_SDR25:
> +	case MMC_TIMING_UHS_DDR50:
> +		pinctrl = imx_data->pins_default;
> +		break;
> +	case MMC_TIMING_UHS_SDR50:
> +		pinctrl = imx_data->pins_100mhz;
> +		break;
> +	case MMC_TIMING_UHS_SDR104:
> +		pinctrl = imx_data->pins_200mhz;
> +		break;
> +	default:
> +		/* back to default state for other legacy timing */
> +		pinctrl = imx_data->pins_default;
> +	}

So it can be equally written as below?

	switch (uhs) {
	case MMC_TIMING_UHS_SDR104:
		pinctrl = imx_data->pins_200mhz;
		break;
	case MMC_TIMING_UHS_SDR50:
		pinctrl = imx_data->pins_100mhz;
		break;
	default:
		pinctrl = imx_data->pins_default;
	}
	
> +
> +	if (pinctrl == imx_data->pins_current)
> +		return 0;

I think pinctrl_select_state() already take care of the check?

> +
> +	ret = pinctrl_select_state(imx_data->pinctrl, pinctrl);
> +	if (!ret)
> +		imx_data->pins_current = pinctrl;
> +
> +	return ret;
> +}
> +
> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +
> +	switch (uhs) {
> +	case MMC_TIMING_UHS_SDR12:
> +		imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12;
> +		break;
> +	case MMC_TIMING_UHS_SDR25:
> +		imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25;
> +		break;
> +	case MMC_TIMING_UHS_SDR50:
> +		imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50;
> +		break;
> +	case MMC_TIMING_UHS_SDR104:
> +		imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104;
> +		break;
> +	case MMC_TIMING_UHS_DDR50:
> +		imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50;
> +		break;
> +	}
> +
> +	return esdhc_change_pinstate(host, uhs);
> +}
> +
>  static const struct sdhci_ops sdhci_esdhc_ops = {
>  	.read_l = esdhc_readl_le,
>  	.read_w = esdhc_readw_le,
> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>  	.get_min_clock = esdhc_pltfm_get_min_clock,
>  	.get_ro = esdhc_pltfm_get_ro,
>  	.platform_bus_width = esdhc_pltfm_bus_width,
> +	.set_uhs_signaling = esdhc_set_uhs_signaling,
>  	.platform_execute_tuning = esdhc_executing_tuning,
>  };
>  
> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>  
>  	of_property_read_u32(np, "max-frequency", &boarddata->f_max);
>  
> +	if (of_find_property(np, "no-1-8-v", NULL))
> +		boarddata->support_vsel = false;
> +	else
> +		boarddata->support_vsel = true;

So you check "no-1-8-v" property, set support_vsel here and then set
SDHCI_QUIRK2_NO_1_8_V accordingly below in sdhci_esdhc_imx_probe().
But sdhci_get_of_property() - sdhci-pltfm.c already does the job.

Shawn

> +
>  	return 0;
>  }
>  #else
> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  	clk_prepare_enable(imx_data->clk_ipg);
>  	clk_prepare_enable(imx_data->clk_ahb);
>  
> -	imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +	imx_data->pinctrl = devm_pinctrl_get(&pdev->dev);
>  	if (IS_ERR(imx_data->pinctrl)) {
>  		err = PTR_ERR(imx_data->pinctrl);
>  		goto disable_clk;
>  	}
>  
> +	imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl,
> +						PINCTRL_STATE_DEFAULT);
> +	if (IS_ERR(imx_data->pins_default)) {
> +		err = PTR_ERR(imx_data->pins_default);
> +		dev_err(mmc_dev(host->mmc), "could not get default state\n");
> +		goto disable_clk;
> +	}
> +
>  	host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>  
>  	if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  		break;
>  	}
>  
> +	/* sdr50 and sdr104 needs work on 1.8v signal voltage */
> +	if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) {
> +		imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
> +						ESDHC_PINCTRL_STATE_100MHZ);
> +		imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
> +						ESDHC_PINCTRL_STATE_200MHZ);
> +		if (IS_ERR(imx_data->pins_100mhz) ||
> +				IS_ERR(imx_data->pins_200mhz)) {
> +			dev_warn(mmc_dev(host->mmc),
> +				"could not get ultra high speed state, work on normal mode\n");
> +			/* fall back to not support uhs by specify no 1.8v quirk */
> +			host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
> +		}
> +	} else {
> +		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
> +	}
> +
>  	err = sdhci_add_host(host);
>  	if (err)
>  		goto disable_clk;
> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
> index d44912d..a0f5a8f 100644
> --- a/include/linux/platform_data/mmc-esdhc-imx.h
> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
> @@ -10,6 +10,8 @@
>  #ifndef __ASM_ARCH_IMX_ESDHC_H
>  #define __ASM_ARCH_IMX_ESDHC_H
>  
> +#include <linux/types.h>
> +
>  enum wp_types {
>  	ESDHC_WP_NONE,		/* no WP, neither controller nor gpio */
>  	ESDHC_WP_CONTROLLER,	/* mmc controller internal WP */
> @@ -32,6 +34,7 @@ enum cd_types {
>   * @cd_gpio:	gpio for card_detect interrupt
>   * @wp_type:	type of write_protect method (see wp_types enum above)
>   * @cd_type:	type of card_detect method (see cd_types enum above)
> + * @support_vsel:  indicate it supports 1.8v switching
>   */
>  
>  struct esdhc_platform_data {
> @@ -41,5 +44,6 @@ struct esdhc_platform_data {
>  	enum cd_types cd_type;
>  	int max_bus_width;
>  	unsigned int f_max;
> +	bool support_vsel;
>  };
>  #endif /* __ASM_ARCH_IMX_ESDHC_H */
> -- 
> 1.7.1
> 
> 

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Dong Aisheng]

On Thu, Sep 5, 2013 at 2:34 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Sep 04, 2013 at 08:54:14PM +0800, Dong Aisheng wrote:
>> Without proper pinctrl state, the card may not be able to work
>> on high speed stablely. e.g. SDR104.
>>
>> This patch add pinctrl state switch code according to different
>> uhs mode include 100mhz sate, 200mhz sate and normal state
>> (50Mhz and below).
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/mmc/host/sdhci-esdhc-imx.c          |  110 ++++++++++++++++++++++++++-
>>  include/linux/platform_data/mmc-esdhc-imx.h |    4 +
>>  2 files changed, 113 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 36b9f63..3e89863 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -49,6 +49,10 @@
>>
>>  #define ESDHC_TUNING_BLOCK_PATTERN_LEN       64
>>
>> +/* pinctrl state */
>> +#define ESDHC_PINCTRL_STATE_100MHZ   "state_100mhz"
>> +#define ESDHC_PINCTRL_STATE_200MHZ   "state_200mhz"
>> +
>>  /*
>>   * Our interpretation of the SDHCI_HOST_CONTROL register
>>   */
>> @@ -90,6 +94,10 @@ struct pltfm_imx_data {
>>       u32 scratchpad;
>>       enum imx_esdhc_type devtype;
>>       struct pinctrl *pinctrl;
>> +     struct pinctrl_state *pins_current;
>> +     struct pinctrl_state *pins_default;
>> +     struct pinctrl_state *pins_100mhz;
>> +     struct pinctrl_state *pins_200mhz;
>>       struct esdhc_platform_data boarddata;
>>       struct clk *clk_ipg;
>>       struct clk *clk_ahb;
>> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
>>       return ret;
>>  }
>>
>> +static int esdhc_change_pinstate(struct sdhci_host *host,
>> +                                             unsigned int uhs)
>> +{
>> +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +     struct pltfm_imx_data *imx_data = pltfm_host->priv;
>> +     struct pinctrl_state *pinctrl;
>> +     int ret;
>> +
>> +     dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
>> +
>> +     if (IS_ERR(imx_data->pinctrl) ||
>> +             IS_ERR(imx_data->pins_default) ||
>> +             IS_ERR(imx_data->pins_100mhz) ||
>> +             IS_ERR(imx_data->pins_200mhz))
>> +             return -EINVAL;
>> +
>> +     switch (uhs) {
>> +     case MMC_TIMING_UHS_SDR12:
>> +     case MMC_TIMING_UHS_SDR25:
>> +     case MMC_TIMING_UHS_DDR50:
>> +             pinctrl = imx_data->pins_default;
>> +             break;
>> +     case MMC_TIMING_UHS_SDR50:
>> +             pinctrl = imx_data->pins_100mhz;
>> +             break;
>> +     case MMC_TIMING_UHS_SDR104:
>> +             pinctrl = imx_data->pins_200mhz;
>> +             break;
>> +     default:
>> +             /* back to default state for other legacy timing */
>> +             pinctrl = imx_data->pins_default;
>> +     }
>
> So it can be equally written as below?
>
>         switch (uhs) {
>         case MMC_TIMING_UHS_SDR104:
>                 pinctrl = imx_data->pins_200mhz;
>                 break;
>         case MMC_TIMING_UHS_SDR50:
>                 pinctrl = imx_data->pins_100mhz;
>                 break;
>         default:
>                 pinctrl = imx_data->pins_default;
>         }
>

Seems good.

>> +
>> +     if (pinctrl == imx_data->pins_current)
>> +             return 0;
>
> I think pinctrl_select_state() already take care of the check?
>

Looks right.
I will remove it.

>> +
>> +     ret = pinctrl_select_state(imx_data->pinctrl, pinctrl);
>> +     if (!ret)
>> +             imx_data->pins_current = pinctrl;
>> +
>> +     return ret;
>> +}
>> +
>> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>> +{
>> +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +     struct pltfm_imx_data *imx_data = pltfm_host->priv;
>> +
>> +     switch (uhs) {
>> +     case MMC_TIMING_UHS_SDR12:
>> +             imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12;
>> +             break;
>> +     case MMC_TIMING_UHS_SDR25:
>> +             imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25;
>> +             break;
>> +     case MMC_TIMING_UHS_SDR50:
>> +             imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50;
>> +             break;
>> +     case MMC_TIMING_UHS_SDR104:
>> +             imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104;
>> +             break;
>> +     case MMC_TIMING_UHS_DDR50:
>> +             imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50;
>> +             break;
>> +     }
>> +
>> +     return esdhc_change_pinstate(host, uhs);
>> +}
>> +
>>  static const struct sdhci_ops sdhci_esdhc_ops = {
>>       .read_l = esdhc_readl_le,
>>       .read_w = esdhc_readw_le,
>> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>>       .get_min_clock = esdhc_pltfm_get_min_clock,
>>       .get_ro = esdhc_pltfm_get_ro,
>>       .platform_bus_width = esdhc_pltfm_bus_width,
>> +     .set_uhs_signaling = esdhc_set_uhs_signaling,
>>       .platform_execute_tuning = esdhc_executing_tuning,
>>  };
>>
>> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>>
>>       of_property_read_u32(np, "max-frequency", &boarddata->f_max);
>>
>> +     if (of_find_property(np, "no-1-8-v", NULL))
>> +             boarddata->support_vsel = false;
>> +     else
>> +             boarddata->support_vsel = true;
>
> So you check "no-1-8-v" property, set support_vsel here and then set
> SDHCI_QUIRK2_NO_1_8_V accordingly below in sdhci_esdhc_imx_probe().
> But sdhci_get_of_property() - sdhci-pltfm.c already does the job.
>

We're not using sdhci_get_of_property.
I will consider to use it, that can be another patch later.

Regards
Dong Aisheng

> Shawn
>
>> +
>>       return 0;
>>  }
>>  #else
>> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>>       clk_prepare_enable(imx_data->clk_ipg);
>>       clk_prepare_enable(imx_data->clk_ahb);
>>
>> -     imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>> +     imx_data->pinctrl = devm_pinctrl_get(&pdev->dev);
>>       if (IS_ERR(imx_data->pinctrl)) {
>>               err = PTR_ERR(imx_data->pinctrl);
>>               goto disable_clk;
>>       }
>>
>> +     imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl,
>> +                                             PINCTRL_STATE_DEFAULT);
>> +     if (IS_ERR(imx_data->pins_default)) {
>> +             err = PTR_ERR(imx_data->pins_default);
>> +             dev_err(mmc_dev(host->mmc), "could not get default state\n");
>> +             goto disable_clk;
>> +     }
>> +
>>       host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>
>>       if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
>> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>>               break;
>>       }
>>
>> +     /* sdr50 and sdr104 needs work on 1.8v signal voltage */
>> +     if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) {
>> +             imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
>> +                                             ESDHC_PINCTRL_STATE_100MHZ);
>> +             imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
>> +                                             ESDHC_PINCTRL_STATE_200MHZ);
>> +             if (IS_ERR(imx_data->pins_100mhz) ||
>> +                             IS_ERR(imx_data->pins_200mhz)) {
>> +                     dev_warn(mmc_dev(host->mmc),
>> +                             "could not get ultra high speed state, work on normal mode\n");
>> +                     /* fall back to not support uhs by specify no 1.8v quirk */
>> +                     host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>> +             }
>> +     } else {
>> +             host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>> +     }
>> +
>>       err = sdhci_add_host(host);
>>       if (err)
>>               goto disable_clk;
>> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
>> index d44912d..a0f5a8f 100644
>> --- a/include/linux/platform_data/mmc-esdhc-imx.h
>> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
>> @@ -10,6 +10,8 @@
>>  #ifndef __ASM_ARCH_IMX_ESDHC_H
>>  #define __ASM_ARCH_IMX_ESDHC_H
>>
>> +#include <linux/types.h>
>> +
>>  enum wp_types {
>>       ESDHC_WP_NONE,          /* no WP, neither controller nor gpio */
>>       ESDHC_WP_CONTROLLER,    /* mmc controller internal WP */
>> @@ -32,6 +34,7 @@ enum cd_types {
>>   * @cd_gpio: gpio for card_detect interrupt
>>   * @wp_type: type of write_protect method (see wp_types enum above)
>>   * @cd_type: type of card_detect method (see cd_types enum above)
>> + * @support_vsel:  indicate it supports 1.8v switching
>>   */
>>
>>  struct esdhc_platform_data {
>> @@ -41,5 +44,6 @@ struct esdhc_platform_data {
>>       enum cd_types cd_type;
>>       int max_bus_width;
>>       unsigned int f_max;
>> +     bool support_vsel;
>>  };
>>  #endif /* __ASM_ARCH_IMX_ESDHC_H */
>> --
>> 1.7.1
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Ulf Hansson]

On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
> Without proper pinctrl state, the card may not be able to work
> on high speed stablely. e.g. SDR104.
>
> This patch add pinctrl state switch code according to different
> uhs mode include 100mhz sate, 200mhz sate and normal state
> (50Mhz and below).
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c          |  110 ++++++++++++++++++++++++++-
>  include/linux/platform_data/mmc-esdhc-imx.h |    4 +
>  2 files changed, 113 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 36b9f63..3e89863 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -49,6 +49,10 @@
>
>  #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64
>
> +/* pinctrl state */
> +#define ESDHC_PINCTRL_STATE_100MHZ     "state_100mhz"
> +#define ESDHC_PINCTRL_STATE_200MHZ     "state_200mhz"
> +
>  /*
>   * Our interpretation of the SDHCI_HOST_CONTROL register
>   */
> @@ -90,6 +94,10 @@ struct pltfm_imx_data {
>         u32 scratchpad;
>         enum imx_esdhc_type devtype;
>         struct pinctrl *pinctrl;
> +       struct pinctrl_state *pins_current;
> +       struct pinctrl_state *pins_default;
> +       struct pinctrl_state *pins_100mhz;
> +       struct pinctrl_state *pins_200mhz;
>         struct esdhc_platform_data boarddata;
>         struct clk *clk_ipg;
>         struct clk *clk_ahb;
> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
>         return ret;
>  }
>
> +static int esdhc_change_pinstate(struct sdhci_host *host,
> +                                               unsigned int uhs)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +       struct pinctrl_state *pinctrl;
> +       int ret;
> +
> +       dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
> +
> +       if (IS_ERR(imx_data->pinctrl) ||
> +               IS_ERR(imx_data->pins_default) ||
> +               IS_ERR(imx_data->pins_100mhz) ||
> +               IS_ERR(imx_data->pins_200mhz))
> +               return -EINVAL;
> +
> +       switch (uhs) {
> +       case MMC_TIMING_UHS_SDR12:
> +       case MMC_TIMING_UHS_SDR25:
> +       case MMC_TIMING_UHS_DDR50:
> +               pinctrl = imx_data->pins_default;
> +               break;
> +       case MMC_TIMING_UHS_SDR50:
> +               pinctrl = imx_data->pins_100mhz;
> +               break;
> +       case MMC_TIMING_UHS_SDR104:
> +               pinctrl = imx_data->pins_200mhz;
> +               break;
> +       default:
> +               /* back to default state for other legacy timing */
> +               pinctrl = imx_data->pins_default;
> +       }
> +
> +       if (pinctrl == imx_data->pins_current)
> +               return 0;
> +
> +       ret = pinctrl_select_state(imx_data->pinctrl, pinctrl);
> +       if (!ret)
> +               imx_data->pins_current = pinctrl;
> +
> +       return ret;
> +}
> +
> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct pltfm_imx_data *imx_data = pltfm_host->priv;
> +
> +       switch (uhs) {
> +       case MMC_TIMING_UHS_SDR12:
> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12;
> +               break;
> +       case MMC_TIMING_UHS_SDR25:
> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25;
> +               break;
> +       case MMC_TIMING_UHS_SDR50:
> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50;
> +               break;
> +       case MMC_TIMING_UHS_SDR104:
> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104;
> +               break;
> +       case MMC_TIMING_UHS_DDR50:
> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50;
> +               break;
> +       }
> +
> +       return esdhc_change_pinstate(host, uhs);
> +}
> +
>  static const struct sdhci_ops sdhci_esdhc_ops = {
>         .read_l = esdhc_readl_le,
>         .read_w = esdhc_readw_le,
> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>         .get_min_clock = esdhc_pltfm_get_min_clock,
>         .get_ro = esdhc_pltfm_get_ro,
>         .platform_bus_width = esdhc_pltfm_bus_width,
> +       .set_uhs_signaling = esdhc_set_uhs_signaling,
>         .platform_execute_tuning = esdhc_executing_tuning,
>  };
>
> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>
>         of_property_read_u32(np, "max-frequency", &boarddata->f_max);
>
> +       if (of_find_property(np, "no-1-8-v", NULL))
> +               boarddata->support_vsel = false;
> +       else
> +               boarddata->support_vsel = true;
> +
>         return 0;
>  }
>  #else
> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>         clk_prepare_enable(imx_data->clk_ipg);
>         clk_prepare_enable(imx_data->clk_ahb);
>
> -       imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +       imx_data->pinctrl = devm_pinctrl_get(&pdev->dev);
>         if (IS_ERR(imx_data->pinctrl)) {
>                 err = PTR_ERR(imx_data->pinctrl);
>                 goto disable_clk;
>         }
>
> +       imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl,
> +                                               PINCTRL_STATE_DEFAULT);

I believe you should look into the new pinctrl APIs and the new
sequence at device driver core probe, which automatically fetches
default, idle and sleep state pins. Additionally it sets the default
state.

It should likely simplifies some code in this patch.

Kind regards
Ulf Hansson


> +       if (IS_ERR(imx_data->pins_default)) {
> +               err = PTR_ERR(imx_data->pins_default);
> +               dev_err(mmc_dev(host->mmc), "could not get default state\n");
> +               goto disable_clk;
> +       }
> +
>         host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>
>         if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>                 break;
>         }
>
> +       /* sdr50 and sdr104 needs work on 1.8v signal voltage */
> +       if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) {
> +               imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
> +                                               ESDHC_PINCTRL_STATE_100MHZ);
> +               imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
> +                                               ESDHC_PINCTRL_STATE_200MHZ);
> +               if (IS_ERR(imx_data->pins_100mhz) ||
> +                               IS_ERR(imx_data->pins_200mhz)) {
> +                       dev_warn(mmc_dev(host->mmc),
> +                               "could not get ultra high speed state, work on normal mode\n");
> +                       /* fall back to not support uhs by specify no 1.8v quirk */
> +                       host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
> +               }
> +       } else {
> +               host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
> +       }
> +
>         err = sdhci_add_host(host);
>         if (err)
>                 goto disable_clk;
> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
> index d44912d..a0f5a8f 100644
> --- a/include/linux/platform_data/mmc-esdhc-imx.h
> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
> @@ -10,6 +10,8 @@
>  #ifndef __ASM_ARCH_IMX_ESDHC_H
>  #define __ASM_ARCH_IMX_ESDHC_H
>
> +#include <linux/types.h>
> +
>  enum wp_types {
>         ESDHC_WP_NONE,          /* no WP, neither controller nor gpio */
>         ESDHC_WP_CONTROLLER,    /* mmc controller internal WP */
> @@ -32,6 +34,7 @@ enum cd_types {
>   * @cd_gpio:   gpio for card_detect interrupt
>   * @wp_type:   type of write_protect method (see wp_types enum above)
>   * @cd_type:   type of card_detect method (see cd_types enum above)
> + * @support_vsel:  indicate it supports 1.8v switching
>   */
>
>  struct esdhc_platform_data {
> @@ -41,5 +44,6 @@ struct esdhc_platform_data {
>         enum cd_types cd_type;
>         int max_bus_width;
>         unsigned int f_max;
> +       bool support_vsel;
>  };
>  #endif /* __ASM_ARCH_IMX_ESDHC_H */
> --
> 1.7.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Dong Aisheng]

Hi Ulf,

On Thu, Sep 5, 2013 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
>> Without proper pinctrl state, the card may not be able to work
>> on high speed stablely. e.g. SDR104.
>>
>> This patch add pinctrl state switch code according to different
>> uhs mode include 100mhz sate, 200mhz sate and normal state
>> (50Mhz and below).
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  drivers/mmc/host/sdhci-esdhc-imx.c          |  110 ++++++++++++++++++++++++++-
>>  include/linux/platform_data/mmc-esdhc-imx.h |    4 +
>>  2 files changed, 113 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 36b9f63..3e89863 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -49,6 +49,10 @@
>>
>>  #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64
>>
>> +/* pinctrl state */
>> +#define ESDHC_PINCTRL_STATE_100MHZ     "state_100mhz"
>> +#define ESDHC_PINCTRL_STATE_200MHZ     "state_200mhz"
>> +
>>  /*
>>   * Our interpretation of the SDHCI_HOST_CONTROL register
>>   */
>> @@ -90,6 +94,10 @@ struct pltfm_imx_data {
>>         u32 scratchpad;
>>         enum imx_esdhc_type devtype;
>>         struct pinctrl *pinctrl;
>> +       struct pinctrl_state *pins_current;
>> +       struct pinctrl_state *pins_default;
>> +       struct pinctrl_state *pins_100mhz;
>> +       struct pinctrl_state *pins_200mhz;
>>         struct esdhc_platform_data boarddata;
>>         struct clk *clk_ipg;
>>         struct clk *clk_ahb;
>> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
>>         return ret;
>>  }
>>
>> +static int esdhc_change_pinstate(struct sdhci_host *host,
>> +                                               unsigned int uhs)
>> +{
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +       struct pltfm_imx_data *imx_data = pltfm_host->priv;
>> +       struct pinctrl_state *pinctrl;
>> +       int ret;
>> +
>> +       dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
>> +
>> +       if (IS_ERR(imx_data->pinctrl) ||
>> +               IS_ERR(imx_data->pins_default) ||
>> +               IS_ERR(imx_data->pins_100mhz) ||
>> +               IS_ERR(imx_data->pins_200mhz))
>> +               return -EINVAL;
>> +
>> +       switch (uhs) {
>> +       case MMC_TIMING_UHS_SDR12:
>> +       case MMC_TIMING_UHS_SDR25:
>> +       case MMC_TIMING_UHS_DDR50:
>> +               pinctrl = imx_data->pins_default;
>> +               break;
>> +       case MMC_TIMING_UHS_SDR50:
>> +               pinctrl = imx_data->pins_100mhz;
>> +               break;
>> +       case MMC_TIMING_UHS_SDR104:
>> +               pinctrl = imx_data->pins_200mhz;
>> +               break;
>> +       default:
>> +               /* back to default state for other legacy timing */
>> +               pinctrl = imx_data->pins_default;
>> +       }
>> +
>> +       if (pinctrl == imx_data->pins_current)
>> +               return 0;
>> +
>> +       ret = pinctrl_select_state(imx_data->pinctrl, pinctrl);
>> +       if (!ret)
>> +               imx_data->pins_current = pinctrl;
>> +
>> +       return ret;
>> +}
>> +
>> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>> +{
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +       struct pltfm_imx_data *imx_data = pltfm_host->priv;
>> +
>> +       switch (uhs) {
>> +       case MMC_TIMING_UHS_SDR12:
>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12;
>> +               break;
>> +       case MMC_TIMING_UHS_SDR25:
>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25;
>> +               break;
>> +       case MMC_TIMING_UHS_SDR50:
>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50;
>> +               break;
>> +       case MMC_TIMING_UHS_SDR104:
>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104;
>> +               break;
>> +       case MMC_TIMING_UHS_DDR50:
>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50;
>> +               break;
>> +       }
>> +
>> +       return esdhc_change_pinstate(host, uhs);
>> +}
>> +
>>  static const struct sdhci_ops sdhci_esdhc_ops = {
>>         .read_l = esdhc_readl_le,
>>         .read_w = esdhc_readw_le,
>> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>>         .get_min_clock = esdhc_pltfm_get_min_clock,
>>         .get_ro = esdhc_pltfm_get_ro,
>>         .platform_bus_width = esdhc_pltfm_bus_width,
>> +       .set_uhs_signaling = esdhc_set_uhs_signaling,
>>         .platform_execute_tuning = esdhc_executing_tuning,
>>  };
>>
>> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>>
>>         of_property_read_u32(np, "max-frequency", &boarddata->f_max);
>>
>> +       if (of_find_property(np, "no-1-8-v", NULL))
>> +               boarddata->support_vsel = false;
>> +       else
>> +               boarddata->support_vsel = true;
>> +
>>         return 0;
>>  }
>>  #else
>> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>>         clk_prepare_enable(imx_data->clk_ipg);
>>         clk_prepare_enable(imx_data->clk_ahb);
>>
>> -       imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>> +       imx_data->pinctrl = devm_pinctrl_get(&pdev->dev);
>>         if (IS_ERR(imx_data->pinctrl)) {
>>                 err = PTR_ERR(imx_data->pinctrl);
>>                 goto disable_clk;
>>         }
>>
>> +       imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl,
>> +                                               PINCTRL_STATE_DEFAULT);
>
> I believe you should look into the new pinctrl APIs and the new
> sequence at device driver core probe, which automatically fetches
> default, idle and sleep state pins. Additionally it sets the default
> state.
>
> It should likely simplifies some code in this patch.
>

After looking into the pinctrl automatically fetch states, it seems
above two line may could be replaced as:
imx_data->pinctrl = (&pdev->dev)->pins->p;
imx_data->pins_default = (&pdev->dev)->pins->default_state;

However, i'm not sure touching the pinctrl internal implementations in
device core is a good choice.
So i may still like to keep the exist using.

Regards
Dong Aisheng

> Kind regards
> Ulf Hansson
>
>
>> +       if (IS_ERR(imx_data->pins_default)) {
>> +               err = PTR_ERR(imx_data->pins_default);
>> +               dev_err(mmc_dev(host->mmc), "could not get default state\n");
>> +               goto disable_clk;
>> +       }
>> +
>>         host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>
>>         if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
>> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>>                 break;
>>         }
>>
>> +       /* sdr50 and sdr104 needs work on 1.8v signal voltage */
>> +       if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) {
>> +               imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
>> +                                               ESDHC_PINCTRL_STATE_100MHZ);
>> +               imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
>> +                                               ESDHC_PINCTRL_STATE_200MHZ);
>> +               if (IS_ERR(imx_data->pins_100mhz) ||
>> +                               IS_ERR(imx_data->pins_200mhz)) {
>> +                       dev_warn(mmc_dev(host->mmc),
>> +                               "could not get ultra high speed state, work on normal mode\n");
>> +                       /* fall back to not support uhs by specify no 1.8v quirk */
>> +                       host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>> +               }
>> +       } else {
>> +               host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>> +       }
>> +
>>         err = sdhci_add_host(host);
>>         if (err)
>>                 goto disable_clk;
>> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
>> index d44912d..a0f5a8f 100644
>> --- a/include/linux/platform_data/mmc-esdhc-imx.h
>> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
>> @@ -10,6 +10,8 @@
>>  #ifndef __ASM_ARCH_IMX_ESDHC_H
>>  #define __ASM_ARCH_IMX_ESDHC_H
>>
>> +#include <linux/types.h>
>> +
>>  enum wp_types {
>>         ESDHC_WP_NONE,          /* no WP, neither controller nor gpio */
>>         ESDHC_WP_CONTROLLER,    /* mmc controller internal WP */
>> @@ -32,6 +34,7 @@ enum cd_types {
>>   * @cd_gpio:   gpio for card_detect interrupt
>>   * @wp_type:   type of write_protect method (see wp_types enum above)
>>   * @cd_type:   type of card_detect method (see cd_types enum above)
>> + * @support_vsel:  indicate it supports 1.8v switching
>>   */
>>
>>  struct esdhc_platform_data {
>> @@ -41,5 +44,6 @@ struct esdhc_platform_data {
>>         enum cd_types cd_type;
>>         int max_bus_width;
>>         unsigned int f_max;
>> +       bool support_vsel;
>>  };
>>  #endif /* __ASM_ARCH_IMX_ESDHC_H */
>> --
>> 1.7.1
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Ulf Hansson]

On 5 September 2013 18:04, Dong Aisheng <dongas86@gmail.com> wrote:
> Hi Ulf,
>
> On Thu, Sep 5, 2013 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
>>> Without proper pinctrl state, the card may not be able to work
>>> on high speed stablely. e.g. SDR104.
>>>
>>> This patch add pinctrl state switch code according to different
>>> uhs mode include 100mhz sate, 200mhz sate and normal state
>>> (50Mhz and below).
>>>
>>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>>> ---
>>>  drivers/mmc/host/sdhci-esdhc-imx.c          |  110 ++++++++++++++++++++++++++-
>>>  include/linux/platform_data/mmc-esdhc-imx.h |    4 +
>>>  2 files changed, 113 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> index 36b9f63..3e89863 100644
>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> @@ -49,6 +49,10 @@
>>>
>>>  #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64
>>>
>>> +/* pinctrl state */
>>> +#define ESDHC_PINCTRL_STATE_100MHZ     "state_100mhz"
>>> +#define ESDHC_PINCTRL_STATE_200MHZ     "state_200mhz"
>>> +
>>>  /*
>>>   * Our interpretation of the SDHCI_HOST_CONTROL register
>>>   */
>>> @@ -90,6 +94,10 @@ struct pltfm_imx_data {
>>>         u32 scratchpad;
>>>         enum imx_esdhc_type devtype;
>>>         struct pinctrl *pinctrl;
>>> +       struct pinctrl_state *pins_current;
>>> +       struct pinctrl_state *pins_default;
>>> +       struct pinctrl_state *pins_100mhz;
>>> +       struct pinctrl_state *pins_200mhz;
>>>         struct esdhc_platform_data boarddata;
>>>         struct clk *clk_ipg;
>>>         struct clk *clk_ahb;
>>> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
>>>         return ret;
>>>  }
>>>
>>> +static int esdhc_change_pinstate(struct sdhci_host *host,
>>> +                                               unsigned int uhs)
>>> +{
>>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> +       struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>> +       struct pinctrl_state *pinctrl;
>>> +       int ret;
>>> +
>>> +       dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
>>> +
>>> +       if (IS_ERR(imx_data->pinctrl) ||
>>> +               IS_ERR(imx_data->pins_default) ||
>>> +               IS_ERR(imx_data->pins_100mhz) ||
>>> +               IS_ERR(imx_data->pins_200mhz))
>>> +               return -EINVAL;
>>> +
>>> +       switch (uhs) {
>>> +       case MMC_TIMING_UHS_SDR12:
>>> +       case MMC_TIMING_UHS_SDR25:
>>> +       case MMC_TIMING_UHS_DDR50:
>>> +               pinctrl = imx_data->pins_default;
>>> +               break;
>>> +       case MMC_TIMING_UHS_SDR50:
>>> +               pinctrl = imx_data->pins_100mhz;
>>> +               break;
>>> +       case MMC_TIMING_UHS_SDR104:
>>> +               pinctrl = imx_data->pins_200mhz;
>>> +               break;
>>> +       default:
>>> +               /* back to default state for other legacy timing */
>>> +               pinctrl = imx_data->pins_default;
>>> +       }
>>> +
>>> +       if (pinctrl == imx_data->pins_current)
>>> +               return 0;
>>> +
>>> +       ret = pinctrl_select_state(imx_data->pinctrl, pinctrl);
>>> +       if (!ret)
>>> +               imx_data->pins_current = pinctrl;
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>> +{
>>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> +       struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>> +
>>> +       switch (uhs) {
>>> +       case MMC_TIMING_UHS_SDR12:
>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12;
>>> +               break;
>>> +       case MMC_TIMING_UHS_SDR25:
>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25;
>>> +               break;
>>> +       case MMC_TIMING_UHS_SDR50:
>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50;
>>> +               break;
>>> +       case MMC_TIMING_UHS_SDR104:
>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104;
>>> +               break;
>>> +       case MMC_TIMING_UHS_DDR50:
>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50;
>>> +               break;
>>> +       }
>>> +
>>> +       return esdhc_change_pinstate(host, uhs);
>>> +}
>>> +
>>>  static const struct sdhci_ops sdhci_esdhc_ops = {
>>>         .read_l = esdhc_readl_le,
>>>         .read_w = esdhc_readw_le,
>>> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>>>         .get_min_clock = esdhc_pltfm_get_min_clock,
>>>         .get_ro = esdhc_pltfm_get_ro,
>>>         .platform_bus_width = esdhc_pltfm_bus_width,
>>> +       .set_uhs_signaling = esdhc_set_uhs_signaling,
>>>         .platform_execute_tuning = esdhc_executing_tuning,
>>>  };
>>>
>>> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>>>
>>>         of_property_read_u32(np, "max-frequency", &boarddata->f_max);
>>>
>>> +       if (of_find_property(np, "no-1-8-v", NULL))
>>> +               boarddata->support_vsel = false;
>>> +       else
>>> +               boarddata->support_vsel = true;
>>> +
>>>         return 0;
>>>  }
>>>  #else
>>> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>>>         clk_prepare_enable(imx_data->clk_ipg);
>>>         clk_prepare_enable(imx_data->clk_ahb);
>>>
>>> -       imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>>> +       imx_data->pinctrl = devm_pinctrl_get(&pdev->dev);
>>>         if (IS_ERR(imx_data->pinctrl)) {
>>>                 err = PTR_ERR(imx_data->pinctrl);
>>>                 goto disable_clk;
>>>         }
>>>
>>> +       imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl,
>>> +                                               PINCTRL_STATE_DEFAULT);
>>
>> I believe you should look into the new pinctrl APIs and the new
>> sequence at device driver core probe, which automatically fetches
>> default, idle and sleep state pins. Additionally it sets the default
>> state.
>>
>> It should likely simplifies some code in this patch.
>>
>
> After looking into the pinctrl automatically fetch states, it seems
> above two line may could be replaced as:
> imx_data->pinctrl = (&pdev->dev)->pins->p;
> imx_data->pins_default = (&pdev->dev)->pins->default_state;

pinctrl_pm_select_default_state(dev)
pinctrl_pm_select_ilde_state(dev)
pinctrl_pm_select_sleep_state(dev)

The states are fetched from device core at probe.

Kind regards
Ulf Hansson


>
> However, i'm not sure touching the pinctrl internal implementations in
> device core is a good choice.
> So i may still like to keep the exist using.
>
> Regards
> Dong Aisheng
>
>> Kind regards
>> Ulf Hansson
>>
>>
>>> +       if (IS_ERR(imx_data->pins_default)) {
>>> +               err = PTR_ERR(imx_data->pins_default);
>>> +               dev_err(mmc_dev(host->mmc), "could not get default state\n");
>>> +               goto disable_clk;
>>> +       }
>>> +
>>>         host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>>
>>>         if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
>>> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>>>                 break;
>>>         }
>>>
>>> +       /* sdr50 and sdr104 needs work on 1.8v signal voltage */
>>> +       if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) {
>>> +               imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
>>> +                                               ESDHC_PINCTRL_STATE_100MHZ);
>>> +               imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
>>> +                                               ESDHC_PINCTRL_STATE_200MHZ);
>>> +               if (IS_ERR(imx_data->pins_100mhz) ||
>>> +                               IS_ERR(imx_data->pins_200mhz)) {
>>> +                       dev_warn(mmc_dev(host->mmc),
>>> +                               "could not get ultra high speed state, work on normal mode\n");
>>> +                       /* fall back to not support uhs by specify no 1.8v quirk */
>>> +                       host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>> +               }
>>> +       } else {
>>> +               host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>> +       }
>>> +
>>>         err = sdhci_add_host(host);
>>>         if (err)
>>>                 goto disable_clk;
>>> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
>>> index d44912d..a0f5a8f 100644
>>> --- a/include/linux/platform_data/mmc-esdhc-imx.h
>>> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
>>> @@ -10,6 +10,8 @@
>>>  #ifndef __ASM_ARCH_IMX_ESDHC_H
>>>  #define __ASM_ARCH_IMX_ESDHC_H
>>>
>>> +#include <linux/types.h>
>>> +
>>>  enum wp_types {
>>>         ESDHC_WP_NONE,          /* no WP, neither controller nor gpio */
>>>         ESDHC_WP_CONTROLLER,    /* mmc controller internal WP */
>>> @@ -32,6 +34,7 @@ enum cd_types {
>>>   * @cd_gpio:   gpio for card_detect interrupt
>>>   * @wp_type:   type of write_protect method (see wp_types enum above)
>>>   * @cd_type:   type of card_detect method (see cd_types enum above)
>>> + * @support_vsel:  indicate it supports 1.8v switching
>>>   */
>>>
>>>  struct esdhc_platform_data {
>>> @@ -41,5 +44,6 @@ struct esdhc_platform_data {
>>>         enum cd_types cd_type;
>>>         int max_bus_width;
>>>         unsigned int f_max;
>>> +       bool support_vsel;
>>>  };
>>>  #endif /* __ASM_ARCH_IMX_ESDHC_H */
>>> --
>>> 1.7.1
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Dong Aisheng]

Hi Ulf,

Thanks for the reply.

On Fri, Sep 13, 2013 at 10:01 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 5 September 2013 18:04, Dong Aisheng <dongas86@gmail.com> wrote:
>> Hi Ulf,
>>
>> On Thu, Sep 5, 2013 at 3:38 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
>>>> Without proper pinctrl state, the card may not be able to work
>>>> on high speed stablely. e.g. SDR104.
>>>>
>>>> This patch add pinctrl state switch code according to different
>>>> uhs mode include 100mhz sate, 200mhz sate and normal state
>>>> (50Mhz and below).
>>>>
>>>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci-esdhc-imx.c          |  110 ++++++++++++++++++++++++++-
>>>>  include/linux/platform_data/mmc-esdhc-imx.h |    4 +
>>>>  2 files changed, 113 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>>>> index 36b9f63..3e89863 100644
>>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>>> @@ -49,6 +49,10 @@
>>>>
>>>>  #define ESDHC_TUNING_BLOCK_PATTERN_LEN 64
>>>>
>>>> +/* pinctrl state */
>>>> +#define ESDHC_PINCTRL_STATE_100MHZ     "state_100mhz"
>>>> +#define ESDHC_PINCTRL_STATE_200MHZ     "state_200mhz"
>>>> +
>>>>  /*
>>>>   * Our interpretation of the SDHCI_HOST_CONTROL register
>>>>   */
>>>> @@ -90,6 +94,10 @@ struct pltfm_imx_data {
>>>>         u32 scratchpad;
>>>>         enum imx_esdhc_type devtype;
>>>>         struct pinctrl *pinctrl;
>>>> +       struct pinctrl_state *pins_current;
>>>> +       struct pinctrl_state *pins_default;
>>>> +       struct pinctrl_state *pins_100mhz;
>>>> +       struct pinctrl_state *pins_200mhz;
>>>>         struct esdhc_platform_data boarddata;
>>>>         struct clk *clk_ipg;
>>>>         struct clk *clk_ahb;
>>>> @@ -642,6 +650,75 @@ static int esdhc_executing_tuning(struct sdhci_host *host, u32 opcode)
>>>>         return ret;
>>>>  }
>>>>
>>>> +static int esdhc_change_pinstate(struct sdhci_host *host,
>>>> +                                               unsigned int uhs)
>>>> +{
>>>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +       struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>>> +       struct pinctrl_state *pinctrl;
>>>> +       int ret;
>>>> +
>>>> +       dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
>>>> +
>>>> +       if (IS_ERR(imx_data->pinctrl) ||
>>>> +               IS_ERR(imx_data->pins_default) ||
>>>> +               IS_ERR(imx_data->pins_100mhz) ||
>>>> +               IS_ERR(imx_data->pins_200mhz))
>>>> +               return -EINVAL;
>>>> +
>>>> +       switch (uhs) {
>>>> +       case MMC_TIMING_UHS_SDR12:
>>>> +       case MMC_TIMING_UHS_SDR25:
>>>> +       case MMC_TIMING_UHS_DDR50:
>>>> +               pinctrl = imx_data->pins_default;
>>>> +               break;
>>>> +       case MMC_TIMING_UHS_SDR50:
>>>> +               pinctrl = imx_data->pins_100mhz;
>>>> +               break;
>>>> +       case MMC_TIMING_UHS_SDR104:
>>>> +               pinctrl = imx_data->pins_200mhz;
>>>> +               break;
>>>> +       default:
>>>> +               /* back to default state for other legacy timing */
>>>> +               pinctrl = imx_data->pins_default;
>>>> +       }
>>>> +
>>>> +       if (pinctrl == imx_data->pins_current)
>>>> +               return 0;
>>>> +
>>>> +       ret = pinctrl_select_state(imx_data->pinctrl, pinctrl);
>>>> +       if (!ret)
>>>> +               imx_data->pins_current = pinctrl;
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static int esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>>> +{
>>>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +       struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>>> +
>>>> +       switch (uhs) {
>>>> +       case MMC_TIMING_UHS_SDR12:
>>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR12;
>>>> +               break;
>>>> +       case MMC_TIMING_UHS_SDR25:
>>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR25;
>>>> +               break;
>>>> +       case MMC_TIMING_UHS_SDR50:
>>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR50;
>>>> +               break;
>>>> +       case MMC_TIMING_UHS_SDR104:
>>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_SDR104;
>>>> +               break;
>>>> +       case MMC_TIMING_UHS_DDR50:
>>>> +               imx_data->uhs_mode = SDHCI_CTRL_UHS_DDR50;
>>>> +               break;
>>>> +       }
>>>> +
>>>> +       return esdhc_change_pinstate(host, uhs);
>>>> +}
>>>> +
>>>>  static const struct sdhci_ops sdhci_esdhc_ops = {
>>>>         .read_l = esdhc_readl_le,
>>>>         .read_w = esdhc_readw_le,
>>>> @@ -653,6 +730,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>>>>         .get_min_clock = esdhc_pltfm_get_min_clock,
>>>>         .get_ro = esdhc_pltfm_get_ro,
>>>>         .platform_bus_width = esdhc_pltfm_bus_width,
>>>> +       .set_uhs_signaling = esdhc_set_uhs_signaling,
>>>>         .platform_execute_tuning = esdhc_executing_tuning,
>>>>  };
>>>>
>>>> @@ -695,6 +773,11 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>>>>
>>>>         of_property_read_u32(np, "max-frequency", &boarddata->f_max);
>>>>
>>>> +       if (of_find_property(np, "no-1-8-v", NULL))
>>>> +               boarddata->support_vsel = false;
>>>> +       else
>>>> +               boarddata->support_vsel = true;
>>>> +
>>>>         return 0;
>>>>  }
>>>>  #else
>>>> @@ -757,12 +840,20 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>>>>         clk_prepare_enable(imx_data->clk_ipg);
>>>>         clk_prepare_enable(imx_data->clk_ahb);
>>>>
>>>> -       imx_data->pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>>>> +       imx_data->pinctrl = devm_pinctrl_get(&pdev->dev);
>>>>         if (IS_ERR(imx_data->pinctrl)) {
>>>>                 err = PTR_ERR(imx_data->pinctrl);
>>>>                 goto disable_clk;
>>>>         }
>>>>
>>>> +       imx_data->pins_default = pinctrl_lookup_state(imx_data->pinctrl,
>>>> +                                               PINCTRL_STATE_DEFAULT);
>>>
>>> I believe you should look into the new pinctrl APIs and the new
>>> sequence at device driver core probe, which automatically fetches
>>> default, idle and sleep state pins. Additionally it sets the default
>>> state.
>>>
>>> It should likely simplifies some code in this patch.
>>>
>>
>> After looking into the pinctrl automatically fetch states, it seems
>> above two line may could be replaced as:
>> imx_data->pinctrl = (&pdev->dev)->pins->p;
>> imx_data->pins_default = (&pdev->dev)->pins->default_state;
>
> pinctrl_pm_select_default_state(dev)
> pinctrl_pm_select_ilde_state(dev)
> pinctrl_pm_select_sleep_state(dev)
>
> The states are fetched from device core at probe.
>

I did not use idle and sleep state, instead, i need get another two private
defined states,  state_100mhz, state_200mhz and select between them,
so these API seems not so useful for my case.
Even i want to use this API, i can only use it for default state which seems
not so neccesary.

Regards
Dong Aisheng

> Kind regards
> Ulf Hansson
>
>
>>
>> However, i'm not sure touching the pinctrl internal implementations in
>> device core is a good choice.
>> So i may still like to keep the exist using.
>>
>> Regards
>> Dong Aisheng
>>
>>> Kind regards
>>> Ulf Hansson
>>>
>>>
>>>> +       if (IS_ERR(imx_data->pins_default)) {
>>>> +               err = PTR_ERR(imx_data->pins_default);
>>>> +               dev_err(mmc_dev(host->mmc), "could not get default state\n");
>>>> +               goto disable_clk;
>>>> +       }
>>>> +
>>>>         host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>>>>
>>>>         if (is_imx25_esdhc(imx_data) || is_imx35_esdhc(imx_data))
>>>> @@ -839,6 +930,23 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>>>>                 break;
>>>>         }
>>>>
>>>> +       /* sdr50 and sdr104 needs work on 1.8v signal voltage */
>>>> +       if ((boarddata->support_vsel) && is_imx6q_usdhc(imx_data)) {
>>>> +               imx_data->pins_100mhz = pinctrl_lookup_state(imx_data->pinctrl,
>>>> +                                               ESDHC_PINCTRL_STATE_100MHZ);
>>>> +               imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
>>>> +                                               ESDHC_PINCTRL_STATE_200MHZ);
>>>> +               if (IS_ERR(imx_data->pins_100mhz) ||
>>>> +                               IS_ERR(imx_data->pins_200mhz)) {
>>>> +                       dev_warn(mmc_dev(host->mmc),
>>>> +                               "could not get ultra high speed state, work on normal mode\n");
>>>> +                       /* fall back to not support uhs by specify no 1.8v quirk */
>>>> +                       host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>>> +               }
>>>> +       } else {
>>>> +               host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>>>> +       }
>>>> +
>>>>         err = sdhci_add_host(host);
>>>>         if (err)
>>>>                 goto disable_clk;
>>>> diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
>>>> index d44912d..a0f5a8f 100644
>>>> --- a/include/linux/platform_data/mmc-esdhc-imx.h
>>>> +++ b/include/linux/platform_data/mmc-esdhc-imx.h
>>>> @@ -10,6 +10,8 @@
>>>>  #ifndef __ASM_ARCH_IMX_ESDHC_H
>>>>  #define __ASM_ARCH_IMX_ESDHC_H
>>>>
>>>> +#include <linux/types.h>
>>>> +
>>>>  enum wp_types {
>>>>         ESDHC_WP_NONE,          /* no WP, neither controller nor gpio */
>>>>         ESDHC_WP_CONTROLLER,    /* mmc controller internal WP */
>>>> @@ -32,6 +34,7 @@ enum cd_types {
>>>>   * @cd_gpio:   gpio for card_detect interrupt
>>>>   * @wp_type:   type of write_protect method (see wp_types enum above)
>>>>   * @cd_type:   type of card_detect method (see cd_types enum above)
>>>> + * @support_vsel:  indicate it supports 1.8v switching
>>>>   */
>>>>
>>>>  struct esdhc_platform_data {
>>>> @@ -41,5 +44,6 @@ struct esdhc_platform_data {
>>>>         enum cd_types cd_type;
>>>>         int max_bus_width;
>>>>         unsigned int f_max;
>>>> +       bool support_vsel;
>>>>  };
>>>>  #endif /* __ASM_ARCH_IMX_ESDHC_H */
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Matt Sealey]

> +       if (of_find_property(np, "no-1-8-v", NULL))
> +               boarddata->support_vsel = false;
> +       else
> +               boarddata->support_vsel = true;
> +
>         return 0;

No, no, no, no, no :(

Please do not just strip the prefix from a definition in code, or just
convert the underscores into hyphens to make a device tree property.
SDHCI_QUIRK2_NO_1_8_V should never, ever, EVER become no-1-8-v and NO
DEFINITION FROM SOURCE CODE should EVER make it into a device tree.
This is how we ended up with manure like "dr_mode" in the USB
bindings. Please follow good common sense as well as the bindings.

In this case a suitable standard for describing a voltage would be
no-1v8 (replacing the decimal point with the v) which is how 99.9% of
schematics are written in the world where a period is not a desirable
character in a net name or component/signal description. But even that
is stupid and not expandable.

This property should probably be called "voltage-selects" or something
and you should put in a list of supported voltages (in uV units just
like cpufreq operating points and regulators, please, for consistency,
and update the binding).

Then you can parse the list and find out precisely what voltages you
can switch to - there are more possibilities than an implied 3-ish
volts and a switch to 1.8V for MMC. The standard for the OCR register
is a bunch of ranges - so specify which voltages you HAVE available,
if you can switch. The driver (as per standard) can determine if this
is in one range or another.

Lack of this property would imply that whatever the chip is driving
right now for logic is what it should stay at (support_vsel = false in
the code).

--
Matt Sealey <neko@bakuhatsu.net>

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Dong Aisheng]

Hi Matt,

Sorry for the late reply.

On Fri, Sep 06, 2013 at 02:40:06AM +0800, Matt Sealey wrote:
> > +       if (of_find_property(np, "no-1-8-v", NULL))
> > +               boarddata->support_vsel = false;
> > +       else
> > +               boarddata->support_vsel = true;
> > +
> >         return 0;
> 
> No, no, no, no, no :(
> 
> Please do not just strip the prefix from a definition in code, or just
> convert the underscores into hyphens to make a device tree property.
> SDHCI_QUIRK2_NO_1_8_V should never, ever, EVER become no-1-8-v and NO
> DEFINITION FROM SOURCE CODE should EVER make it into a device tree.
> This is how we ended up with manure like "dr_mode" in the USB
> bindings. Please follow good common sense as well as the bindings.
> 
> In this case a suitable standard for describing a voltage would be
> no-1v8 (replacing the decimal point with the v) which is how 99.9% of
> schematics are written in the world where a period is not a desirable
> character in a net name or component/signal description. But even that
> is stupid and not expandable.
> 

I somehow agree with your idea on the naming.
But currently i'm just using the standard property defined in:
Documentation/devicetree/bindings/mmc/mmc.txt
- no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
  this system, even if the controller claims it is.

> This property should probably be called "voltage-selects" or something
> and you should put in a list of supported voltages (in uV units just
> like cpufreq operating points and regulators, please, for consistency,
> and update the binding).
> 
> Then you can parse the list and find out precisely what voltages you
> can switch to - there are more possibilities than an implied 3-ish
> volts and a switch to 1.8V for MMC. The standard for the OCR register
> is a bunch of ranges - so specify which voltages you HAVE available,
> if you can switch. The driver (as per standard) can determine if this
> is in one range or another.
> 
> Lack of this property would imply that whatever the chip is driving
> right now for logic is what it should stay at (support_vsel = false in
> the code).
> 

Here the no-1-8-v is for indicating no 1.8v signal voltage switch support,
not the host supply voltage.
The spec only defines 1.8v/3.3v switch, so no-1-8-v seems ok for me currently.

And for host supply voltage, it's usually provided by host controller and
can be retrieved from host capability register.
And even if it's using an external regulator, this capability can still be
retrieved from the standard regulator API regulator_is_supported_voltage.
That's how sdhci driver used currently.
So i'm not sure if we really need add the voltage-selects on device tree
for mmc since that's regulator's work.

Anyway, above is just my initial thoughts on your question.
Since this issue actually is not related to this series,
maybe you could create a new mail thread about this if you want
or patching is welcome for the further discussion more specific.

Regards
Dong Aisheng

> --
> Matt Sealey <neko@bakuhatsu.net>

Re: [PATCH 5/8] sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode

[Matt Sealey]

On Wed, Sep 11, 2013 at 4:26 AM, Dong Aisheng <b29396@freescale.com> wrote:
> Hi Matt,
>
> I somehow agree with your idea on the naming.
> But currently i'm just using the standard property defined in:
> Documentation/devicetree/bindings/mmc/mmc.txt
> - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
>   this system, even if the controller claims it is.

Whoever did that needs kicking :<

>> This property should probably be called "voltage-selects" or something
>> and you should put in a list of supported voltages (in uV units just
>> like cpufreq operating points and regulators, please, for consistency,
>> and update the binding).
>
> Here the no-1-8-v is for indicating no 1.8v signal voltage switch support,
> not the host supply voltage.
> The spec only defines 1.8v/3.3v switch, so no-1-8-v seems ok for me currently.

I thought the spec also has a 1.2V mode.. so what do the MMC guys
propose, adding no-1-2-v properties too? And if there exists in the
future a 0.8V mode, another property? And another?

> That's how sdhci driver used currently.
> So i'm not sure if we really need add the voltage-selects on device tree
> for mmc since that's regulator's work.

Understood. My initial thought is simply that the MMC class should
define a behavior which you could assume - in this case, that a host
supports a certain set of voltages (for supply and for signalling),
and a card supports a certain set of voltages (for supply and for
signalling). In the case of the combinations here, this is all in the
spec and defined.

However, what device trees should do is essentially provide
information where the defaults or documentation contain assumptions,
and supplant them when necessary.

An MMC host controller given a regulator now knows it can change
voltages. But to what values? Most regulators are fixed or have a wide
range. A regulator that is defined for 1.8V and 3.3V is defined as
1.8V *through* 3.3V meaning you COULD set it to 2.5V if you wanted to.
A property defining the valid voltages is good here.

For the case where someone screwed a board design or a regulator is
weak somehow, you could put 1.85V in the property to say, this is my
close-enough-to-1.8V value. Otherwise what happens is you get a
regulator set failure if the range didn't include 1.8V, or weird
issues because the voltage isn't high enough *for the design*.

For signalling voltage, sure, there could be 1.8V and 3.3V defined in
the host and the card, but the same applies - assume those in the
driver, and the DT can define which ones are valid. For a driver
probing, it can look in the DT table and see, I have a matching
1800000uV and a matching 3300000uV voltage. If it sees 1200000uV and
it has no clue how to orient the system into using that voltage, it
ignores it (it couldn't use it anyway, since it would have no
knowledge of how to instruct a card to signal at that voltage). This
works for the supply regulator setting and the signalling setting with
a common, future-proof format..

In the future, it may work when the kernel is updated to support this
as-of-yet non-existing functionality and the DT would have been
correct from the start.

> Anyway, above is just my initial thoughts on your question.
> Since this issue actually is not related to this series,
> maybe you could create a new mail thread about this if you want
> or patching is welcome for the further discussion more specific.

Well, a new thread.. sure. That might be for another day (it took me 2
weeks to get back to this one, I don't know the next time I will have
enough time to go through LAKML :)

I have written it on my whiteboard for future discussion, so I'll
bring it up again as soon as I can..

-- 
Matt Sealey <neko@bakuhatsu.net>

[PATCH 6/8] mmc: sdhci-esdhc: correct pre_div for imx6q

[Dong Aisheng]

According to spec, the pre_div for imx6q should be 1, or the biggest clock
rate we can get is a half of host clock rate.
This may cause we can not get the proper clock rate as we want.
e.g. if the desired clock is 200Mhz, however, the host clock is 200Mhz too,
then it causes the actual clock we get is 100Mhz due to pre_div is 2.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index 86fcd5b..c8a773b 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -66,6 +66,9 @@ static inline void esdhc_set_clock(struct sdhci_host *host, unsigned int clock,
 	u32 temp, val;
 
 	dev = mmc_dev(host->mmc);
+	if (of_device_is_compatible(dev->of_node, "fsl,imx6q-usdhc"))
+		pre_div = 1;
+
 	if (clock == 0) {
 		if (of_device_is_compatible(dev->of_node, "fsl,imx6q-usdhc")) {
 			val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
-- 
1.7.1

[PATCH 7/8] mmc: sdhci-esdhc: set actual_clock in clock setting

[Dong Aisheng]

This enables access the actual_clock via sys.
root@imx6qsabreauto:~# cat /sys/kernel/debug/mmc0/ios
clock:          198000000 Hz
actual clock:   198000000 Hz
vdd:            17 (2.9 ~ 3.0 V)
bus mode:       2 (push-pull)
chip select:    0 (don't care)
power mode:     2 (on)
bus width:      2 (4 bits)
timing spec:    6 (sd uhs SDR104)
signal voltage: 0 (1.80 V)

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/mmc/host/sdhci-esdhc.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index c8a773b..eb22e5f 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -89,8 +89,9 @@ static inline void esdhc_set_clock(struct sdhci_host *host, unsigned int clock,
 	while (host_clock / pre_div / div > clock && div < 16)
 		div++;
 
+	host->mmc->actual_clock = host_clock / pre_div / div;
 	dev_dbg(mmc_dev(host->mmc), "desired SD clock: %d, actual: %d\n",
-		clock, host_clock / pre_div / div);
+		clock, host->mmc->actual_clock);
 
 	pre_div >>= 1;
 	div--;
-- 
1.7.1

[PATCH 8/8] ARM: dts: imx6qdl: add uhs pinctrl state for usdhc3

[Dong Aisheng]

This is needed for supporting ultra high speed cards like SD3.0 cards.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 arch/arm/boot/dts/imx6dl.dtsi            |   33 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/imx6q.dtsi             |   33 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |    4 ++-
 3 files changed, 69 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
index 2b3ecd6..e983b81 100644
--- a/arch/arm/boot/dts/imx6dl.dtsi
+++ b/arch/arm/boot/dts/imx6dl.dtsi
@@ -203,6 +203,39 @@
 							MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x17059
 						>;
 					};
+
+                                       pinctrl_usdhc3_3: usdhc3grp-3 { /* 100Mhz */
+                                               fsl,pins = <
+                                                       MX6DL_PAD_SD3_CMD__SD3_CMD 0x170B9
+                                                       MX6DL_PAD_SD3_CLK__SD3_CLK 0x100B9
+                                                       MX6DL_PAD_SD3_DAT0__SD3_DATA0 0x170B9
+                                                       MX6DL_PAD_SD3_DAT1__SD3_DATA1 0x170B9
+                                                       MX6DL_PAD_SD3_DAT2__SD3_DATA2 0x170B9
+                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x170B9
+                                                       MX6DL_PAD_SD3_DAT4__SD3_DATA4 0x170B9
+                                                       MX6DL_PAD_SD3_DAT5__SD3_DATA5 0x170B9
+                                                       MX6DL_PAD_SD3_DAT6__SD3_DATA6 0x170B9
+                                                       MX6DL_PAD_SD3_DAT7__SD3_DATA7 0x170B9
+                                                       MX6DL_PAD_GPIO_18__SD3_VSELECT 0x17059
+                                               >;
+                                       };
+
+                                       pinctrl_usdhc3_4: usdhc3grp-4 { /* 200Mhz */
+                                               fsl,pins = <
+                                                       MX6DL_PAD_SD3_CMD__SD3_CMD 0x170F9
+                                                       MX6DL_PAD_SD3_CLK__SD3_CLK 0x100F9
+                                                       MX6DL_PAD_SD3_DAT0__SD3_DATA0 0x170F9
+                                                       MX6DL_PAD_SD3_DAT1__SD3_DATA1 0x170F9
+                                                       MX6DL_PAD_SD3_DAT2__SD3_DATA2 0x170F9
+                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x170F9
+                                                       MX6DL_PAD_SD3_DAT4__SD3_DATA4 0x170F9
+                                                       MX6DL_PAD_SD3_DAT5__SD3_DATA5 0x170F9
+                                                       MX6DL_PAD_SD3_DAT6__SD3_DATA6 0x170F9
+                                                       MX6DL_PAD_SD3_DAT7__SD3_DATA7 0x170F9
+                                                       MX6DL_PAD_GPIO_18__SD3_VSELECT 0x17059
+                                               >;
+                                       };
+
 				};
 
 				weim {
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index ba09dc3..a63b623 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -337,6 +337,39 @@
 							MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x17059
 						>;
 					};
+
+                                       pinctrl_usdhc3_3: usdhc3grp-3 { /* 100Mhz */
+                                               fsl,pins = <
+                                                       MX6Q_PAD_SD3_CMD__SD3_CMD 0x170B9
+                                                       MX6Q_PAD_SD3_CLK__SD3_CLK 0x100B9
+                                                       MX6Q_PAD_SD3_DAT0__SD3_DATA0 0x170B9
+                                                       MX6Q_PAD_SD3_DAT1__SD3_DATA1 0x170B9
+                                                       MX6Q_PAD_SD3_DAT2__SD3_DATA2 0x170B9
+                                                       MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x170B9
+                                                       MX6Q_PAD_SD3_DAT4__SD3_DATA4 0x170B9
+                                                       MX6Q_PAD_SD3_DAT5__SD3_DATA5 0x170B9
+                                                       MX6Q_PAD_SD3_DAT6__SD3_DATA6 0x170B9
+                                                       MX6Q_PAD_SD3_DAT7__SD3_DATA7 0x170B9
+                                                       MX6Q_PAD_GPIO_18__SD3_VSELECT 0x17059
+                                               >;
+                                       };
+
+                                       pinctrl_usdhc3_4: usdhc3grp-4 { /* 200Mhz */
+                                               fsl,pins = <
+                                                       MX6Q_PAD_SD3_CMD__SD3_CMD 0x170F9
+                                                       MX6Q_PAD_SD3_CLK__SD3_CLK 0x100F9
+                                                       MX6Q_PAD_SD3_DAT0__SD3_DATA0 0x170F9
+                                                       MX6Q_PAD_SD3_DAT1__SD3_DATA1 0x170F9
+                                                       MX6Q_PAD_SD3_DAT2__SD3_DATA2 0x170F9
+                                                       MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x170F9
+                                                       MX6Q_PAD_SD3_DAT4__SD3_DATA4 0x170F9
+                                                       MX6Q_PAD_SD3_DAT5__SD3_DATA5 0x170F9
+                                                       MX6Q_PAD_SD3_DAT6__SD3_DATA6 0x170F9
+                                                       MX6Q_PAD_SD3_DAT7__SD3_DATA7 0x170F9
+                                                       MX6Q_PAD_GPIO_18__SD3_VSELECT 0x17059
+                                               >;
+                                       };
+
 				};
 
 				usdhc4 {
diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index e994011..c2c4d85 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -52,8 +52,10 @@
 };
 
 &usdhc3 {
-	pinctrl-names = "default";
+	pinctrl-names = "default", "state_100mhz", "state_200mhz";
 	pinctrl-0 = <&pinctrl_usdhc3_1>;
+	pinctrl-1 = <&pinctrl_usdhc3_3>;
+	pinctrl-2 = <&pinctrl_usdhc3_4>;
 	cd-gpios = <&gpio6 15 0>;
 	wp-gpios = <&gpio1 13 0>;
 	status = "okay";
-- 
1.7.1

Re: [PATCH 8/8] ARM: dts: imx6qdl: add uhs pinctrl state for usdhc3

[Shawn Guo]

On Wed, Sep 04, 2013 at 08:54:17PM +0800, Dong Aisheng wrote:
> This is needed for supporting ultra high speed cards like SD3.0 cards.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  arch/arm/boot/dts/imx6dl.dtsi            |   33 ++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/imx6q.dtsi             |   33 ++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |    4 ++-
>  3 files changed, 69 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
> index 2b3ecd6..e983b81 100644
> --- a/arch/arm/boot/dts/imx6dl.dtsi
> +++ b/arch/arm/boot/dts/imx6dl.dtsi
> @@ -203,6 +203,39 @@
>  							MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x17059
>  						>;
>  					};
> +
> +                                       pinctrl_usdhc3_3: usdhc3grp-3 { /* 100Mhz */
> +                                               fsl,pins = <
> +                                                       MX6DL_PAD_SD3_CMD__SD3_CMD 0x170B9
> +                                                       MX6DL_PAD_SD3_CLK__SD3_CLK 0x100B9
> +                                                       MX6DL_PAD_SD3_DAT0__SD3_DATA0 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT1__SD3_DATA1 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT2__SD3_DATA2 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT4__SD3_DATA4 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT5__SD3_DATA5 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT6__SD3_DATA6 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT7__SD3_DATA7 0x170B9
> +                                                       MX6DL_PAD_GPIO_18__SD3_VSELECT 0x17059

The patch needs to be rebased on my for-next, or linux-next or v3.12-rc1
(to be available).  Also please use lowercase for hex values.

Shawn

> +                                               >;
> +                                       };
> +
> +                                       pinctrl_usdhc3_4: usdhc3grp-4 { /* 200Mhz */
> +                                               fsl,pins = <
> +                                                       MX6DL_PAD_SD3_CMD__SD3_CMD 0x170F9
> +                                                       MX6DL_PAD_SD3_CLK__SD3_CLK 0x100F9
> +                                                       MX6DL_PAD_SD3_DAT0__SD3_DATA0 0x170F9
> +                                                       MX6DL_PAD_SD3_DAT1__SD3_DATA1 0x170F9
> +                                                       MX6DL_PAD_SD3_DAT2__SD3_DATA2 0x170F9
> +                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x170F9
> +                                                       MX6DL_PAD_SD3_DAT4__SD3_DATA4 0x170F9
> +                                                       MX6DL_PAD_SD3_DAT5__SD3_DATA5 0x170F9
> +                                                       MX6DL_PAD_SD3_DAT6__SD3_DATA6 0x170F9
> +                                                       MX6DL_PAD_SD3_DAT7__SD3_DATA7 0x170F9
> +                                                       MX6DL_PAD_GPIO_18__SD3_VSELECT 0x17059
> +                                               >;
> +                                       };
> +
>  				};
>  
>  				weim {
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index ba09dc3..a63b623 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -337,6 +337,39 @@
>  							MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x17059
>  						>;
>  					};
> +
> +                                       pinctrl_usdhc3_3: usdhc3grp-3 { /* 100Mhz */
> +                                               fsl,pins = <
> +                                                       MX6Q_PAD_SD3_CMD__SD3_CMD 0x170B9
> +                                                       MX6Q_PAD_SD3_CLK__SD3_CLK 0x100B9
> +                                                       MX6Q_PAD_SD3_DAT0__SD3_DATA0 0x170B9
> +                                                       MX6Q_PAD_SD3_DAT1__SD3_DATA1 0x170B9
> +                                                       MX6Q_PAD_SD3_DAT2__SD3_DATA2 0x170B9
> +                                                       MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x170B9
> +                                                       MX6Q_PAD_SD3_DAT4__SD3_DATA4 0x170B9
> +                                                       MX6Q_PAD_SD3_DAT5__SD3_DATA5 0x170B9
> +                                                       MX6Q_PAD_SD3_DAT6__SD3_DATA6 0x170B9
> +                                                       MX6Q_PAD_SD3_DAT7__SD3_DATA7 0x170B9
> +                                                       MX6Q_PAD_GPIO_18__SD3_VSELECT 0x17059
> +                                               >;
> +                                       };
> +
> +                                       pinctrl_usdhc3_4: usdhc3grp-4 { /* 200Mhz */
> +                                               fsl,pins = <
> +                                                       MX6Q_PAD_SD3_CMD__SD3_CMD 0x170F9
> +                                                       MX6Q_PAD_SD3_CLK__SD3_CLK 0x100F9
> +                                                       MX6Q_PAD_SD3_DAT0__SD3_DATA0 0x170F9
> +                                                       MX6Q_PAD_SD3_DAT1__SD3_DATA1 0x170F9
> +                                                       MX6Q_PAD_SD3_DAT2__SD3_DATA2 0x170F9
> +                                                       MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x170F9
> +                                                       MX6Q_PAD_SD3_DAT4__SD3_DATA4 0x170F9
> +                                                       MX6Q_PAD_SD3_DAT5__SD3_DATA5 0x170F9
> +                                                       MX6Q_PAD_SD3_DAT6__SD3_DATA6 0x170F9
> +                                                       MX6Q_PAD_SD3_DAT7__SD3_DATA7 0x170F9
> +                                                       MX6Q_PAD_GPIO_18__SD3_VSELECT 0x17059
> +                                               >;
> +                                       };
> +
>  				};
>  
>  				usdhc4 {
> diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> index e994011..c2c4d85 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> @@ -52,8 +52,10 @@
>  };
>  
>  &usdhc3 {
> -	pinctrl-names = "default";
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
>  	pinctrl-0 = <&pinctrl_usdhc3_1>;
> +	pinctrl-1 = <&pinctrl_usdhc3_3>;
> +	pinctrl-2 = <&pinctrl_usdhc3_4>;
>  	cd-gpios = <&gpio6 15 0>;
>  	wp-gpios = <&gpio1 13 0>;
>  	status = "okay";
> -- 
> 1.7.1
> 
> 

Re: [PATCH 8/8] ARM: dts: imx6qdl: add uhs pinctrl state for usdhc3

[Dong Aisheng]

On Thu, Sep 5, 2013 at 2:43 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Sep 04, 2013 at 08:54:17PM +0800, Dong Aisheng wrote:
>> This is needed for supporting ultra high speed cards like SD3.0 cards.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  arch/arm/boot/dts/imx6dl.dtsi            |   33 ++++++++++++++++++++++++++++++
>>  arch/arm/boot/dts/imx6q.dtsi             |   33 ++++++++++++++++++++++++++++++
>>  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |    4 ++-
>>  3 files changed, 69 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
>> index 2b3ecd6..e983b81 100644
>> --- a/arch/arm/boot/dts/imx6dl.dtsi
>> +++ b/arch/arm/boot/dts/imx6dl.dtsi
>> @@ -203,6 +203,39 @@
>>                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x17059
>>                                               >;
>>                                       };
>> +
>> +                                       pinctrl_usdhc3_3: usdhc3grp-3 { /* 100Mhz */
>> +                                               fsl,pins = <
>> +                                                       MX6DL_PAD_SD3_CMD__SD3_CMD 0x170B9
>> +                                                       MX6DL_PAD_SD3_CLK__SD3_CLK 0x100B9
>> +                                                       MX6DL_PAD_SD3_DAT0__SD3_DATA0 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT1__SD3_DATA1 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT2__SD3_DATA2 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT4__SD3_DATA4 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT5__SD3_DATA5 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT6__SD3_DATA6 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT7__SD3_DATA7 0x170B9
>> +                                                       MX6DL_PAD_GPIO_18__SD3_VSELECT 0x17059
>
> The patch needs to be rebased on my for-next, or linux-next or v3.12-rc1
> (to be available).  Also please use lowercase for hex values.
>

Okay, got it.

Regards
Dong Aisheng

> Shawn
>
>> +                                               >;
>> +                                       };
>> +
>> +                                       pinctrl_usdhc3_4: usdhc3grp-4 { /* 200Mhz */
>> +                                               fsl,pins = <
>> +                                                       MX6DL_PAD_SD3_CMD__SD3_CMD 0x170F9
>> +                                                       MX6DL_PAD_SD3_CLK__SD3_CLK 0x100F9
>> +                                                       MX6DL_PAD_SD3_DAT0__SD3_DATA0 0x170F9
>> +                                                       MX6DL_PAD_SD3_DAT1__SD3_DATA1 0x170F9
>> +                                                       MX6DL_PAD_SD3_DAT2__SD3_DATA2 0x170F9
>> +                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x170F9
>> +                                                       MX6DL_PAD_SD3_DAT4__SD3_DATA4 0x170F9
>> +                                                       MX6DL_PAD_SD3_DAT5__SD3_DATA5 0x170F9
>> +                                                       MX6DL_PAD_SD3_DAT6__SD3_DATA6 0x170F9
>> +                                                       MX6DL_PAD_SD3_DAT7__SD3_DATA7 0x170F9
>> +                                                       MX6DL_PAD_GPIO_18__SD3_VSELECT 0x17059
>> +                                               >;
>> +                                       };
>> +
>>                               };
>>
>>                               weim {
>> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
>> index ba09dc3..a63b623 100644
>> --- a/arch/arm/boot/dts/imx6q.dtsi
>> +++ b/arch/arm/boot/dts/imx6q.dtsi
>> @@ -337,6 +337,39 @@
>>                                                       MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x17059
>>                                               >;
>>                                       };
>> +
>> +                                       pinctrl_usdhc3_3: usdhc3grp-3 { /* 100Mhz */
>> +                                               fsl,pins = <
>> +                                                       MX6Q_PAD_SD3_CMD__SD3_CMD 0x170B9
>> +                                                       MX6Q_PAD_SD3_CLK__SD3_CLK 0x100B9
>> +                                                       MX6Q_PAD_SD3_DAT0__SD3_DATA0 0x170B9
>> +                                                       MX6Q_PAD_SD3_DAT1__SD3_DATA1 0x170B9
>> +                                                       MX6Q_PAD_SD3_DAT2__SD3_DATA2 0x170B9
>> +                                                       MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x170B9
>> +                                                       MX6Q_PAD_SD3_DAT4__SD3_DATA4 0x170B9
>> +                                                       MX6Q_PAD_SD3_DAT5__SD3_DATA5 0x170B9
>> +                                                       MX6Q_PAD_SD3_DAT6__SD3_DATA6 0x170B9
>> +                                                       MX6Q_PAD_SD3_DAT7__SD3_DATA7 0x170B9
>> +                                                       MX6Q_PAD_GPIO_18__SD3_VSELECT 0x17059
>> +                                               >;
>> +                                       };
>> +
>> +                                       pinctrl_usdhc3_4: usdhc3grp-4 { /* 200Mhz */
>> +                                               fsl,pins = <
>> +                                                       MX6Q_PAD_SD3_CMD__SD3_CMD 0x170F9
>> +                                                       MX6Q_PAD_SD3_CLK__SD3_CLK 0x100F9
>> +                                                       MX6Q_PAD_SD3_DAT0__SD3_DATA0 0x170F9
>> +                                                       MX6Q_PAD_SD3_DAT1__SD3_DATA1 0x170F9
>> +                                                       MX6Q_PAD_SD3_DAT2__SD3_DATA2 0x170F9
>> +                                                       MX6Q_PAD_SD3_DAT3__SD3_DATA3 0x170F9
>> +                                                       MX6Q_PAD_SD3_DAT4__SD3_DATA4 0x170F9
>> +                                                       MX6Q_PAD_SD3_DAT5__SD3_DATA5 0x170F9
>> +                                                       MX6Q_PAD_SD3_DAT6__SD3_DATA6 0x170F9
>> +                                                       MX6Q_PAD_SD3_DAT7__SD3_DATA7 0x170F9
>> +                                                       MX6Q_PAD_GPIO_18__SD3_VSELECT 0x17059
>> +                                               >;
>> +                                       };
>> +
>>                               };
>>
>>                               usdhc4 {
>> diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
>> index e994011..c2c4d85 100644
>> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
>> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
>> @@ -52,8 +52,10 @@
>>  };
>>
>>  &usdhc3 {
>> -     pinctrl-names = "default";
>> +     pinctrl-names = "default", "state_100mhz", "state_200mhz";
>>       pinctrl-0 = <&pinctrl_usdhc3_1>;
>> +     pinctrl-1 = <&pinctrl_usdhc3_3>;
>> +     pinctrl-2 = <&pinctrl_usdhc3_4>;
>>       cd-gpios = <&gpio6 15 0>;
>>       wp-gpios = <&gpio1 13 0>;
>>       status = "okay";
>> --
>> 1.7.1
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] ARM: dts: imx6qdl: add uhs pinctrl state for usdhc3

[Sascha Hauer]

On Wed, Sep 04, 2013 at 08:54:17PM +0800, Dong Aisheng wrote:
> This is needed for supporting ultra high speed cards like SD3.0 cards.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> ---
>  arch/arm/boot/dts/imx6dl.dtsi            |   33 ++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/imx6q.dtsi             |   33 ++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |    4 ++-
>  3 files changed, 69 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
> index 2b3ecd6..e983b81 100644
> --- a/arch/arm/boot/dts/imx6dl.dtsi
> +++ b/arch/arm/boot/dts/imx6dl.dtsi
> @@ -203,6 +203,39 @@
>  							MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x17059
>  						>;
>  					};
> +
> +                                       pinctrl_usdhc3_3: usdhc3grp-3 { /* 100Mhz */
> +                                               fsl,pins = <
> +                                                       MX6DL_PAD_SD3_CMD__SD3_CMD 0x170B9
> +                                                       MX6DL_PAD_SD3_CLK__SD3_CLK 0x100B9
> +                                                       MX6DL_PAD_SD3_DAT0__SD3_DATA0 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT1__SD3_DATA1 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT2__SD3_DATA2 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT4__SD3_DATA4 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT5__SD3_DATA5 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT6__SD3_DATA6 0x170B9
> +                                                       MX6DL_PAD_SD3_DAT7__SD3_DATA7 0x170B9
> +                                                       MX6DL_PAD_GPIO_18__SD3_VSELECT 0x17059
> +                                               >;
> +                                       };

No please.

in pinctrl_usdhc3_x 'x' is the mux option. Lets do not degrade this to
an arbitrary number. We should use prefixes like '4bit', '100mhz' or
combinations thereof for further options.

Sascha

-- 
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 8/8] ARM: dts: imx6qdl: add uhs pinctrl state for usdhc3

[Dong Aisheng]

On Thu, Sep 5, 2013 at 4:03 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Sep 04, 2013 at 08:54:17PM +0800, Dong Aisheng wrote:
>> This is needed for supporting ultra high speed cards like SD3.0 cards.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> ---
>>  arch/arm/boot/dts/imx6dl.dtsi            |   33 ++++++++++++++++++++++++++++++
>>  arch/arm/boot/dts/imx6q.dtsi             |   33 ++++++++++++++++++++++++++++++
>>  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |    4 ++-
>>  3 files changed, 69 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
>> index 2b3ecd6..e983b81 100644
>> --- a/arch/arm/boot/dts/imx6dl.dtsi
>> +++ b/arch/arm/boot/dts/imx6dl.dtsi
>> @@ -203,6 +203,39 @@
>>                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x17059
>>                                               >;
>>                                       };
>> +
>> +                                       pinctrl_usdhc3_3: usdhc3grp-3 { /* 100Mhz */
>> +                                               fsl,pins = <
>> +                                                       MX6DL_PAD_SD3_CMD__SD3_CMD 0x170B9
>> +                                                       MX6DL_PAD_SD3_CLK__SD3_CLK 0x100B9
>> +                                                       MX6DL_PAD_SD3_DAT0__SD3_DATA0 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT1__SD3_DATA1 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT2__SD3_DATA2 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT3__SD3_DATA3 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT4__SD3_DATA4 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT5__SD3_DATA5 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT6__SD3_DATA6 0x170B9
>> +                                                       MX6DL_PAD_SD3_DAT7__SD3_DATA7 0x170B9
>> +                                                       MX6DL_PAD_GPIO_18__SD3_VSELECT 0x17059
>> +                                               >;
>> +                                       };
>
> No please.
>
> in pinctrl_usdhc3_x 'x' is the mux option. Lets do not degrade this to
> an arbitrary number. We should use prefixes like '4bit', '100mhz' or
> combinations thereof for further options.
>

The original design does not have this assumption.
The 'x' includes different mux or config.
It may be hard to name for all different configs since it's board related.
However i don't think it's bad idea for this case.
How about pinctrl_usdhc3_3_100mhz and pinctrl_usdhc3_3_200mhz?
e.g.
pinctrl_usdhc3_3: usdhc3grp-3 { /* default */
    fsl,pins = <...>;
}

pinctrl_usdhc3_3_100mhz: usdhc3grp-3-100mhz {
    fsl,pins = <...>;
}

pinctrl_usdhc3_3_200mhz: usdhc3grp-3-200mhz {
    fsl,pins = <...>;
}

Regards
Dong Aisheng

> Sascha
>
> --
> 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 |
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] mmc: sdhci-esdhc-imx: add SD3.0 support

[Ulf Hansson]

On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
> This patch series add SD3.0 support for i.MX6Q/DL.
> Since freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
> the standard tuning process defined in host controller spec v3.0.
> So we add a hook to allow execute platform specific tuning instead of
> standard host controller tuning.
>
> The main difference are:
> 1) not only generate Buffer Read Ready interrupt when tuning is performing.
> It generates all other DATA interrupts like the normal data command.
> 2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
> instead it's controlled by SW.
> 3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
> it's controlled by SW.
> 4) the clock delay for every tuning is set by SW.

An overall question. Do you have any thoughts around periodic/idle
re-tuning. It is has nothing directly to do with this patchset, but
since you implemented the tuning sequence here, maybe you have some
thoughts around it?

Kind regards
Ulf Hansson

>
> Tested on i.MX6Q Sabreauto board.
>
> The series is based on latest Linus tree.
>
> Dong Aisheng (8):
>   mmc: sdhci: add hooks for platform specific tuning
>   mmc: sdhci: allow platform access of sdhci_send_command
>   sdhci: sdhci-esdhc-imx: support real clock on and off for imx6q
>   sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support
>   sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode
>   mmc: sdhci-esdhc: correct pre_div for imx6q
>   mmc: sdhci-esdhc: set actual_clock in clock setting
>   ARM: dts: imx6qdl: add uhs pinctrl state for usdhc3
>
>  arch/arm/boot/dts/imx6dl.dtsi               |   33 +++
>  arch/arm/boot/dts/imx6q.dtsi                |   33 +++
>  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi    |    4 +-
>  drivers/mmc/host/sdhci-esdhc-imx.c          |  307 ++++++++++++++++++++++++++-
>  drivers/mmc/host/sdhci-esdhc.h              |   35 +++-
>  drivers/mmc/host/sdhci.c                    |   12 +-
>  drivers/mmc/host/sdhci.h                    |    3 +
>  include/linux/platform_data/mmc-esdhc-imx.h |    4 +
>  8 files changed, 419 insertions(+), 12 deletions(-)
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] mmc: sdhci-esdhc-imx: add SD3.0 support

[Dong Aisheng]

Hi Ulf,

On Thu, Sep 5, 2013 at 3:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 4 September 2013 14:54, Dong Aisheng <b29396@freescale.com> wrote:
>> This patch series add SD3.0 support for i.MX6Q/DL.
>> Since freescale i.MX6Q/DL uSDHC clock tuning progress is a little different from
>> the standard tuning process defined in host controller spec v3.0.
>> So we add a hook to allow execute platform specific tuning instead of
>> standard host controller tuning.
>>
>> The main difference are:
>> 1) not only generate Buffer Read Ready interrupt when tuning is performing.
>> It generates all other DATA interrupts like the normal data command.
>> 2) SDHCI_CTRL_EXEC_TUNING is not automatically cleared by HW,
>> instead it's controlled by SW.
>> 3) SDHCI_CTRL_TUNED_CLK is not automatically set by HW,
>> it's controlled by SW.
>> 4) the clock delay for every tuning is set by SW.
>
> An overall question. Do you have any thoughts around periodic/idle
> re-tuning. It is has nothing directly to do with this patchset, but
> since you implemented the tuning sequence here, maybe you have some
> thoughts around it?
>

It seems sdhci already implements re-tuning mode1 based on timer.
The i.MX6 supports auto-retuning, so we may not use timer
based(periodic) re-tuning.
I plan to add auto-retuning support based on standard host
controller spec into sdhci driver later if no one else does it.

Regards
Dong Aisheng

> Kind regards
> Ulf Hansson
>
>>
>> Tested on i.MX6Q Sabreauto board.
>>
>> The series is based on latest Linus tree.
>>
>> Dong Aisheng (8):
>>   mmc: sdhci: add hooks for platform specific tuning
>>   mmc: sdhci: allow platform access of sdhci_send_command
>>   sdhci: sdhci-esdhc-imx: support real clock on and off for imx6q
>>   sdhci: sdhci-esdhci-imx: add sd3.0 clock tuning support
>>   sdhci: sdhci-esdhc-imx: change pinctrl state according to uhs mode
>>   mmc: sdhci-esdhc: correct pre_div for imx6q
>>   mmc: sdhci-esdhc: set actual_clock in clock setting
>>   ARM: dts: imx6qdl: add uhs pinctrl state for usdhc3
>>
>>  arch/arm/boot/dts/imx6dl.dtsi               |   33 +++
>>  arch/arm/boot/dts/imx6q.dtsi                |   33 +++
>>  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi    |    4 +-
>>  drivers/mmc/host/sdhci-esdhc-imx.c          |  307 ++++++++++++++++++++++++++-
>>  drivers/mmc/host/sdhci-esdhc.h              |   35 +++-
>>  drivers/mmc/host/sdhci.c                    |   12 +-
>>  drivers/mmc/host/sdhci.h                    |    3 +
>>  include/linux/platform_data/mmc-esdhc-imx.h |    4 +
>>  8 files changed, 419 insertions(+), 12 deletions(-)
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel