[PATCH 0/5] Some fixes for mmc core and sdhci/arasan

[PATCH 0/5] Some fixes for mmc core and sdhci/arasan

[Shawn Lin]

patch 1 is gonna fix the longstanding issues which may fail the
mmc_test. Per spec, block size is always 512 bytes for dual rate mode.
Any attempt to change it will be treated as illegal one.So we
need to check hs400(es) and DDR50 as well.

patch 2 and 3 actually fix my previous introduction for hs400es.
That happened to work for the controller and devices I was using
for test at that time. I believe Doug and Jaehoon didn't see failure
when testing it as well. After several numbers of compatibility test
and stability test for mass production, we are sure that patch 3 is
needed.

patch 4 also fix a longtanding issue of sdhci which try to switch
voltage without checking the cap. We found this issue as arasan,5.1
doesn't support VDD_330 but we still find sdhci try to switch to
3v3.

patch 5 will nak my patch[0]. It more or less looks like a hack from
Jaehoon's point. I think I find the root cause after numerous repro and
debug work with my ASIC team guys.

Thanks for Ziyuan Xu and Xiao Yao who help me test my massive hack patch
and finally we found what the problem is.

[0]: https://patchwork.kernel.org/patch/9238663



Shawn Lin (3):
  mmc: core: switch to 1V8 or 1V2 for hs400es mode
  mmc: core: changes frequency to hs_max_dtr when selecting hs400es
  mmc: sdhci-of-arasan: add sdhci_arasan_voltage_switch for arasan,5.1

Ziyuan Xu (2):
  mmc: core: don't try to switch block size for dual rate mode
  mmc: sdhci: Don't try to switch to unsupported voltage

 drivers/mmc/core/core.c            |  3 ++-
 drivers/mmc/core/mmc.c             | 12 ++++++++++++
 drivers/mmc/host/sdhci-of-arasan.c | 24 ++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c           |  6 ++++--
 4 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.3.7

[PATCH 1/5] mmc: core: don't try to switch block size for dual rate mode

[Shawn Lin]

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

Per spec, block size should always be 512 bytes for dual rate mode,
so any attempts to switch the block size under dual rate mode should
be neglected.

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

---

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index f0ed0af..2553d90 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2576,7 +2576,8 @@ int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen)
 {
 	struct mmc_command cmd = {0};
 
-	if (mmc_card_blockaddr(card) || mmc_card_ddr52(card))
+	if (mmc_card_blockaddr(card) || mmc_card_ddr52(card) ||
+	    mmc_card_hs400(card) || mmc_card_hs400es(card))
 		return 0;
 
 	cmd.opcode = MMC_SET_BLOCKLEN;
-- 
2.3.7

[PATCH 2/5] 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>
---

 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..3163bb9 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_HS200_1_2V)
+		err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
+
+	if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_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

[PATCH 3/5] 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. It seems there is no difference if
we don't change frequency to <= 52MHz as f_init is already less than
52MHz. But actually it does make difference. When doing compatibility
test we see failures for some eMMC devices without changing the
frequency to hs_max_dtr. And let's read the spec again, we could see
that "Host may changes frequency to 200MHz" implies that it's not
mandatory. But the "Host need to change frequency to <= 52MHz" implies
that we should do this.

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

 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 3163bb9..989d37e 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

[PATCH 4/5] 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>
---

 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

[PATCH 5/5] 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>
---

 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 33601a8..9162495 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -245,6 +245,28 @@ static void sdhci_arasan_hs400_enhanced_strobe(struct mmc_host *mmc,
 	writel(vendor, host->ioaddr + SDHCI_ARASAN_VENDOR_REGISTER);
 }
 
+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,
@@ -636,6 +658,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: [PATCH 3/5] mmc: core: changes frequency to hs_max_dtr when selecting hs400es

[Ulf Hansson]

On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 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. It seems there is no difference if
> we don't change frequency to <= 52MHz as f_init is already less than
> 52MHz. But actually it does make difference. When doing compatibility
> test we see failures for some eMMC devices without changing the
> frequency to hs_max_dtr. And let's read the spec again, we could see
> that "Host may changes frequency to 200MHz" implies that it's not
> mandatory. But the "Host need to change frequency to <= 52MHz" implies
> that we should do this.

I don't get this. Are you saying that f_init > 52 MHz? That should not
be impossible, right!?

So either the core has changed the clock rate by mistake at some other
execution path, or the host driver didn't set the correct clock rate
the first time when invoked via mmc_power_up()?

Kind regards
Uffe

>
> Reported-by: Xiao Yao <xiaoyao@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
>  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 3163bb9..989d37e 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: [PATCH 2/5] mmc: core: switch to 1V8 or 1V2 for hs400es mode

[Ulf Hansson]

On 21 September 2016 at 03:43, 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>
> ---
>
>  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..3163bb9 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_HS200_1_2V)

/s/EXT_CSD_CARD_TYPE_HS200_1_2V/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_HS200_1_8V)

/s/EXT_CSD_CARD_TYPE_HS200_1_8V/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
>
>

Kind regards
Uffe

Re: [PATCH 1/5] mmc: core: don't try to switch block size for dual rate mode

[Ulf Hansson]

On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> From: Ziyuan Xu <xzy.xu@rock-chips.com>
>
> Per spec, block size should always be 512 bytes for dual rate mode,
> so any attempts to switch the block size under dual rate mode should
> be neglected.
>
> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

Thanks, applied for next!

Kind regards
Uffe

>
> ---
>
>  drivers/mmc/core/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index f0ed0af..2553d90 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2576,7 +2576,8 @@ int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen)
>  {
>         struct mmc_command cmd = {0};
>
> -       if (mmc_card_blockaddr(card) || mmc_card_ddr52(card))
> +       if (mmc_card_blockaddr(card) || mmc_card_ddr52(card) ||
> +           mmc_card_hs400(card) || mmc_card_hs400es(card))
>                 return 0;
>
>         cmd.opcode = MMC_SET_BLOCKLEN;
> --
> 2.3.7
>
>

Re: [PATCH 3/5] mmc: core: changes frequency to hs_max_dtr when selecting hs400es

[Shawn Lin]

Hi ulf,

在 2016/9/22 17:38, Ulf Hansson 写道:
> On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> 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. It seems there is no difference if
>> we don't change frequency to <= 52MHz as f_init is already less than
>> 52MHz. But actually it does make difference. When doing compatibility
>> test we see failures for some eMMC devices without changing the
>> frequency to hs_max_dtr. And let's read the spec again, we could see
>> that "Host may changes frequency to 200MHz" implies that it's not
>> mandatory. But the "Host need to change frequency to <= 52MHz" implies
>> that we should do this.
>
> I don't get this. Are you saying that f_init > 52 MHz? That should not
> be impossible, right!?

nope, I was saying that the spec implies we to set clock after
setting HS_TIMING to 0x1 when doing hs400es selection.

I thought there is no difference because the spec says "Host need to
change frequency to <= 52MHz", and the f_init(<=400k) is <= 52MHz,
right? So I didn't set clock to hs_max_dtr. But I think I misunderstood
the spec, so this patch will fix this.


>
> So either the core has changed the clock rate by mistake at some other
> execution path, or the host driver didn't set the correct clock rate
> the first time when invoked via mmc_power_up()?
>
> Kind regards
> Uffe
>
>>
>> Reported-by: Xiao Yao <xiaoyao@rock-chips.com>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>  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 3163bb9..989d37e 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
>>
>>
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 3/5] mmc: core: changes frequency to hs_max_dtr when selecting hs400es

[Ulf Hansson]

On 22 September 2016 at 12:06, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Hi ulf,
>
> 在 2016/9/22 17:38, Ulf Hansson 写道:
>>
>> On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>
>>> 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. It seems there is no difference if
>>> we don't change frequency to <= 52MHz as f_init is already less than
>>> 52MHz. But actually it does make difference. When doing compatibility
>>> test we see failures for some eMMC devices without changing the
>>> frequency to hs_max_dtr. And let's read the spec again, we could see
>>> that "Host may changes frequency to 200MHz" implies that it's not
>>> mandatory. But the "Host need to change frequency to <= 52MHz" implies
>>> that we should do this.
>>
>>
>> I don't get this. Are you saying that f_init > 52 MHz? That should not
>> be impossible, right!?
>
>
> nope, I was saying that the spec implies we to set clock after
> setting HS_TIMING to 0x1 when doing hs400es selection.
>
> I thought there is no difference because the spec says "Host need to
> change frequency to <= 52MHz", and the f_init(<=400k) is <= 52MHz,
> right? So I didn't set clock to hs_max_dtr. But I think I misunderstood
> the spec, so this patch will fix this.

Okay, I see what you mean now!

In other words:
The card expects the clock rate to increase from the current used
f_init (which is <= 400KHz), but still being <= 52MHz, when you have
set HS_TIMING to 0x1.

Okay, we can do that change! Could you try to improve the change log a
little bit or you want me to help?

Kind regards
Uffe

Re: [PATCH 3/5] mmc: core: changes frequency to hs_max_dtr when selecting hs400es

[Shawn Lin]

On 2016/9/22 18:21, Ulf Hansson wrote:
> On 22 September 2016 at 12:06, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> Hi ulf,
>>
>> 在 2016/9/22 17:38, Ulf Hansson 写道:
>>>
>>> On 21 September 2016 at 03:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>
>>>> 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. It seems there is no difference if
>>>> we don't change frequency to <= 52MHz as f_init is already less than
>>>> 52MHz. But actually it does make difference. When doing compatibility
>>>> test we see failures for some eMMC devices without changing the
>>>> frequency to hs_max_dtr. And let's read the spec again, we could see
>>>> that "Host may changes frequency to 200MHz" implies that it's not
>>>> mandatory. But the "Host need to change frequency to <= 52MHz" implies
>>>> that we should do this.
>>>
>>>
>>> I don't get this. Are you saying that f_init > 52 MHz? That should not
>>> be impossible, right!?
>>
>>
>> nope, I was saying that the spec implies we to set clock after
>> setting HS_TIMING to 0x1 when doing hs400es selection.
>>
>> I thought there is no difference because the spec says "Host need to
>> change frequency to <= 52MHz", and the f_init(<=400k) is <= 52MHz,
>> right? So I didn't set clock to hs_max_dtr. But I think I misunderstood
>> the spec, so this patch will fix this.
>
> Okay, I see what you mean now!
>
> In other words:
> The card expects the clock rate to increase from the current used
> f_init (which is <= 400KHz), but still being <= 52MHz, when you have
> set HS_TIMING to 0x1.
>
> Okay, we can do that change! Could you try to improve the change log a
> little bit or you want me to help?

yep, I could change the commit msg a bit and fix another
copy-paste error, then respin v2.
BTW, I noticed you have applied one of these 5 patches, so
I will remove that one for V2.

Thanks, Ulf.

>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin