Hi Ulf,
Thank you for reviewing the patch!
On 10/13/16, 10:36 AM, "Ulf Hansson" <ulf.hansson@xxxxxxxxxx> wrote:
>On 1 September 2016 at 16:24, alex lemberg <alex.lemberg@xxxxxxxxxxx> wrote:
>> Rescheduling Suspend in case of BKOPS Level >= 1
>> in order to let eMMC device to complete its internal GC.
>> Applicable for Runtime Suspend Only.
>>
>> Signed-off-by: alex lemberg <alex.lemberg@xxxxxxxxxxx>
>> ---
>> drivers/mmc/core/mmc.c | 30 +++++++++++++++++++++++-------
>> include/linux/mmc/mmc.h | 1 +
>> 2 files changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index e2e987f..c4c6326 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1904,7 +1904,8 @@ static void mmc_detect(struct mmc_host *host)
>> }
>> }
>>
>> -static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>> +static int _mmc_suspend(struct mmc_host *host, bool is_suspend,
>> + bool is_runtime_pm)
>> {
>> int err = 0;
>> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>> @@ -1918,10 +1919,25 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>> if (mmc_card_suspended(host->card))
>> goto out;
>>
>> - if (mmc_card_doing_bkops(host->card)) {
>> - err = mmc_stop_bkops(host->card);
>> - if (err)
>> + if (mmc_card_doing_bkops(host->card) ||
>> + host->card->ext_csd.auto_bkops_en) {
>> + err = mmc_read_bkops_status(host->card);
>> + if (err) {
>> + pr_err("%s: error %d reading BKOPS Status\n",
>> + mmc_hostname(host), err);
>> + goto out;
>> + }
>
>I would appreciate some simple comment of what you want to do here below.
>
>> + if (is_runtime_pm && host->card->ext_csd.raw_bkops_status >=
>> + EXT_CSD_BKOPS_LEVEL_1) {
>> + pm_schedule_suspend(&host->card->dev,
>> + MMC_RUNTIME_SUSPEND_DELAY_MS);
>> goto out;
>
>If I understand correctly, you would like to abort the runtime suspend
>and allow the background operations to complete. That seems
>reasonable, although in such case you need to return -EBUSY from this
>function.
In my mind I wanted to reschedule runtime suspend, but your recommendation
is makes sense. I will change it and re-submit.
>Moreover, perhaps we should discuss at what BKOPS_LEVEL* we should
>allow to abort.
Agree, the BKOPS_LEVEL value is something that can be interpreted differently
by different vendors.
Should I define a default value like “BKOPS_LEVEL_TO_USE”, and
also maybe add a sysfs entry to allow user configuration?
>
>> + }
>> + if (mmc_card_doing_bkops(host->card)) {
>> + err = mmc_stop_bkops(host->card);
>> + if (err)
>> + goto out;
>> + }
>> }
>>
>> err = mmc_flush_cache(host->card);
>> @@ -1952,7 +1968,7 @@ static int mmc_suspend(struct mmc_host *host)
>> {
>> int err;
>>
>> - err = _mmc_suspend(host, true);
>> + err = _mmc_suspend(host, true, false);
>> if (!err) {
>> pm_runtime_disable(&host->card->dev);
>> pm_runtime_set_suspended(&host->card->dev);
>> @@ -2002,7 +2018,7 @@ static int mmc_shutdown(struct mmc_host *host)
>> err = _mmc_resume(host);
>>
>> if (!err)
>> - err = _mmc_suspend(host, false);
>> + err = _mmc_suspend(host, false, false);
>>
>> return err;
>> }
>> @@ -2026,7 +2042,7 @@ static int mmc_runtime_suspend(struct mmc_host *host)
>> if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>> return 0;
>>
>> - err = _mmc_suspend(host, true);
>> + err = _mmc_suspend(host, true, true);
>> if (err)
>> pr_err("%s: error %d doing aggressive suspend\n",
>> mmc_hostname(host), err);
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index 0fe3908..0f08d5c 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -430,6 +430,7 @@ struct _mmc_csd {
>> /*
>> * BKOPS status level
>> */
>> +#define EXT_CSD_BKOPS_LEVEL_1 0x1
>> #define EXT_CSD_BKOPS_LEVEL_2 0x2
>>
>> /*
>> --
>> 1.9.1
>>
>
>Another idea I had, was to actually make use of that we know the card
>will be inactive. So instead of checking if there is an ongoing BKOPS
>(mmc_card_doing_bkops()), in case of the manual BKOPS, we could
>potentially check the needed BKOPS LEVEL and schedule a BKOPS to
>start. In other words, do manual BKOPS when the card is idle.
If I understand correctly, the idea is to allow MANUAL_ BKOPS in
a similar way as it done for AUTO_BKOPS during runtime suspend, right?
>
>Of course, one need to be able to interrupt/cancel the BKOPS if there
>is a new request that needs to be managed. I am not sure how we can
>achieve that, but I hope you get the idea.
Right, in case of MANUAL_BKOPS, the need the HPI to be issued
in order to stop MANUAL_BKOPS immediately on a new request.
BTW, the same challenge we had on SLEEP_NOTIFICATION…
Since the AUTO_BKOPS is more simple case and doesn’t require
the interruption ability from the driver, may I suggest to make it
in a separate patchset?
>
>Kind regards
>Uffe
��.n��������+%������w��{.n�����{��i��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
