diff mbox

[1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

Message ID 1362142035-8403-2-git-send-email-ulf.hansson@stericsson.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ulf Hansson March 1, 2013, 12:47 p.m. UTC
From: Ulf Hansson <ulf.hansson@linaro.org>

The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
func drivers are also moving towards use of runtime pm to accomplish the
the same operation and since this API is not used for mmc and sd it makes
sense to remove the corresponding bus_ops.

Moreover, at least for eMMC the mmc bus_ops is broken, since no consideration
is taken for mmc sleep and mmc power off notify.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c |   14 --------------
 drivers/mmc/core/sd.c  |   14 --------------
 2 files changed, 28 deletions(-)

Comments

Maya Erez April 3, 2013, 11:08 a.m. UTC | #1
Acked-by: Maya Erez <merez@codeaurora.org>

> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> The mmc_power_restore|save_host API is only used by SDIO func drivers.
> SDIO
> func drivers are also moving towards use of runtime pm to accomplish the
> the same operation and since this API is not used for mmc and sd it makes
> sense to remove the corresponding bus_ops.
>
> Moreover, at least for eMMC the mmc bus_ops is broken, since no
> consideration
> is taken for mmc sleep and mmc power off notify.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c |   14 --------------
>  drivers/mmc/core/sd.c  |   14 --------------
>  2 files changed, 28 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c8f3d6e..edc6bc4 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1445,18 +1445,6 @@ static int mmc_resume(struct mmc_host *host)
>  	return err;
>  }
>
> -static int mmc_power_restore(struct mmc_host *host)
> -{
> -	int ret;
> -
> -	host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
> -	mmc_claim_host(host);
> -	ret = mmc_init_card(host, host->ocr, host->card);
> -	mmc_release_host(host);
> -
> -	return ret;
> -}
> -
>  static int mmc_sleep(struct mmc_host *host)
>  {
>  	struct mmc_card *card = host->card;
> @@ -1494,7 +1482,6 @@ static const struct mmc_bus_ops mmc_ops = {
>  	.detect = mmc_detect,
>  	.suspend = NULL,
>  	.resume = NULL,
> -	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
>  };
>
> @@ -1505,7 +1492,6 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>  	.detect = mmc_detect,
>  	.suspend = mmc_suspend,
>  	.resume = mmc_resume,
> -	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
>  };
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 9e645e1..b71d906 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1095,24 +1095,11 @@ static int mmc_sd_resume(struct mmc_host *host)
>  	return err;
>  }
>
> -static int mmc_sd_power_restore(struct mmc_host *host)
> -{
> -	int ret;
> -
> -	host->card->state &= ~MMC_STATE_HIGHSPEED;
> -	mmc_claim_host(host);
> -	ret = mmc_sd_init_card(host, host->ocr, host->card);
> -	mmc_release_host(host);
> -
> -	return ret;
> -}
> -
>  static const struct mmc_bus_ops mmc_sd_ops = {
>  	.remove = mmc_sd_remove,
>  	.detect = mmc_sd_detect,
>  	.suspend = NULL,
>  	.resume = NULL,
> -	.power_restore = mmc_sd_power_restore,
>  	.alive = mmc_sd_alive,
>  };
>
> @@ -1121,7 +1108,6 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe =
> {
>  	.detect = mmc_sd_detect,
>  	.suspend = mmc_sd_suspend,
>  	.resume = mmc_sd_resume,
> -	.power_restore = mmc_sd_power_restore,
>  	.alive = mmc_sd_alive,
>  };
>
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Adrian Hunter April 4, 2013, 8:46 a.m. UTC | #2
On 01/03/13 14:47, Ulf Hansson wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
> 
> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO

NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()

> func drivers are also moving towards use of runtime pm to accomplish the
> the same operation and since this API is not used for mmc and sd it makes
> sense to remove the corresponding bus_ops.
> 
> Moreover, at least for eMMC the mmc bus_ops is broken, since no consideration
> is taken for mmc sleep and mmc power off notify.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c |   14 --------------
>  drivers/mmc/core/sd.c  |   14 --------------
>  2 files changed, 28 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c8f3d6e..edc6bc4 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1445,18 +1445,6 @@ static int mmc_resume(struct mmc_host *host)
>  	return err;
>  }
>  
> -static int mmc_power_restore(struct mmc_host *host)
> -{
> -	int ret;
> -
> -	host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
> -	mmc_claim_host(host);
> -	ret = mmc_init_card(host, host->ocr, host->card);
> -	mmc_release_host(host);
> -
> -	return ret;
> -}
> -
>  static int mmc_sleep(struct mmc_host *host)
>  {
>  	struct mmc_card *card = host->card;
> @@ -1494,7 +1482,6 @@ static const struct mmc_bus_ops mmc_ops = {
>  	.detect = mmc_detect,
>  	.suspend = NULL,
>  	.resume = NULL,
> -	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
>  };
>  
> @@ -1505,7 +1492,6 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>  	.detect = mmc_detect,
>  	.suspend = mmc_suspend,
>  	.resume = mmc_resume,
> -	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
>  };
>  
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 9e645e1..b71d906 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1095,24 +1095,11 @@ static int mmc_sd_resume(struct mmc_host *host)
>  	return err;
>  }
>  
> -static int mmc_sd_power_restore(struct mmc_host *host)
> -{
> -	int ret;
> -
> -	host->card->state &= ~MMC_STATE_HIGHSPEED;
> -	mmc_claim_host(host);
> -	ret = mmc_sd_init_card(host, host->ocr, host->card);
> -	mmc_release_host(host);
> -
> -	return ret;
> -}
> -
>  static const struct mmc_bus_ops mmc_sd_ops = {
>  	.remove = mmc_sd_remove,
>  	.detect = mmc_sd_detect,
>  	.suspend = NULL,
>  	.resume = NULL,
> -	.power_restore = mmc_sd_power_restore,
>  	.alive = mmc_sd_alive,
>  };
>  
> @@ -1121,7 +1108,6 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
>  	.detect = mmc_sd_detect,
>  	.suspend = mmc_sd_suspend,
>  	.resume = mmc_sd_resume,
> -	.power_restore = mmc_sd_power_restore,
>  	.alive = mmc_sd_alive,
>  };
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson April 4, 2013, 9:55 a.m. UTC | #3
On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 01/03/13 14:47, Ulf Hansson wrote:
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>
> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()

True for eMMC, but for SD card the bus_ops can go away. Thanks for
spotting this Adrian!

Although, I see some serious problems with the mmc_do_hw_reset
function - it could cause eMMC data corruption.

Issuing hw reset and doing re-initialization by using the mmc
bus_ops->power_restore will mean no consideration is taken to "cache
ctrl", "bkops" and "power off notify". I think it must.

So the more proper way instead of calling power_restore, should be to
use bus_ops->suspend and bus_ops->resume callbacks from the
mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
successfully, we should be able to skip the actual hw reset and just
do bus_ops->resume.

Do you have any thoughts on this?

Kind regards
Ulf Hansson

>
>> func drivers are also moving towards use of runtime pm to accomplish the
>> the same operation and since this API is not used for mmc and sd it makes
>> sense to remove the corresponding bus_ops.
>>
>> Moreover, at least for eMMC the mmc bus_ops is broken, since no consideration
>> is taken for mmc sleep and mmc power off notify.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/mmc.c |   14 --------------
>>  drivers/mmc/core/sd.c  |   14 --------------
>>  2 files changed, 28 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index c8f3d6e..edc6bc4 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1445,18 +1445,6 @@ static int mmc_resume(struct mmc_host *host)
>>       return err;
>>  }
>>
>> -static int mmc_power_restore(struct mmc_host *host)
>> -{
>> -     int ret;
>> -
>> -     host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
>> -     mmc_claim_host(host);
>> -     ret = mmc_init_card(host, host->ocr, host->card);
>> -     mmc_release_host(host);
>> -
>> -     return ret;
>> -}
>> -
>>  static int mmc_sleep(struct mmc_host *host)
>>  {
>>       struct mmc_card *card = host->card;
>> @@ -1494,7 +1482,6 @@ static const struct mmc_bus_ops mmc_ops = {
>>       .detect = mmc_detect,
>>       .suspend = NULL,
>>       .resume = NULL,
>> -     .power_restore = mmc_power_restore,
>>       .alive = mmc_alive,
>>  };
>>
>> @@ -1505,7 +1492,6 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>>       .detect = mmc_detect,
>>       .suspend = mmc_suspend,
>>       .resume = mmc_resume,
>> -     .power_restore = mmc_power_restore,
>>       .alive = mmc_alive,
>>  };
>>
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 9e645e1..b71d906 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -1095,24 +1095,11 @@ static int mmc_sd_resume(struct mmc_host *host)
>>       return err;
>>  }
>>
>> -static int mmc_sd_power_restore(struct mmc_host *host)
>> -{
>> -     int ret;
>> -
>> -     host->card->state &= ~MMC_STATE_HIGHSPEED;
>> -     mmc_claim_host(host);
>> -     ret = mmc_sd_init_card(host, host->ocr, host->card);
>> -     mmc_release_host(host);
>> -
>> -     return ret;
>> -}
>> -
>>  static const struct mmc_bus_ops mmc_sd_ops = {
>>       .remove = mmc_sd_remove,
>>       .detect = mmc_sd_detect,
>>       .suspend = NULL,
>>       .resume = NULL,
>> -     .power_restore = mmc_sd_power_restore,
>>       .alive = mmc_sd_alive,
>>  };
>>
>> @@ -1121,7 +1108,6 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
>>       .detect = mmc_sd_detect,
>>       .suspend = mmc_sd_suspend,
>>       .resume = mmc_sd_resume,
>> -     .power_restore = mmc_sd_power_restore,
>>       .alive = mmc_sd_alive,
>>  };
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter April 4, 2013, 11:44 a.m. UTC | #4
On 04/04/13 12:55, Ulf Hansson wrote:
> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 01/03/13 14:47, Ulf Hansson wrote:
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>
>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
> 
> True for eMMC, but for SD card the bus_ops can go away. Thanks for
> spotting this Adrian!
> 
> Although, I see some serious problems with the mmc_do_hw_reset
> function - it could cause eMMC data corruption.
> 
> Issuing hw reset and doing re-initialization by using the mmc
> bus_ops->power_restore will mean no consideration is taken to "cache
> ctrl", "bkops" and "power off notify". I think it must.
> 
> So the more proper way instead of calling power_restore, should be to
> use bus_ops->suspend and bus_ops->resume callbacks from the
> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
> successfully, we should be able to skip the actual hw reset and just
> do bus_ops->resume.
> 
> Do you have any thoughts on this?

Certainly the bootloader should leave the eMMC is a safe state including:
flushing the cache or turning it off (why did it turn on?), stopping
background operations (why did it start them?), disabling power-off
notification CMD0? (again why it it enable it?)

Note that according to spec. CMD0 anyway clears the cache so you have lost
your data anyway.


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson April 4, 2013, 11:52 a.m. UTC | #5
On 4 April 2013 13:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/04/13 12:55, Ulf Hansson wrote:
>> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 01/03/13 14:47, Ulf Hansson wrote:
>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>>
>>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
>>
>> True for eMMC, but for SD card the bus_ops can go away. Thanks for
>> spotting this Adrian!
>>
>> Although, I see some serious problems with the mmc_do_hw_reset
>> function - it could cause eMMC data corruption.
>>
>> Issuing hw reset and doing re-initialization by using the mmc
>> bus_ops->power_restore will mean no consideration is taken to "cache
>> ctrl", "bkops" and "power off notify". I think it must.
>>
>> So the more proper way instead of calling power_restore, should be to
>> use bus_ops->suspend and bus_ops->resume callbacks from the
>> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
>> successfully, we should be able to skip the actual hw reset and just
>> do bus_ops->resume.
>>
>> Do you have any thoughts on this?
>
> Certainly the bootloader should leave the eMMC is a safe state including:
> flushing the cache or turning it off (why did it turn on?), stopping
> background operations (why did it start them?), disabling power-off
> notification CMD0? (again why it it enable it?)

Not sure what you mean here. What has a booloader to do with this?

>
> Note that according to spec. CMD0 anyway clears the cache so you have lost
> your data anyway.

What I am saying that we can try send "cache ctrl" and "power off
notify" before we send CMD0 / do hw reset. The no data shall be lost.

>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter April 4, 2013, noon UTC | #6
On 04/04/13 14:52, Ulf Hansson wrote:
> On 4 April 2013 13:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 04/04/13 12:55, Ulf Hansson wrote:
>>> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 01/03/13 14:47, Ulf Hansson wrote:
>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>
>>>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>>>
>>>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
>>>
>>> True for eMMC, but for SD card the bus_ops can go away. Thanks for
>>> spotting this Adrian!
>>>
>>> Although, I see some serious problems with the mmc_do_hw_reset
>>> function - it could cause eMMC data corruption.
>>>
>>> Issuing hw reset and doing re-initialization by using the mmc
>>> bus_ops->power_restore will mean no consideration is taken to "cache
>>> ctrl", "bkops" and "power off notify". I think it must.
>>>
>>> So the more proper way instead of calling power_restore, should be to
>>> use bus_ops->suspend and bus_ops->resume callbacks from the
>>> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
>>> successfully, we should be able to skip the actual hw reset and just
>>> do bus_ops->resume.
>>>
>>> Do you have any thoughts on this?
>>
>> Certainly the bootloader should leave the eMMC is a safe state including:
>> flushing the cache or turning it off (why did it turn on?), stopping
>> background operations (why did it start them?), disabling power-off
>> notification CMD0? (again why it it enable it?)
> 
> Not sure what you mean here. What has a booloader to do with this?

When do you think hw reset is used?

> 
>>
>> Note that according to spec. CMD0 anyway clears the cache so you have lost
>> your data anyway.
> 
> What I am saying that we can try send "cache ctrl" and "power off
> notify" before we send CMD0 / do hw reset. The no data shall be lost.

With an uninitialized bus?  Or an unresponsive card?

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson April 4, 2013, 2:58 p.m. UTC | #7
On 4 April 2013 14:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/04/13 14:52, Ulf Hansson wrote:
>> On 4 April 2013 13:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 04/04/13 12:55, Ulf Hansson wrote:
>>>> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 01/03/13 14:47, Ulf Hansson wrote:
>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>
>>>>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>>>>
>>>>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
>>>>
>>>> True for eMMC, but for SD card the bus_ops can go away. Thanks for
>>>> spotting this Adrian!
>>>>
>>>> Although, I see some serious problems with the mmc_do_hw_reset
>>>> function - it could cause eMMC data corruption.
>>>>
>>>> Issuing hw reset and doing re-initialization by using the mmc
>>>> bus_ops->power_restore will mean no consideration is taken to "cache
>>>> ctrl", "bkops" and "power off notify". I think it must.
>>>>
>>>> So the more proper way instead of calling power_restore, should be to
>>>> use bus_ops->suspend and bus_ops->resume callbacks from the
>>>> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
>>>> successfully, we should be able to skip the actual hw reset and just
>>>> do bus_ops->resume.
>>>>
>>>> Do you have any thoughts on this?
>>>
>>> Certainly the bootloader should leave the eMMC is a safe state including:
>>> flushing the cache or turning it off (why did it turn on?), stopping
>>> background operations (why did it start them?), disabling power-off
>>> notification CMD0? (again why it it enable it?)
>>
>> Not sure what you mean here. What has a booloader to do with this?
>
> When do you think hw reset is used?

Two types is being used.

1. At the mmc_rescan sequence. At this point mmc_do_hw_reset is _not_
used. Instead mmc_hw_reset_for_init, which onlcy makes use of
host->ops->hw_reset, no "power_restore" of course. So this has no
issues whatsoever with this patch.

2. At blk request errors, which I thought we were discussing from the
beginning. In this case mmc_do_hw_reset is used. And it here my
proposals doing bus_ops->suspend|resume make sense.


>
>>
>>>
>>> Note that according to spec. CMD0 anyway clears the cache so you have lost
>>> your data anyway.
>>
>> What I am saying that we can try send "cache ctrl" and "power off
>> notify" before we send CMD0 / do hw reset. The no data shall be lost.
>
> With an uninitialized bus?  Or an unresponsive card?

See above.

The bus is never uninitialized.

The card has responded with an error, but that does not have to mean
that it is completely "unresponsive".

>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter April 5, 2013, 8:50 a.m. UTC | #8
On 04/04/13 17:58, Ulf Hansson wrote:
> On 4 April 2013 14:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 04/04/13 14:52, Ulf Hansson wrote:
>>> On 4 April 2013 13:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 04/04/13 12:55, Ulf Hansson wrote:
>>>>> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> On 01/03/13 14:47, Ulf Hansson wrote:
>>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>>
>>>>>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>>>>>
>>>>>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
>>>>>
>>>>> True for eMMC, but for SD card the bus_ops can go away. Thanks for
>>>>> spotting this Adrian!
>>>>>
>>>>> Although, I see some serious problems with the mmc_do_hw_reset
>>>>> function - it could cause eMMC data corruption.
>>>>>
>>>>> Issuing hw reset and doing re-initialization by using the mmc
>>>>> bus_ops->power_restore will mean no consideration is taken to "cache
>>>>> ctrl", "bkops" and "power off notify". I think it must.
>>>>>
>>>>> So the more proper way instead of calling power_restore, should be to
>>>>> use bus_ops->suspend and bus_ops->resume callbacks from the
>>>>> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
>>>>> successfully, we should be able to skip the actual hw reset and just
>>>>> do bus_ops->resume.
>>>>>
>>>>> Do you have any thoughts on this?
>>>>
>>>> Certainly the bootloader should leave the eMMC is a safe state including:
>>>> flushing the cache or turning it off (why did it turn on?), stopping
>>>> background operations (why did it start them?), disabling power-off
>>>> notification CMD0? (again why it it enable it?)
>>>
>>> Not sure what you mean here. What has a booloader to do with this?
>>
>> When do you think hw reset is used?
> 
> Two types is being used.
> 
> 1. At the mmc_rescan sequence. At this point mmc_do_hw_reset is _not_
> used. Instead mmc_hw_reset_for_init, which onlcy makes use of
> host->ops->hw_reset, no "power_restore" of course. So this has no
> issues whatsoever with this patch.
> 
> 2. At blk request errors, which I thought we were discussing from the
> beginning. In this case mmc_do_hw_reset is used. And it here my
> proposals doing bus_ops->suspend|resume make sense.
> 
> 
>>
>>>
>>>>
>>>> Note that according to spec. CMD0 anyway clears the cache so you have lost
>>>> your data anyway.
>>>
>>> What I am saying that we can try send "cache ctrl" and "power off
>>> notify" before we send CMD0 / do hw reset. The no data shall be lost.
>>
>> With an uninitialized bus?  Or an unresponsive card?
> 
> See above.
> 
> The bus is never uninitialized.
> 
> The card has responded with an error, but that does not have to mean
> that it is completely "unresponsive".

True.  Arguably caching should be disabled at the first sign of trouble
and never re-enabled.

However reset could attempt to flush the cache.  Then, even if the reset is
successful, an error must still be returned if the flush failed.


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c8f3d6e..edc6bc4 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1445,18 +1445,6 @@  static int mmc_resume(struct mmc_host *host)
 	return err;
 }
 
-static int mmc_power_restore(struct mmc_host *host)
-{
-	int ret;
-
-	host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
-	mmc_claim_host(host);
-	ret = mmc_init_card(host, host->ocr, host->card);
-	mmc_release_host(host);
-
-	return ret;
-}
-
 static int mmc_sleep(struct mmc_host *host)
 {
 	struct mmc_card *card = host->card;
@@ -1494,7 +1482,6 @@  static const struct mmc_bus_ops mmc_ops = {
 	.detect = mmc_detect,
 	.suspend = NULL,
 	.resume = NULL,
-	.power_restore = mmc_power_restore,
 	.alive = mmc_alive,
 };
 
@@ -1505,7 +1492,6 @@  static const struct mmc_bus_ops mmc_ops_unsafe = {
 	.detect = mmc_detect,
 	.suspend = mmc_suspend,
 	.resume = mmc_resume,
-	.power_restore = mmc_power_restore,
 	.alive = mmc_alive,
 };
 
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 9e645e1..b71d906 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1095,24 +1095,11 @@  static int mmc_sd_resume(struct mmc_host *host)
 	return err;
 }
 
-static int mmc_sd_power_restore(struct mmc_host *host)
-{
-	int ret;
-
-	host->card->state &= ~MMC_STATE_HIGHSPEED;
-	mmc_claim_host(host);
-	ret = mmc_sd_init_card(host, host->ocr, host->card);
-	mmc_release_host(host);
-
-	return ret;
-}
-
 static const struct mmc_bus_ops mmc_sd_ops = {
 	.remove = mmc_sd_remove,
 	.detect = mmc_sd_detect,
 	.suspend = NULL,
 	.resume = NULL,
-	.power_restore = mmc_sd_power_restore,
 	.alive = mmc_sd_alive,
 };
 
@@ -1121,7 +1108,6 @@  static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
 	.detect = mmc_sd_detect,
 	.suspend = mmc_sd_suspend,
 	.resume = mmc_sd_resume,
-	.power_restore = mmc_sd_power_restore,
 	.alive = mmc_sd_alive,
 };