Hi Ulf,
On Fri, Jun 15, 2012 at 9:22 AM, 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)
The remove parameter takes care of the remove case.
>>>
>>> @@ -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.
I have addressed this in my prototype patch above. The remove param in
__suspend tells if we are removing or just suspending. This approach
actually removes lines of code in core.c mmc_stop_host().
>
>
>>
>>>
>>> 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. :-)
>
I'm in favour of simplifying this patch. CMD0 instead of CMD5 reduces
the number of lines in this patch. It also make this patch work :)
Using CMD 5 to wake up could be done in a separate patch.
Bottom line. If the patch is clean and works I'm happy.
I can help out and test the patch.
Regards,
Per
--
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
