[RESEND PATCH v2 1/4] mmc: core: switch to 1V8 or 1V2 for hs400es mode

[RESEND PATCH v2 1/4] mmc: core: switch to 1V8 or 1V2 for hs400es mode

[Shawn Lin]

When introducing hs400es, I didn't notice that we haven't
switched voltage to 1V2 or 1V8 for it. That happens to work
as the first controller claiming to support hs400es, arasan(5.1),
which is designed to only support 1V8. So the voltage is fixed to 1V8.
But it actually is wrong, and will not fit for other host controllers.
Let's fix it.

Fixes: commit 81ac2af65793ecf ("mmc: core: implement enhanced strobe support")
Cc: <stable@vger.kernel.org> 4.4#
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- fix copy-paste error, mea culpa

 drivers/mmc/core/mmc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3486bc7..f4ed5ac 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1263,6 +1263,16 @@ static int mmc_select_hs400es(struct mmc_card *card)
 		goto out_err;
 	}
 
+	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_2V)
+		err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
+
+	if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_8V)
+		err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
+
+	/* If fails try again during next card power cycle */
+	if (err)
+		goto out_err;
+
 	err = mmc_select_bus_width(card);
 	if (err < 0)
 		goto out_err;
-- 
2.3.7

[RESEND PATCH v2 2/4] mmc: core: changes frequency to hs_max_dtr when selecting hs400es

[Shawn Lin]

Per JESD84-B51 P69, Host need to change frequency to <=52MHz
after setting HS_TIMING to 0x1, and host may changes frequency
to <= 200MHz after setting HS_TIMING to 0x3. That means the card
expects the clock rate to increase from the current used f_init
(which is less than 400KHz, but still being less than 52MHz) to
52MHz, otherwise we find some eMMC devices significantly report
failure when sending status.

Reported-by: Xiao Yao <xiaoyao@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- improve the changelog

 drivers/mmc/core/mmc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index f4ed5ac..39fc5b2 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1282,6 +1282,8 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	if (err)
 		goto out_err;
 
+	mmc_set_clock(host, card->ext_csd.hs_max_dtr);
+
 	err = mmc_switch_status(card);
 	if (err)
 		goto out_err;
-- 
2.3.7

Re: [RESEND PATCH v2 2/4] mmc: core: changes frequency to hs_max_dtr when selecting hs400es

[Doug Anderson]

Hi,

On Thu, Sep 22, 2016 at 5:12 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Per JESD84-B51 P69, Host need to change frequency to <=52MHz

Technically Page 49.  In the PDF you go to page 69, but the heading on
the top of the page says 49.

> after setting HS_TIMING to 0x1, and host may changes frequency
> to <= 200MHz after setting HS_TIMING to 0x3. That means the card
> expects the clock rate to increase from the current used f_init
> (which is less than 400KHz, but still being less than 52MHz) to
> 52MHz, otherwise we find some eMMC devices significantly report
> failure when sending status.

Technically it seems like things ought to be OK at the slow speed
since technically we're allowed to talk "high speed" at 400 kHZ (or at
any rate <= 52MHz).  ...but I guess I could also see some cards not
liking that.  Though I'm no expert, feel free to add my reviewed-by if
it is useful:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[RESEDN PATCH v2 3/4] mmc: sdhci: Don't try to switch to unsupported voltage

[Shawn Lin]

From: Ziyuan Xu <xzy.xu@rock-chips.com>

Sdhci shouldn't switch to the unsupported voltage if claiming
that it can not support the requested voltage. Let's fix it.

Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 4805566..b1f1edd 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1845,7 +1845,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 
 	switch (ios->signal_voltage) {
 	case MMC_SIGNAL_VOLTAGE_330:
-		if (!(host->flags & SDHCI_SIGNALING_330))
+		if (!(host->flags & SDHCI_SIGNALING_330) ||
+		    !(host->caps & SDHCI_CAN_VDD_330))
 			return -EINVAL;
 		/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
 		ctrl &= ~SDHCI_CTRL_VDD_180;
@@ -1872,7 +1873,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 
 		return -EAGAIN;
 	case MMC_SIGNAL_VOLTAGE_180:
-		if (!(host->flags & SDHCI_SIGNALING_180))
+		if (!(host->flags & SDHCI_SIGNALING_180) ||
+		    !(host->caps & SDHCI_CAN_VDD_180))
 			return -EINVAL;
 		if (!IS_ERR(mmc->supply.vqmmc)) {
 			ret = mmc_regulator_set_vqmmc(mmc, ios);
-- 
2.3.7

Re: [RESEDN PATCH v2 3/4] mmc: sdhci: Don't try to switch to unsupported voltage

[Adrian Hunter]

On 23/09/16 03:12, Shawn Lin wrote:
> From: Ziyuan Xu <xzy.xu@rock-chips.com>
> 
> Sdhci shouldn't switch to the unsupported voltage if claiming
> that it can not support the requested voltage. Let's fix it.
> 
> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 4805566..b1f1edd 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1845,7 +1845,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>  
>  	switch (ios->signal_voltage) {
>  	case MMC_SIGNAL_VOLTAGE_330:
> -		if (!(host->flags & SDHCI_SIGNALING_330))
> +		if (!(host->flags & SDHCI_SIGNALING_330) ||
> +		    !(host->caps & SDHCI_CAN_VDD_330))

You are mixing signal voltage and supply voltage here.  Note, I don't
believe all drivers set the capabilities correctly when they are using
regulators, but in any case this is not the place in the code to make such
assumptions.

Better for the driver to remove SDHCI_SIGNALING_330 from flags when it is
not supported.

>  			return -EINVAL;
>  		/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
>  		ctrl &= ~SDHCI_CTRL_VDD_180;
> @@ -1872,7 +1873,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>  
>  		return -EAGAIN;
>  	case MMC_SIGNAL_VOLTAGE_180:
> -		if (!(host->flags & SDHCI_SIGNALING_180))
> +		if (!(host->flags & SDHCI_SIGNALING_180) ||
> +		    !(host->caps & SDHCI_CAN_VDD_180))

You are mixing signal voltage and supply voltage here, and this is not
correct: not being able to do 1.8V supply does not mean you can't do 1.8V
signaling.  The SDHCI way of determining supported signal voltages is based
on supported transfer modes.

If the driver can't get the transfer modes correct (e.g. we still don't have
MMC_CAP2_3_3V_ONLY_DDR or equivalent), remove SDHCI_SIGNALING_180 from the
flags.

>  			return -EINVAL;
>  		if (!IS_ERR(mmc->supply.vqmmc)) {
>  			ret = mmc_regulator_set_vqmmc(mmc, ios);
> 

Re: [RESEDN PATCH v2 3/4] mmc: sdhci: Don't try to switch to unsupported voltage

[Shawn Lin]

在 2016/9/23 15:12, Adrian Hunter 写道:
> On 23/09/16 03:12, Shawn Lin wrote:
>> From: Ziyuan Xu <xzy.xu@rock-chips.com>
>>
>> Sdhci shouldn't switch to the unsupported voltage if claiming
>> that it can not support the requested voltage. Let's fix it.
>>
>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v2: None
>>
>>  drivers/mmc/host/sdhci.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 4805566..b1f1edd 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1845,7 +1845,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>>
>>  	switch (ios->signal_voltage) {
>>  	case MMC_SIGNAL_VOLTAGE_330:
>> -		if (!(host->flags & SDHCI_SIGNALING_330))
>> +		if (!(host->flags & SDHCI_SIGNALING_330) ||
>> +		    !(host->caps & SDHCI_CAN_VDD_330))
>
> You are mixing signal voltage and supply voltage here.  Note, I don't

Oh, I see what you mean, driver could still set 3.3 voltage supply
even they don't support 3V3 signal voltage.

> believe all drivers set the capabilities correctly when they are using
> regulators, but in any case this is not the place in the code to make such
> assumptions.

Indeed, there is a risk of breaking some drivers if they don't set the
capabilities correctly.

So, I think we could drop this patch since patch 4 is actually what we
need in order to slove our problem. :)

>
> Better for the driver to remove SDHCI_SIGNALING_330 from flags when it is
> not supported.
>
>>  			return -EINVAL;
>>  		/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
>>  		ctrl &= ~SDHCI_CTRL_VDD_180;
>> @@ -1872,7 +1873,8 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>>
>>  		return -EAGAIN;
>>  	case MMC_SIGNAL_VOLTAGE_180:
>> -		if (!(host->flags & SDHCI_SIGNALING_180))
>> +		if (!(host->flags & SDHCI_SIGNALING_180) ||
>> +		    !(host->caps & SDHCI_CAN_VDD_180))
>
> You are mixing signal voltage and supply voltage here, and this is not
> correct: not being able to do 1.8V supply does not mean you can't do 1.8V
> signaling.  The SDHCI way of determining supported signal voltages is based
> on supported transfer modes.
>
> If the driver can't get the transfer modes correct (e.g. we still don't have
> MMC_CAP2_3_3V_ONLY_DDR or equivalent), remove SDHCI_SIGNALING_180 from the
> flags.
>
>>  			return -EINVAL;
>>  		if (!IS_ERR(mmc->supply.vqmmc)) {
>>  			ret = mmc_regulator_set_vqmmc(mmc, ios);
>>
>
>
>
>


-- 
Best Regards
Shawn Lin

[RESEND PATCH v2 4/4] mmc: sdhci-of-arasan: add sdhci_arasan_voltage_switch for arasan,5.1

[Shawn Lin]

Per the vendor's requirement, we shouldn't do any setting for
1.8V Signaling Enable, otherwise the interaction/behaviour between
phy and controller will be undefined. Mostly it works fine if we do
that, but we still see failures. Anyway, let's fix it to meet the
vendor's requirement. The error log looks like:

 [   93.405085] mmc1: unexpected status 0x800900 after switch
 [   93.408474] mmc1: switch to bus width 1 failed
 [   93.408482] mmc1: mmc_select_hs200 failed, error -110
 [   93.408492] mmc1: error -110 during resume (card was removed?)
 [   93.408705] PM: resume of devices complete after 213.453 msecs

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci-of-arasan.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index da8e40a..1573a8d 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -265,6 +265,28 @@ void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
 	}
 }
 
+static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
+				       struct mmc_ios *ios)
+{
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_180:
+		/*
+		 * Plese don't switch to 1V8 as arasan,5.1 doesn't
+		 * actually refer to this setting to indicate the
+		 * signal voltage and the state machine will be broken
+		 * actually if we force to enable 1V8. That's something
+		 * like broken quirk but we could work around here.
+		 */
+		return 0;
+	case MMC_SIGNAL_VOLTAGE_330:
+	case MMC_SIGNAL_VOLTAGE_120:
+		/* We don't support 3V3 and 1V2 */
+		break;
+	}
+
+	return -EINVAL;
+}
+
 static struct sdhci_ops sdhci_arasan_ops = {
 	.set_clock = sdhci_arasan_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
@@ -661,6 +683,8 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 
 		host->mmc_host_ops.hs400_enhanced_strobe =
 					sdhci_arasan_hs400_enhanced_strobe;
+		host->mmc_host_ops.start_signal_voltage_switch =
+					sdhci_arasan_voltage_switch;
 	}
 
 	ret = sdhci_add_host(host);
-- 
2.3.7

Re: [RESEND PATCH v2 4/4] mmc: sdhci-of-arasan: add sdhci_arasan_voltage_switch for arasan,5.1

[Adrian Hunter]

On 23/09/16 03:12, Shawn Lin wrote:
> Per the vendor's requirement, we shouldn't do any setting for
> 1.8V Signaling Enable, otherwise the interaction/behaviour between
> phy and controller will be undefined. Mostly it works fine if we do
> that, but we still see failures. Anyway, let's fix it to meet the
> vendor's requirement. The error log looks like:
> 
>  [   93.405085] mmc1: unexpected status 0x800900 after switch
>  [   93.408474] mmc1: switch to bus width 1 failed
>  [   93.408482] mmc1: mmc_select_hs200 failed, error -110
>  [   93.408492] mmc1: error -110 during resume (card was removed?)
>  [   93.408705] PM: resume of devices complete after 213.453 msecs
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci-of-arasan.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index da8e40a..1573a8d 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -265,6 +265,28 @@ void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
>  	}
>  }
>  
> +static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
> +				       struct mmc_ios *ios)
> +{
> +	switch (ios->signal_voltage) {
> +	case MMC_SIGNAL_VOLTAGE_180:
> +		/*
> +		 * Plese don't switch to 1V8 as arasan,5.1 doesn't
> +		 * actually refer to this setting to indicate the
> +		 * signal voltage and the state machine will be broken
> +		 * actually if we force to enable 1V8. That's something
> +		 * like broken quirk but we could work around here.
> +		 */
> +		return 0;
> +	case MMC_SIGNAL_VOLTAGE_330:
> +	case MMC_SIGNAL_VOLTAGE_120:
> +		/* We don't support 3V3 and 1V2 */
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static struct sdhci_ops sdhci_arasan_ops = {
>  	.set_clock = sdhci_arasan_set_clock,
>  	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> @@ -661,6 +683,8 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  
>  		host->mmc_host_ops.hs400_enhanced_strobe =
>  					sdhci_arasan_hs400_enhanced_strobe;
> +		host->mmc_host_ops.start_signal_voltage_switch =
> +					sdhci_arasan_voltage_switch;
>  	}
>  
>  	ret = sdhci_add_host(host);
> 

Re: [RESEND PATCH v2 1/4] mmc: core: switch to 1V8 or 1V2 for hs400es mode

[Doug Anderson]

Hi,

On Thu, Sep 22, 2016 at 5:12 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> When introducing hs400es, I didn't notice that we haven't
> switched voltage to 1V2 or 1V8 for it. That happens to work
> as the first controller claiming to support hs400es, arasan(5.1),
> which is designed to only support 1V8. So the voltage is fixed to 1V8.
> But it actually is wrong, and will not fit for other host controllers.
> Let's fix it.
>
> Fixes: commit 81ac2af65793ecf ("mmc: core: implement enhanced strobe support")
> Cc: <stable@vger.kernel.org> 4.4#
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> Changes in v2:
> - fix copy-paste error, mea culpa
>
>  drivers/mmc/core/mmc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

This matches what's done for hs200 and also seems sane.

Reviewed-by: Douglas Anderson <dianders@chromium.org>