Re: [PATCH] mmc: sleep notification

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10 March 2015 at 10:36, Avi Shchislowski
<avi.shchislowski@xxxxxxxxxxx> wrote:
> 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.
> The max timeout allowed for busy line de-assertion defined
> in SLEEP_NOTIFICATION_TIME byte in EXT_CSD [216].
> HPI may interrupt the SLEEP_NOTIFICATION operation.
> In that case POWER_OFF_NOTIFICATION byte will restore to POWERED_ON.

Oh, just great! JEDEC has invented yet another way of how to send a
device to sleep state. I wonder what was wrong with the CMD5 option.

Anyway, thanks for looking into this.

>
> Signed-off-by: Alex Lemberg <alex.lemberg@xxxxxxxxxxx>
> Signed-off-by: Avi Shchislowski <avi.shchislowski@xxxxxxxxxxx>
> ---
>  drivers/mmc/card/block.c |   19 +++++-
>  drivers/mmc/core/core.c  |   16 +++++-
>  drivers/mmc/core/core.h  |    2 +
>  drivers/mmc/core/mmc.c   |  143 +++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/mmc/card.h |    6 ++
>  include/linux/mmc/host.h |    1 +
>  include/linux/mmc/mmc.h  |    2 +
>  7 files changed, 180 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 4409d79..f511ecc3 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -2010,9 +2010,26 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>         unsigned long flags;
>         unsigned int cmd_flags = req ? req->cmd_flags : 0;
>
> -       if (req && !mq->mqrq_prev->req)
> +       if (unlikely(req && mmc_card_mmc(card) &&
> +               (card->ext_csd.power_off_notification ==
> +               EXT_CSD_SLEEP_NOTIFICATION))) {
> +               /* restoring the power_off_notification
> +                * field's state to as it was before so
> +                * that the sleep notification will be
> +                * able to resume later
> +                */
> +               card->ext_csd.power_off_notification = EXT_CSD_POWER_ON;
> +       }
> +
> +       if (req && !mq->mqrq_prev->req) {
>                 /* claim host only for the first request */
>                 mmc_get_card(card);
> +               if (unlikely(req &&
> +                       mmc_card_doing_sleep_notify(card))) {
> +                       mmc_interrupt_hpi(card);
> +                       mmc_card_clr_sleep_notify(card);
> +               }
> +       }

Both above new added code blocks makes no sense to me. Why does the
mmc block layer need to care about this?

>
>         ret = mmc_blk_part_switch(card, md);
>         if (ret) {
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5bda29b..a090593 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -271,7 +271,8 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
>
>         BUG_ON(!card);
>
> -       if (!card->ext_csd.bkops_en || mmc_card_doing_bkops(card))
> +       if (!card->ext_csd.bkops_en || mmc_card_doing_bkops(card) ||
> +               mmc_card_doing_sleep_notify(card))

We can't do BKOPS when the card is suspended (which is when the card
may be put in sleep state), so no need to protect for this.

>                 return;
>
>         err = mmc_read_bkops_status(card);
> @@ -2630,6 +2631,19 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>                         err = host->bus_ops->pre_suspend(host);
>                 if (!err)
>                         break;
> +               if (host->card && host->bus_ops->suspend) {

Nope, this is the wrong place for this code. You should look into
mmc.c for adding this.

> +                       err = mmc_sleep_notify(host->card);
> +                       /* We assume that HPI was sent
> +                        * in case of -ETIMEDOUT error,
> +                        * so suspend flow can be continued
> +                        */
> +                       if (err && err != -ETIMEDOUT) {
> +                               pr_err("%s:sleep notify err=%d\n",
> +                                       __func__, err);
> +                               return -EBUSY;
> +                       }
> +                       break;
> +               }
>
>                 /* Calling bus_ops->remove() with a claimed host can deadlock */
>                 host->bus_ops->remove(host);
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index d76597c..b6b4431 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>
>  #define MMC_CMD_RETRIES        3
> +#define MMC_SLEEP_NOTIFY_MAX_TIME      0x17
>
>  struct mmc_bus_ops {
>         void (*remove)(struct mmc_host *);
> @@ -33,6 +34,7 @@ void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
>  void mmc_detach_bus(struct mmc_host *host);
>
>  void mmc_init_erase(struct mmc_card *card);
> +int mmc_sleep_notify(struct mmc_card *card);

If we need a separate function to deal with this, it should be static
function within mmc.c.

>
>  void mmc_set_chip_select(struct mmc_host *host, int mode);
>  void mmc_set_clock(struct mmc_host *host, unsigned int hz);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 02ad792..1d97d24 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -57,6 +57,11 @@ static const unsigned int tacc_mant[] = {
>                 __res & __mask;                                         \
>         })
>
> +#define GET_SLEEP_NOTIFY_TIME(value) \
> +       (10 * (1 << (unsigned int)(value)))
> +#define GET_SLEEP_NOTIFY_TIME_MSEC(value) \
> +       (DIV_ROUND_UP(GET_SLEEP_NOTIFY_TIME(value), 1000))
> +
>  /*
>   * Given the decoded CSD structure, decode the raw CID to our CID structure.
>   */
> @@ -571,6 +576,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;
> @@ -1468,6 +1475,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>                         card->ext_csd.hpi_en = 1;
>         }
>
> +       /* sleep notify enable/disable for eMMC 5.0 and above */
> +       if ((card->ext_csd.rev >= 7) &&  card->ext_csd.hpi_en &&

Why depend on ext_csd.hpi_en?

> +                       (card->host->caps2 & MMC_CAP2_SLEEP_NOTIFY) &&

Please don't add a new CAP for this. MMC_CAP2_FULL_PWR_CYCLE tells us
if the card can be fully power gated, I think we can use that
instead!?

> +                       card->ext_csd.sleep_notify_time > 0 &&
> +                       card->ext_csd.sleep_notify_time <=
> +                               MMC_SLEEP_NOTIFY_MAX_TIME) {
> +               card->can_sleep_notify = 1;

Instead of adding "can_sleep_notify", let's just encapsulate similar
code as above in a helper function which tells us if card supports
"sleep notification".

Compare how mmc_can_sleep() is implemented and used.

> +       }
> +
>         /*
>          * If cache size is higher than 0, this indicates
>          * the existence of cache and it can be turned on.
> @@ -1576,6 +1592,33 @@ static int mmc_sleep(struct mmc_host *host)
>         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 mmc_can_poweroff_notify(const struct mmc_card *card)
>  {
>         return card &&
> @@ -1585,22 +1628,108 @@ static int mmc_can_poweroff_notify(const struct mmc_card *card)
>
>  static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
>  {
> -       unsigned int timeout = card->ext_csd.generic_cmd6_time;
> +       unsigned int timeout_ms = card->ext_csd.generic_cmd6_time;

What's reason to do a rename in this patch? If you want that as a
clarification of the code, please do that in a separate patch.

>         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;
> +               timeout_ms = 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_ms = GET_SLEEP_NOTIFY_TIME_MSEC(
> +                       card->ext_csd.sleep_notify_time);
> +       }
>
> +       /* do not wait on busy signal in case of
> +        * Sleep Notification - to let host get
> +        * another requests

At this point, there should be no other request. Or what am I missing?

> +        */
> +       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_ms,
> +                       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_ms);
> +       }
> +
> +       return err;
> +}
>
> -       /* Disable the power off notification after the switch operation. */
> -       card->ext_csd.power_off_notification = EXT_CSD_NO_POWER_NOTIFICATION;

Why remove this?

> +int mmc_sleep_notify(struct mmc_card *card)

I don't understand the need for this function. From what scenarios do
we want to invoke it?

> +{
> +       int err = 0;
> +       bool is_busy = 0;
> +       unsigned long prg_wait = 0;
> +
> +       if (!card->can_sleep_notify || !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;
> +               }
> +
> +               prg_wait = jiffies +
> +                       msecs_to_jiffies(GET_SLEEP_NOTIFY_TIME_MSEC(
> +                       card->ext_csd.sleep_notify_time));
> +               }
> +
> +       /*
> +        * Loop will run until:
> +        * 1. Device is no more in Busy state
> +        * 2. Sleep notification is not interrupted by HPI & IO request
> +        */
> +       do {
> +               /* added some delay to avoid sending card status too often */
> +               msleep(20);
> +               err = 0;
> +               /* Stop polling in case sleep notification was HPIed already */
> +               if (!mmc_card_doing_sleep_notify(card)) {
> +                       is_busy = mmc_device_prg_state(card);
> +                       if (is_busy)
> +                               err = -EBUSY;
> +                       break;
> +               }
> +               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;
>  }
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 4d69c00..6998344 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 */

I think MMC_STATE_SUSPENDED should be enough, we don't need another
one specific for 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 */
> @@ -309,6 +311,7 @@ struct mmc_card {
>         struct dentry           *debugfs_root;
>         struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical partitions */
>         unsigned int    nr_parts;
> +       u8 can_sleep_notify;    /* sleep_notify on/off */

As stated, please remove.

>  };
>
>  /*
> @@ -427,6 +430,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 +441,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 9f32270..4c0542a 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -291,6 +291,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 49ad7a9..69bda9a 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -314,6 +314,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 */
> @@ -408,6 +409,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
>

Kind regards
Uffe
--
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




[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux