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

Groups > linux.kernel > #1235729 > unrolled thread

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

Started byChaotian Jing <chaotian.jing@mediatek.com>
First post2015-09-30 05:30 +0200
Last post2015-09-30 15:10 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] mmc: core: Fix init_card in 52Mhz Chaotian Jing <chaotian.jing@mediatek.com> - 2015-09-30 05:30 +0200
    Re: [PATCH] mmc: core: Fix init_card in 52Mhz Adrian Hunter <adrian.hunter@intel.com> - 2015-09-30 10:30 +0200
      Re: [PATCH] mmc: core: Fix init_card in 52Mhz Chaotian Jing <chaotian.jing@mediatek.com> - 2015-09-30 11:10 +0200
        Re: [PATCH] mmc: core: Fix init_card in 52Mhz Adrian Hunter <adrian.hunter@intel.com> - 2015-09-30 15:10 +0200

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

FromChaotian Jing <chaotian.jing@mediatek.com>
Date2015-09-30 05:30 +0200
Subject[PATCH] mmc: core: Fix init_card in 52Mhz
Message-ID<qegfD-1bp-5@gated-at.bofh.it>
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, that results in card init
was doing at 52Mhz, So need ensure frequency is lower than 400Khz when
re-init card.

Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
 drivers/mmc/core/mmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index e726903..f2d0c2a 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1945,6 +1945,7 @@ static int mmc_reset(struct mmc_host *host)
 
 	/* Set initial state and call mmc_set_ios */
 	mmc_set_initial_state(host);
+	mmc_set_clock(host, host->f_init);
 	mmc_host_clk_release(host);
 
 	return mmc_init_card(host, card->ocr, card);
-- 
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]

#1235889

FromAdrian Hunter <adrian.hunter@intel.com>
Date2015-09-30 10:30 +0200
Message-ID<qekVZ-7TY-21@gated-at.bofh.it>
In reply to#1235729
On 30/09/15 06:28, Chaotian Jing 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, that results in card init
> was doing at 52Mhz, So need ensure frequency is lower than 400Khz when
> re-init card.

The call to mmc_send_status() was originally only done for mmc_test and
doesn't belong in a real reset.  However the test parameter was removed
to simplify the code by:

commit 83533ab28380f6957af39a7b322e639e42dbdaf1
Author: Johan Rudholm <johan.rudholm@axis.com>
Date:   Mon Jan 12 15:38:04 2015 +0100

    mmc: core: always check status after reset
    
    Always check if the card is alive after a successful reset. This allows
    us to remove mmc_hw_reset_check(), leaving mmc_hw_reset() as the only
    card reset interface.
    
    Signed-off-by: Johan Rudholm <johanru@axis.com>
    Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

IMHO having a test to check whether hw reset actually did reset the card
is useful, so I would suggest putting back mmc_hw_reset_check() and
removing the check from the non-test path.

An alternative is to recognize that we don't want retuning here, and it is
unlikely to be needed in the mmc_test case.  So if retuning is needed, just
skip the call to mmc_send_status() i.e.

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index f2d0c2ae85b1..c3749ba42739 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;
 	}

Could then make mmc_test aware of that:

diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index b78cf5d403a3..7b105b97be78 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -2272,6 +2272,16 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
 	if (!mmc_card_mmc(card) || !mmc_can_reset(card))
 		return RESULT_UNSUP_CARD;
 
+	err = mmc_send_status(card, NULL);
+	if (err)
+		return err;
+
+	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 f2d0c2ae85b1..c3749ba42739 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;
 	}




> 
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/core/mmc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index e726903..f2d0c2a 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1945,6 +1945,7 @@ static int mmc_reset(struct mmc_host *host)
>  
>  	/* Set initial state and call mmc_set_ios */
>  	mmc_set_initial_state(host);
> +	mmc_set_clock(host, host->f_init);
>  	mmc_host_clk_release(host);
>  
>  	return mmc_init_card(host, card->ocr, card);
> 

--
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]

#1235918

FromChaotian Jing <chaotian.jing@mediatek.com>
Date2015-09-30 11:10 +0200
Message-ID<qelyF-rL-19@gated-at.bofh.it>
In reply to#1235889
On Wed, 2015-09-30 at 11:24 +0300, Adrian Hunter wrote:
> On 30/09/15 06:28, Chaotian Jing 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, that results in card init
> > was doing at 52Mhz, So need ensure frequency is lower than 400Khz when
> > re-init card.
> 
> The call to mmc_send_status() was originally only done for mmc_test and
> doesn't belong in a real reset.  However the test parameter was removed
> to simplify the code by:
> 
> commit 83533ab28380f6957af39a7b322e639e42dbdaf1
> Author: Johan Rudholm <johan.rudholm@axis.com>
> Date:   Mon Jan 12 15:38:04 2015 +0100
> 
>     mmc: core: always check status after reset
>     
>     Always check if the card is alive after a successful reset. This allows
>     us to remove mmc_hw_reset_check(), leaving mmc_hw_reset() as the only
>     card reset interface.
>     
>     Signed-off-by: Johan Rudholm <johanru@axis.com>
>     Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> IMHO having a test to check whether hw reset actually did reset the card
> is useful, so I would suggest putting back mmc_hw_reset_check() and
> removing the check from the non-test path.
> 
> An alternative is to recognize that we don't want retuning here, and it is
> unlikely to be needed in the mmc_test case.  So if retuning is needed, just
> skip the call to mmc_send_status() i.e.
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index f2d0c2ae85b1..c3749ba42739 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;
>  	}
> 
> Could then make mmc_test aware of that:
>  

> This solution is better, please help to upstream it. Thx!
> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
> index b78cf5d403a3..7b105b97be78 100644
> --- a/drivers/mmc/card/mmc_test.c
> +++ b/drivers/mmc/card/mmc_test.c
> @@ -2272,6 +2272,16 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
>  	if (!mmc_card_mmc(card) || !mmc_can_reset(card))
>  		return RESULT_UNSUP_CARD;
>  
> +	err = mmc_send_status(card, NULL);
> +	if (err)
> +		return err;
> +
> +	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 f2d0c2ae85b1..c3749ba42739 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;
>  	}
> 
> 
> 
> 
> > 
> > Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> > ---
> >  drivers/mmc/core/mmc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index e726903..f2d0c2a 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -1945,6 +1945,7 @@ static int mmc_reset(struct mmc_host *host)
> >  
> >  	/* Set initial state and call mmc_set_ios */
> >  	mmc_set_initial_state(host);
> > +	mmc_set_clock(host, host->f_init);
> >  	mmc_host_clk_release(host);
> >  
> >  	return mmc_init_card(host, card->ocr, card);
> > 
> 


--
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]

#1236271

FromAdrian Hunter <adrian.hunter@intel.com>
Date2015-09-30 15:10 +0200
Message-ID<qepiW-5S4-7@gated-at.bofh.it>
In reply to#1235918
On 30/09/15 12:03, Chaotian Jing wrote:
> On Wed, 2015-09-30 at 11:24 +0300, Adrian Hunter wrote:
>> On 30/09/15 06:28, Chaotian Jing 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, that results in card init
>>> was doing at 52Mhz, So need ensure frequency is lower than 400Khz when
>>> re-init card.
>>
>> The call to mmc_send_status() was originally only done for mmc_test and
>> doesn't belong in a real reset.  However the test parameter was removed
>> to simplify the code by:
>>
>> commit 83533ab28380f6957af39a7b322e639e42dbdaf1
>> Author: Johan Rudholm <johan.rudholm@axis.com>
>> Date:   Mon Jan 12 15:38:04 2015 +0100
>>
>>     mmc: core: always check status after reset
>>     
>>     Always check if the card is alive after a successful reset. This allows
>>     us to remove mmc_hw_reset_check(), leaving mmc_hw_reset() as the only
>>     card reset interface.
>>     
>>     Signed-off-by: Johan Rudholm <johanru@axis.com>
>>     Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> IMHO having a test to check whether hw reset actually did reset the card
>> is useful, so I would suggest putting back mmc_hw_reset_check() and
>> removing the check from the non-test path.
>>
>> An alternative is to recognize that we don't want retuning here, and it is
>> unlikely to be needed in the mmc_test case.  So if retuning is needed, just
>> skip the call to mmc_send_status() i.e.
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index f2d0c2ae85b1..c3749ba42739 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;
>>  	}
>>
>> Could then make mmc_test aware of that:
>>  
> 
>> This solution is better, please help to upstream it. Thx!

Then please, make it a patch, and test it and submit it.  You can add:

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


>> diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
>> index b78cf5d403a3..7b105b97be78 100644
>> --- a/drivers/mmc/card/mmc_test.c
>> +++ b/drivers/mmc/card/mmc_test.c
>> @@ -2272,6 +2272,16 @@ static int mmc_test_hw_reset(struct mmc_test_card *test)
>>  	if (!mmc_card_mmc(card) || !mmc_can_reset(card))
>>  		return RESULT_UNSUP_CARD;
>>  
>> +	err = mmc_send_status(card, NULL);
>> +	if (err)
>> +		return err;
>> +
>> +	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 f2d0c2ae85b1..c3749ba42739 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;
>>  	}
>>
>>
>>
>>
>>>
>>> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
>>> ---
>>>  drivers/mmc/core/mmc.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index e726903..f2d0c2a 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1945,6 +1945,7 @@ static int mmc_reset(struct mmc_host *host)
>>>  
>>>  	/* Set initial state and call mmc_set_ios */
>>>  	mmc_set_initial_state(host);
>>> +	mmc_set_clock(host, host->f_init);
>>>  	mmc_host_clk_release(host);
>>>  
>>>  	return mmc_init_card(host, card->ocr, card);
>>>
>>
> 
> 
> 

--
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