mmc: sdhci: fixes and enhancements

mmc: sdhci: fixes and enhancements

[Kevin Liu]

This patchset does as follows:
[PATCH v6 01/14] mmc: sdhci: fix transfer mode setting bug for cmds w/o data transfer
[PATCH v6 02/14] mmc: sdhci: refine voltage support caps setting
[PATCH v6 03/14] mmc: sdhci: refine code for sd clock disable/enable in set ios
[PATCH v6 04/14] mmc: sdhci: keep the saved clock var up to date
[PATCH v6 05/14] mmc: sdhci: fix null return check of regulator_get
[PATCH v6 06/14] mmc: sdhci-pxav3: controller can't get base clock
[PATCH v6 07/14] mmc: host: adjust uhs timing value
[PATCH v6 08/14] mmc: sdhci: enhance preset value function
[PATCH v6 09/14] mmc: sdhci-pxav3: add quirks2
[PATCH v6 10/14] mmc: sdhci: introduce signal_voltage_switch callback function
[PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc
[PATCH v6 12/14] mmc: sdhci-pxav3: remove set_uhs_signaling function
[PATCH v6 13/14] mmc: sdhci: solve several vmmc/vqmmc regulator issues
[PATCH v6 14/14] mmc: sdhci: add function to get retunig timer count

changelog v1->v2:
        - remove the patch "mmc: sdhci-pxav3: fix build error"
        - update the patch 05/15 by avoid warning with return null
        - add patches 06/15 ~ 08/15
changelog v2->v3:
        - update some comments
        - add patches 09/15 ~ 11/15
changelog v3->v4:
        - update the patch 01/15 with data null check
        - add patches 12/15 ~ 15/15
changelog v4->v5:
        - drop below two patches since Johan is updating voltage switch code:
        - drop the patch "mmc: core: add new 1.8v flag for mmc"
        - drop the patch "mmc: sdhci: add mmc 1.8v signal voltage switch function"
        - drop the patch "mmc: sdhci-pxav3: add signal_voltage_switch function" which calls plat callback function
        - update the patch 07/13 with adding function get_max_clock
        - update the patch 03/13 with voltage setting
        - add patch 02/13
changelog v5->v6:
	- drop the patch "mmc: sdhci: use regulator min/max voltage range according to spec"
	- add patch 13/14
	- add patch 14/14

Kevin Liu (14)
 mmc: sdhci: fix transfer mode setting bug for cmds w/o data transfer
 mmc: sdhci: refine voltage support caps setting
 mmc: sdhci: refine code for sd clock disable/enable in set ios
 mmc: sdhci: keep the saved clock var up to date
 mmc: sdhci: fix null return check of regulator_get
 mmc: sdhci-pxav3: controller can't get base clock
 mmc: host: adjust uhs timing value
 mmc: sdhci: enhance preset value function
 mmc: sdhci-pxav3: add quirks2
 mmc: sdhci: introduce signal_voltage_switch callback function
 mmc: sdhci: fix the bug that DDR50 can't work for emmc
 mmc: sdhci-pxav3: remove set_uhs_signaling function
 mmc: sdhci: solve several vmmc/vqmmc regulator issues
 mmc: sdhci: add function to get retunig timer count

 drivers/mmc/core/sd.c                   |   17 --
 drivers/mmc/host/sdhci-pxav3.c          |   44 +-----
 drivers/mmc/host/sdhci.c                |  260 ++++++++++++++++++++-----------
 drivers/mmc/host/sdhci.h                |   14 ++
 include/linux/mmc/host.h                |   13 +-
 include/linux/mmc/sdhci.h               |    2 +-
 include/linux/platform_data/pxa_sdhci.h |    2 +
 7 files changed, 202 insertions(+), 150 deletions(-)

[PATCH v6 01/14] mmc: sdhci: fix transfer mode setting bug for cmds w/o data transfer

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

Commands without data transfer like cmd5/cmd7 will use previous
transfer mode setting, which may lead to error since some bits
may have been set unexpectedly.
For example, cmd5 following cmd18/cmd25 will have timeout error
since audo cmd23 has been enabled.

Reviewed By: Girish K S <girish.shivananjappa@linaro.org>
Reviewed-by: Zhangfei Gao <zhangfei.gao@marvell.com>
Signed-off-by: Jialing Fu <jlfu@marvell.com>
Signed-off-by: Tim Wang <wangtt@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7922adb..dc493ba 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -886,8 +886,21 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
 	u16 mode;
 	struct mmc_data *data = cmd->data;
 
-	if (data == NULL)
+	if (!data) {
+		if (cmd->opcode == MMC_SEND_TUNING_BLOCK ||
+			cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200)
+			/*
+			 * The tuning block is sent by the card to the host
+			 * controller. So we set the TRNS_READ bit in the
+			 * Transfer Mode register. This also takes care of
+			 * setting DMA Enable and Multi Block Select in the
+			 * same register to 0.
+			 */
+			sdhci_writew(host, SDHCI_TRNS_READ, SDHCI_TRANSFER_MODE);
+		else
+			sdhci_writew(host, 0, SDHCI_TRANSFER_MODE);
 		return;
+	}
 
 	WARN_ON(!host->data);
 
@@ -1850,13 +1863,6 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 				     SDHCI_BLOCK_SIZE);
 		}
 
-		/*
-		 * The tuning block is sent by the card to the host controller.
-		 * So we set the TRNS_READ bit in the Transfer Mode register.
-		 * This also takes care of setting DMA Enable and Multi Block
-		 * Select in the same register to 0.
-		 */
-		sdhci_writew(host, SDHCI_TRNS_READ, SDHCI_TRANSFER_MODE);
 
 		sdhci_send_command(host, &cmd);
 
-- 
1.7.0.4

[PATCH v6 02/14] mmc: sdhci: refine voltage support caps setting

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

No need to disable the voltage support caps if
it was NOT supported originally

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index dc493ba..94dab9c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2920,15 +2920,15 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (host->vmmc) {
 		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
 			3300000);
-		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
+		if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_330))
 			caps[0] &= ~SDHCI_CAN_VDD_330;
 		ret = regulator_is_supported_voltage(host->vmmc, 3000000,
 			3000000);
-		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
+		if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_300))
 			caps[0] &= ~SDHCI_CAN_VDD_300;
 		ret = regulator_is_supported_voltage(host->vmmc, 1800000,
 			1800000);
-		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
+		if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_180))
 			caps[0] &= ~SDHCI_CAN_VDD_180;
 	}
 #endif /* CONFIG_REGULATOR */
-- 
1.7.0.4

[PATCH v6 03/14] mmc: sdhci: refine code for sd clock disable/enable in set ios

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

With preset value enabled, there are two continuous times
of sd clock disable/enable. They can be combined into one
to save time and make code cleaner.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |   29 +++++++++--------------------
 1 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 94dab9c..eb621dd 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1449,33 +1449,22 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 				ctrl_2 |= SDHCI_CTRL_DRV_TYPE_C;
 
 			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
-		} else {
-			/*
-			 * According to SDHC Spec v3.00, if the Preset Value
-			 * Enable in the Host Control 2 register is set, we
-			 * need to reset SD Clock Enable before changing High
-			 * Speed Enable to avoid generating clock gliches.
-			 */
-
-			/* Reset SD Clock Enable */
-			clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-			clk &= ~SDHCI_CLOCK_CARD_EN;
-			sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
-
-			sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
-
-			/* Re-enable SD Clock */
-			clock = host->clock;
-			host->clock = 0;
-			sdhci_set_clock(host, clock);
 		}
 
-
 		/* Reset SD Clock Enable */
 		clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
 		clk &= ~SDHCI_CLOCK_CARD_EN;
 		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
+		/*
+		 * According to SDHC Spec v3.00, if the Preset Value
+		 * Enable in the Host Control 2 register is set, we
+		 * need to reset SD Clock Enable before changing High
+		 * Speed Enable to avoid generating clock gliches.
+		 */
+		if (ctrl_2 & SDHCI_CTRL_PRESET_VAL_ENABLE)
+			sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+
 		if (host->ops->set_uhs_signaling)
 			host->ops->set_uhs_signaling(host, ios->timing);
 		else {
-- 
1.7.0.4

[PATCH v6 04/14] mmc: sdhci: keep the saved clock var up to date

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

The clock rate set to the sdh controller may not exactly as requested
by the mmc core, this patch make the clock rate saved in the mmc_ios
and sdhci_host updated with the actual setting as in the controller. Thus
"/sys/kernel/debug/mmcx/ios" and card detect prints can show the correct
clock rate.
Otherwise, mmc power class may be set to wrong value with wrong ios.clock

Signed-off-by: Bin Wang <binw@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index eb621dd..8e6a6f0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1199,7 +1199,10 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 
 out:
-	host->clock = clock;
+	if (real_div)
+		host->clock = host->mmc->actual_clock;
+	else
+		host->clock = clock;
 }
 
 static int sdhci_set_power(struct sdhci_host *host, unsigned short power)
@@ -1375,6 +1378,7 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 	}
 
 	sdhci_set_clock(host, ios->clock);
+	ios->clock = host->clock;
 
 	if (ios->power_mode == MMC_POWER_OFF)
 		vdd_bit = sdhci_set_power(host, -1);
-- 
1.7.0.4

[PATCH v6 05/14] mmc: sdhci: fix null return check of regulator_get

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

regulator_get() returns NULL when CONFIG_REGULATOR not defined,
which should not print out the warning.

Reviewed-by: Philip Rakity <prakity@marvell.com>
Signed-off-by: Bin Wang <binw@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8e6a6f0..0104ae9 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2845,9 +2845,12 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
 	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
-	if (IS_ERR(host->vqmmc)) {
-		pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
-		host->vqmmc = NULL;
+	if (IS_ERR_OR_NULL(host->vqmmc)) {
+		if (PTR_ERR(host->vqmmc) < 0) {
+			pr_info("%s: no vqmmc regulator found\n",
+				mmc_hostname(mmc));
+			host->vqmmc = NULL;
+		}
 	}
 	else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
 		regulator_enable(host->vqmmc);
@@ -2903,9 +2906,12 @@ int sdhci_add_host(struct sdhci_host *host)
 	ocr_avail = 0;
 
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
-	if (IS_ERR(host->vmmc)) {
-		pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
-		host->vmmc = NULL;
+	if (IS_ERR_OR_NULL(host->vmmc)) {
+		if (PTR_ERR(host->vmmc) < 0) {
+			pr_info("%s: no vmmc regulator found\n",
+				mmc_hostname(mmc));
+			host->vmmc = NULL;
+		}
 	} else
 		regulator_enable(host->vmmc);
 
-- 
1.7.0.4

Re: [PATCH v6 05/14] mmc: sdhci: fix null return check of regulator_get

[Chris Ball]

Hi,

On Wed, Oct 17 2012, Kevin Liu wrote:
> From: Kevin Liu <kliu5@marvell.com>
>
> regulator_get() returns NULL when CONFIG_REGULATOR not defined,
> which should not print out the warning.
>
> Reviewed-by: Philip Rakity <prakity@marvell.com>
> Signed-off-by: Bin Wang <binw@marvell.com>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/host/sdhci.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8e6a6f0..0104ae9 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2845,9 +2845,12 @@ int sdhci_add_host(struct sdhci_host *host)
>  
>  	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>  	host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
> -	if (IS_ERR(host->vqmmc)) {
> -		pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
> -		host->vqmmc = NULL;
> +	if (IS_ERR_OR_NULL(host->vqmmc)) {
> +		if (PTR_ERR(host->vqmmc) < 0) {
> +			pr_info("%s: no vqmmc regulator found\n",
> +				mmc_hostname(mmc));
> +			host->vqmmc = NULL;
> +		}
>  	}
>  	else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
>  		regulator_enable(host->vqmmc);
> @@ -2903,9 +2906,12 @@ int sdhci_add_host(struct sdhci_host *host)
>  	ocr_avail = 0;
>  
>  	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
> -	if (IS_ERR(host->vmmc)) {
> -		pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
> -		host->vmmc = NULL;
> +	if (IS_ERR_OR_NULL(host->vmmc)) {
> +		if (PTR_ERR(host->vmmc) < 0) {
> +			pr_info("%s: no vmmc regulator found\n",
> +				mmc_hostname(mmc));
> +			host->vmmc = NULL;
> +		}
>  	} else
>  		regulator_enable(host->vmmc);

Pushed to mmc-next for 3.7 with this commit message:

Author: Kevin Liu <kliu5@marvell.com>
Date:   Wed Oct 17 19:04:44 2012 +0800

    mmc: sdhci: fix IS_ERR() checking of regulator_get()
    
    There are two problems here:
    
    The check for vmmc was printing an unnecessary pr_info() when
    host->vmmc is NULL.
    
    The intent of the check for vqmmc was to only remove UHS if we have a
    regulator that doesn't support the required voltage, but since IS_ERR()
    doesn't catch NULL, we were actually removing UHS modes if vqmmc isn't
    present at all -- since it isn't present for most users, this breaks
    UHS for them.  This patch fixes that UHS regression in 3.7-rc1.
    
    Signed-off-by: Kevin Liu <kliu5@marvell.com>
    Signed-off-by: Bin Wang <binw@marvell.com>
    Reviewed-by: Philip Rakity <prakity@marvell.com>
    Signed-off-by: Chris Ball <cjb@laptop.org>

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v6 05/14] mmc: sdhci: fix null return check of regulator_get

[Kevin Liu]

2012/11/5 Chris Ball <cjb@laptop.org>:
> Hi,
>
> On Wed, Oct 17 2012, Kevin Liu wrote:
>> From: Kevin Liu <kliu5@marvell.com>
>>
>> regulator_get() returns NULL when CONFIG_REGULATOR not defined,
>> which should not print out the warning.
>>
>> Reviewed-by: Philip Rakity <prakity@marvell.com>
>> Signed-off-by: Bin Wang <binw@marvell.com>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>  drivers/mmc/host/sdhci.c |   18 ++++++++++++------
>>  1 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 8e6a6f0..0104ae9 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2845,9 +2845,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>
>>       /* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>>       host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
>> -     if (IS_ERR(host->vqmmc)) {
>> -             pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
>> -             host->vqmmc = NULL;
>> +     if (IS_ERR_OR_NULL(host->vqmmc)) {
>> +             if (PTR_ERR(host->vqmmc) < 0) {
>> +                     pr_info("%s: no vqmmc regulator found\n",
>> +                             mmc_hostname(mmc));
>> +                     host->vqmmc = NULL;
>> +             }
>>       }
>>       else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
>>               regulator_enable(host->vqmmc);
>> @@ -2903,9 +2906,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>       ocr_avail = 0;
>>
>>       host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
>> -     if (IS_ERR(host->vmmc)) {
>> -             pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
>> -             host->vmmc = NULL;
>> +     if (IS_ERR_OR_NULL(host->vmmc)) {
>> +             if (PTR_ERR(host->vmmc) < 0) {
>> +                     pr_info("%s: no vmmc regulator found\n",
>> +                             mmc_hostname(mmc));
>> +                     host->vmmc = NULL;
>> +             }
>>       } else
>>               regulator_enable(host->vmmc);
>
> Pushed to mmc-next for 3.7 with this commit message:
>

Chris,

Thanks for the push.
But how about the other 13 patches in the same patchset ([PATCH v6
00/14] mmc: sdhci: mmc: sdhci: fixes and enhancements)?
Most of them also aim to fix bugs.

Kevin

Re: [PATCH v6 05/14] mmc: sdhci: fix null return check of regulator_get

[Kevin Liu]

Updated Haojian and Philip's mail address.

2012/11/12 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/11/5 Chris Ball <cjb@laptop.org>:
>> Hi,
>>
>> On Wed, Oct 17 2012, Kevin Liu wrote:
>>> From: Kevin Liu <kliu5@marvell.com>
>>>
>>> regulator_get() returns NULL when CONFIG_REGULATOR not defined,
>>> which should not print out the warning.
>>>
>>> Reviewed-by: Philip Rakity <prakity@marvell.com>
>>> Signed-off-by: Bin Wang <binw@marvell.com>
>>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>>> ---
>>>  drivers/mmc/host/sdhci.c |   18 ++++++++++++------
>>>  1 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 8e6a6f0..0104ae9 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2845,9 +2845,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>>
>>>       /* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>>>       host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
>>> -     if (IS_ERR(host->vqmmc)) {
>>> -             pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
>>> -             host->vqmmc = NULL;
>>> +     if (IS_ERR_OR_NULL(host->vqmmc)) {
>>> +             if (PTR_ERR(host->vqmmc) < 0) {
>>> +                     pr_info("%s: no vqmmc regulator found\n",
>>> +                             mmc_hostname(mmc));
>>> +                     host->vqmmc = NULL;
>>> +             }
>>>       }
>>>       else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
>>>               regulator_enable(host->vqmmc);
>>> @@ -2903,9 +2906,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>>       ocr_avail = 0;
>>>
>>>       host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
>>> -     if (IS_ERR(host->vmmc)) {
>>> -             pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
>>> -             host->vmmc = NULL;
>>> +     if (IS_ERR_OR_NULL(host->vmmc)) {
>>> +             if (PTR_ERR(host->vmmc) < 0) {
>>> +                     pr_info("%s: no vmmc regulator found\n",
>>> +                             mmc_hostname(mmc));
>>> +                     host->vmmc = NULL;
>>> +             }
>>>       } else
>>>               regulator_enable(host->vmmc);
>>
>> Pushed to mmc-next for 3.7 with this commit message:
>>
>
> Chris,
>
> Thanks for the push.
> But how about the other 13 patches in the same patchset ([PATCH v6
> 00/14] mmc: sdhci: mmc: sdhci: fixes and enhancements)?
> Most of them also aim to fix bugs.
>
> Kevin

Re: [PATCH v6 05/14] mmc: sdhci: fix null return check of regulator_get

[Haojian Zhuang]

On Tue, Nov 13, 2012 at 11:00 AM, Kevin Liu <keyuan.liu@gmail.com> wrote:
> Updated Haojian and Philip's mail address.
>
> 2012/11/12 Kevin Liu <keyuan.liu@gmail.com>:
>> 2012/11/5 Chris Ball <cjb@laptop.org>:
>>> Hi,
>>>
>>> On Wed, Oct 17 2012, Kevin Liu wrote:
>>>> From: Kevin Liu <kliu5@marvell.com>
>>>>
>>>> regulator_get() returns NULL when CONFIG_REGULATOR not defined,
>>>> which should not print out the warning.
>>>>
>>>> Reviewed-by: Philip Rakity <prakity@marvell.com>
>>>> Signed-off-by: Bin Wang <binw@marvell.com>
>>>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci.c |   18 ++++++++++++------
>>>>  1 files changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index 8e6a6f0..0104ae9 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -2845,9 +2845,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>
>>>>       /* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>>>>       host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
>>>> -     if (IS_ERR(host->vqmmc)) {
>>>> -             pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc));
>>>> -             host->vqmmc = NULL;
>>>> +     if (IS_ERR_OR_NULL(host->vqmmc)) {
>>>> +             if (PTR_ERR(host->vqmmc) < 0) {
>>>> +                     pr_info("%s: no vqmmc regulator found\n",
>>>> +                             mmc_hostname(mmc));
>>>> +                     host->vqmmc = NULL;
>>>> +             }
>>>>       }
>>>>       else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
>>>>               regulator_enable(host->vqmmc);
>>>> @@ -2903,9 +2906,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>       ocr_avail = 0;
>>>>
>>>>       host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
>>>> -     if (IS_ERR(host->vmmc)) {
>>>> -             pr_info("%s: no vmmc regulator found\n", mmc_hostname(mmc));
>>>> -             host->vmmc = NULL;
>>>> +     if (IS_ERR_OR_NULL(host->vmmc)) {
>>>> +             if (PTR_ERR(host->vmmc) < 0) {
>>>> +                     pr_info("%s: no vmmc regulator found\n",
>>>> +                             mmc_hostname(mmc));
>>>> +                     host->vmmc = NULL;
>>>> +             }
>>>>       } else
>>>>               regulator_enable(host->vmmc);
>>>
>>> Pushed to mmc-next for 3.7 with this commit message:
>>>
>>
>> Chris,
>>
>> Thanks for the push.
>> But how about the other 13 patches in the same patchset ([PATCH v6
>> 00/14] mmc: sdhci: mmc: sdhci: fixes and enhancements)?
>> Most of them also aim to fix bugs.
>>
>> Kevin

Hi Chris,

How do you think about other patches in this patch set?

Regards
Haojian

[PATCH v6 06/14] mmc: sdhci-pxav3: controller can't get base clock

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

Enable the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN since
SD_CAPABILITIES_1[15:8](BASE_FREQ) can't get correct base
clock value. It return a fixed pre-set value like 200 on
some sdhci-pxav3 based platforms like MMP3 while return 0
on the other sdhci-pxav3 based platforms.
So we enable the quirk and get the base clock via function
get_max_clock.
Also add get_max_clock.

Reported-by: Philip Rakity <prakity@marvell.com>
Reviewed-by: Philip Rakity <prakity@Marvell.com>
Acked-by:  Zhangfei Gao <zhangfei.gao@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci-pxav3.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index e918a2b..ccd1906 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -163,10 +163,18 @@ static int pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 	return 0;
 }
 
+static u32 pxav3_get_max_clock(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+
+	return clk_get_rate(pltfm_host->clk);
+}
+
 static struct sdhci_ops pxav3_sdhci_ops = {
 	.platform_reset_exit = pxav3_set_private_registers,
 	.set_uhs_signaling = pxav3_set_uhs_signaling,
 	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
+	.get_max_clock = pxav3_get_max_clock,
 };
 
 #ifdef CONFIG_OF
@@ -249,7 +257,8 @@ static int __devinit sdhci_pxav3_probe(struct platform_device *pdev)
 
 	host->quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL
 		| SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC
-		| SDHCI_QUIRK_32BIT_ADMA_SIZE;
+		| SDHCI_QUIRK_32BIT_ADMA_SIZE
+		| SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
 
 	/* enable 1/8V DDR capable */
 	host->mmc->caps |= MMC_CAP_1_8V_DDR;
-- 
1.7.0.4

Re: [PATCH v6 06/14] mmc: sdhci-pxav3: controller can't get base clock

[Chris Ball]

Hi,

On Wed, Oct 17 2012, Kevin Liu wrote:
> From: Kevin Liu <kliu5@marvell.com>
>
> Enable the quirk SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN since
> SD_CAPABILITIES_1[15:8](BASE_FREQ) can't get correct base
> clock value. It return a fixed pre-set value like 200 on
> some sdhci-pxav3 based platforms like MMP3 while return 0
> on the other sdhci-pxav3 based platforms.
> So we enable the quirk and get the base clock via function
> get_max_clock.
> Also add get_max_clock.

Thanks, pushed to mmc-next for 3.8.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH v6 07/14] mmc: host: adjust uhs timing value

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

Both of MMC_TIMING_LEGACY and MMC_TIMING_UHS_SDR12 are defined
to 0. And ios->timing is set to MMC_TIMING_LEGACY during power up.
But set_ios can't distinguish these two timing if host support
spec 3.0. Just adjust timing values to be different can resolve
this issue without any other impact.

Reviewed By: Girish K S <girish.shivananjappa@linaro.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 include/linux/mmc/host.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 7abb0e1..8f5f6c0 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -53,12 +53,12 @@ struct mmc_ios {
 #define MMC_TIMING_LEGACY	0
 #define MMC_TIMING_MMC_HS	1
 #define MMC_TIMING_SD_HS	2
-#define MMC_TIMING_UHS_SDR12	MMC_TIMING_LEGACY
-#define MMC_TIMING_UHS_SDR25	MMC_TIMING_SD_HS
-#define MMC_TIMING_UHS_SDR50	3
-#define MMC_TIMING_UHS_SDR104	4
-#define MMC_TIMING_UHS_DDR50	5
-#define MMC_TIMING_MMC_HS200	6
+#define MMC_TIMING_UHS_SDR12	3
+#define MMC_TIMING_UHS_SDR25	4
+#define MMC_TIMING_UHS_SDR50	5
+#define MMC_TIMING_UHS_SDR104	6
+#define MMC_TIMING_UHS_DDR50	7
+#define MMC_TIMING_MMC_HS200	8
 
 #define MMC_SDR_MODE		0
 #define MMC_1_2V_DDR_MODE	1
-- 
1.7.0.4

Re: [PATCH v6 07/14] mmc: host: adjust uhs timing value

[Chris Ball]

Hi,

On Wed, Oct 17 2012, Kevin Liu wrote:
> From: Kevin Liu <kliu5@marvell.com>
>
> Both of MMC_TIMING_LEGACY and MMC_TIMING_UHS_SDR12 are defined
> to 0. And ios->timing is set to MMC_TIMING_LEGACY during power up.
> But set_ios can't distinguish these two timing if host support
> spec 3.0. Just adjust timing values to be different can resolve
> this issue without any other impact.
>
> Reviewed By: Girish K S <girish.shivananjappa@linaro.org>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  include/linux/mmc/host.h |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 7abb0e1..8f5f6c0 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -53,12 +53,12 @@ struct mmc_ios {
>  #define MMC_TIMING_LEGACY	0
>  #define MMC_TIMING_MMC_HS	1
>  #define MMC_TIMING_SD_HS	2
> -#define MMC_TIMING_UHS_SDR12	MMC_TIMING_LEGACY
> -#define MMC_TIMING_UHS_SDR25	MMC_TIMING_SD_HS
> -#define MMC_TIMING_UHS_SDR50	3
> -#define MMC_TIMING_UHS_SDR104	4
> -#define MMC_TIMING_UHS_DDR50	5
> -#define MMC_TIMING_MMC_HS200	6
> +#define MMC_TIMING_UHS_SDR12	3
> +#define MMC_TIMING_UHS_SDR25	4
> +#define MMC_TIMING_UHS_SDR50	5
> +#define MMC_TIMING_UHS_SDR104	6
> +#define MMC_TIMING_UHS_DDR50	7
> +#define MMC_TIMING_MMC_HS200	8
>  
>  #define MMC_SDR_MODE		0
>  #define MMC_1_2V_DDR_MODE	1

Thanks, pushed to mmc-next for 3.8.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH v6 08/14] mmc: sdhci: enhance preset value function

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

Preset value support was added by 4d55c5a1.

But preset value is enabled after setting clock finished,
which means the clock is still set by driver firstly and
then switch to preset value at this point. So the
driver setting beforehand is useless and unnecessary.
What's more, driver is still using the old value which may
differ from the preset one. The better way is enable preset
value just after switch to UHS mode so the preset value can
be used directly. So move preset value enable from
mmc_sd_init_card to sdhci_set_ios which will be called during
set timing.

And preset value is disabled at the begining of mmc_attach_sd.
It's a bit late since low freq should be set in mmc_power_up.
So move preset value disable to sdhci_set_ios which will be
called during power up.

Also update host->clock and ios->drv_type according to the
preset value.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/core/sd.c     |   17 ------
 drivers/mmc/host/sdhci.c  |  128 ++++++++++++++++++++++++++++++--------------
 drivers/mmc/host/sdhci.h  |   12 ++++
 include/linux/mmc/host.h  |    1 -
 include/linux/mmc/sdhci.h |    2 +-
 5 files changed, 100 insertions(+), 60 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 74972c2..3dafb54 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -960,16 +960,6 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 
 		/* Card is an ultra-high-speed card */
 		mmc_card_set_uhs(card);
-
-		/*
-		 * Since initialization is now complete, enable preset
-		 * value registers for UHS-I cards.
-		 */
-		if (host->ops->enable_preset_value) {
-			mmc_host_clk_hold(card->host);
-			host->ops->enable_preset_value(host, true);
-			mmc_host_clk_release(card->host);
-		}
 	} else {
 		/*
 		 * Attempt to change to high-speed (if supported)
@@ -1148,13 +1138,6 @@ int mmc_attach_sd(struct mmc_host *host)
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
 
-	/* Disable preset value enable if already set since last time */
-	if (host->ops->enable_preset_value) {
-		mmc_host_clk_hold(host);
-		host->ops->enable_preset_value(host, false);
-		mmc_host_clk_release(host);
-	}
-
 	err = mmc_send_app_op_cond(host, 0, &ocr);
 	if (err)
 		return err;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0104ae9..a662640 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -53,6 +53,7 @@ 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);
+static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
 
 #ifdef CONFIG_PM_RUNTIME
 static int sdhci_runtime_pm_get(struct sdhci_host *host);
@@ -1095,6 +1096,34 @@ static void sdhci_finish_command(struct sdhci_host *host)
 	}
 }
 
+static u16 sdhci_get_preset_value(struct sdhci_host *host)
+{
+	u16 ctrl, preset = 0;
+
+	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+
+	switch (ctrl & SDHCI_CTRL_UHS_MASK) {
+	case SDHCI_CTRL_UHS_SDR12:
+		preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR12);
+		break;
+	case SDHCI_CTRL_UHS_SDR25:
+		preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR25);
+		break;
+	case SDHCI_CTRL_UHS_SDR50:
+		preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR50);
+		break;
+	case SDHCI_CTRL_UHS_SDR104:
+		preset = sdhci_readw(host, SDHCI_PRESET_FOR_SDR104);
+		break;
+	case SDHCI_CTRL_UHS_DDR50:
+		preset = sdhci_readw(host, SDHCI_PRESET_FOR_DDR50);
+		break;
+	default:
+		BUG();
+	}
+	return preset;
+}
+
 static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	int div = 0; /* Initialized for compiler warning */
@@ -1119,35 +1148,45 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 		goto out;
 
 	if (host->version >= SDHCI_SPEC_300) {
+		if (sdhci_readw(host, SDHCI_HOST_CONTROL2) &
+			SDHCI_CTRL_PRESET_VAL_ENABLE) {
+			u16 pre_val;
+
+			clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+			pre_val = sdhci_get_preset_value(host);
+			div = (pre_val & SDHCI_PRESET_SDCLK_FREQ_MASK)
+				>> SDHCI_PRESET_SDCLK_FREQ_SHIFT;
+			if (host->clk_mul &&
+				(pre_val & SDHCI_PRESET_CLKGEN_SEL_MASK)) {
+				clk = SDHCI_PROG_CLOCK_MODE;
+				real_div = div + 1;
+				clk_mul = host->clk_mul;
+			} else {
+				if (div == 0)
+					real_div = 1;
+				else
+					real_div = div << 1;
+			}
+			goto preset;
+		}
 		/*
 		 * Check if the Host Controller supports Programmable Clock
 		 * Mode.
 		 */
 		if (host->clk_mul) {
-			u16 ctrl;
-
+			for (div = 1; div <= 1024; div++) {
+				if (((host->max_clk * host->clk_mul) /
+							div) <= clock)
+					break;
+			}
 			/*
-			 * We need to figure out whether the Host Driver needs
-			 * to select Programmable Clock Mode, or the value can
-			 * be set automatically by the Host Controller based on
-			 * the Preset Value registers.
+			 * Set Programmable Clock Mode in the Clock
+			 * Control register.
 			 */
-			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
-			if (!(ctrl & SDHCI_CTRL_PRESET_VAL_ENABLE)) {
-				for (div = 1; div <= 1024; div++) {
-					if (((host->max_clk * host->clk_mul) /
-					      div) <= clock)
-						break;
-				}
-				/*
-				 * Set Programmable Clock Mode in the Clock
-				 * Control register.
-				 */
-				clk = SDHCI_PROG_CLOCK_MODE;
-				real_div = div;
-				clk_mul = host->clk_mul;
-				div--;
-			}
+			clk = SDHCI_PROG_CLOCK_MODE;
+			real_div = div;
+			clk_mul = host->clk_mul;
+			div--;
 		} else {
 			/* Version 3.00 divisors must be a multiple of 2. */
 			if (host->max_clk <= clock)
@@ -1172,6 +1211,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 		div >>= 1;
 	}
 
+preset:
 	if (real_div)
 		host->mmc->actual_clock = (host->max_clk * clk_mul) / real_div;
 
@@ -1377,6 +1417,10 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 		sdhci_reinit(host);
 	}
 
+	if (host->version >= SDHCI_SPEC_300 &&
+		(ios->power_mode == MMC_POWER_UP))
+		sdhci_enable_preset_value(host, false);
+
 	sdhci_set_clock(host, ios->clock);
 	ios->clock = host->clock;
 
@@ -1489,7 +1533,19 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 				ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
 			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
 		}
-
+		if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&
+			((ios->timing == MMC_TIMING_UHS_SDR12) ||
+			 (ios->timing == MMC_TIMING_UHS_SDR25) ||
+			 (ios->timing == MMC_TIMING_UHS_SDR50) ||
+			 (ios->timing == MMC_TIMING_UHS_SDR104) ||
+			 (ios->timing == MMC_TIMING_UHS_DDR50))) {
+			u16 preset;
+
+			sdhci_enable_preset_value(host, true);
+			preset = sdhci_get_preset_value(host);
+			ios->drv_type = (preset & SDHCI_PRESET_DRV_MASK)
+					>> SDHCI_PRESET_DRV_SHIFT;
+		}
 		/* Re-enable SD Clock */
 		clock = host->clock;
 		host->clock = 0;
@@ -1951,17 +2007,15 @@ out:
 	return err;
 }
 
-static void sdhci_do_enable_preset_value(struct sdhci_host *host, bool enable)
+
+static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable)
 {
 	u16 ctrl;
-	unsigned long flags;
 
 	/* Host Controller v3.00 defines preset value registers */
 	if (host->version < SDHCI_SPEC_300)
 		return;
 
-	spin_lock_irqsave(&host->lock, flags);
-
 	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 
 	/*
@@ -1977,17 +2031,6 @@ static void sdhci_do_enable_preset_value(struct sdhci_host *host, bool enable)
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 		host->flags &= ~SDHCI_PV_ENABLED;
 	}
-
-	spin_unlock_irqrestore(&host->lock, flags);
-}
-
-static void sdhci_enable_preset_value(struct mmc_host *mmc, bool enable)
-{
-	struct sdhci_host *host = mmc_priv(mmc);
-
-	sdhci_runtime_pm_get(host);
-	sdhci_do_enable_preset_value(host, enable);
-	sdhci_runtime_pm_put(host);
 }
 
 static const struct mmc_host_ops sdhci_ops = {
@@ -1998,7 +2041,6 @@ static const struct mmc_host_ops sdhci_ops = {
 	.enable_sdio_irq = sdhci_enable_sdio_irq,
 	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
 	.execute_tuning			= sdhci_execute_tuning,
-	.enable_preset_value		= sdhci_enable_preset_value,
 };
 
 /*****************************************************************************\
@@ -2588,8 +2630,12 @@ int sdhci_runtime_resume_host(struct sdhci_host *host)
 	sdhci_do_set_ios(host, &host->mmc->ios);
 
 	sdhci_do_start_signal_voltage_switch(host, &host->mmc->ios);
-	if (host_flags & SDHCI_PV_ENABLED)
-		sdhci_do_enable_preset_value(host, true);
+	if ((host_flags & SDHCI_PV_ENABLED) &&
+		!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN)) {
+		spin_lock_irqsave(&host->lock, flags);
+		sdhci_enable_preset_value(host, true);
+		spin_unlock_irqrestore(&host->lock, flags);
+	}
 
 	/* Set the re-tuning expiration flag */
 	if (host->flags & SDHCI_USING_RETUNING_TIMER)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 97653ea..5eb27bf 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -227,6 +227,18 @@
 
 /* 60-FB reserved */
 
+#define SDHCI_PRESET_FOR_SDR12 0x66
+#define SDHCI_PRESET_FOR_SDR25 0x68
+#define SDHCI_PRESET_FOR_SDR50 0x6A
+#define SDHCI_PRESET_FOR_SDR104        0x6C
+#define SDHCI_PRESET_FOR_DDR50 0x6E
+#define SDHCI_PRESET_DRV_MASK  0xC000
+#define SDHCI_PRESET_DRV_SHIFT  14
+#define SDHCI_PRESET_CLKGEN_SEL_MASK   0x400
+#define SDHCI_PRESET_CLKGEN_SEL_SHIFT	10
+#define SDHCI_PRESET_SDCLK_FREQ_MASK   0x3FF
+#define SDHCI_PRESET_SDCLK_FREQ_SHIFT	0
+
 #define SDHCI_SLOT_INT_STATUS	0xFC
 
 #define SDHCI_HOST_VERSION	0xFE
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 8f5f6c0..565d712 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -133,7 +133,6 @@ struct mmc_host_ops {
 
 	/* The tuning command opcode value is different for SD and eMMC cards */
 	int	(*execute_tuning)(struct mmc_host *host, u32 opcode);
-	void	(*enable_preset_value)(struct mmc_host *host, bool enable);
 	int	(*select_drive_strength)(unsigned int max_dtr, int host_drv, int card_drv);
 	void	(*hw_reset)(struct mmc_host *host);
 };
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index fa8529a..d09c20d 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -91,7 +91,7 @@ struct sdhci_host {
 	unsigned int quirks2;	/* More deviations from spec. */
 
 #define SDHCI_QUIRK2_HOST_OFF_CARD_ON			(1<<0)
-
+#define SDHCI_QUIRK2_PRESET_VALUE_BROKEN		(1<<1)
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
 
-- 
1.7.0.4

[PATCH v6 09/14] mmc: sdhci-pxav3: add quirks2

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

Acked-by:  Zhangfei Gao <zhangfei.gao@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci-pxav3.c          |    2 ++
 include/linux/platform_data/pxa_sdhci.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index ccd1906..60829c9 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -280,6 +280,8 @@ static int __devinit sdhci_pxav3_probe(struct platform_device *pdev)
 
 		if (pdata->quirks)
 			host->quirks |= pdata->quirks;
+		if (pdata->quirks2)
+			host->quirks2 |= pdata->quirks2;
 		if (pdata->host_caps)
 			host->mmc->caps |= pdata->host_caps;
 		if (pdata->host_caps2)
diff --git a/include/linux/platform_data/pxa_sdhci.h b/include/linux/platform_data/pxa_sdhci.h
index 59acd98..fdf38d6 100644
--- a/include/linux/platform_data/pxa_sdhci.h
+++ b/include/linux/platform_data/pxa_sdhci.h
@@ -38,6 +38,7 @@
  * @max_speed: the maximum speed supported
  * @host_caps: Standard MMC host capabilities bit field.
  * @quirks: quirks of platfrom
+ * @quirks2: quirks2 of platfrom
  * @pm_caps: pm_caps of platfrom
  */
 struct sdhci_pxa_platdata {
@@ -51,6 +52,7 @@ struct sdhci_pxa_platdata {
 	unsigned int	host_caps;
 	unsigned int	host_caps2;
 	unsigned int	quirks;
+	unsigned int	quirks2;
 	unsigned int	pm_caps;
 };
 
-- 
1.7.0.4

Re: [PATCH v6 09/14] mmc: sdhci-pxav3: add quirks2

[Chris Ball]

Hi,

On Wed, Oct 17 2012, Kevin Liu wrote:
> From: Kevin Liu <kliu5@marvell.com>
>
> Acked-by:  Zhangfei Gao <zhangfei.gao@marvell.com>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/host/sdhci-pxav3.c          |    2 ++
>  include/linux/platform_data/pxa_sdhci.h |    2 ++
>  2 files changed, 4 insertions(+), 0 deletions(-)

It looks like you aren't using any of the quirks in quirks2, is that
right?  I'm still hoping we might be able to get rid of quirks2, so
I'd rather wait until you need it (or we get rid of it) before applying
this.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v6 09/14] mmc: sdhci-pxav3: add quirks2

[Kevin Liu]

2012/11/18 Chris Ball <cjb@laptop.org>:
> Hi,
>
> On Wed, Oct 17 2012, Kevin Liu wrote:
>> From: Kevin Liu <kliu5@marvell.com>
>>
>> Acked-by:  Zhangfei Gao <zhangfei.gao@marvell.com>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>  drivers/mmc/host/sdhci-pxav3.c          |    2 ++
>>  include/linux/platform_data/pxa_sdhci.h |    2 ++
>>  2 files changed, 4 insertions(+), 0 deletions(-)
>
> It looks like you aren't using any of the quirks in quirks2, is that
> right?  I'm still hoping we might be able to get rid of quirks2, so
> I'd rather wait until you need it (or we get rid of it) before applying
> this.
>

My previous patch "[PATCH v6 08/14] mmc: sdhci: enhance preset value
function" introduced a quirk SDHCI_QUIRK2_PRESET_VALUE_BROKEN, which
aim to disable preset feature since the preset setting is fixed only
for 200Mhz base clock on pxav3 platform. So we should disable preset
if the base clock is other than 200Mhz.

Thanks
Kevin

Re: [PATCH v6 09/14] mmc: sdhci-pxav3: add quirks2

[Chris Ball]

Hi,

On Sun, Nov 18 2012, Kevin Liu wrote:
>>> From: Kevin Liu <kliu5@marvell.com>
>>>
>>> Acked-by:  Zhangfei Gao <zhangfei.gao@marvell.com>
>>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>>> ---
>>>  drivers/mmc/host/sdhci-pxav3.c          |    2 ++
>>>  include/linux/platform_data/pxa_sdhci.h |    2 ++
>>>  2 files changed, 4 insertions(+), 0 deletions(-)
>>
>> It looks like you aren't using any of the quirks in quirks2, is that
>> right?  I'm still hoping we might be able to get rid of quirks2, so
>> I'd rather wait until you need it (or we get rid of it) before applying
>> this.
>
> My previous patch "[PATCH v6 08/14] mmc: sdhci: enhance preset value
> function" introduced a quirk SDHCI_QUIRK2_PRESET_VALUE_BROKEN, which
> aim to disable preset feature since the preset setting is fixed only
> for 200Mhz base clock on pxav3 platform. So we should disable preset
> if the base clock is other than 200Mhz.

Thanks, pushed to mmc-next for 3.8.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH v6 10/14] mmc: sdhci: introduce signal_voltage_switch callback function

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

Some soc/platform need specific handling for signal voltage
switch. For example, mmp2/mmp3 need to set the AIB IO domain
control register accordingly.

Signed-off-by: Bin Wang <binw@marvell.com>
Signed-off-by: Philip Rakity <prakity@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |   17 +++++++++++++++++
 drivers/mmc/host/sdhci.h |    1 +
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a662640..c3e786d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1684,6 +1684,15 @@ static int sdhci_do_3_3v_signal_voltage_switch(struct sdhci_host *host,
 			return -EIO;
 		}
 	}
+
+	/*
+	 * May need to apply soc/platfrom settings for the
+	 * voltage switch
+	 */
+	if (host->ops->signal_voltage_switch)
+		host->ops->signal_voltage_switch(host,
+				host->mmc->ios.signal_voltage);
+
 	/* Wait for 5ms */
 	usleep_range(5000, 5500);
 
@@ -1726,6 +1735,14 @@ static int sdhci_do_1_8v_signal_voltage_switch(struct sdhci_host *host,
 			ret = 0;
 
 		if (!ret) {
+			/*
+			 * May need to apply soc/platfrom settings for the
+			 * voltage switch
+			 */
+			if (host->ops->signal_voltage_switch)
+				host->ops->signal_voltage_switch(host,
+					host->mmc->ios.signal_voltage);
+
 			ctrl |= SDHCI_CTRL_VDD_180;
 			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 5eb27bf..5f20d32 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -290,6 +290,7 @@ struct sdhci_ops {
 	void	(*hw_reset)(struct sdhci_host *host);
 	void	(*platform_suspend)(struct sdhci_host *host);
 	void	(*platform_resume)(struct sdhci_host *host);
+	void	(*signal_voltage_switch)(struct sdhci_host *host, u8 vol);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.7.0.4

[PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

Host controller must enable 1.8v signal for UHS modes. Otherwise
UHS modes won't take effect. But mmc core does NOT switch to 1.8v
for DDR50 mode. So enable the 1.8v signal for mmc DDR50 mode in host
driver.

In JEDEC spec, there are two emmc device types which support DDR50.
One type can work under signal 1.2v while the other type work under
signal 1.8v or 3v. So current code just keep 3v for the second device
type.

But in SD host spec, 1.8v signal must be enabled for all UHS modes
taking effect. So the fact is, if using SD host to work with emmc chip,
then 1.8v signal must be selected in order to enable DDR50.
Current code missed this.

Because DDR50 shall can work under both signal 1.8v and 3v according
to JEDEC spec,  So should not switch to 1.8v in mmc core code.
It's the host controller requirement that 1.8v signal must be enabled
in order to enable DDR50, which conflict with emmc spec. So add this
in host driver.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c3e786d..522e501 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1529,8 +1529,15 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
 				ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
 			else if (ios->timing == MMC_TIMING_UHS_SDR104)
 				ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
-			else if (ios->timing == MMC_TIMING_UHS_DDR50)
+			else if (ios->timing == MMC_TIMING_UHS_DDR50) {
+				struct mmc_card	*card;
+
 				ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
+				card = container_of(&(host->mmc),
+					struct mmc_card, host);
+				if (mmc_card_mmc(card))
+					ctrl_2 |= SDHCI_CTRL_VDD_180;
+			}
 			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
 		}
 		if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&
-- 
1.7.0.4

Re: [PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

[Chris Ball]

Hi,

On Wed, Oct 17 2012, Kevin Liu wrote:
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c3e786d..522e501 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1529,8 +1529,15 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>  				ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
>  			else if (ios->timing == MMC_TIMING_UHS_SDR104)
>  				ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
> -			else if (ios->timing == MMC_TIMING_UHS_DDR50)
> +			else if (ios->timing == MMC_TIMING_UHS_DDR50) {
> +				struct mmc_card	*card;
> +
>  				ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
> +				card = container_of(&(host->mmc),
> +					struct mmc_card, host);
> +				if (mmc_card_mmc(card))
> +					ctrl_2 |= SDHCI_CTRL_VDD_180;
> +			}
>  			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>  		}
>  		if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&

I'm using DDR50 on an eMMC that's only powered by 3.3V (no 1.8V
available) with sdhci-pxav3, so it sounds like I don't want to merge
this patch?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

[Kevin Liu]

2012/11/18 Chris Ball <cjb@laptop.org>:
> Hi,
>
> On Wed, Oct 17 2012, Kevin Liu wrote:
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index c3e786d..522e501 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1529,8 +1529,15 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>>                               ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
>>                       else if (ios->timing == MMC_TIMING_UHS_SDR104)
>>                               ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
>> -                     else if (ios->timing == MMC_TIMING_UHS_DDR50)
>> +                     else if (ios->timing == MMC_TIMING_UHS_DDR50) {
>> +                             struct mmc_card *card;
>> +
>>                               ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>> +                             card = container_of(&(host->mmc),
>> +                                     struct mmc_card, host);
>> +                             if (mmc_card_mmc(card))
>> +                                     ctrl_2 |= SDHCI_CTRL_VDD_180;
>> +                     }
>>                       sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>>               }
>>               if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&
>
> I'm using DDR50 on an eMMC that's only powered by 3.3V (no 1.8V
> available) with sdhci-pxav3, so it sounds like I don't want to merge
> this patch?
>
This patch is NEEDED for both 3.3v and 1.8v signaling.
In the SD host spec for host control 2 register, 1.8v signaling enable
bit must be set in order for UHS-I mode taking effect. Otherwise, the
DDR50 mode won't take effect on host even it is selected.
It's the SD host requirement.

Thanks
Kevin
Kevin

Re: [PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

[Kevin Liu]

2012/11/19 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/11/18 Chris Ball <cjb@laptop.org>:
>> Hi,
>>
>> On Wed, Oct 17 2012, Kevin Liu wrote:
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index c3e786d..522e501 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1529,8 +1529,15 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>>>                               ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
>>>                       else if (ios->timing == MMC_TIMING_UHS_SDR104)
>>>                               ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
>>> -                     else if (ios->timing == MMC_TIMING_UHS_DDR50)
>>> +                     else if (ios->timing == MMC_TIMING_UHS_DDR50) {
>>> +                             struct mmc_card *card;
>>> +
>>>                               ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>>> +                             card = container_of(&(host->mmc),
>>> +                                     struct mmc_card, host);
>>> +                             if (mmc_card_mmc(card))
>>> +                                     ctrl_2 |= SDHCI_CTRL_VDD_180;
>>> +                     }
>>>                       sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>>>               }
>>>               if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN) &&
>>
>> I'm using DDR50 on an eMMC that's only powered by 3.3V (no 1.8V
>> available) with sdhci-pxav3, so it sounds like I don't want to merge
>> this patch?
>>
> This patch is NEEDED for both 3.3v and 1.8v signaling.
> In the SD host spec for host control 2 register, 1.8v signaling enable
> bit must be set in order for UHS-I mode taking effect. Otherwise, the
> DDR50 mode won't take effect on host even it is selected.
> It's the SD host requirement.

In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
with SD host.
You must enable 1.8v signaling on host for UHS-I modes, but you set
3.3v for emmc vccq. It's conflictable.
The only way for emmc DDR50 to work is to set 1.8v for vccq although
JEDEC spec said both 1.8v and 3.3v are ok.

Thanks
Kevin

Re: [PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

[Chris Ball]

Hi,

On Mon, Nov 19 2012, Kevin Liu wrote:
> In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
> with SD host.

I've checked on the scope that we're reaching DDR50 with our 3.3V vccq
eMMC, so it looks like this isn't true.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

[Kevin Liu]

2012/12/8 Chris Ball <cjb@laptop.org>:
> Hi,
>
> On Mon, Nov 19 2012, Kevin Liu wrote:
>> In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
>> with SD host.
>
> I've checked on the scope that we're reaching DDR50 with our 3.3V vccq
> eMMC, so it looks like this isn't true.
>

Chris,

Thanks for the check.
If you are using sdhci-pxav3.c, 3.3v vccq can work since it enabled
the 1.8v signaling in the callback function set_uhs_signaling.
What I want to say is if you don't enable the 1.8v signaling bit while
using 3.3v vccq, DDR50 on emmc should NOT work. (Use default sdhci
code without implementing set_uhs_signaling as the other drivers.)
You can just remove the set_uhs_signaling in sdhci-pxav3.c and try again.
So we should do this in sdhci.c by default and remove the callback
function, which only aim to enabling the 1.8v signaling bit.

Kevin

Re: [PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

[Kevin Liu]

2012/12/8 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/12/8 Chris Ball <cjb@laptop.org>:
>> Hi,
>>
>> On Mon, Nov 19 2012, Kevin Liu wrote:
>>> In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
>>> with SD host.
>>
>> I've checked on the scope that we're reaching DDR50 with our 3.3V vccq
>> eMMC, so it looks like this isn't true.
>>
>
> Chris,
>
> Thanks for the check.
> If you are using sdhci-pxav3.c, 3.3v vccq can work since it enabled
> the 1.8v signaling in the callback function set_uhs_signaling.
> What I want to say is if you don't enable the 1.8v signaling bit while
> using 3.3v vccq, DDR50 on emmc should NOT work. (Use default sdhci
> code without implementing set_uhs_signaling as the other drivers.)
> You can just remove the set_uhs_signaling in sdhci-pxav3.c and try again.
> So we should do this in sdhci.c by default and remove the callback
> function, which only aim to enabling the 1.8v signaling bit.
>

Chris,

I would like to say more about my undersanding of 1.8v signaling bit
and UHS mode select. (Host Control 2 register, offset 0x3E<3:0>)
According to spec, when 1.8v signaling bit is set, the signaling
voltage will be changed from 3.3v to 1.8v. But we still provide 3.3v
vccq in current code. So originally I think it's conflictable in logic
and we must change to 1.8v vccq. But the fact is both 1.8v and 3.3v
vccq can work with 1.8v signaling bit set. The reason is below:
The host controller can't provide and totally control the card power by itself.
In actual implementation, the card power is supplied by external
regulator on board (vmmc and vmmcq) rather than host controller. So
for the 1.8v signaling bit, it will NOT impact signal voltage as spec
said either. It's just a flag but MUST be checked when UHS-I mode
selected. That's why we can enable 1.8v signaling bit while using 3.3v
vccq regulator for emmc DDR50 working. I confirmed this with mmp3 sdh
owner.

So we just enable the 1.8v signaling bit in host while keep 1.8v or
3.3v vccq regulator since emmc support DDR50 with both 1.8v and 3.3v
vccq. In other words, we make use of the incomplete feature of 1.8v
signaling bit (not matched with sd host spec that this bit should
control the signal voltage) to let both 3.3v and 1.8v signaling work
(well matched with JEDEC spec that DDR50 can work under both 1.8v and
3.3v vccq). I think it's good, otherwise only 1.8v can work which make
things complex and add limitation for board hardware.

So there are below conslusions:
1. If card power is supplied by external regulator, then setting 1.8v
signaling bit with both 1.8v and 3.3v vccq can work for emmc DDR50.
2. If host controller provide and totally control the card power, then
setting 1.8v signaling bit with only 1.8v vccq can work.
3. no matter which one of above two, 1.8v signaling bit MUST be set
for DDR50 working.
My patch assure item 3 in sdhci.c by default rather than adding
callback function to do this.

How do you think?

Thanks
Kevin

Re: [PATCH v6 11/14] mmc: sdhci: fix the bug that DDR50 can't work for emmc

[Kevin Liu]

2012/12/7 Kevin Liu <keyuan.liu@gmail.com>:
> 2012/12/8 Kevin Liu <keyuan.liu@gmail.com>:
>> 2012/12/8 Chris Ball <cjb@laptop.org>:
>>> Hi,
>>>
>>> On Mon, Nov 19 2012, Kevin Liu wrote:
>>>> In fact, I don't think 3.3v vccq for emmc can work under DDR50 mode
>>>> with SD host.
>>>
>>> I've checked on the scope that we're reaching DDR50 with our 3.3V vccq
>>> eMMC, so it looks like this isn't true.
>>>
>>
>> Chris,
>>
>> Thanks for the check.
>> If you are using sdhci-pxav3.c, 3.3v vccq can work since it enabled
>> the 1.8v signaling in the callback function set_uhs_signaling.
>> What I want to say is if you don't enable the 1.8v signaling bit while
>> using 3.3v vccq, DDR50 on emmc should NOT work. (Use default sdhci
>> code without implementing set_uhs_signaling as the other drivers.)
>> You can just remove the set_uhs_signaling in sdhci-pxav3.c and try again.
>> So we should do this in sdhci.c by default and remove the callback
>> function, which only aim to enabling the 1.8v signaling bit.
>>
>
> Chris,
>
> I would like to say more about my undersanding of 1.8v signaling bit
> and UHS mode select. (Host Control 2 register, offset 0x3E<3:0>)
> According to spec, when 1.8v signaling bit is set, the signaling
> voltage will be changed from 3.3v to 1.8v. But we still provide 3.3v
> vccq in current code. So originally I think it's conflictable in logic
> and we must change to 1.8v vccq. But the fact is both 1.8v and 3.3v
> vccq can work with 1.8v signaling bit set. The reason is below:
> The host controller can't provide and totally control the card power by itself.
> In actual implementation, the card power is supplied by external
> regulator on board (vmmc and vmmcq) rather than host controller. So
> for the 1.8v signaling bit, it will NOT impact signal voltage as spec
> said either. It's just a flag but MUST be checked when UHS-I mode
> selected. That's why we can enable 1.8v signaling bit while using 3.3v
> vccq regulator for emmc DDR50 working. I confirmed this with mmp3 sdh
> owner.
>
> So we just enable the 1.8v signaling bit in host while keep 1.8v or
> 3.3v vccq regulator since emmc support DDR50 with both 1.8v and 3.3v
> vccq. In other words, we make use of the incomplete feature of 1.8v
> signaling bit (not matched with sd host spec that this bit should
> control the signal voltage) to let both 3.3v and 1.8v signaling work
> (well matched with JEDEC spec that DDR50 can work under both 1.8v and
> 3.3v vccq). I think it's good, otherwise only 1.8v can work which make
> things complex and add limitation for board hardware.
>
> So there are below conslusions:
> 1. If card power is supplied by external regulator, then setting 1.8v
> signaling bit with both 1.8v and 3.3v vccq can work for emmc DDR50.
> 2. If host controller provide and totally control the card power, then
> setting 1.8v signaling bit with only 1.8v vccq can work.
> 3. no matter which one of above two, 1.8v signaling bit MUST be set
> for DDR50 working.
> My patch assure item 3 in sdhci.c by default rather than adding
> callback function to do this.
>
> How do you think?
>
Chris,

Hope I'm clear with the 1.8v signaling setting.
So any comments for this patch or the other patches in the same patchset?

Thanks
Kevin

[PATCH v6 12/14] mmc: sdhci-pxav3: remove set_uhs_signaling function

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

Because sdhci can do the same thing so no need to implement this.

Acked-by:  Zhangfei Gao <zhangfei.gao@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci-pxav3.c |   39 ---------------------------------------
 1 files changed, 0 insertions(+), 39 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 60829c9..6aecc91 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -125,44 +125,6 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)
 	pxa->power_mode = power_mode;
 }
 
-static int pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
-{
-	u16 ctrl_2;
-
-	/*
-	 * Set V18_EN -- UHS modes do not work without this.
-	 * does not change signaling voltage
-	 */
-	ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
-
-	/* Select Bus Speed Mode for host */
-	ctrl_2 &= ~SDHCI_CTRL_UHS_MASK;
-	switch (uhs) {
-	case MMC_TIMING_UHS_SDR12:
-		ctrl_2 |= SDHCI_CTRL_UHS_SDR12;
-		break;
-	case MMC_TIMING_UHS_SDR25:
-		ctrl_2 |= SDHCI_CTRL_UHS_SDR25;
-		break;
-	case MMC_TIMING_UHS_SDR50:
-		ctrl_2 |= SDHCI_CTRL_UHS_SDR50 | SDHCI_CTRL_VDD_180;
-		break;
-	case MMC_TIMING_UHS_SDR104:
-		ctrl_2 |= SDHCI_CTRL_UHS_SDR104 | SDHCI_CTRL_VDD_180;
-		break;
-	case MMC_TIMING_UHS_DDR50:
-		ctrl_2 |= SDHCI_CTRL_UHS_DDR50 | SDHCI_CTRL_VDD_180;
-		break;
-	}
-
-	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
-	dev_dbg(mmc_dev(host->mmc),
-		"%s uhs = %d, ctrl_2 = %04X\n",
-		__func__, uhs, ctrl_2);
-
-	return 0;
-}
-
 static u32 pxav3_get_max_clock(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -172,7 +134,6 @@ static u32 pxav3_get_max_clock(struct sdhci_host *host)
 
 static struct sdhci_ops pxav3_sdhci_ops = {
 	.platform_reset_exit = pxav3_set_private_registers,
-	.set_uhs_signaling = pxav3_set_uhs_signaling,
 	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
 	.get_max_clock = pxav3_get_max_clock,
 };
-- 
1.7.0.4

Re: [PATCH v6 12/14] mmc: sdhci-pxav3: remove set_uhs_signaling function

[Chris Ball]

Hi,

On Wed, Oct 17 2012, Kevin Liu wrote:
> From: Kevin Liu <kliu5@marvell.com>
>
> Because sdhci can do the same thing so no need to implement this.
>
> Acked-by:  Zhangfei Gao <zhangfei.gao@marvell.com>
> Signed-off-by: Kevin Liu <kliu5@marvell.com>

In that case, we can remove the callbacks from sdhci.c/sdhci.h too --
sdhci-pxav3 was the only user.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v6 12/14] mmc: sdhci-pxav3: remove set_uhs_signaling function

[Kevin Liu]

2012/11/18 Chris Ball <cjb@laptop.org>:
> Hi,
>
> On Wed, Oct 17 2012, Kevin Liu wrote:
>> From: Kevin Liu <kliu5@marvell.com>
>>
>> Because sdhci can do the same thing so no need to implement this.
>>
>> Acked-by:  Zhangfei Gao <zhangfei.gao@marvell.com>
>> Signed-off-by: Kevin Liu <kliu5@marvell.com>
>
> In that case, we can remove the callbacks from sdhci.c/sdhci.h too --
> sdhci-pxav3 was the only user.
>
updated the patch in v8. thanks!

Kevin

[PATCH v6 13/14] mmc: sdhci: solve several vmmc/vqmmc regulator issues

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

1. The vmmc regulator enable in sdhci_add_host is NOT necessary since
it can be enabled during mmc_power_up by function mmc_regulator_set_ocr.
And this extra enable will make regulator_enable/regulator_disable
unbalanced. Consequently, vmmc can't be disabled during mmc_power_off.

2. If regulator vqmmc exist, it should be enabled regardless it support
1.8v or not.

3. If regulator framework is disabled, regulator_get will return NULL.
So the CONFIG_REGULATOR judgement can be removed.

Signed-off-by: Philip Rakity <prakity@marvell.com>
Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 522e501..9f44efd 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2921,12 +2921,14 @@ int sdhci_add_host(struct sdhci_host *host)
 				mmc_hostname(mmc));
 			host->vqmmc = NULL;
 		}
-	}
-	else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
+	} else {
 		regulator_enable(host->vqmmc);
-	else
-		caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
-		       SDHCI_SUPPORT_DDR50);
+		if (!regulator_is_supported_voltage(host->vqmmc, 1800000,
+			1800000))
+			caps[1] &= ~(SDHCI_SUPPORT_SDR104 |
+					SDHCI_SUPPORT_SDR50 |
+					SDHCI_SUPPORT_DDR50);
+	}
 
 	/* Any UHS-I mode in caps implies SDR12 and SDR25 support. */
 	if (caps[1] & (SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 |
@@ -2982,10 +2984,8 @@ int sdhci_add_host(struct sdhci_host *host)
 				mmc_hostname(mmc));
 			host->vmmc = NULL;
 		}
-	} else
-		regulator_enable(host->vmmc);
+	}
 
-#ifdef CONFIG_REGULATOR
 	if (host->vmmc) {
 		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
 			3300000);
@@ -3000,7 +3000,6 @@ int sdhci_add_host(struct sdhci_host *host)
 		if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_180))
 			caps[0] &= ~SDHCI_CAN_VDD_180;
 	}
-#endif /* CONFIG_REGULATOR */
 
 	/*
 	 * According to SD Host Controller spec v3.00, if the Host System
-- 
1.7.0.4

Re: [PATCH v6 13/14] mmc: sdhci: solve several vmmc/vqmmc regulator issues

[Chris Ball]

Hi,

On Wed, Oct 17 2012, Kevin Liu wrote:
> From: Kevin Liu <kliu5@marvell.com>
>
> 1. The vmmc regulator enable in sdhci_add_host is NOT necessary since
> it can be enabled during mmc_power_up by function mmc_regulator_set_ocr.
> And this extra enable will make regulator_enable/regulator_disable
> unbalanced. Consequently, vmmc can't be disabled during mmc_power_off.
>
> 2. If regulator vqmmc exist, it should be enabled regardless it support
> 1.8v or not.
>
> 3. If regulator framework is disabled, regulator_get will return NULL.
> So the CONFIG_REGULATOR judgement can be removed.

Pushed to mmc-next for 3.8 without "3." above, because it causes a
compile error -- now we use regulator_count_voltages() inside that
block and it's not a stub function in the regulator API, so the
ifdef is required.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH v6 14/14] mmc: sdhci: add function to get retunig timer count

[Kevin Liu]

From: Kevin Liu <kliu5@marvell.com>

According to spec, if timer count for retuning return 0xF,
it means get information from other source.

Signed-off-by: Kevin Liu <kliu5@marvell.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 9f44efd..882223a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2963,6 +2963,14 @@ int sdhci_add_host(struct sdhci_host *host)
 	/* Initial value for re-tuning timer count */
 	host->tuning_count = (caps[1] & SDHCI_RETUNING_TIMER_COUNT_MASK) >>
 			      SDHCI_RETUNING_TIMER_COUNT_SHIFT;
+	if (host->tuning_count == 0xF) {
+		if (host->ops->get_tuning_count)
+			host->tuning_count =
+				host->ops->get_tuning_count(host) & 0xF;
+		else
+			pr_err("%s: Hardware doesn't specify tuning count.\n",
+				mmc_hostname(mmc));
+	}
 
 	/*
 	 * In case Re-tuning Timer is not disabled, the actual value of
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 5f20d32..d124f37 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -291,6 +291,7 @@ struct sdhci_ops {
 	void	(*platform_suspend)(struct sdhci_host *host);
 	void	(*platform_resume)(struct sdhci_host *host);
 	void	(*signal_voltage_switch)(struct sdhci_host *host, u8 vol);
+	unsigned int	(*get_tuning_count)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.7.0.4