Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1242009 > unrolled thread

[PATCH v2] mmc: core: Fix init_card in 52Mhz

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] mmc: core: Fix init_card in 52Mhz Chaotian Jing <chaotian.jing@mediatek.com> - 2015-10-08 09:10 +0200
    Re: [PATCH v2] mmc: core: Fix init_card in 52Mhz Ulf Hansson <ulf.hansson@linaro.org> - 2015-10-08 13:00 +0200
      Re: [PATCH v2] mmc: core: Fix init_card in 52Mhz Adrian Hunter <adrian.hunter@intel.com> - 2015-10-08 15:10 +0200
        Re: [PATCH v2] mmc: core: Fix init_card in 52Mhz Ulf Hansson <ulf.hansson@linaro.org> - 2015-10-08 15:40 +0200
          Re: [PATCH v2] mmc: core: Fix init_card in 52Mhz Adrian Hunter <adrian.hunter@intel.com> - 2015-10-09 10:10 +0200
            Re: [PATCH v2] mmc: core: Fix init_card in 52Mhz Ulf Hansson <ulf.hansson@linaro.org> - 2015-10-09 10:40 +0200

#1242009 — [PATCH v2] mmc: core: Fix init_card in 52Mhz

Suppose that we got a data crc error, and it triggers the mmc_reset.
mmc_reset will call mmc_send_status to see if HW reset was supported.
before issue CMD13, it will do retune, and if EMMC was in HS400 mode,
it will reduce frequency to 52Mhz firstly, then results in card init
was doing at 52Mhz.
The mmc_send_status was originally only done for mmc_test, so if retune
needed, do not call mmc_send_status.

Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/card/mmc_test.c | 6 ++++++
 drivers/mmc/core/mmc.c      | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index b78cf5d..2f78bfb 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -2272,6 +2272,12 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
 	if (!mmc_card_mmc(card) || !mmc_can_reset(card))
 		return RESULT_UNSUP_CARD;
 
+	if (host->need_retune) {
+		pr_info("%s: cannot test hw reset because retune needed\n",
+			mmc_hostname(test->card->host));
+		return RESULT_FAIL;
+	}
+
 	err = mmc_hw_reset(host);
 	if (!err)
 		return RESULT_OK;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index e726903..647c96d 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1938,7 +1938,7 @@ static int mmc_reset(struct mmc_host *host)
 	host->ops->hw_reset(host);
 
 	/* If the reset has happened, then a status command will fail */
-	if (!mmc_send_status(card, &status)) {
+	if (!host->need_retune && !mmc_send_status(card, &status)) {
 		mmc_host_clk_release(host);
 		return -ENOSYS;
 	}
-- 
1.8.1.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1242189

On 8 October 2015 at 09:09, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> Suppose that we got a data crc error, and it triggers the mmc_reset.
> mmc_reset will call mmc_send_status to see if HW reset was supported.
> before issue CMD13, it will do retune, and if EMMC was in HS400 mode,
> it will reduce frequency to 52Mhz firstly, then results in card init
> was doing at 52Mhz.
> The mmc_send_status was originally only done for mmc_test, so if retune
> needed, do not call mmc_send_status.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/card/mmc_test.c | 6 ++++++
>  drivers/mmc/core/mmc.c      | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
> index b78cf5d..2f78bfb 100644
> --- a/drivers/mmc/card/mmc_test.c
> +++ b/drivers/mmc/card/mmc_test.c
> @@ -2272,6 +2272,12 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
>         if (!mmc_card_mmc(card) || !mmc_can_reset(card))
>                 return RESULT_UNSUP_CARD;
>
> +       if (host->need_retune) {
> +               pr_info("%s: cannot test hw reset because retune needed\n",
> +                       mmc_hostname(test->card->host));
> +               return RESULT_FAIL;
> +       }
> +
>         err = mmc_hw_reset(host);
>         if (!err)
>                 return RESULT_OK;
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index e726903..647c96d 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1938,7 +1938,7 @@ static int mmc_reset(struct mmc_host *host)
>         host->ops->hw_reset(host);
>
>         /* If the reset has happened, then a status command will fail */
> -       if (!mmc_send_status(card, &status)) {
> +       if (!host->need_retune && !mmc_send_status(card, &status)) {

No, this seem like the wrong solution! The main purpose of mmc_reset()
is to try to reset and re-initiate the card, to make it operational
again.

I can't find a good reason to why we want to do a mmc_send_status() at
this point, as even if it succeeds it will only tell us that the card
has not been "hw-reset". No matter what, we should still try to make
it fully operational again and thus give mmc_init_card() a try.

From this reasoning, I suggest we remove the call to mmc_send_status()
from this path, as that will also address your issue with CRC errors
in combination with re-tune.

>                 mmc_host_clk_release(host);
>                 return -ENOSYS;
>         }
> --
> 1.8.1.1.dirty
>

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242323

On 08/10/15 13:59, Ulf Hansson wrote:
> On 8 October 2015 at 09:09, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>> Suppose that we got a data crc error, and it triggers the mmc_reset.
>> mmc_reset will call mmc_send_status to see if HW reset was supported.
>> before issue CMD13, it will do retune, and if EMMC was in HS400 mode,
>> it will reduce frequency to 52Mhz firstly, then results in card init
>> was doing at 52Mhz.
>> The mmc_send_status was originally only done for mmc_test, so if retune
>> needed, do not call mmc_send_status.
>>
>> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
>> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/card/mmc_test.c | 6 ++++++
>>  drivers/mmc/core/mmc.c      | 2 +-
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
>> index b78cf5d..2f78bfb 100644
>> --- a/drivers/mmc/card/mmc_test.c
>> +++ b/drivers/mmc/card/mmc_test.c
>> @@ -2272,6 +2272,12 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
>>         if (!mmc_card_mmc(card) || !mmc_can_reset(card))
>>                 return RESULT_UNSUP_CARD;
>>
>> +       if (host->need_retune) {
>> +               pr_info("%s: cannot test hw reset because retune needed\n",
>> +                       mmc_hostname(test->card->host));
>> +               return RESULT_FAIL;
>> +       }
>> +
>>         err = mmc_hw_reset(host);
>>         if (!err)
>>                 return RESULT_OK;
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index e726903..647c96d 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1938,7 +1938,7 @@ static int mmc_reset(struct mmc_host *host)
>>         host->ops->hw_reset(host);
>>
>>         /* If the reset has happened, then a status command will fail */
>> -       if (!mmc_send_status(card, &status)) {
>> +       if (!host->need_retune && !mmc_send_status(card, &status)) {
> 
> No, this seem like the wrong solution! The main purpose of mmc_reset()
> is to try to reset and re-initiate the card, to make it operational
> again.
> 
> I can't find a good reason to why we want to do a mmc_send_status() at
> this point, as even if it succeeds it will only tell us that the card
> has not been "hw-reset". No matter what, we should still try to make
> it fully operational again and thus give mmc_init_card() a try.
> 
>>From this reasoning, I suggest we remove the call to mmc_send_status()
> from this path, as that will also address your issue with CRC errors
> in combination with re-tune.

Then you need to remove the hw_reset test from mmc_test.  Refer:

	http://marc.info/?l=linux-mmc&m=144360165906544&w=2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242349

On 8 October 2015 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 08/10/15 13:59, Ulf Hansson wrote:
>> On 8 October 2015 at 09:09, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>>> Suppose that we got a data crc error, and it triggers the mmc_reset.
>>> mmc_reset will call mmc_send_status to see if HW reset was supported.
>>> before issue CMD13, it will do retune, and if EMMC was in HS400 mode,
>>> it will reduce frequency to 52Mhz firstly, then results in card init
>>> was doing at 52Mhz.
>>> The mmc_send_status was originally only done for mmc_test, so if retune
>>> needed, do not call mmc_send_status.
>>>
>>> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
>>> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  drivers/mmc/card/mmc_test.c | 6 ++++++
>>>  drivers/mmc/core/mmc.c      | 2 +-
>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
>>> index b78cf5d..2f78bfb 100644
>>> --- a/drivers/mmc/card/mmc_test.c
>>> +++ b/drivers/mmc/card/mmc_test.c
>>> @@ -2272,6 +2272,12 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
>>>         if (!mmc_card_mmc(card) || !mmc_can_reset(card))
>>>                 return RESULT_UNSUP_CARD;
>>>
>>> +       if (host->need_retune) {
>>> +               pr_info("%s: cannot test hw reset because retune needed\n",
>>> +                       mmc_hostname(test->card->host));
>>> +               return RESULT_FAIL;
>>> +       }
>>> +
>>>         err = mmc_hw_reset(host);
>>>         if (!err)
>>>                 return RESULT_OK;
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index e726903..647c96d 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1938,7 +1938,7 @@ static int mmc_reset(struct mmc_host *host)
>>>         host->ops->hw_reset(host);
>>>
>>>         /* If the reset has happened, then a status command will fail */
>>> -       if (!mmc_send_status(card, &status)) {
>>> +       if (!host->need_retune && !mmc_send_status(card, &status)) {
>>
>> No, this seem like the wrong solution! The main purpose of mmc_reset()
>> is to try to reset and re-initiate the card, to make it operational
>> again.
>>
>> I can't find a good reason to why we want to do a mmc_send_status() at
>> this point, as even if it succeeds it will only tell us that the card
>> has not been "hw-reset". No matter what, we should still try to make
>> it fully operational again and thus give mmc_init_card() a try.
>>
>>>From this reasoning, I suggest we remove the call to mmc_send_status()
>> from this path, as that will also address your issue with CRC errors
>> in combination with re-tune.
>
> Then you need to remove the hw_reset test from mmc_test.  Refer:
>
>         http://marc.info/?l=linux-mmc&m=144360165906544&w=2
>

I realize that the test becomes a bit different, but I don't think it's useless.

If we add a check for MMC_CAP_HW_RESET and verify that the
host->ops->hw_reset exists, then we can assume that the "hw_reset"
sequence has executed. And if mmc_init_card() fails, that would
probably mean that the reset also failed, right?

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243100

On 08/10/15 16:35, Ulf Hansson wrote:
> On 8 October 2015 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 08/10/15 13:59, Ulf Hansson wrote:
>>> On 8 October 2015 at 09:09, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>>>> Suppose that we got a data crc error, and it triggers the mmc_reset.
>>>> mmc_reset will call mmc_send_status to see if HW reset was supported.
>>>> before issue CMD13, it will do retune, and if EMMC was in HS400 mode,
>>>> it will reduce frequency to 52Mhz firstly, then results in card init
>>>> was doing at 52Mhz.
>>>> The mmc_send_status was originally only done for mmc_test, so if retune
>>>> needed, do not call mmc_send_status.
>>>>
>>>> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
>>>> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/card/mmc_test.c | 6 ++++++
>>>>  drivers/mmc/core/mmc.c      | 2 +-
>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
>>>> index b78cf5d..2f78bfb 100644
>>>> --- a/drivers/mmc/card/mmc_test.c
>>>> +++ b/drivers/mmc/card/mmc_test.c
>>>> @@ -2272,6 +2272,12 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
>>>>         if (!mmc_card_mmc(card) || !mmc_can_reset(card))
>>>>                 return RESULT_UNSUP_CARD;
>>>>
>>>> +       if (host->need_retune) {
>>>> +               pr_info("%s: cannot test hw reset because retune needed\n",
>>>> +                       mmc_hostname(test->card->host));
>>>> +               return RESULT_FAIL;
>>>> +       }
>>>> +
>>>>         err = mmc_hw_reset(host);
>>>>         if (!err)
>>>>                 return RESULT_OK;
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index e726903..647c96d 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1938,7 +1938,7 @@ static int mmc_reset(struct mmc_host *host)
>>>>         host->ops->hw_reset(host);
>>>>
>>>>         /* If the reset has happened, then a status command will fail */
>>>> -       if (!mmc_send_status(card, &status)) {
>>>> +       if (!host->need_retune && !mmc_send_status(card, &status)) {
>>>
>>> No, this seem like the wrong solution! The main purpose of mmc_reset()
>>> is to try to reset and re-initiate the card, to make it operational
>>> again.
>>>
>>> I can't find a good reason to why we want to do a mmc_send_status() at
>>> this point, as even if it succeeds it will only tell us that the card
>>> has not been "hw-reset". No matter what, we should still try to make
>>> it fully operational again and thus give mmc_init_card() a try.
>>>
>>> >From this reasoning, I suggest we remove the call to mmc_send_status()
>>> from this path, as that will also address your issue with CRC errors
>>> in combination with re-tune.
>>
>> Then you need to remove the hw_reset test from mmc_test.  Refer:
>>
>>         http://marc.info/?l=linux-mmc&m=144360165906544&w=2
>>
> 
> I realize that the test becomes a bit different, but I don't think it's useless.
> 
> If we add a check for MMC_CAP_HW_RESET and verify that the
> host->ops->hw_reset exists, then we can assume that the "hw_reset"
> sequence has executed. And if mmc_init_card() fails, that would
> probably mean that the reset also failed, right?

In the test case, the card is in a working state.  Generally I would then
expect reinitialization to work irrespective of whether or not the hardware
is actually reset.

Here are some other options:
	1. have mmc_test hook the host->ops->hw_reset() fn and do the send_status
itself.
	2. have mmc_test set a flag on the card that it is being tested
and only do the send_status if the flag is set
	3. remove the send_status call and rename the mmc_test from "eMMC hardware
reset" to just "Reset test (doesn't check hw reset did reset)"

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243126

[...]

>>> Then you need to remove the hw_reset test from mmc_test.  Refer:
>>>
>>>         http://marc.info/?l=linux-mmc&m=144360165906544&w=2
>>>
>>
>> I realize that the test becomes a bit different, but I don't think it's useless.
>>
>> If we add a check for MMC_CAP_HW_RESET and verify that the
>> host->ops->hw_reset exists, then we can assume that the "hw_reset"
>> sequence has executed. And if mmc_init_card() fails, that would
>> probably mean that the reset also failed, right?
>
> In the test case, the card is in a working state.  Generally I would then
> expect reinitialization to work irrespective of whether or not the hardware
> is actually reset.

That's not always the case. I have seen many strange things happening
while trying to re-initialize/reset the card. :-)

>
> Here are some other options:
>         1. have mmc_test hook the host->ops->hw_reset() fn and do the send_status
> itself.
>         2. have mmc_test set a flag on the card that it is being tested
> and only do the send_status if the flag is set

I assume that both 1) and 2) still means we need to manage the
scenario with re-tuning, which I rather would like us to prevent.

Unless we find a way to call mmc_set_initial_state() before doing the
reset, as that would disable re-tuning...

>         3. remove the send_status call and rename the mmc_test from "eMMC hardware
> reset" to just "Reset test (doesn't check hw reset did reset)"

That's would work and perhaps this is the best way to go as we would
then also be able to use the test for SD-cards. Let's do this then!

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web