| Message ID | 20171118010634.1111-1-gwendal@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18 November 2017 at 02:06, Gwendal Grignou <gwendal@chromium.org> wrote: > eMMC 5.0 spec introduces a new power off notification, SLEEP_NOTIFICATION, > the host may send before issuing the sleep commamd. > > Some eMMC expect the SLEEP_NOTIFICATION for optimal performance: > It is recommended by Sandisk for its iNAND 7232 to empty the iNAND > SmartTLC buffer, See "Emptying the iNAND SmartSLC buffer" of datasheet. > Hynix eMMC also resume faster when SLEEP_NOTIFICATION is sent before > going to sleep. > > SLEEP_NOTIFICATION PON can theoretically take a long time, iNAND sleep > notification timeout is set to 83.88s. But it has to be sent just before > CMD5, otherwise newer commands may make it moot. I think this change log needs some more clarification. At least for my understanding. Once SLEEP_NOTIFICATION was introduced, it became *mandatory* to first set POWER_OFF_NOTIFICATION to SLEEP_NOTIFICATION (in case we earlier have set POWER_OFF_NOTIFICATION byte to POWERED_ON), before putting the device into sleep using a CMD5. My point is, the above changelog seems to state that it *is not mandatory*, but optional. What is really the case here, do you know? Some additional comments below. > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > --- > There were several attempts to add SLEEP_NOTIFICATION, here is another try. > For refrence: > http://thread.gmane.org/gmane.linux.kernel.mmc/23471 [1/2034] > http://www.spinics.net/lists/linux-mmc/msg31344.html [6/2015] > https://patchwork.kernel.org/patch/9360633/ [10/2016] > > > drivers/mmc/core/mmc.c | 46 +++++++++++++++++++++++++++++++++++++--------- > include/linux/mmc/card.h | 3 ++- > include/linux/mmc/mmc.h | 2 ++ > 3 files changed, 41 insertions(+), 10 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 36217ad5e9b1..00ee7dd5ba07 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -633,6 +633,13 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd) > ext_csd[EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_A]; > card->ext_csd.device_life_time_est_typ_b = > ext_csd[EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_B]; > + > + if (ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME] == 0 || > + ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME] >= 0x18) > + card->ext_csd.sleep_notification_time = 0; > + else > + card->ext_csd.sleep_notification_time = > + max((10 << ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME]) / 1000, 1); > } > > /* eMMC v5.1 or later */ > @@ -652,6 +659,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd) > card->ext_csd.cmdq_depth); > } > } > + White space. > out: > return err; > } > @@ -1865,18 +1873,34 @@ static int mmc_can_poweroff_notify(const struct mmc_card *card) > (card->ext_csd.power_off_notification == EXT_CSD_POWER_ON); > } > > +static int mmc_can_sleep_notify(const struct mmc_card *card) > +{ > + return mmc_can_poweroff_notify(card) && > + (card->ext_csd.sleep_notification_time > 0); > +} > + > 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; > + bool use_busy_signal = true; > int err; > > - /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ > - if (notify_type == EXT_CSD_POWER_OFF_LONG) > + switch (notify_type) { > + case EXT_CSD_POWER_OFF_LONG: > timeout = card->ext_csd.power_off_longtime; > + break; > + case EXT_CSD_SLEEP_NOTIFICATION: > + timeout = card->ext_csd.sleep_notification_time; > + use_busy_signal = false; This is wrong. If you set use_busy_signal to false, it means that we don't care about waiting for the card to stop signaling busy on DAT0 after setting EXT_CSD_SLEEP_NOTIFICATION. This then becomes a violation of the eMMC spec, because we must not issue a CMD5 before the card have stopped signaling busy. I realize that for those devices that don't support MMC_CAP_WAIT_WHILE_BUSY or have the ->card_busy() callback implemented, the consequence may be that mmc_poll_for_busy() may call mmc_delay() with a very big timeout. This could be a big problem, I guess. > + break; > + default: > + /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ > + timeout = card->ext_csd.generic_cmd6_time; > + } > > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > EXT_CSD_POWER_OFF_NOTIFICATION, > - notify_type, timeout, 0, true, false, false); > + notify_type, timeout, 0, use_busy_signal, false, false); > if (err) > pr_err("%s: Power Off Notification timed out, %u\n", > mmc_hostname(card->host), timeout); > @@ -1952,13 +1976,17 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > goto out; > > if (mmc_can_poweroff_notify(host->card) && > - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) > + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { > err = mmc_poweroff_notify(host->card, notify_type); > - else if (mmc_can_sleep(host->card)) > - err = mmc_sleep(host); > - else if (!mmc_host_is_spi(host)) > + } else if (mmc_can_sleep(host->card)) { > + if (mmc_can_sleep_notify(host->card)) > + err = mmc_poweroff_notify(host->card, > + EXT_CSD_SLEEP_NOTIFICATION); > + if (!err) > + err = mmc_sleep(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 279b39008a33..5af3661bd98a 100644 > --- a/include/linux/mmc/card.h > +++ b/include/linux/mmc/card.h > @@ -59,8 +59,9 @@ struct mmc_ext_csd { > u8 packed_event_en; > unsigned int part_time; /* Units: ms */ > unsigned int sa_timeout; /* Units: 100ns */ > - unsigned int generic_cmd6_time; /* Units: 10ms */ > + unsigned int generic_cmd6_time; /* Units: ms */ White space - or perhaps make it a separate change. > unsigned int power_off_longtime; /* Units: ms */ > + unsigned int sleep_notification_time; /* Units: ms */ > u8 power_off_notification; /* state */ > unsigned int hs_max_dtr; > unsigned int hs200_max_dtr; > diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h > index 3ffc27aaeeaf..901e124dee72 100644 > --- a/include/linux/mmc/mmc.h > +++ b/include/linux/mmc/mmc.h > @@ -277,6 +277,7 @@ static inline bool mmc_op_multi(u32 opcode) > #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 */ > @@ -379,6 +380,7 @@ static inline bool mmc_op_multi(u32 opcode) > #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 */ > -- > 2.15.0.448.gf294e3d99a-goog > 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
[...] >> 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; >> + bool use_busy_signal = true; >> int err; >> >> - /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ >> - if (notify_type == EXT_CSD_POWER_OFF_LONG) >> + switch (notify_type) { >> + case EXT_CSD_POWER_OFF_LONG: >> timeout = card->ext_csd.power_off_longtime; >> + break; >> + case EXT_CSD_SLEEP_NOTIFICATION: >> + timeout = card->ext_csd.sleep_notification_time; >> + use_busy_signal = false; > > This is wrong. > > If you set use_busy_signal to false, it means that we don't care about > waiting for the card to stop signaling busy on DAT0 after setting > EXT_CSD_SLEEP_NOTIFICATION. This then becomes a violation of the eMMC > spec, because we must not issue a CMD5 before the card have stopped > signaling busy. > > I realize that for those devices that don't support > MMC_CAP_WAIT_WHILE_BUSY or have the ->card_busy() callback > implemented, the consequence may be that mmc_poll_for_busy() may call > mmc_delay() with a very big timeout. This could be a big problem, I > guess. Well, it may also be a big problem even if the host supports ->card_busy() and/or MMC_CAP_WAIT_WHILE_BUSY (but maybe not as big), simply because we can't be waiting here for several seconds to allow the card to deal with background operations. I guess we need to try and see what happens. Perhaps you can share some data about the timeouts you get? > >> + break; >> + default: >> + /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ >> + timeout = card->ext_csd.generic_cmd6_time; >> + } >> >> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >> EXT_CSD_POWER_OFF_NOTIFICATION, >> - notify_type, timeout, 0, true, false, false); >> + notify_type, timeout, 0, use_busy_signal, false, false); >> if (err) >> pr_err("%s: Power Off Notification timed out, %u\n", >> mmc_hostname(card->host), timeout); [...] 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
On 29 November 2017 at 15:14, Ulf Hansson <ulf.hansson@linaro.org> wrote: > [...] > >>> 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; >>> + bool use_busy_signal = true; >>> int err; >>> >>> - /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ >>> - if (notify_type == EXT_CSD_POWER_OFF_LONG) >>> + switch (notify_type) { >>> + case EXT_CSD_POWER_OFF_LONG: >>> timeout = card->ext_csd.power_off_longtime; >>> + break; >>> + case EXT_CSD_SLEEP_NOTIFICATION: >>> + timeout = card->ext_csd.sleep_notification_time; >>> + use_busy_signal = false; >> >> This is wrong. >> >> If you set use_busy_signal to false, it means that we don't care about >> waiting for the card to stop signaling busy on DAT0 after setting >> EXT_CSD_SLEEP_NOTIFICATION. This then becomes a violation of the eMMC >> spec, because we must not issue a CMD5 before the card have stopped >> signaling busy. >> >> I realize that for those devices that don't support >> MMC_CAP_WAIT_WHILE_BUSY or have the ->card_busy() callback >> implemented, the consequence may be that mmc_poll_for_busy() may call >> mmc_delay() with a very big timeout. This could be a big problem, I >> guess. > > Well, it may also be a big problem even if the host supports > ->card_busy() and/or MMC_CAP_WAIT_WHILE_BUSY (but maybe not as big), > simply because we can't be waiting here for several seconds to allow > the card to deal with background operations. > > I guess we need to try and see what happens. Perhaps you can share > some data about the timeouts you get? > >> >>> + break; >>> + default: >>> + /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ >>> + timeout = card->ext_csd.generic_cmd6_time; >>> + } >>> >>> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >>> EXT_CSD_POWER_OFF_NOTIFICATION, >>> - notify_type, timeout, 0, true, false, false); >>> + notify_type, timeout, 0, use_busy_signal, false, false); >>> if (err) >>> pr_err("%s: Power Off Notification timed out, %u\n", >>> mmc_hostname(card->host), timeout); > > [...] > > Kind regards > Uffe Gwendal, any news on this? As changes for the SLEEP_NOTIFICATION keeps being posted, clearly there is a need for us to deal with it in some way. I have been thinking of a fall-back solution, if we can't get it it work, but anyway I am eager to find a solution so we finally can sort this out. 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
Ufe, I have been side-tracked but will get back on it. I will be the worst case scenario (lot of random write before suspend for different eMMC). I also need to rework the wait for DAT0 to deassert instead of the busy line as usual. Gwendal. On Fri, Dec 15, 2017 at 1:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 29 November 2017 at 15:14, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> [...] >> >>>> 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; >>>> + bool use_busy_signal = true; >>>> int err; >>>> >>>> - /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ >>>> - if (notify_type == EXT_CSD_POWER_OFF_LONG) >>>> + switch (notify_type) { >>>> + case EXT_CSD_POWER_OFF_LONG: >>>> timeout = card->ext_csd.power_off_longtime; >>>> + break; >>>> + case EXT_CSD_SLEEP_NOTIFICATION: >>>> + timeout = card->ext_csd.sleep_notification_time; >>>> + use_busy_signal = false; >>> >>> This is wrong. >>> >>> If you set use_busy_signal to false, it means that we don't care about >>> waiting for the card to stop signaling busy on DAT0 after setting >>> EXT_CSD_SLEEP_NOTIFICATION. This then becomes a violation of the eMMC >>> spec, because we must not issue a CMD5 before the card have stopped >>> signaling busy. >>> >>> I realize that for those devices that don't support >>> MMC_CAP_WAIT_WHILE_BUSY or have the ->card_busy() callback >>> implemented, the consequence may be that mmc_poll_for_busy() may call >>> mmc_delay() with a very big timeout. This could be a big problem, I >>> guess. >> >> Well, it may also be a big problem even if the host supports >> ->card_busy() and/or MMC_CAP_WAIT_WHILE_BUSY (but maybe not as big), >> simply because we can't be waiting here for several seconds to allow >> the card to deal with background operations. >> >> I guess we need to try and see what happens. Perhaps you can share >> some data about the timeouts you get? >> >>> >>>> + break; >>>> + default: >>>> + /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ >>>> + timeout = card->ext_csd.generic_cmd6_time; >>>> + } >>>> >>>> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >>>> EXT_CSD_POWER_OFF_NOTIFICATION, >>>> - notify_type, timeout, 0, true, false, false); >>>> + notify_type, timeout, 0, use_busy_signal, false, false); >>>> if (err) >>>> pr_err("%s: Power Off Notification timed out, %u\n", >>>> mmc_hostname(card->host), timeout); >> >> [...] >> >> Kind regards >> Uffe > > Gwendal, any news on this? > > As changes for the SLEEP_NOTIFICATION keeps being posted, clearly > there is a need for us to deal with it in some way. I have been > thinking of a fall-back solution, if we can't get it it work, but > anyway I am eager to find a solution so we finally can sort this out. > > 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 --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 36217ad5e9b1..00ee7dd5ba07 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -633,6 +633,13 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd) ext_csd[EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_A]; card->ext_csd.device_life_time_est_typ_b = ext_csd[EXT_CSD_DEVICE_LIFE_TIME_EST_TYP_B]; + + if (ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME] == 0 || + ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME] >= 0x18) + card->ext_csd.sleep_notification_time = 0; + else + card->ext_csd.sleep_notification_time = + max((10 << ext_csd[EXT_CSD_SLEEP_NOTIFICATION_TIME]) / 1000, 1); } /* eMMC v5.1 or later */ @@ -652,6 +659,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd) card->ext_csd.cmdq_depth); } } + out: return err; } @@ -1865,18 +1873,34 @@ static int mmc_can_poweroff_notify(const struct mmc_card *card) (card->ext_csd.power_off_notification == EXT_CSD_POWER_ON); } +static int mmc_can_sleep_notify(const struct mmc_card *card) +{ + return mmc_can_poweroff_notify(card) && + (card->ext_csd.sleep_notification_time > 0); +} + 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; + bool use_busy_signal = true; int err; - /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ - if (notify_type == EXT_CSD_POWER_OFF_LONG) + switch (notify_type) { + case EXT_CSD_POWER_OFF_LONG: timeout = card->ext_csd.power_off_longtime; + break; + case EXT_CSD_SLEEP_NOTIFICATION: + timeout = card->ext_csd.sleep_notification_time; + use_busy_signal = false; + break; + default: + /* Use EXT_CSD_POWER_OFF_SHORT as default notification type. */ + timeout = card->ext_csd.generic_cmd6_time; + } err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_POWER_OFF_NOTIFICATION, - notify_type, timeout, 0, true, false, false); + notify_type, timeout, 0, use_busy_signal, false, false); if (err) pr_err("%s: Power Off Notification timed out, %u\n", mmc_hostname(card->host), timeout); @@ -1952,13 +1976,17 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) goto out; if (mmc_can_poweroff_notify(host->card) && - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { err = mmc_poweroff_notify(host->card, notify_type); - else if (mmc_can_sleep(host->card)) - err = mmc_sleep(host); - else if (!mmc_host_is_spi(host)) + } else if (mmc_can_sleep(host->card)) { + if (mmc_can_sleep_notify(host->card)) + err = mmc_poweroff_notify(host->card, + EXT_CSD_SLEEP_NOTIFICATION); + if (!err) + err = mmc_sleep(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 279b39008a33..5af3661bd98a 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -59,8 +59,9 @@ struct mmc_ext_csd { u8 packed_event_en; unsigned int part_time; /* Units: ms */ unsigned int sa_timeout; /* Units: 100ns */ - unsigned int generic_cmd6_time; /* Units: 10ms */ + unsigned int generic_cmd6_time; /* Units: ms */ unsigned int power_off_longtime; /* Units: ms */ + unsigned int sleep_notification_time; /* Units: ms */ u8 power_off_notification; /* state */ unsigned int hs_max_dtr; unsigned int hs200_max_dtr; diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index 3ffc27aaeeaf..901e124dee72 100644 --- a/include/linux/mmc/mmc.h +++ b/include/linux/mmc/mmc.h @@ -277,6 +277,7 @@ static inline bool mmc_op_multi(u32 opcode) #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 */ @@ -379,6 +380,7 @@ static inline bool mmc_op_multi(u32 opcode) #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 */
eMMC 5.0 spec introduces a new power off notification, SLEEP_NOTIFICATION, the host may send before issuing the sleep commamd. Some eMMC expect the SLEEP_NOTIFICATION for optimal performance: It is recommended by Sandisk for its iNAND 7232 to empty the iNAND SmartTLC buffer, See "Emptying the iNAND SmartSLC buffer" of datasheet. Hynix eMMC also resume faster when SLEEP_NOTIFICATION is sent before going to sleep. SLEEP_NOTIFICATION PON can theoretically take a long time, iNAND sleep notification timeout is set to 83.88s. But it has to be sent just before CMD5, otherwise newer commands may make it moot. Signed-off-by: Gwendal Grignou <gwendal@chromium.org> --- There were several attempts to add SLEEP_NOTIFICATION, here is another try. For refrence: http://thread.gmane.org/gmane.linux.kernel.mmc/23471 [1/2034] http://www.spinics.net/lists/linux-mmc/msg31344.html [6/2015] https://patchwork.kernel.org/patch/9360633/ [10/2016] drivers/mmc/core/mmc.c | 46 +++++++++++++++++++++++++++++++++++++--------- include/linux/mmc/card.h | 3 ++- include/linux/mmc/mmc.h | 2 ++ 3 files changed, 41 insertions(+), 10 deletions(-)