On 15 June 2012 12:52, Ulf Hansson <ulf.hansson@xxxxxxxxxxxxxx> wrote:
> On 06/15/2012 05:49 AM, Saugata Das wrote:
>>
>> On 15 June 2012 00:36, Per Forlin<per.lkml@xxxxxxxxx> wrote:
>>>
>>> Hi Saugata,
>>>
>>> I can have a go and test it. But first I would like to bring up 3
>>> concerns that I have with this patch.
>>>
>>> 1. This patch should be sent to cc-stable in order to repair the bug
>>> introduced in 3.4
>>
>>
>> I shall sent it out today.
>>
>>> 2. Is the bus_ops for poweroff_notify really necessary since only mmc
>>> use it?
>>
>>
>> This was recommended in the review from Ulf. If it is not adding to a
>> bug, I propose to keep it this way. Otherwise, we will be going in
>> circles :-)
>
>
> Moreover it seems close related to sleep, which is implemented with bus_ops.
> So I would say, keep as is. We might change later, then both sleep and
> poweroff_notify together.
>
>>
>>> There are already bus_ops for suspend/resume,
>>> power_save/power_restore and remove. It feels like it would be
>>> possible to address poweroff_notify internally from mmc.c from theses
>>> bus_ops. I would be nice to add this feature without having to touch
>>> core.c.
>>>
>>> For instance. Call mmc_suspend() from mmc_remove()
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
>>> + __mmc_suspend(host, true);
>>> mmc_remove_card(host->card);
>>>
>>> @@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
>>> -static int mmc_suspend(struct mmc_host *host)
>>> +static int __mmc_suspend(struct mmc_host *host, bool remove)
>>>
>>> @@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
>>> mmc_claim_host(host);
>>> if (mmc_can_poweroff_notify(host->card)&&
>>> - (host->caps2& MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>>> + (host->caps2& MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
>>>
>>> + remove)) {
>>> err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>>> } else {
>>> if (mmc_card_can_sleep(host))
>>>
>>> @@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
>>> +static int mmc_suspend(struct mmc_host *host)
>>> +{
>>> + return __mmc_suspend(host, false);
>>> +}
>>> +
>>>
>>> Calling mmc_suspend from mmc_remove() has another advantage since it
>>> may issue SLEEP (CMD5) before the card is removed and power off. This
>>> is recommended by eMMC Vendors in order to shutdown the eMMC safely to
>>> prevent data corruption. When the platform shuts down the power to the
>>> eMMC will be turned off no matter what.
>>
>>
>> May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
>> has to be power OFF notify when the power is removed. Lets do it for
>> another patch, since the intention of this patch is to fix the issues
>> around power OFF notify.
>
>
> I agree with you Saugata, the exact same sequence as in suspend can not be
> used. The reason is simply that we do not want to consider
> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, as we
> want in suspend.
>
> Leave this to be fixed in a separate patch instead.
>
>
>>
>>>
>>> 3. About the sleep and awake issue. This is not really related to
>>> poweroff_notify() as I see it. I would recommend to use CMD 0 to
>>> re-init the card safely after sleep in this patch. Then you could send
>>> out a sleep/awake patch that address this separately. This would also
>>> make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
>>> new feature and should not be sent to the cc-stable list. What do you
>>> think?
>>
>>
>> The problem in sending CMD0 without power OFF notify is possibility of
>> some data loss in MMC-4.5 devices.
>
>
> I don't see the problem here. You will be sending power OFF notify when you
> can. The only difference is when you "wake" the device from sleep mode.
> Instead of using CMD5, which may work is some cases and in some cases not
> (without restoring ios). So using CMD0 as common way of waking up from
> suspend should be fine. Unless I missed something of course. :-)
>
CMD0 is a reset. I expect with power OFF notify enable, the eMMC
device will postpone some control information update to its internal
non-volatile memory (e.g. some data structures which are kept in the
controller buffers and not stored in NAND). If we do a CMD0, then the
eMMC device will be reset and we may lose some data. In addition to
that, doing complete card initialization will increase the wakeup time
(for 4.4 devices).
Till now, we have done complete card initialization during resume and
not done a real resume. I am not sure, if this patch is exposing some
host drivers issue now. So, please check the drivers as well.
> Kind regards
> Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
