diff mbox

[RFC,v3] mmc: sleep notification

Message ID FDD07FEB422EF948A392FDC201AEEAE64FAAD701@SACMBXIP02.sdcorp.global.sandisk.com (mailing list archive)
State New, archived
Headers show

Commit Message

Avi Shchislowski June 8, 2015, 6:46 a.m. UTC 
Hi Ulf,

Here is a V3 per your request.
Please review.

This patch is implements the new additional state of
Power_Off_Notification - SLEEP_NOTIFICATION.
Until now, the implementation of Power_Off_Notification
supported only three modes - POWERED_ON (0x01),
POWER_OFF_SHORT (0x02) and POWER_OFF_LONG (0x03).

As part of eMMC5.0 before moving to Sleep state hosts may set the
POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
After setting SLEEP_NOTIFICATION, host should wait for
the busy line to be de-asserted.

Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
Signed-off-by: Avi Shchislowski <avi.shchislowski@sandisk.com>

---------
V3:
- remove macros
- remove hpi_en dependency
- add helper function for sleep notification

v2:
- remove calling mmc_interrupt_hpi in case the device is doing
  sleep notification from block.c
- remove call of "mmc_card_doing_sleep_notify" from core.c
- mmc_sleep_notify changed to static function

Comments

Ulf Hansson June 8, 2015, 9:03 a.m. UTC | #1 
On 8 June 2015 at 08:46, Avi Shchislowski <Avi.Shchislowski@sandisk.com> wrote:
> Hi Ulf,
>
> Here is a V3 per your request.
> Please review.
>
> This patch is implements the new additional state of
> Power_Off_Notification - SLEEP_NOTIFICATION.
> Until now, the implementation of Power_Off_Notification
> supported only three modes - POWERED_ON (0x01),
> POWER_OFF_SHORT (0x02) and POWER_OFF_LONG (0x03).
>
> As part of eMMC5.0 before moving to Sleep state hosts may set the
> POWER_OFF_NOTIFICATION byte to SLEEP_NOTIFICATION (0x04).
> After setting SLEEP_NOTIFICATION, host should wait for
> the busy line to be de-asserted.
>
> Signed-off-by: Alex Lemberg <alex.lemberg@sandisk.com>
> Signed-off-by: Avi Shchislowski <avi.shchislowski@sandisk.com>

You need to trim your change-log here. I can't apply patches which
comes in this format and run checkpatch, please.

Moreover, try answer the *what* and *why* question in the change log.

>
> ---------

It's nice that you provide a history, but don't include that into the
commit message.
Change the above nine "-" to three "-", to make the history below be
posted as a appended message for the patch.

> V3:
> - remove macros
> - remove hpi_en dependency
> - add helper function for sleep notification
>
> v2:
> - remove calling mmc_interrupt_hpi in case the device is doing
>   sleep notification from block.c
> - remove call of "mmc_card_doing_sleep_notify" from core.c
> - mmc_sleep_notify changed to static function
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index a802863..56dae18 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -60,6 +60,14 @@ static const unsigned int tacc_mant[] = {
>         })
>
>  /*
> + * Indicates the maximum allowed timeout - 83.88s
> + * as defined in eMMC Spec for the Sleep Notification
> + * SWITCH command when notifying the device that it is
> + * about to be moved to sleep state.
> + */
> +#define MMC_SLEEP_NOTIFY_MAX_TIME      0x17
> +
> +/*
>   * Given the decoded CSD structure, decode the raw CID to our CID structure.
>   */
>  static int mmc_decode_cid(struct mmc_card *card)
> @@ -582,6 +590,8 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>                 card->ext_csd.ffu_capable =
>                         (ext_csd[EXT_CSD_SUPPORTED_MODE] & 0x1) &&
>                         !(ext_csd[EXT_CSD_FW_CONFIG] & 0x1);
> +               card->ext_csd.sleep_notify_time =
> +                       ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME];
>         }
>  out:
>         return err;
> @@ -1642,6 +1652,44 @@ out_release:
>         return err;
>  }
>
> +/*
> + * check if device is in program state (busy)
> + */
> +static bool mmc_device_prg_state(struct mmc_card *card)
> +{
> +       int rc;
> +       u32 status;
> +       bool state;
> +
> +       mmc_get_card(card);
> +       rc = mmc_send_status(card, &status);
> +       if (rc) {
> +               pr_err("%s: Get card status fail. rc=%d\n",
> +                       mmc_hostname(card->host), rc);
> +               state = false;
> +               goto out;
> +       }
> +
> +       if (R1_CURRENT_STATE(status) == R1_STATE_PRG)
> +               state =  true;
> +       else
> +               state =  false;
> +out:
> +       mmc_put_card(card);
> +       return state;
> +}
> +
> +static int can_sleep_notify(struct mmc_card *card)
> +{
> +       /* sleep notify enable/disable for eMMC 5.0 and above */
> +       return (card && card->ext_csd.rev >= 7 &&
> +                       (card->host->caps2 & MMC_CAP2_SLEEP_NOTIFY) &&
> +                       card->ext_csd.sleep_notify_time > 0 &&
> +                       card->ext_csd.sleep_notify_time <=
> +                               MMC_SLEEP_NOTIFY_MAX_TIME &&
> +                       card->ext_csd.power_off_notification == EXT_CSD_POWER_ON);
> +}
> +
>  static int mmc_can_poweroff_notify(const struct mmc_card *card)
>  {
>         return card &&
> @@ -1653,17 +1701,32 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
>  {
>         unsigned int timeout = card->ext_csd.generic_cmd6_time;
>         int err;
> +       bool use_busy_signal;
>
>         /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */
>         if (notify_type == EXT_CSD_POWER_OFF_LONG)
>                 timeout = card->ext_csd.power_off_longtime;
> +       else if (notify_type == EXT_CSD_SLEEP_NOTIFICATION) {
> +               /* calculate the maximum timeout for the
> +                * switch command when notifying the device
> +                * that it is about to move to sleep */
>
> +               timeout = DIV_ROUND_UP((10 *
> +                       (1 << (unsigned int)(card->ext_csd.sleep_notify_time))),1000);
> +               }
> +
> +       use_busy_signal = (notify_type == EXT_CSD_SLEEP_NOTIFICATION) ?
> +                       false : true;
>         err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>                         EXT_CSD_POWER_OFF_NOTIFICATION,
> -                       notify_type, timeout, true, false, false);
> -       if (err)
> +                       notify_type, timeout,
> +                       use_busy_signal, false, false);
> +       if (!err) {
> +               card->ext_csd.power_off_notification = notify_type;
> +       } else {
>                 pr_err("%s: Power Off Notification timed out, %u\n",
> -                      mmc_hostname(card->host), timeout);
> +                       mmc_hostname(card->host), timeout);
> +       }
>
>         /* Disable the power off notification after the switch operation. */
>         card->ext_csd.power_off_notification = EXT_CSD_NO_POWER_NOTIFICATION;
> @@ -1671,6 +1734,70 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
>         return err;
>  }
>
> +static int mmc_sleep_notify(struct mmc_card *card)
> +{
> +       int err = 0;
> +       bool is_busy = 0;
> +       unsigned long prg_wait = 0;
> +       unsigned int timeout;
> +
> +       if (!can_sleep_notify(card) || !mmc_can_poweroff_notify(card))
> +               return 0;
> +
> +       if (!mmc_card_doing_sleep_notify(card)) {
> +               mmc_get_card(card);
> +               mmc_card_set_sleep_notify(card);
> +               err = mmc_poweroff_notify(card,
> +                       EXT_CSD_SLEEP_NOTIFICATION);
> +               mmc_put_card(card);
> +               if (err) {
> +                       pr_err("%s: mmc_poweroff_notify failed with %d\n",
> +                              __func__, err);
> +                       goto out;
> +               }
> +               timeout = DIV_ROUND_UP((10 *
> +                               (1 << (unsigned int)(card->ext_csd.sleep_notify_time)))
> +                               ,1000);
> +               prg_wait = jiffies + msecs_to_jiffies(timeout);
> +       }
> +       /*
> +                * Loop will run until  Device is no
> +                * more in Busy state
> +                */
> +
> +       do {
> +               /* added some delay to avoid sending card status too often */
> +               msleep(20);
> +               err = 0;
> +               is_busy = mmc_device_prg_state(card);
> +               if (is_busy && time_after(jiffies, prg_wait)) {
> +                       /*
> +                        * making sure we are still in busy before
> +                        * sending HPI due to timeout error
> +                        */
> +                       is_busy = mmc_device_prg_state(card);
> +                       if (is_busy) {
> +                               if (mmc_card_doing_sleep_notify(card)) {
> +                                       card->ext_csd.power_off_notification =
> +                                               EXT_CSD_POWER_ON;
> +                                       mmc_interrupt_hpi(card);
> +                               }
> +                               err = -ETIMEDOUT;
> +                               break;
> +                       }
> +               }
> +       } while (is_busy);
> +
> +out:
> +       mmc_card_clr_sleep_notify(card);
> +       if (err) {
> +               pr_err("%s: mmc_poweroff_notify for sleep failed with %d\n",
> +                      mmc_hostname(card->host), err);
> +       }
> +
> +       return err;
> +}
> +
>  /*
>   * Host is being removed. Free up the current card.
>   */
> @@ -1744,14 +1871,19 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>         if (err)
>                 goto out;
>
> -       if (mmc_can_poweroff_notify(host->card) &&
> -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
> -               err = mmc_poweroff_notify(host->card, notify_type);
> -       else if (mmc_can_sleep(host->card))
> +    if (mmc_can_poweroff_notify(host->card)) {
> +       if (!can_sleep_notify(host->card)) {
> +               err = mmc_poweroff_notify(host->card, notify_type);
> +       } else if (is_suspend) {
> +               err = mmc_sleep_notify(host->card);
> +               if (err != -ETIMEDOUT)
> +                       err = mmc_sleep(host);
> +       }
> +    } else if (mmc_can_sleep(host->card)) {
>                 err = mmc_sleep(host);
> -       else if (!mmc_host_is_spi(host))
> +    } else if (!mmc_host_is_spi(host)) {
>                 err = mmc_deselect_cards(host);
> -
> +    }
>         if (!err) {
>                 mmc_power_off(host);
>                 mmc_card_set_suspended(host->card);
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 19f0175..3189e73 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -62,6 +62,7 @@ struct mmc_ext_csd {
>         unsigned int            sa_timeout;             /* Units: 100ns */
>         unsigned int            generic_cmd6_time;      /* Units: 10ms */
>         unsigned int            power_off_longtime;     /* Units: ms */
> +       unsigned int            sleep_notify_time;      /* Units: us */
>         u8                      power_off_notification; /* state */
>         unsigned int            hs_max_dtr;
>         unsigned int            hs200_max_dtr;
> @@ -262,6 +263,7 @@ struct mmc_card {
>  #define MMC_CARD_REMOVED       (1<<4)          /* card has been removed */
>  #define MMC_STATE_DOING_BKOPS  (1<<5)          /* card is doing BKOPS */
>  #define MMC_STATE_SUSPENDED    (1<<6)          /* card is suspended */
> +#define MMC_STATE_SLEEP_NOTIFY (1<<7)          /* card in sleep notify */
>         unsigned int            quirks;         /* card quirks */
>  #define MMC_QUIRK_LENIENT_FN0  (1<<0)          /* allow SDIO FN0 writes outside of the VS CCCR range */
>  #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1)   /* use func->cur_blksize */
> @@ -427,6 +429,7 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
>  #define mmc_card_removed(c)    ((c) && ((c)->state & MMC_CARD_REMOVED))
>  #define mmc_card_doing_bkops(c)        ((c)->state & MMC_STATE_DOING_BKOPS)
>  #define mmc_card_suspended(c)  ((c)->state & MMC_STATE_SUSPENDED)
> +#define mmc_card_doing_sleep_notify(c) ((c)->state & MMC_STATE_SLEEP_NOTIFY)
>
>  #define mmc_card_set_present(c)        ((c)->state |= MMC_STATE_PRESENT)
>  #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
> @@ -437,6 +440,8 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
>  #define mmc_card_clr_doing_bkops(c)    ((c)->state &= ~MMC_STATE_DOING_BKOPS)
>  #define mmc_card_set_suspended(c) ((c)->state |= MMC_STATE_SUSPENDED)
>  #define mmc_card_clr_suspended(c) ((c)->state &= ~MMC_STATE_SUSPENDED)
> +#define mmc_card_set_sleep_notify(c)   ((c)->state |= MMC_STATE_SLEEP_NOTIFY)
> +#define mmc_card_clr_sleep_notify(c)   ((c)->state &= ~MMC_STATE_SLEEP_NOTIFY)
>
>  /*
>   * Quirk add/remove for MMC products.
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index f471193..111e05d 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -286,6 +286,7 @@ struct mmc_host {
>                                  MMC_CAP2_HS400_1_2V)
>  #define MMC_CAP2_HSX00_1_2V    (MMC_CAP2_HS200_1_2V_SDR | MMC_CAP2_HS400_1_2V)
>  #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
> +#define MMC_CAP2_SLEEP_NOTIFY  (1 << 18)       /* sleep notify supported */
>
>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index 124f562..bbb71ae 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -309,6 +309,7 @@ struct _mmc_csd {
>  #define EXT_CSD_PWR_CL_52_360          202     /* RO */
>  #define EXT_CSD_PWR_CL_26_360          203     /* RO */
>  #define EXT_CSD_SEC_CNT                        212     /* RO, 4 bytes */
> +#define EXT_CSD_SLEEP_NOTIFICATION_TIME        216     /* RO */
>  #define EXT_CSD_S_A_TIMEOUT            217     /* RO */
>  #define EXT_CSD_REL_WR_SEC_C           222     /* RO */
>  #define EXT_CSD_HC_WP_GRP_SIZE         221     /* RO */
> @@ -403,6 +404,7 @@ struct _mmc_csd {
>  #define EXT_CSD_POWER_ON               1
>  #define EXT_CSD_POWER_OFF_SHORT                2
>  #define EXT_CSD_POWER_OFF_LONG         3
> +#define EXT_CSD_SLEEP_NOTIFICATION     4
>
>  #define EXT_CSD_PWR_CL_8BIT_MASK       0xF0    /* 8 bit PWR CLS */
>  #define EXT_CSD_PWR_CL_4BIT_MASK       0x0F    /* 8 bit PWR CLS */
> --
> 1.7.9.5
>
>
>

One of my comments for v2, was that I think you should remove all code
which was related to HPI to interrupt sleep notification from the
runtime PM resume path. Instead I wanted you to add that functionality
as separate patch based on top of this patch.

You haven't done that in v3, why?

Kind regards
Uffe
--
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
Alex Lemberg June 8, 2015, 1:17 p.m. UTC | #2 
SGkgVWxmLA0KDQpbLi4uXQ0KDQo+IA0KPiBPbmUgb2YgbXkgY29tbWVudHMgZm9yIHYyLCB3YXMg
dGhhdCBJIHRoaW5rIHlvdSBzaG91bGQgcmVtb3ZlIGFsbCBjb2RlIHdoaWNoDQo+IHdhcyByZWxh
dGVkIHRvIEhQSSB0byBpbnRlcnJ1cHQgc2xlZXAgbm90aWZpY2F0aW9uIGZyb20gdGhlIHJ1bnRp
bWUgUE0gcmVzdW1lDQo+IHBhdGguIEluc3RlYWQgSSB3YW50ZWQgeW91IHRvIGFkZCB0aGF0IGZ1
bmN0aW9uYWxpdHkgYXMgc2VwYXJhdGUgcGF0Y2ggYmFzZWQgb24NCj4gdG9wIG9mIHRoaXMgcGF0
Y2guDQo+IA0KPiBZb3UgaGF2ZW4ndCBkb25lIHRoYXQgaW4gdjMsIHdoeT8NCg0KVGhlIHNsZWVw
X25vdGlmeSBjYWxsIHdhcyBtb3ZlZCB0byBzdXNwZW5kKCkgcGVyIHlvdXIgcmVjb21tZW5kYXRp
b24uDQpBcyBmYXIgYXMgSSB1bmRlcnN0YW5kLCBubyBuZXcgcmVxdWVzdHMgc2hvdWxkIGJlIHNl
bnQgZHVyaW5nIG1tY19zdXNwZW5kKCkgcHJvY2VzcywNCnRodXMgSFBJIHN1cHBvcnQgaXMgbm90
IG5lZWRlZCBhbnltb3JlLg0KSXMgdGhpcyB0aGUgY29ycmVjdCBhc3N1bXB0aW9uPw0KVGhlIG9u
bHkgY2FzZSB3aGVyZSBIUEkgaXMgdXNlZCBpbiB0aGlzIHBhdGNoIC0gaXMgZHVyaW5nIHNsZWVw
X25vdGlmeSB0aW1lb3V0IGVycm9yLg0KDQpUaGFua3MsDQpBbGV4DQoNCg0K
--
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 June 15, 2015, 8:23 a.m. UTC | #3 
On 8 June 2015 at 15:17, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
> Hi Ulf,
>
> [...]
>
>>
>> One of my comments for v2, was that I think you should remove all code which
>> was related to HPI to interrupt sleep notification from the runtime PM resume
>> path. Instead I wanted you to add that functionality as separate patch based on
>> top of this patch.
>>
>> You haven't done that in v3, why?
>
> The sleep_notify call was moved to suspend() per your recommendation.
> As far as I understand, no new requests should be sent during mmc_suspend() process,
> thus HPI support is not needed anymore.
> Is this the correct assumption?

Yes.

I don't think you need mmc_card_set_sleep_notify() and the
corresponding new MMC_STATE_SLEEP_NOTIFY , mmc_device_prg_state(),
etc.

Overall, I think this patch could be simplified yet another step.

> The only case where HPI is used in this patch - is during sleep_notify timeout error.

Why?

One final question, I noticed that you have removed the check for
MMC_CAP2_FULL_PWR_CYCLE in the _mmc_suspend() function, why?

Kind regards
Uffe
--
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
Alex Lemberg June 18, 2015, 2:57 p.m. UTC | #4 
SGkgVWxmLA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFVsZiBIYW5z
c29uIFttYWlsdG86dWxmLmhhbnNzb25AbGluYXJvLm9yZ10NCj4gU2VudDogTW9uZGF5LCBKdW5l
IDE1LCAyMDE1IDExOjI0IEFNDQo+IFRvOiBBbGV4IExlbWJlcmcNCj4gQ2M6IEF2aSBTaGNoaXNs
b3dza2k7IGxpbnV4LW1tYw0KPiBTdWJqZWN0OiBSZTogW1JGQyBQQVRDSCB2M10gbW1jOiBzbGVl
cCBub3RpZmljYXRpb24NCj4gDQo+IE9uIDggSnVuZSAyMDE1IGF0IDE1OjE3LCBBbGV4IExlbWJl
cmcgPEFsZXguTGVtYmVyZ0BzYW5kaXNrLmNvbT4NCj4gd3JvdGU6DQo+ID4gSGkgVWxmLA0KPiA+
DQo+ID4gWy4uLl0NCj4gPg0KPiA+Pg0KPiA+PiBPbmUgb2YgbXkgY29tbWVudHMgZm9yIHYyLCB3
YXMgdGhhdCBJIHRoaW5rIHlvdSBzaG91bGQgcmVtb3ZlIGFsbA0KPiA+PiBjb2RlIHdoaWNoIHdh
cyByZWxhdGVkIHRvIEhQSSB0byBpbnRlcnJ1cHQgc2xlZXAgbm90aWZpY2F0aW9uIGZyb20NCj4g
Pj4gdGhlIHJ1bnRpbWUgUE0gcmVzdW1lIHBhdGguIEluc3RlYWQgSSB3YW50ZWQgeW91IHRvIGFk
ZCB0aGF0DQo+ID4+IGZ1bmN0aW9uYWxpdHkgYXMgc2VwYXJhdGUgcGF0Y2ggYmFzZWQgb24gdG9w
IG9mIHRoaXMgcGF0Y2guDQo+ID4+DQo+ID4+IFlvdSBoYXZlbid0IGRvbmUgdGhhdCBpbiB2Mywg
d2h5Pw0KPiA+DQo+ID4gVGhlIHNsZWVwX25vdGlmeSBjYWxsIHdhcyBtb3ZlZCB0byBzdXNwZW5k
KCkgcGVyIHlvdXIgcmVjb21tZW5kYXRpb24uDQo+ID4gQXMgZmFyIGFzIEkgdW5kZXJzdGFuZCwg
bm8gbmV3IHJlcXVlc3RzIHNob3VsZCBiZSBzZW50IGR1cmluZw0KPiA+IG1tY19zdXNwZW5kKCkg
cHJvY2VzcywgdGh1cyBIUEkgc3VwcG9ydCBpcyBub3QgbmVlZGVkIGFueW1vcmUuDQo+ID4gSXMg
dGhpcyB0aGUgY29ycmVjdCBhc3N1bXB0aW9uPw0KPiANCj4gWWVzLg0KPiANCj4gSSBkb24ndCB0
aGluayB5b3UgbmVlZCBtbWNfY2FyZF9zZXRfc2xlZXBfbm90aWZ5KCkgYW5kIHRoZSBjb3JyZXNw
b25kaW5nDQo+IG5ldyBNTUNfU1RBVEVfU0xFRVBfTk9USUZZICwgbW1jX2RldmljZV9wcmdfc3Rh
dGUoKSwgZXRjLg0KDQptbWNfY2FyZF9zZXRfc2xlZXBfbm90aWZ5KCksIE1NQ19TVEFURV9TTEVF
UF9OT1RJRlkgYW5kIHRoZSBjb3JyZXNwb25kaW5nIC0gV2UgYXJlIHJlbW92aW5nIGl0IGZyb20g
dGhlIGN1cnJlbnQgcGF0Y2guIFByb2JhYmx5IHdpbGwgYWRkIHRoZW0gYXMgYSBzZXBhcmF0ZSBw
YXRjaCBpbiBmdXR1cmUuDQptbWNfZGV2aWNlX3ByZ19zdGF0ZSgpIC0gaXMgcmVxdWlyZWQgZm9y
IHdhaXRpbmcgZm9yIFNsZWVwX05vdGlmaWNhdGlvbiBjb21wbGV0aW9uL3RpbWVvdXQsIGFsdGhv
dWdoIHdlIHdvdWxkIGxpa2UgdG8gbGVhdmUgdGhpcyBmdW5jdGlvbi4NCg0KPiANCj4gT3ZlcmFs
bCwgSSB0aGluayB0aGlzIHBhdGNoIGNvdWxkIGJlIHNpbXBsaWZpZWQgeWV0IGFub3RoZXIgc3Rl
cC4NCj4gDQo+ID4gVGhlIG9ubHkgY2FzZSB3aGVyZSBIUEkgaXMgdXNlZCBpbiB0aGlzIHBhdGNo
IC0gaXMgZHVyaW5nIHNsZWVwX25vdGlmeQ0KPiB0aW1lb3V0IGVycm9yLg0KPiANCj4gV2h5Pw0K
DQpJbiBjYXNlIG9mIHRpbWVvdXQgZXJyb3IsIHdlIHdvdWxkIGxpa2UgdG8gaGFuZGxlIGl0IGJ5
IHNlbmRpbmcgSFBJIC0gdG8gbGV0IGRldmljZSBpbnRlcnJ1cHQvc3RvcCB0aGUgcHJnIHN0YXRl
LiANCg0KPiANCj4gT25lIGZpbmFsIHF1ZXN0aW9uLCBJIG5vdGljZWQgdGhhdCB5b3UgaGF2ZSBy
ZW1vdmVkIHRoZSBjaGVjayBmb3INCj4gTU1DX0NBUDJfRlVMTF9QV1JfQ1lDTEUgaW4gdGhlIF9t
bWNfc3VzcGVuZCgpIGZ1bmN0aW9uLCB3aHk/DQoNCkFzIGZhciBhcyB3ZSB1bmRlcnN0YW5kLCB0
aGlzIE1NQ19DQVAyX0ZVTExfUFdSX0NZQ0xFIHdhcyB1c2VkIHRvIGRpc3Rpbmd1aXNoIGJldHdl
ZW4gUE9OICh3aGVuIHRoZSBwb3dlciBjYW4gYmUgY3V0KSANCmFuZCBTbGVlcCAod2hlbiB0aGUg
cG93ZXIgaGF2ZSB0byBzdGF5IE9OKS4gDQpOb3cgd2UgaGF2ZSBTbGVlcF9Ob3RpZmljYXRpb24g
UE9OLCB3aGljaCBhbGxvd3MgdG8gY3V0IHRoZSBwb3dlciBhbHNvIGluIGNhc2Ugb2YgU2xlZXAu
DQpIb3BlIHRoZSBpbnRlcnByZXRhdGlvbiBhYm92ZSBpcyBjb3JyZWN0Lg0KDQo+IA0KPiBLaW5k
IHJlZ2FyZHMNCj4gVWZmZQ0K
--
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 July 22, 2015, 2:40 p.m. UTC | #5 
On 18 June 2015 at 16:57, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:
> Hi Ulf,
>
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Monday, June 15, 2015 11:24 AM
>> To: Alex Lemberg
>> Cc: Avi Shchislowski; linux-mmc
>> Subject: Re: [RFC PATCH v3] mmc: sleep notification
>>
>> On 8 June 2015 at 15:17, Alex Lemberg <Alex.Lemberg@sandisk.com>
>> wrote:
>> > Hi Ulf,
>> >
>> > [...]
>> >
>> >>
>> >> One of my comments for v2, was that I think you should remove all
>> >> code which was related to HPI to interrupt sleep notification from
>> >> the runtime PM resume path. Instead I wanted you to add that
>> >> functionality as separate patch based on top of this patch.
>> >>
>> >> You haven't done that in v3, why?
>> >
>> > The sleep_notify call was moved to suspend() per your recommendation.
>> > As far as I understand, no new requests should be sent during
>> > mmc_suspend() process, thus HPI support is not needed anymore.
>> > Is this the correct assumption?
>>
>> Yes.
>>
>> I don't think you need mmc_card_set_sleep_notify() and the corresponding
>> new MMC_STATE_SLEEP_NOTIFY , mmc_device_prg_state(), etc.
>
> mmc_card_set_sleep_notify(), MMC_STATE_SLEEP_NOTIFY and the corresponding - We are removing it from the current patch. Probably will add them as a separate patch in future.

Great!

> mmc_device_prg_state() - is required for waiting for Sleep_Notification completion/timeout, although we would like to leave this function.

Doesn't __mmc_switch() already deal with this? No, I don't think it's needed.

>
>>
>> Overall, I think this patch could be simplified yet another step.
>>
>> > The only case where HPI is used in this patch - is during sleep_notify
>> timeout error.
>>
>> Why?
>
> In case of timeout error, we would like to handle it by sending HPI - to let device interrupt/stop the prg state.

Is that according to the spec? I think it would be better to try a
reset sequence to recover, by using CMD0 and so forth, what do you
think?

>
>>
>> One final question, I noticed that you have removed the check for
>> MMC_CAP2_FULL_PWR_CYCLE in the _mmc_suspend() function, why?
>
> As far as we understand, this MMC_CAP2_FULL_PWR_CYCLE was used to distinguish between PON (when the power can be cut)
> and Sleep (when the power have to stay ON).
> Now we have Sleep_Notification PON, which allows to cut the power also in case of Sleep.
> Hope the interpretation above is correct.

Unfortunate your interpretation is wrong.

MMC_CAP2_FULL_PWR_CYCLE, indicates whether the mmc host can cut *both*
VCC and VCCQ.

For the suspend case, the default behaviour is to send CMD5 (sleep),
as we don't trust hosts to set MMC_CAP2_FULL_PWR_CYCLE even if it
potentially could.
Keeping this policy is important, as we don't want to issue PON unless
we know that we will be cutting both VCC and VCCQ.

Kind regards
Uffe
--
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
Alex Lemberg Aug. 3, 2015, 12:44 p.m. UTC | #6 
Hi Ulf,


> -----Original Message-----

> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]

> Sent: Wednesday, July 22, 2015 5:41 PM

> To: Alex Lemberg

> Cc: Avi Shchislowski; linux-mmc

> Subject: Re: [RFC PATCH v3] mmc: sleep notification

> 

> On 18 June 2015 at 16:57, Alex Lemberg <Alex.Lemberg@sandisk.com>

> wrote:

> > Hi Ulf,

> >

> >> -----Original Message-----

> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]

> >> Sent: Monday, June 15, 2015 11:24 AM

> >> To: Alex Lemberg

> >> Cc: Avi Shchislowski; linux-mmc

> >> Subject: Re: [RFC PATCH v3] mmc: sleep notification

> >>

> >> On 8 June 2015 at 15:17, Alex Lemberg <Alex.Lemberg@sandisk.com>

> >> wrote:

> >> > Hi Ulf,

> >> >

> >> > [...]

> >> >

> >> >>

> >> >> One of my comments for v2, was that I think you should remove all

> >> >> code which was related to HPI to interrupt sleep notification from

> >> >> the runtime PM resume path. Instead I wanted you to add that

> >> >> functionality as separate patch based on top of this patch.

> >> >>

> >> >> You haven't done that in v3, why?

> >> >

> >> > The sleep_notify call was moved to suspend() per your

> recommendation.

> >> > As far as I understand, no new requests should be sent during

> >> > mmc_suspend() process, thus HPI support is not needed anymore.

> >> > Is this the correct assumption?

> >>

> >> Yes.

> >>

> >> I don't think you need mmc_card_set_sleep_notify() and the

> >> corresponding new MMC_STATE_SLEEP_NOTIFY ,

> mmc_device_prg_state(), etc.

> >

> > mmc_card_set_sleep_notify(), MMC_STATE_SLEEP_NOTIFY and the

> corresponding - We are removing it from the current patch. Probably will add

> them as a separate patch in future.

> 

> Great!

> 

> > mmc_device_prg_state() - is required for waiting for Sleep_Notification

> completion/timeout, although we would like to leave this function.

> 

> Doesn't __mmc_switch() already deal with this? No, I don't think it's needed.

You are right, __mmc_switch() wait loop is enough, if HPI is not going to be in use during sleep_notification.
> 

> >

> >>

> >> Overall, I think this patch could be simplified yet another step.

> >>

> >> > The only case where HPI is used in this patch - is during

> >> > sleep_notify

> >> timeout error.

> >>

> >> Why?

> >

> > In case of timeout error, we would like to handle it by sending HPI - to let

> device interrupt/stop the prg state.

> 

> Is that according to the spec? I think it would be better to try a reset

> sequence to recover, by using CMD0 and so forth, what do you think?


Not must by the spec, but there are several options how to handle timeout error.
Checking the existing driver, the '-ETIMEDOUT' error is handled by mmc_interrupt_hpi ()
in case of  mmc_wait_for_req_done() function. 
Can we use the same approach?

> 

> >

> >>

> >> One final question, I noticed that you have removed the check for

> >> MMC_CAP2_FULL_PWR_CYCLE in the _mmc_suspend() function, why?

> >

> > As far as we understand, this MMC_CAP2_FULL_PWR_CYCLE was used to

> > distinguish between PON (when the power can be cut) and Sleep (when the

> power have to stay ON).

> > Now we have Sleep_Notification PON, which allows to cut the power also

> in case of Sleep.

> > Hope the interpretation above is correct.

> 

> Unfortunate your interpretation is wrong.

> 

> MMC_CAP2_FULL_PWR_CYCLE, indicates whether the mmc host can cut

> *both* VCC and VCCQ.

> 

> For the suspend case, the default behaviour is to send CMD5 (sleep), as we

> don't trust hosts to set MMC_CAP2_FULL_PWR_CYCLE even if it potentially

> could.

> Keeping this policy is important, as we don't want to issue PON unless we

> know that we will be cutting both VCC and VCCQ.


You are right, spec doesn’t talk about vccq in case of Sleep_Notification, but
only  about vcc.
We will stay with existing logic and add sleep_notification call before sending sleep command.

Thanks,
Alex

> 

> Kind regards

> Uffe
Ulf Hansson Aug. 25, 2015, 12:04 p.m. UTC | #7 
[...]

>> >> > The only case where HPI is used in this patch - is during
>> >> > sleep_notify
>> >> timeout error.
>> >>
>> >> Why?
>> >
>> > In case of timeout error, we would like to handle it by sending HPI - to let
>> device interrupt/stop the prg state.
>>
>> Is that according to the spec? I think it would be better to try a reset
>> sequence to recover, by using CMD0 and so forth, what do you think?
>
> Not must by the spec, but there are several options how to handle timeout error.
> Checking the existing driver, the '-ETIMEDOUT' error is handled by mmc_interrupt_hpi ()
> in case of  mmc_wait_for_req_done() function.
> Can we use the same approach?

Currently the logic to recover from errors during system PM suspend,
relies on the request-retry mechanism which is executed far later.

From the ->suspend() callback we will only propagate the error code
towards the PM core. That means the PM core will bail out and reverse
the actions already done for suspended devices.

In this initial patch, I suggest we stick to that behaviour.

Although, I am open to discuss a more sophisticated error handling,
but let's do that from a separate patch.

[...]

Kind regards
Uffe
--
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 a802863..56dae18 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -60,6 +60,14 @@  static const unsigned int tacc_mant[] = {
 	})
 
 /*
+ * Indicates the maximum allowed timeout - 83.88s
+ * as defined in eMMC Spec for the Sleep Notification
+ * SWITCH command when notifying the device that it is
+ * about to be moved to sleep state.
+ */
+#define MMC_SLEEP_NOTIFY_MAX_TIME	0x17
+
+/*
  * Given the decoded CSD structure, decode the raw CID to our CID structure.
  */
 static int mmc_decode_cid(struct mmc_card *card)
@@ -582,6 +590,8 @@  static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
 		card->ext_csd.ffu_capable =
 			(ext_csd[EXT_CSD_SUPPORTED_MODE] & 0x1) &&
 			!(ext_csd[EXT_CSD_FW_CONFIG] & 0x1);
+		card->ext_csd.sleep_notify_time =
+			ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME];
 	}
 out:
 	return err;
@@ -1642,6 +1652,44 @@  out_release:
 	return err;
 }
 
+/*
+ * check if device is in program state (busy)
+ */
+static bool mmc_device_prg_state(struct mmc_card *card)
+{
+	int rc;
+	u32 status;
+	bool state;
+
+	mmc_get_card(card);
+	rc = mmc_send_status(card, &status);
+	if (rc) {
+		pr_err("%s: Get card status fail. rc=%d\n",
+			mmc_hostname(card->host), rc);
+		state = false;
+		goto out;
+	}
+
+	if (R1_CURRENT_STATE(status) == R1_STATE_PRG)
+		state =  true;
+	else
+		state =  false;
+out:
+	mmc_put_card(card);
+	return state;
+}
+
+static int can_sleep_notify(struct mmc_card *card)
+{
+	/* sleep notify enable/disable for eMMC 5.0 and above */
+	return (card && card->ext_csd.rev >= 7 &&
+			(card->host->caps2 & MMC_CAP2_SLEEP_NOTIFY) &&
+			card->ext_csd.sleep_notify_time > 0 &&
+			card->ext_csd.sleep_notify_time <=
+				MMC_SLEEP_NOTIFY_MAX_TIME &&
+			card->ext_csd.power_off_notification == EXT_CSD_POWER_ON);
+}
+
 static int mmc_can_poweroff_notify(const struct mmc_card *card)
 {
 	return card &&
@@ -1653,17 +1701,32 @@  static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 {
 	unsigned int timeout = card->ext_csd.generic_cmd6_time;
 	int err;
+	bool use_busy_signal;
 
 	/* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */
 	if (notify_type == EXT_CSD_POWER_OFF_LONG)
 		timeout = card->ext_csd.power_off_longtime;
+	else if (notify_type == EXT_CSD_SLEEP_NOTIFICATION) {
+		/* calculate the maximum timeout for the
+		 * switch command when notifying the device
+		 * that it is about to move to sleep */
 
+		timeout = DIV_ROUND_UP((10 *
+			(1 << (unsigned int)(card->ext_csd.sleep_notify_time))),1000);
+		}
+
+	use_busy_signal = (notify_type == EXT_CSD_SLEEP_NOTIFICATION) ?
+			false : true;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			EXT_CSD_POWER_OFF_NOTIFICATION,
-			notify_type, timeout, true, false, false);
-	if (err)
+			notify_type, timeout,
+			use_busy_signal, false, false);
+	if (!err) {
+		card->ext_csd.power_off_notification = notify_type;
+	} else {
 		pr_err("%s: Power Off Notification timed out, %u\n",
-		       mmc_hostname(card->host), timeout);
+			mmc_hostname(card->host), timeout);
+	}
 
 	/* Disable the power off notification after the switch operation. */
 	card->ext_csd.power_off_notification = EXT_CSD_NO_POWER_NOTIFICATION;
@@ -1671,6 +1734,70 @@  static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 	return err;
 }
 
+static int mmc_sleep_notify(struct mmc_card *card)
+{
+	int err = 0;
+	bool is_busy = 0;
+	unsigned long prg_wait = 0;
+	unsigned int timeout;
+
+	if (!can_sleep_notify(card) || !mmc_can_poweroff_notify(card))
+		return 0;
+
+	if (!mmc_card_doing_sleep_notify(card)) {
+		mmc_get_card(card);
+		mmc_card_set_sleep_notify(card);
+		err = mmc_poweroff_notify(card,
+			EXT_CSD_SLEEP_NOTIFICATION);
+		mmc_put_card(card);
+		if (err) {
+			pr_err("%s: mmc_poweroff_notify failed with %d\n",
+			       __func__, err);
+			goto out;
+		}
+		timeout = DIV_ROUND_UP((10 *
+				(1 << (unsigned int)(card->ext_csd.sleep_notify_time)))
+				,1000);
+		prg_wait = jiffies + msecs_to_jiffies(timeout);
+	}
+	/*
+		 * Loop will run until  Device is no
+		 * more in Busy state
+		 */
+
+	do {
+		/* added some delay to avoid sending card status too often */
+		msleep(20);
+		err = 0;
+		is_busy = mmc_device_prg_state(card);
+		if (is_busy && time_after(jiffies, prg_wait)) {
+			/*
+			 * making sure we are still in busy before
+			 * sending HPI due to timeout error
+			 */
+			is_busy = mmc_device_prg_state(card);
+			if (is_busy) {
+				if (mmc_card_doing_sleep_notify(card)) {
+					card->ext_csd.power_off_notification =
+						EXT_CSD_POWER_ON;
+					mmc_interrupt_hpi(card);
+				}
+				err = -ETIMEDOUT;
+				break;
+			}
+		}
+	} while (is_busy);
+
+out:
+	mmc_card_clr_sleep_notify(card);
+	if (err) {
+		pr_err("%s: mmc_poweroff_notify for sleep failed with %d\n",
+		       mmc_hostname(card->host), err);
+	}
+
+	return err;
+}
+
 /*
  * Host is being removed. Free up the current card.
  */
@@ -1744,14 +1871,19 @@  static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 	if (err)
 		goto out;
 
-	if (mmc_can_poweroff_notify(host->card) &&
-		((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
-		err = mmc_poweroff_notify(host->card, notify_type);
-	else if (mmc_can_sleep(host->card))
+    if (mmc_can_poweroff_notify(host->card)) {
+    	if (!can_sleep_notify(host->card)) {
+    		err = mmc_poweroff_notify(host->card, notify_type);
+    	} else if (is_suspend) {
+    		err = mmc_sleep_notify(host->card);
+    		if (err != -ETIMEDOUT)
+    			err = mmc_sleep(host);
+    	}
+    } else if (mmc_can_sleep(host->card)) {
 		err = mmc_sleep(host);
-	else if (!mmc_host_is_spi(host))
+    } else if (!mmc_host_is_spi(host)) {
 		err = mmc_deselect_cards(host);
-
+    }
 	if (!err) {
 		mmc_power_off(host);
 		mmc_card_set_suspended(host->card);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 19f0175..3189e73 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -62,6 +62,7 @@  struct mmc_ext_csd {
 	unsigned int		sa_timeout;		/* Units: 100ns */
 	unsigned int		generic_cmd6_time;	/* Units: 10ms */
 	unsigned int            power_off_longtime;     /* Units: ms */
+	unsigned int		sleep_notify_time;	/* Units: us */
 	u8			power_off_notification;	/* state */
 	unsigned int		hs_max_dtr;
 	unsigned int		hs200_max_dtr;
@@ -262,6 +263,7 @@  struct mmc_card {
 #define MMC_CARD_REMOVED	(1<<4)		/* card has been removed */
 #define MMC_STATE_DOING_BKOPS	(1<<5)		/* card is doing BKOPS */
 #define MMC_STATE_SUSPENDED	(1<<6)		/* card is suspended */
+#define MMC_STATE_SLEEP_NOTIFY (1<<7)		/* card in sleep notify */
 	unsigned int		quirks; 	/* card quirks */
 #define MMC_QUIRK_LENIENT_FN0	(1<<0)		/* allow SDIO FN0 writes outside of the VS CCCR range */
 #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1)	/* use func->cur_blksize */
@@ -427,6 +429,7 @@  static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
 #define mmc_card_removed(c)	((c) && ((c)->state & MMC_CARD_REMOVED))
 #define mmc_card_doing_bkops(c)	((c)->state & MMC_STATE_DOING_BKOPS)
 #define mmc_card_suspended(c)	((c)->state & MMC_STATE_SUSPENDED)
+#define mmc_card_doing_sleep_notify(c) ((c)->state & MMC_STATE_SLEEP_NOTIFY)
 
 #define mmc_card_set_present(c)	((c)->state |= MMC_STATE_PRESENT)
 #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
@@ -437,6 +440,8 @@  static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
 #define mmc_card_clr_doing_bkops(c)	((c)->state &= ~MMC_STATE_DOING_BKOPS)
 #define mmc_card_set_suspended(c) ((c)->state |= MMC_STATE_SUSPENDED)
 #define mmc_card_clr_suspended(c) ((c)->state &= ~MMC_STATE_SUSPENDED)
+#define mmc_card_set_sleep_notify(c)	((c)->state |= MMC_STATE_SLEEP_NOTIFY)
+#define mmc_card_clr_sleep_notify(c)	((c)->state &= ~MMC_STATE_SLEEP_NOTIFY)
 
 /*
  * Quirk add/remove for MMC products.
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index f471193..111e05d 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -286,6 +286,7 @@  struct mmc_host {
 				 MMC_CAP2_HS400_1_2V)
 #define MMC_CAP2_HSX00_1_2V	(MMC_CAP2_HS200_1_2V_SDR | MMC_CAP2_HS400_1_2V)
 #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
+#define MMC_CAP2_SLEEP_NOTIFY	(1 << 18)	/* sleep notify supported */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 124f562..bbb71ae 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -309,6 +309,7 @@  struct _mmc_csd {
 #define EXT_CSD_PWR_CL_52_360		202	/* RO */
 #define EXT_CSD_PWR_CL_26_360		203	/* RO */
 #define EXT_CSD_SEC_CNT			212	/* RO, 4 bytes */
+#define EXT_CSD_SLEEP_NOTIFICATION_TIME	216	/* RO */
 #define EXT_CSD_S_A_TIMEOUT		217	/* RO */
 #define EXT_CSD_REL_WR_SEC_C		222	/* RO */
 #define EXT_CSD_HC_WP_GRP_SIZE		221	/* RO */
@@ -403,6 +404,7 @@  struct _mmc_csd {
 #define EXT_CSD_POWER_ON		1
 #define EXT_CSD_POWER_OFF_SHORT		2
 #define EXT_CSD_POWER_OFF_LONG		3
+#define EXT_CSD_SLEEP_NOTIFICATION	4
 
 #define EXT_CSD_PWR_CL_8BIT_MASK	0xF0	/* 8 bit PWR CLS */
 #define EXT_CSD_PWR_CL_4BIT_MASK	0x0F	/* 8 bit PWR CLS */