[PATCH V3 0/3] mmc: Add partial initialization support

[PATCH V3 0/3] mmc: Add partial initialization support

[Sarthak Garg]

Add the ability to partially initialize the MMC device by
using device sleep/awake sequence (CMD5).
Device will be sent to sleep state during mmc runtime/system suspend
and will be woken up during mmc runtime/system resume.
By using this sequence the device doesn't need full initialization
which gives 25% time reduction in system/runtime resume path.
Also enable this feature along with mmc runtime PM for qualcomm
controllers.

Changes since V2:
	- Fixed one warning reported by kernel test robot.
	  >> drivers/mmc/core/mmc.c:1975: warning: Function parameter
	  or member 'host' not described in 'mmc_sleepawake'
	- Included name of the subystem/driver in the title of the
	  series as suggested by Bryan O'Donoghue.
	- Enabled MMC_CAP_AGGRESSIVE_PM for qualcomm controller and
	  posted as [V3,3/3] as suggested by Wenchao Chen.

Changes since V1:
	- Did minor code cleanup as suggested by Avri Altman.
	- Added _mmc_resume gain data in commit message as suggested by
	  Ulf Hansson.
	- Split patch in two patches.
	  [V2,1/2] mmc: core: Add partial initialization support
	  [V2,2/2] mmc: sdhci-msm: Enable MMC_CAP2_SLEEP_AWAKE for
	  Qualcomm controllers

Sarthak Garg (3):
  mmc: core: Add partial initialization support
  mmc: sdhci-msm: Enable MMC_CAP_AGGRESSIVE_PM for Qualcomm controllers
  mmc: sdhci-msm: Enable MMC_CAP2_SLEEP_AWAKE for Qualcomm controllers

 drivers/mmc/core/mmc.c       | 163 +++++++++++++++++++++++++++++++++--
 drivers/mmc/host/sdhci-msm.c |   2 +
 include/linux/mmc/card.h     |   4 +
 include/linux/mmc/host.h     |   2 +
 4 files changed, 162 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH V3 1/3] mmc: core: Add partial initialization support

[Sarthak Garg]

Add the ability to partially initialize the MMC device by
using device sleep/awake sequence (CMD5).
Device will be sent to sleep state during mmc runtime/system suspend
and will be woken up during mmc runtime/system resume.
By using this sequence the device doesn't need full initialization
which gives 25% time reduction in system/runtime resume path.

1) Micron eMMC (ManfID 0x13)

Partial init				Full Init

a) _mmc_resume: 			_mmc_resume :

Total time : 62ms 			Total time : 84ms
(Improvement % from full init = ~26%)

2) Kingston eMMC (ManfID 0x70)

Partial init				Full Init

a) _mmc_resume:			_mmc_resume :
Total time : 46ms			Total time : 62ms
(Improvement % from full init = ~25%).

Co-developed-by: Veerabhadrarao Badiganti <quic_vbadigan@quicinc.com>
Signed-off-by: Veerabhadrarao Badiganti <quic_vbadigan@quicinc.com>
Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 drivers/mmc/core/mmc.c   | 163 ++++++++++++++++++++++++++++++++++++---
 include/linux/mmc/card.h |   4 +
 include/linux/mmc/host.h |   2 +
 3 files changed, 160 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 8180983bd402..fb33bcf6d360 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1956,7 +1956,28 @@ static int mmc_sleep_busy_cb(void *cb_data, bool *busy)
 	return 0;
 }
 
-static int mmc_sleep(struct mmc_host *host)
+/*
+ * Returns true if card supports sleep/awake command and host can simply do
+ * sleep/awake instead of full card initialization as part of resume.
+ */
+static inline int mmc_can_sleepawake(struct mmc_host *host)
+{
+	return mmc_can_sleep(host->card) && (host->caps2 & MMC_CAP2_SLEEP_AWAKE);
+}
+
+/**
+ * mmc_sleepawake - function to sleep or awake the device
+ * @host: MMC host
+ * @sleep: if true then sleep command is sent else awake
+ *
+ * This function first deselects the card and then sends the sleep command
+ * in case of sleep whereas in case of awake first awake command is send
+ * and then the card is selected.
+ *
+ * Returns 0 on success, non-zero value on failure
+ */
+
+static int mmc_sleepawake(struct mmc_host *host, bool sleep)
 {
 	struct mmc_command cmd = {};
 	struct mmc_card *card = host->card;
@@ -1967,14 +1988,17 @@ static int mmc_sleep(struct mmc_host *host)
 	/* Re-tuning can't be done once the card is deselected */
 	mmc_retune_hold(host);
 
-	err = mmc_deselect_cards(host);
-	if (err)
-		goto out_release;
+	if (sleep) {
+		err = mmc_deselect_cards(host);
+		if (err)
+			goto out_release;
+	}
 
 	cmd.opcode = MMC_SLEEP_AWAKE;
 	cmd.arg = card->rca << 16;
-	cmd.arg |= 1 << 15;
 	use_r1b_resp = mmc_prepare_busy_cmd(host, &cmd, timeout_ms);
+	if (sleep)
+		cmd.arg |= BIT(15);
 
 	err = mmc_wait_for_cmd(host, &cmd, 0);
 	if (err)
@@ -1997,6 +2021,9 @@ static int mmc_sleep(struct mmc_host *host)
 	err = __mmc_poll_for_busy(host, 0, timeout_ms, &mmc_sleep_busy_cb, host);
 
 out_release:
+	if (!sleep)
+		err = mmc_select_card(card);
+
 	mmc_retune_release(host);
 	return err;
 }
@@ -2094,6 +2121,73 @@ static int _mmc_flush_cache(struct mmc_host *host)
 			pr_err("%s: cache flush error %d\n",
 			       mmc_hostname(host), err);
 	}
+	return err;
+}
+
+/*
+ * Save read/write fields of ext_csd register that the sw changes
+ * as part of suspend.
+ */
+static int mmc_save_card_ext_csd(struct mmc_host *host)
+{
+	int err;
+	u8 *ext_csd;
+	struct mmc_card *card = host->card;
+
+	err = mmc_get_ext_csd(card, &ext_csd);
+	if (err || !ext_csd) {
+		pr_err("%s: %s: mmc_get_ext_csd failed (%d)\n",
+			mmc_hostname(host), __func__, err);
+		return err;
+	}
+
+	card->ext_csd.raw_ext_csd_cmdq = ext_csd[EXT_CSD_CMDQ_MODE_EN];
+	card->ext_csd.raw_ext_csd_cache_ctrl = ext_csd[EXT_CSD_CACHE_CTRL];
+	card->ext_csd.raw_ext_csd_bus_width = ext_csd[EXT_CSD_BUS_WIDTH];
+	card->ext_csd.raw_ext_csd_hs_timing = ext_csd[EXT_CSD_HS_TIMING];
+
+	kfree(ext_csd);
+
+	return 0;
+}
+
+/*
+ * Get the ext_csd register from the card post resume and compare with
+ * read/write fields of ext_csd register that the sw changes.
+ */
+static int mmc_test_awake_ext_csd(struct mmc_host *host)
+{
+	struct mmc_card *card = host->card;
+	u8 *ext_csd;
+	int err;
+
+	err = mmc_get_ext_csd(card, &ext_csd);
+	if (err) {
+		pr_err("%s: %s: mmc_get_ext_csd failed (%d)\n",
+			mmc_hostname(host), __func__, err);
+		return err;
+	}
+
+	pr_debug("%s: %s: type(cached:current) cmdq(%d:%d) cache_ctrl(%d:%d) bus_width (%d:%d) timing(%d:%d)\n",
+		mmc_hostname(host), __func__,
+		card->ext_csd.raw_ext_csd_cmdq,
+		ext_csd[EXT_CSD_CMDQ_MODE_EN],
+		card->ext_csd.raw_ext_csd_cache_ctrl,
+		ext_csd[EXT_CSD_CACHE_CTRL],
+		card->ext_csd.raw_ext_csd_bus_width,
+		ext_csd[EXT_CSD_BUS_WIDTH],
+		card->ext_csd.raw_ext_csd_hs_timing,
+		ext_csd[EXT_CSD_HS_TIMING]);
+	err = !((card->ext_csd.raw_ext_csd_cmdq ==
+			ext_csd[EXT_CSD_CMDQ_MODE_EN]) &&
+		(card->ext_csd.raw_ext_csd_cache_ctrl ==
+			ext_csd[EXT_CSD_CACHE_CTRL]) &&
+		(card->ext_csd.raw_ext_csd_bus_width ==
+			ext_csd[EXT_CSD_BUS_WIDTH]) &&
+		(card->ext_csd.raw_ext_csd_hs_timing ==
+			ext_csd[EXT_CSD_HS_TIMING]));
+
+	kfree(ext_csd);
 
 	return err;
 }
@@ -2117,9 +2211,15 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 	    ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend ||
 	     (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_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_sleepawake(host)) {
+			memcpy(&host->cached_ios, &host->ios, sizeof(host->cached_ios));
+			err = mmc_save_card_ext_csd(host);
+			if (err)
+				goto out;
+		}
+		err = mmc_sleepawake(host, true);
+	} else if (!mmc_host_is_spi(host))
 		err = mmc_deselect_cards(host);
 
 	if (!err) {
@@ -2131,6 +2231,39 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 	return err;
 }
 
+static int mmc_partial_init(struct mmc_host *host)
+{
+	int err = 0;
+	struct mmc_card *card = host->card;
+
+	mmc_set_bus_width(host, host->cached_ios.bus_width);
+	mmc_set_timing(host, host->cached_ios.timing);
+	if (host->cached_ios.enhanced_strobe) {
+		host->ios.enhanced_strobe = true;
+		if (host->ops->hs400_enhanced_strobe)
+			host->ops->hs400_enhanced_strobe(host, &host->ios);
+	}
+	mmc_set_clock(host, host->cached_ios.clock);
+	mmc_set_bus_mode(host, host->cached_ios.bus_mode);
+
+	if (!mmc_card_hs400es(card) &&
+			(mmc_card_hs200(card) || mmc_card_hs400(card))) {
+		err = mmc_execute_tuning(card);
+		if (err) {
+			pr_err("%s: %s: Tuning failed (%d)\n",
+				mmc_hostname(host), __func__, err);
+			goto out;
+		}
+	}
+
+	err = mmc_test_awake_ext_csd(host);
+	if (err)
+		pr_debug("%s: %s: fail on ext_csd read (%d)\n",
+			mmc_hostname(host), __func__, err);
+out:
+	return err;
+}
+
 /*
  * Suspend callback
  */
@@ -2161,7 +2294,19 @@ static int _mmc_resume(struct mmc_host *host)
 		goto out;
 
 	mmc_power_up(host, host->card->ocr);
-	err = mmc_init_card(host, host->card->ocr, host->card);
+
+	if (mmc_can_sleepawake(host)) {
+		err = mmc_sleepawake(host, false);
+		if (!err)
+			err = mmc_partial_init(host);
+		else
+			pr_err("%s: %s: awake failed (%d), fallback to full init\n",
+				mmc_hostname(host), __func__, err);
+	}
+
+	if (!mmc_can_sleepawake(host) || err)
+		err = mmc_init_card(host, host->card->ocr, host->card);
+
 	mmc_card_clr_suspended(host->card);
 
 out:
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index daa2f40d9ce6..fbc832ec6d57 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -86,6 +86,8 @@ struct mmc_ext_csd {
 	unsigned int            data_tag_unit_size;     /* DATA TAG UNIT size */
 	unsigned int		boot_ro_lock;		/* ro lock support */
 	bool			boot_ro_lockable;
+	u8			raw_ext_csd_cmdq;	/* 15 */
+	u8			raw_ext_csd_cache_ctrl;	/* 33 */
 	bool			ffu_capable;	/* Firmware upgrade support */
 	bool			cmdq_en;	/* Command Queue enabled */
 	bool			cmdq_support;	/* Command Queue supported */
@@ -96,7 +98,9 @@ struct mmc_ext_csd {
 	u8			raw_partition_support;	/* 160 */
 	u8			raw_rpmb_size_mult;	/* 168 */
 	u8			raw_erased_mem_count;	/* 181 */
+	u8			raw_ext_csd_bus_width;	/* 183 */
 	u8			strobe_support;		/* 184 */
+	u8			raw_ext_csd_hs_timing;	/* 185 */
 	u8			raw_ext_csd_structure;	/* 194 */
 	u8			raw_card_type;		/* 196 */
 	u8			raw_driver_strength;	/* 197 */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 2f445c651742..836382a0b2e9 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -427,6 +427,7 @@ struct mmc_host {
 #define MMC_CAP2_CRYPTO		0
 #endif
 #define MMC_CAP2_ALT_GPT_TEGRA	(1 << 28)	/* Host with eMMC that has GPT entry at a non-standard location */
+#define MMC_CAP2_SLEEP_AWAKE	(1 << 29)	/* Use Sleep/Awake (CMD5) */
 
 	int			fixed_drv_type;	/* fixed driver type for non-removable media */
 
@@ -445,6 +446,7 @@ struct mmc_host {
 	spinlock_t		lock;		/* lock for claim and bus ops */
 
 	struct mmc_ios		ios;		/* current io bus settings */
+	struct mmc_ios		cached_ios;
 
 	/* group bitfields together to minimize padding */
 	unsigned int		use_spi_crc:1;
-- 
2.17.1

[PATCH V3 2/3] mmc: sdhci-msm: Enable MMC_CAP_AGGRESSIVE_PM for Qualcomm controllers

[Sarthak Garg]

Enable MMC_CAP_AGGRESSIVE_PM to enable mmc runtime PM functionality for
qualcomm controllers.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 668e0aceeeba..93c662e28b3b 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2627,6 +2627,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	}
 
 	msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY;
+	msm_host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM;
 
 	/* Set the timeout value to max possible */
 	host->max_timeout_count = 0xF;
-- 
2.17.1

[PATCH V3 3/3] mmc: sdhci-msm: Enable MMC_CAP2_SLEEP_AWAKE for Qualcomm controllers

[Sarthak Garg]

Enable MMC_CAP2_SLEEP_AWAKE for Qualcomm controllers to let them use
sleep/awake functionality for faster eMMC resume instead of
doing full initialization.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 93c662e28b3b..edcf18c02bf7 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2628,6 +2628,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY;
 	msm_host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM;
+	msm_host->mmc->caps2 |= MMC_CAP2_SLEEP_AWAKE;
 
 	/* Set the timeout value to max possible */
 	host->max_timeout_count = 0xF;
-- 
2.17.1

Re: [PATCH V3 1/3] mmc: core: Add partial initialization support

[Ulf Hansson]

On Thu, 19 Oct 2023 at 07:46, Sarthak Garg <quic_sartgarg@quicinc.com> wrote:
>
> Add the ability to partially initialize the MMC device by
> using device sleep/awake sequence (CMD5).
> Device will be sent to sleep state during mmc runtime/system suspend
> and will be woken up during mmc runtime/system resume.
> By using this sequence the device doesn't need full initialization
> which gives 25% time reduction in system/runtime resume path.
>
> 1) Micron eMMC (ManfID 0x13)
>
> Partial init                            Full Init
>
> a) _mmc_resume:                         _mmc_resume :
>
> Total time : 62ms                       Total time : 84ms
> (Improvement % from full init = ~26%)
>
> 2) Kingston eMMC (ManfID 0x70)
>
> Partial init                            Full Init
>
> a) _mmc_resume:                 _mmc_resume :
> Total time : 46ms                       Total time : 62ms
> (Improvement % from full init = ~25%).
>
> Co-developed-by: Veerabhadrarao Badiganti <quic_vbadigan@quicinc.com>
> Signed-off-by: Veerabhadrarao Badiganti <quic_vbadigan@quicinc.com>
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  drivers/mmc/core/mmc.c   | 163 ++++++++++++++++++++++++++++++++++++---
>  include/linux/mmc/card.h |   4 +
>  include/linux/mmc/host.h |   2 +
>  3 files changed, 160 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 8180983bd402..fb33bcf6d360 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1956,7 +1956,28 @@ static int mmc_sleep_busy_cb(void *cb_data, bool *busy)
>         return 0;
>  }
>
> -static int mmc_sleep(struct mmc_host *host)
> +/*
> + * Returns true if card supports sleep/awake command and host can simply do
> + * sleep/awake instead of full card initialization as part of resume.
> + */
> +static inline int mmc_can_sleepawake(struct mmc_host *host)

Nitpick: Please rename to mmc_can_sleep_awake() to make the name more clear.

> +{
> +       return mmc_can_sleep(host->card) && (host->caps2 & MMC_CAP2_SLEEP_AWAKE);
> +}
> +
> +/**
> + * mmc_sleepawake - function to sleep or awake the device
> + * @host: MMC host
> + * @sleep: if true then sleep command is sent else awake
> + *
> + * This function first deselects the card and then sends the sleep command
> + * in case of sleep whereas in case of awake first awake command is send
> + * and then the card is selected.
> + *
> + * Returns 0 on success, non-zero value on failure
> + */
> +
> +static int mmc_sleepawake(struct mmc_host *host, bool sleep)

Nitpick: Please rename into mmc_sleep_awake()

>  {
>         struct mmc_command cmd = {};
>         struct mmc_card *card = host->card;
> @@ -1967,14 +1988,17 @@ static int mmc_sleep(struct mmc_host *host)
>         /* Re-tuning can't be done once the card is deselected */
>         mmc_retune_hold(host);
>
> -       err = mmc_deselect_cards(host);
> -       if (err)
> -               goto out_release;
> +       if (sleep) {
> +               err = mmc_deselect_cards(host);
> +               if (err)
> +                       goto out_release;
> +       }
>
>         cmd.opcode = MMC_SLEEP_AWAKE;
>         cmd.arg = card->rca << 16;
> -       cmd.arg |= 1 << 15;
>         use_r1b_resp = mmc_prepare_busy_cmd(host, &cmd, timeout_ms);
> +       if (sleep)
> +               cmd.arg |= BIT(15);

Please move this above the call to mmc_prepare_busy_cmd(). For consistency.

>
>         err = mmc_wait_for_cmd(host, &cmd, 0);
>         if (err)
> @@ -1997,6 +2021,9 @@ static int mmc_sleep(struct mmc_host *host)
>         err = __mmc_poll_for_busy(host, 0, timeout_ms, &mmc_sleep_busy_cb, host);
>
>  out_release:
> +       if (!sleep)
> +               err = mmc_select_card(card);
> +
>         mmc_retune_release(host);
>         return err;
>  }
> @@ -2094,6 +2121,73 @@ static int _mmc_flush_cache(struct mmc_host *host)
>                         pr_err("%s: cache flush error %d\n",
>                                mmc_hostname(host), err);
>         }
> +       return err;
> +}
> +
> +/*
> + * Save read/write fields of ext_csd register that the sw changes
> + * as part of suspend.
> + */
> +static int mmc_save_card_ext_csd(struct mmc_host *host)
> +{
> +       int err;
> +       u8 *ext_csd;
> +       struct mmc_card *card = host->card;
> +
> +       err = mmc_get_ext_csd(card, &ext_csd);
> +       if (err || !ext_csd) {
> +               pr_err("%s: %s: mmc_get_ext_csd failed (%d)\n",
> +                       mmc_hostname(host), __func__, err);
> +               return err;
> +       }
> +
> +       card->ext_csd.raw_ext_csd_cmdq = ext_csd[EXT_CSD_CMDQ_MODE_EN];
> +       card->ext_csd.raw_ext_csd_cache_ctrl = ext_csd[EXT_CSD_CACHE_CTRL];
> +       card->ext_csd.raw_ext_csd_bus_width = ext_csd[EXT_CSD_BUS_WIDTH];
> +       card->ext_csd.raw_ext_csd_hs_timing = ext_csd[EXT_CSD_HS_TIMING];
> +
> +       kfree(ext_csd);
> +
> +       return 0;
> +}
> +
> +/*
> + * Get the ext_csd register from the card post resume and compare with
> + * read/write fields of ext_csd register that the sw changes.
> + */
> +static int mmc_test_awake_ext_csd(struct mmc_host *host)
> +{
> +       struct mmc_card *card = host->card;
> +       u8 *ext_csd;
> +       int err;
> +
> +       err = mmc_get_ext_csd(card, &ext_csd);
> +       if (err) {
> +               pr_err("%s: %s: mmc_get_ext_csd failed (%d)\n",
> +                       mmc_hostname(host), __func__, err);
> +               return err;
> +       }
> +
> +       pr_debug("%s: %s: type(cached:current) cmdq(%d:%d) cache_ctrl(%d:%d) bus_width (%d:%d) timing(%d:%d)\n",
> +               mmc_hostname(host), __func__,
> +               card->ext_csd.raw_ext_csd_cmdq,
> +               ext_csd[EXT_CSD_CMDQ_MODE_EN],
> +               card->ext_csd.raw_ext_csd_cache_ctrl,
> +               ext_csd[EXT_CSD_CACHE_CTRL],
> +               card->ext_csd.raw_ext_csd_bus_width,
> +               ext_csd[EXT_CSD_BUS_WIDTH],
> +               card->ext_csd.raw_ext_csd_hs_timing,
> +               ext_csd[EXT_CSD_HS_TIMING]);
> +       err = !((card->ext_csd.raw_ext_csd_cmdq ==
> +                       ext_csd[EXT_CSD_CMDQ_MODE_EN]) &&
> +               (card->ext_csd.raw_ext_csd_cache_ctrl ==
> +                       ext_csd[EXT_CSD_CACHE_CTRL]) &&
> +               (card->ext_csd.raw_ext_csd_bus_width ==
> +                       ext_csd[EXT_CSD_BUS_WIDTH]) &&
> +               (card->ext_csd.raw_ext_csd_hs_timing ==
> +                       ext_csd[EXT_CSD_HS_TIMING]));
> +
> +       kfree(ext_csd);
>
>         return err;
>  }
> @@ -2117,9 +2211,15 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>             ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend ||
>              (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_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_sleepawake(host)) {
> +                       memcpy(&host->cached_ios, &host->ios, sizeof(host->cached_ios));
> +                       err = mmc_save_card_ext_csd(host);

I understand that you want to read/save the ext_csd at suspend to
read/compare it at resume.

Although, I don't understand *why* this is needed, can you please clarify?

> +                       if (err)
> +                               goto out;
> +               }
> +               err = mmc_sleepawake(host, true);
> +       } else if (!mmc_host_is_spi(host))
>                 err = mmc_deselect_cards(host);
>
>         if (!err) {
> @@ -2131,6 +2231,39 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>         return err;
>  }
>
> +static int mmc_partial_init(struct mmc_host *host)

Nitpick: Please rename this into mmc_restore_ios().

> +{
> +       int err = 0;
> +       struct mmc_card *card = host->card;
> +
> +       mmc_set_bus_width(host, host->cached_ios.bus_width);
> +       mmc_set_timing(host, host->cached_ios.timing);
> +       if (host->cached_ios.enhanced_strobe) {
> +               host->ios.enhanced_strobe = true;
> +               if (host->ops->hs400_enhanced_strobe)
> +                       host->ops->hs400_enhanced_strobe(host, &host->ios);
> +       }
> +       mmc_set_clock(host, host->cached_ios.clock);
> +       mmc_set_bus_mode(host, host->cached_ios.bus_mode);
> +

Rather than re-using the above APIs and the ->set_ios() callback in
the host, I believe it would be better to add a new host ops to manage
all of the above at once instead. Something along the lines of the
below, would then replace all of the above.

host->ops->restore_ios(host, &host->cached_ios)
memcpy(&host->ios, &host->cached_ios, sizeof(host->ios));

Would that make sense to you too?

> +       if (!mmc_card_hs400es(card) &&
> +                       (mmc_card_hs200(card) || mmc_card_hs400(card))) {
> +               err = mmc_execute_tuning(card);
> +               if (err) {
> +                       pr_err("%s: %s: Tuning failed (%d)\n",
> +                               mmc_hostname(host), __func__, err);

There is already a print being done in mmc_execute_tuning() at
failure. So, let's drop the above print.

> +                       goto out;
> +               }
> +       }
> +
> +       err = mmc_test_awake_ext_csd(host);

Again, I don't get why this is needed, so let's discuss this more.

> +       if (err)
> +               pr_debug("%s: %s: fail on ext_csd read (%d)\n",
> +                       mmc_hostname(host), __func__, err);
> +out:
> +       return err;
> +}
> +
>  /*
>   * Suspend callback
>   */
> @@ -2161,7 +2294,19 @@ static int _mmc_resume(struct mmc_host *host)
>                 goto out;
>
>         mmc_power_up(host, host->card->ocr);
> -       err = mmc_init_card(host, host->card->ocr, host->card);
> +
> +       if (mmc_can_sleepawake(host)) {
> +               err = mmc_sleepawake(host, false);
> +               if (!err)
> +                       err = mmc_partial_init(host);
> +               else
> +                       pr_err("%s: %s: awake failed (%d), fallback to full init\n",
> +                               mmc_hostname(host), __func__, err);

We don't print other errors during resume - and I don't think there is
any special reason to do it for this case only. Please drop it.

> +       }
> +
> +       if (!mmc_can_sleepawake(host) || err)
> +               err = mmc_init_card(host, host->card->ocr, host->card);
> +
>         mmc_card_clr_suspended(host->card);
>
>  out:
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index daa2f40d9ce6..fbc832ec6d57 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -86,6 +86,8 @@ struct mmc_ext_csd {
>         unsigned int            data_tag_unit_size;     /* DATA TAG UNIT size */
>         unsigned int            boot_ro_lock;           /* ro lock support */
>         bool                    boot_ro_lockable;
> +       u8                      raw_ext_csd_cmdq;       /* 15 */
> +       u8                      raw_ext_csd_cache_ctrl; /* 33 */
>         bool                    ffu_capable;    /* Firmware upgrade support */
>         bool                    cmdq_en;        /* Command Queue enabled */
>         bool                    cmdq_support;   /* Command Queue supported */
> @@ -96,7 +98,9 @@ struct mmc_ext_csd {
>         u8                      raw_partition_support;  /* 160 */
>         u8                      raw_rpmb_size_mult;     /* 168 */
>         u8                      raw_erased_mem_count;   /* 181 */
> +       u8                      raw_ext_csd_bus_width;  /* 183 */
>         u8                      strobe_support;         /* 184 */
> +       u8                      raw_ext_csd_hs_timing;  /* 185 */
>         u8                      raw_ext_csd_structure;  /* 194 */
>         u8                      raw_card_type;          /* 196 */
>         u8                      raw_driver_strength;    /* 197 */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 2f445c651742..836382a0b2e9 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -427,6 +427,7 @@ struct mmc_host {
>  #define MMC_CAP2_CRYPTO                0
>  #endif
>  #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28)       /* Host with eMMC that has GPT entry at a non-standard location */
> +#define MMC_CAP2_SLEEP_AWAKE   (1 << 29)       /* Use Sleep/Awake (CMD5) */
>
>         int                     fixed_drv_type; /* fixed driver type for non-removable media */
>
> @@ -445,6 +446,7 @@ struct mmc_host {
>         spinlock_t              lock;           /* lock for claim and bus ops */
>
>         struct mmc_ios          ios;            /* current io bus settings */
> +       struct mmc_ios          cached_ios;
>
>         /* group bitfields together to minimize padding */
>         unsigned int            use_spi_crc:1;

Kind regards
Uffe

Re: [PATCH V3 1/3] mmc: core: Add partial initialization support

[Sarthak Garg]

On 10/19/2023 8:30 PM, Ulf Hansson wrote:
> On Thu, 19 Oct 2023 at 07:46, Sarthak Garg <quic_sartgarg@quicinc.com> wrote:
>>
>> Add the ability to partially initialize the MMC device by
>> using device sleep/awake sequence (CMD5).
>> Device will be sent to sleep state during mmc runtime/system suspend
>> and will be woken up during mmc runtime/system resume.
>> By using this sequence the device doesn't need full initialization
>> which gives 25% time reduction in system/runtime resume path.
>>
>> 1) Micron eMMC (ManfID 0x13)
>>
>> Partial init                            Full Init
>>
>> a) _mmc_resume:                         _mmc_resume :
>>
>> Total time : 62ms                       Total time : 84ms
>> (Improvement % from full init = ~26%)
>>
>> 2) Kingston eMMC (ManfID 0x70)
>>
>> Partial init                            Full Init
>>
>> a) _mmc_resume:                 _mmc_resume :
>> Total time : 46ms                       Total time : 62ms
>> (Improvement % from full init = ~25%).
>>
>> Co-developed-by: Veerabhadrarao Badiganti <quic_vbadigan@quicinc.com>
>> Signed-off-by: Veerabhadrarao Badiganti <quic_vbadigan@quicinc.com>
>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>> ---
>>   drivers/mmc/core/mmc.c   | 163 ++++++++++++++++++++++++++++++++++++---
>>   include/linux/mmc/card.h |   4 +
>>   include/linux/mmc/host.h |   2 +
>>   3 files changed, 160 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 8180983bd402..fb33bcf6d360 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1956,7 +1956,28 @@ static int mmc_sleep_busy_cb(void *cb_data, bool *busy)
>>          return 0;
>>   }
>>
>> -static int mmc_sleep(struct mmc_host *host)
>> +/*
>> + * Returns true if card supports sleep/awake command and host can simply do
>> + * sleep/awake instead of full card initialization as part of resume.
>> + */
>> +static inline int mmc_can_sleepawake(struct mmc_host *host)
> 
> Nitpick: Please rename to mmc_can_sleep_awake() to make the name more clear.

Sure will take care in V4.

> 
>> +{
>> +       return mmc_can_sleep(host->card) && (host->caps2 & MMC_CAP2_SLEEP_AWAKE);
>> +}
>> +
>> +/**
>> + * mmc_sleepawake - function to sleep or awake the device
>> + * @host: MMC host
>> + * @sleep: if true then sleep command is sent else awake
>> + *
>> + * This function first deselects the card and then sends the sleep command
>> + * in case of sleep whereas in case of awake first awake command is send
>> + * and then the card is selected.
>> + *
>> + * Returns 0 on success, non-zero value on failure
>> + */
>> +
>> +static int mmc_sleepawake(struct mmc_host *host, bool sleep)
> 
> Nitpick: Please rename into mmc_sleep_awake()

Sure will take care in V4.

> 
>>   {
>>          struct mmc_command cmd = {};
>>          struct mmc_card *card = host->card;
>> @@ -1967,14 +1988,17 @@ static int mmc_sleep(struct mmc_host *host)
>>          /* Re-tuning can't be done once the card is deselected */
>>          mmc_retune_hold(host);
>>
>> -       err = mmc_deselect_cards(host);
>> -       if (err)
>> -               goto out_release;
>> +       if (sleep) {
>> +               err = mmc_deselect_cards(host);
>> +               if (err)
>> +                       goto out_release;
>> +       }
>>
>>          cmd.opcode = MMC_SLEEP_AWAKE;
>>          cmd.arg = card->rca << 16;
>> -       cmd.arg |= 1 << 15;
>>          use_r1b_resp = mmc_prepare_busy_cmd(host, &cmd, timeout_ms);
>> +       if (sleep)
>> +               cmd.arg |= BIT(15);
> 
> Please move this above the call to mmc_prepare_busy_cmd(). For consistency.
>

Sure will take care in V4.

>>
>>          err = mmc_wait_for_cmd(host, &cmd, 0);
>>          if (err)
>> @@ -1997,6 +2021,9 @@ static int mmc_sleep(struct mmc_host *host)
>>          err = __mmc_poll_for_busy(host, 0, timeout_ms, &mmc_sleep_busy_cb, host);
>>
>>   out_release:
>> +       if (!sleep)
>> +               err = mmc_select_card(card);
>> +
>>          mmc_retune_release(host);
>>          return err;
>>   }
>> @@ -2094,6 +2121,73 @@ static int _mmc_flush_cache(struct mmc_host *host)
>>                          pr_err("%s: cache flush error %d\n",
>>                                 mmc_hostname(host), err);
>>          }
>> +       return err;
>> +}
>> +
>> +/*
>> + * Save read/write fields of ext_csd register that the sw changes
>> + * as part of suspend.
>> + */
>> +static int mmc_save_card_ext_csd(struct mmc_host *host)
>> +{
>> +       int err;
>> +       u8 *ext_csd;
>> +       struct mmc_card *card = host->card;
>> +
>> +       err = mmc_get_ext_csd(card, &ext_csd);
>> +       if (err || !ext_csd) {
>> +               pr_err("%s: %s: mmc_get_ext_csd failed (%d)\n",
>> +                       mmc_hostname(host), __func__, err);
>> +               return err;
>> +       }
>> +
>> +       card->ext_csd.raw_ext_csd_cmdq = ext_csd[EXT_CSD_CMDQ_MODE_EN];
>> +       card->ext_csd.raw_ext_csd_cache_ctrl = ext_csd[EXT_CSD_CACHE_CTRL];
>> +       card->ext_csd.raw_ext_csd_bus_width = ext_csd[EXT_CSD_BUS_WIDTH];
>> +       card->ext_csd.raw_ext_csd_hs_timing = ext_csd[EXT_CSD_HS_TIMING];
>> +
>> +       kfree(ext_csd);
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Get the ext_csd register from the card post resume and compare with
>> + * read/write fields of ext_csd register that the sw changes.
>> + */
>> +static int mmc_test_awake_ext_csd(struct mmc_host *host)
>> +{
>> +       struct mmc_card *card = host->card;
>> +       u8 *ext_csd;
>> +       int err;
>> +
>> +       err = mmc_get_ext_csd(card, &ext_csd);
>> +       if (err) {
>> +               pr_err("%s: %s: mmc_get_ext_csd failed (%d)\n",
>> +                       mmc_hostname(host), __func__, err);
>> +               return err;
>> +       }
>> +
>> +       pr_debug("%s: %s: type(cached:current) cmdq(%d:%d) cache_ctrl(%d:%d) bus_width (%d:%d) timing(%d:%d)\n",
>> +               mmc_hostname(host), __func__,
>> +               card->ext_csd.raw_ext_csd_cmdq,
>> +               ext_csd[EXT_CSD_CMDQ_MODE_EN],
>> +               card->ext_csd.raw_ext_csd_cache_ctrl,
>> +               ext_csd[EXT_CSD_CACHE_CTRL],
>> +               card->ext_csd.raw_ext_csd_bus_width,
>> +               ext_csd[EXT_CSD_BUS_WIDTH],
>> +               card->ext_csd.raw_ext_csd_hs_timing,
>> +               ext_csd[EXT_CSD_HS_TIMING]);
>> +       err = !((card->ext_csd.raw_ext_csd_cmdq ==
>> +                       ext_csd[EXT_CSD_CMDQ_MODE_EN]) &&
>> +               (card->ext_csd.raw_ext_csd_cache_ctrl ==
>> +                       ext_csd[EXT_CSD_CACHE_CTRL]) &&
>> +               (card->ext_csd.raw_ext_csd_bus_width ==
>> +                       ext_csd[EXT_CSD_BUS_WIDTH]) &&
>> +               (card->ext_csd.raw_ext_csd_hs_timing ==
>> +                       ext_csd[EXT_CSD_HS_TIMING]));
>> +
>> +       kfree(ext_csd);
>>
>>          return err;
>>   }
>> @@ -2117,9 +2211,15 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>              ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend ||
>>               (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_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_sleepawake(host)) {
>> +                       memcpy(&host->cached_ios, &host->ios, sizeof(host->cached_ios));
>> +                       err = mmc_save_card_ext_csd(host);
> 
> I understand that you want to read/save the ext_csd at suspend to
> read/compare it at resume.
> 
> Although, I don't understand *why* this is needed, can you please clarify?
> 
>> +                       if (err)
>> +                               goto out;
>> +               }
>> +               err = mmc_sleepawake(host, true);
>> +       } else if (!mmc_host_is_spi(host))
>>                  err = mmc_deselect_cards(host);
>>
>>          if (!err) {
>> @@ -2131,6 +2231,39 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>>          return err;
>>   }
>>
>> +static int mmc_partial_init(struct mmc_host *host)
> 
> Nitpick: Please rename this into mmc_restore_ios().
> 

Sure will take care in V4.

>> +{
>> +       int err = 0;
>> +       struct mmc_card *card = host->card;
>> +
>> +       mmc_set_bus_width(host, host->cached_ios.bus_width);
>> +       mmc_set_timing(host, host->cached_ios.timing);
>> +       if (host->cached_ios.enhanced_strobe) {
>> +               host->ios.enhanced_strobe = true;
>> +               if (host->ops->hs400_enhanced_strobe)
>> +                       host->ops->hs400_enhanced_strobe(host, &host->ios);
>> +       }
>> +       mmc_set_clock(host, host->cached_ios.clock);
>> +       mmc_set_bus_mode(host, host->cached_ios.bus_mode);
>> +
> 
> Rather than re-using the above APIs and the ->set_ios() callback in
> the host, I believe it would be better to add a new host ops to manage
> all of the above at once instead. Something along the lines of the
> below, would then replace all of the above.
> 
> host->ops->restore_ios(host, &host->cached_ios)
> memcpy(&host->ios, &host->cached_ios, sizeof(host->ios));
> 
> Would that make sense to you too?
> 


I didn't get this completely. Do you mean that we should implement a new 
restore_ios callback (e.g. sdhci_restore_ios) similar to sdhci_set_ios 
and removing all the redundant code from sdhci_set_ios which should 
achieve the behaviour same as calling all the above mmc_set_* API's ?


>> +       if (!mmc_card_hs400es(card) &&
>> +                       (mmc_card_hs200(card) || mmc_card_hs400(card))) {
>> +               err = mmc_execute_tuning(card);
>> +               if (err) {
>> +                       pr_err("%s: %s: Tuning failed (%d)\n",
>> +                               mmc_hostname(host), __func__, err);
> 
> There is already a print being done in mmc_execute_tuning() at
> failure. So, let's drop the above print.
> 

Sure will take care in V4.

>> +                       goto out;
>> +               }
>> +       }
>> +
>> +       err = mmc_test_awake_ext_csd(host);
> 
> Again, I don't get why this is needed, so let's discuss this more.
> 

This is just a safety check added because ext_csd has some W/E_P or
W/C_P registers which gets reset if any HW reset happens to the card.
So this will check for those cases if any other vendor is doing reset as 
part of suspend and compare a subset of those W/E_P and W/C_P registers 
and if they are changed then we will bail out of this partial init 
feature and go for full initialization.
We are also fine with removing this function but just added for the 
above mentioned case.

>> +       if (err)
>> +               pr_debug("%s: %s: fail on ext_csd read (%d)\n",
>> +                       mmc_hostname(host), __func__, err);
>> +out:
>> +       return err;
>> +}
>> +
>>   /*
>>    * Suspend callback
>>    */
>> @@ -2161,7 +2294,19 @@ static int _mmc_resume(struct mmc_host *host)
>>                  goto out;
>>
>>          mmc_power_up(host, host->card->ocr);
>> -       err = mmc_init_card(host, host->card->ocr, host->card);
>> +
>> +       if (mmc_can_sleepawake(host)) {
>> +               err = mmc_sleepawake(host, false);
>> +               if (!err)
>> +                       err = mmc_partial_init(host);
>> +               else
>> +                       pr_err("%s: %s: awake failed (%d), fallback to full init\n",
>> +                               mmc_hostname(host), __func__, err);
> 
> We don't print other errors during resume - and I don't think there is
> any special reason to do it for this case only. Please drop it.
> 

Sure will take care in V4.

>> +       }
>> +
>> +       if (!mmc_can_sleepawake(host) || err)
>> +               err = mmc_init_card(host, host->card->ocr, host->card);
>> +
>>          mmc_card_clr_suspended(host->card);
>>
>>   out:
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index daa2f40d9ce6..fbc832ec6d57 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -86,6 +86,8 @@ struct mmc_ext_csd {
>>          unsigned int            data_tag_unit_size;     /* DATA TAG UNIT size */
>>          unsigned int            boot_ro_lock;           /* ro lock support */
>>          bool                    boot_ro_lockable;
>> +       u8                      raw_ext_csd_cmdq;       /* 15 */
>> +       u8                      raw_ext_csd_cache_ctrl; /* 33 */
>>          bool                    ffu_capable;    /* Firmware upgrade support */
>>          bool                    cmdq_en;        /* Command Queue enabled */
>>          bool                    cmdq_support;   /* Command Queue supported */
>> @@ -96,7 +98,9 @@ struct mmc_ext_csd {
>>          u8                      raw_partition_support;  /* 160 */
>>          u8                      raw_rpmb_size_mult;     /* 168 */
>>          u8                      raw_erased_mem_count;   /* 181 */
>> +       u8                      raw_ext_csd_bus_width;  /* 183 */
>>          u8                      strobe_support;         /* 184 */
>> +       u8                      raw_ext_csd_hs_timing;  /* 185 */
>>          u8                      raw_ext_csd_structure;  /* 194 */
>>          u8                      raw_card_type;          /* 196 */
>>          u8                      raw_driver_strength;    /* 197 */
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 2f445c651742..836382a0b2e9 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -427,6 +427,7 @@ struct mmc_host {
>>   #define MMC_CAP2_CRYPTO                0
>>   #endif
>>   #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28)       /* Host with eMMC that has GPT entry at a non-standard location */
>> +#define MMC_CAP2_SLEEP_AWAKE   (1 << 29)       /* Use Sleep/Awake (CMD5) */
>>
>>          int                     fixed_drv_type; /* fixed driver type for non-removable media */
>>
>> @@ -445,6 +446,7 @@ struct mmc_host {
>>          spinlock_t              lock;           /* lock for claim and bus ops */
>>
>>          struct mmc_ios          ios;            /* current io bus settings */
>> +       struct mmc_ios          cached_ios;
>>
>>          /* group bitfields together to minimize padding */
>>          unsigned int            use_spi_crc:1;
> 
> Kind regards
> Uffe

Re: [PATCH V3 1/3] mmc: core: Add partial initialization support

[Ulf Hansson]

[...]

> >> +{
> >> +       int err = 0;
> >> +       struct mmc_card *card = host->card;
> >> +
> >> +       mmc_set_bus_width(host, host->cached_ios.bus_width);
> >> +       mmc_set_timing(host, host->cached_ios.timing);
> >> +       if (host->cached_ios.enhanced_strobe) {
> >> +               host->ios.enhanced_strobe = true;
> >> +               if (host->ops->hs400_enhanced_strobe)
> >> +                       host->ops->hs400_enhanced_strobe(host, &host->ios);
> >> +       }
> >> +       mmc_set_clock(host, host->cached_ios.clock);
> >> +       mmc_set_bus_mode(host, host->cached_ios.bus_mode);
> >> +
> >
> > Rather than re-using the above APIs and the ->set_ios() callback in
> > the host, I believe it would be better to add a new host ops to manage
> > all of the above at once instead. Something along the lines of the
> > below, would then replace all of the above.
> >
> > host->ops->restore_ios(host, &host->cached_ios)
> > memcpy(&host->ios, &host->cached_ios, sizeof(host->ios));
> >
> > Would that make sense to you too?
> >
>
>
> I didn't get this completely. Do you mean that we should implement a new
> restore_ios callback (e.g. sdhci_restore_ios) similar to sdhci_set_ios
> and removing all the redundant code from sdhci_set_ios which should
> achieve the behaviour same as calling all the above mmc_set_* API's ?

Correct. Would it not simply the things in the driver too?

>
>
> >> +       if (!mmc_card_hs400es(card) &&
> >> +                       (mmc_card_hs200(card) || mmc_card_hs400(card))) {
> >> +               err = mmc_execute_tuning(card);
> >> +               if (err) {
> >> +                       pr_err("%s: %s: Tuning failed (%d)\n",
> >> +                               mmc_hostname(host), __func__, err);
> >
> > There is already a print being done in mmc_execute_tuning() at
> > failure. So, let's drop the above print.
> >
>
> Sure will take care in V4.
>
> >> +                       goto out;
> >> +               }
> >> +       }
> >> +
> >> +       err = mmc_test_awake_ext_csd(host);
> >
> > Again, I don't get why this is needed, so let's discuss this more.
> >
>
> This is just a safety check added because ext_csd has some W/E_P or
> W/C_P registers which gets reset if any HW reset happens to the card.
> So this will check for those cases if any other vendor is doing reset as
> part of suspend and compare a subset of those W/E_P and W/C_P registers
> and if they are changed then we will bail out of this partial init
> feature and go for full initialization.
> We are also fine with removing this function but just added for the
> above mentioned case.

In that case, I would rather remove it as I think it's superfluous.

More precisely, I would expect that we fail to wake up the card with a
CMD5 (get an error response for the CMD) if there has been a HW reset
somewhere done before.

Another reason to *not* read the ext_csd would be to further improve
the resume time, as reading it takes time too. I would be curious to
know how much though. :-)

[...]

Kind regards
Uffe

Re: [PATCH V3 1/3] mmc: core: Add partial initialization support

[Sarthak Garg]

On 10/27/2023 3:23 PM, Ulf Hansson wrote:
> [...]
> 
>>>> +{
>>>> +       int err = 0;
>>>> +       struct mmc_card *card = host->card;
>>>> +
>>>> +       mmc_set_bus_width(host, host->cached_ios.bus_width);
>>>> +       mmc_set_timing(host, host->cached_ios.timing);
>>>> +       if (host->cached_ios.enhanced_strobe) {
>>>> +               host->ios.enhanced_strobe = true;
>>>> +               if (host->ops->hs400_enhanced_strobe)
>>>> +                       host->ops->hs400_enhanced_strobe(host, &host->ios);
>>>> +       }
>>>> +       mmc_set_clock(host, host->cached_ios.clock);
>>>> +       mmc_set_bus_mode(host, host->cached_ios.bus_mode);
>>>> +
>>>
>>> Rather than re-using the above APIs and the ->set_ios() callback in
>>> the host, I believe it would be better to add a new host ops to manage
>>> all of the above at once instead. Something along the lines of the
>>> below, would then replace all of the above.
>>>
>>> host->ops->restore_ios(host, &host->cached_ios)
>>> memcpy(&host->ios, &host->cached_ios, sizeof(host->ios));
>>>
>>> Would that make sense to you too?
>>>
>>
>>
>> I didn't get this completely. Do you mean that we should implement a new
>> restore_ios callback (e.g. sdhci_restore_ios) similar to sdhci_set_ios
>> and removing all the redundant code from sdhci_set_ios which should
>> achieve the behaviour same as calling all the above mmc_set_* API's ?
> 
> Correct. Would it not simply the things in the driver too?
> 
>>
>>
>>>> +       if (!mmc_card_hs400es(card) &&
>>>> +                       (mmc_card_hs200(card) || mmc_card_hs400(card))) {
>>>> +               err = mmc_execute_tuning(card);
>>>> +               if (err) {
>>>> +                       pr_err("%s: %s: Tuning failed (%d)\n",
>>>> +                               mmc_hostname(host), __func__, err);
>>>
>>> There is already a print being done in mmc_execute_tuning() at
>>> failure. So, let's drop the above print.
>>>
>>
>> Sure will take care in V4.
>>
>>>> +                       goto out;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       err = mmc_test_awake_ext_csd(host);
>>>
>>> Again, I don't get why this is needed, so let's discuss this more.
>>>
>>
>> This is just a safety check added because ext_csd has some W/E_P or
>> W/C_P registers which gets reset if any HW reset happens to the card.
>> So this will check for those cases if any other vendor is doing reset as
>> part of suspend and compare a subset of those W/E_P and W/C_P registers
>> and if they are changed then we will bail out of this partial init
>> feature and go for full initialization.
>> We are also fine with removing this function but just added for the
>> above mentioned case.
> 
> In that case, I would rather remove it as I think it's superfluous.
> 
> More precisely, I would expect that we fail to wake up the card with a
> CMD5 (get an error response for the CMD) if there has been a HW reset
> somewhere done before.
> 
> Another reason to *not* read the ext_csd would be to further improve
> the resume time, as reading it takes time too. I would be curious to
> know how much though. :-)
> 
> [...]
> 
> Kind regards
> Uffe

Hi ulf,

Sorry for the delay but we are seeing some stability issues when testing 
this feature with HS400 cards which I am debugging and may take some 
time and will come back.
Note: This feature is working perfectly fine with HS400ES cards.

Re: [PATCH V3 0/3] mmc: Add partial initialization support

[Sarthak Garg]

On 10/17/2023 5:09 PM, Ulf Hansson wrote:
> On Tue, 17 Oct 2023 at 08:13, Sarthak Garg <quic_sartgarg@quicinc.com> wrote:
>>
>> Add the ability to partially initialize the MMC device by
>> using device sleep/awake sequence (CMD5).
>> Device will be sent to sleep state during mmc runtime/system suspend
>> and will be woken up during mmc runtime/system resume.
>> By using this sequence the device doesn't need full initialization
>> which gives 25% time reduction in system/runtime resume path.
>> Also enable this feature along with mmc runtime PM for qualcomm
>> controllers.
>>
>> Sarthak Garg (3):
>>    mmc: core: Add partial initialization support
>>    mmc: sdhci-msm: Enable MMC_CAP_AGGRESSIVE_PM for Qualcomm controllers
>>    mmc: sdhci-msm: Enable MMC_CAP2_SLEEP_AWAKE for Qualcomm controllers
>>
>>   drivers/mmc/core/mmc.c       | 163 +++++++++++++++++++++++++++++++++--
>>   drivers/mmc/host/sdhci-msm.c |   2 +
>>   include/linux/mmc/card.h     |   4 +
>>   include/linux/mmc/host.h     |   2 +
>>   4 files changed, 162 insertions(+), 9 deletions(-)
> 
> Would mind resending this version and while doing that, please add
> some version information to each patch in the series. This helps while
> reviewing.
> 
> Kind regards
> Uffe

Sure will repost with the version history.

Thanks,
Sarthak

Re: [PATCH V3 0/3] mmc: Add partial initialization support

[Ulf Hansson]

On Tue, 17 Oct 2023 at 08:13, Sarthak Garg <quic_sartgarg@quicinc.com> wrote:
>
> Add the ability to partially initialize the MMC device by
> using device sleep/awake sequence (CMD5).
> Device will be sent to sleep state during mmc runtime/system suspend
> and will be woken up during mmc runtime/system resume.
> By using this sequence the device doesn't need full initialization
> which gives 25% time reduction in system/runtime resume path.
> Also enable this feature along with mmc runtime PM for qualcomm
> controllers.
>
> Sarthak Garg (3):
>   mmc: core: Add partial initialization support
>   mmc: sdhci-msm: Enable MMC_CAP_AGGRESSIVE_PM for Qualcomm controllers
>   mmc: sdhci-msm: Enable MMC_CAP2_SLEEP_AWAKE for Qualcomm controllers
>
>  drivers/mmc/core/mmc.c       | 163 +++++++++++++++++++++++++++++++++--
>  drivers/mmc/host/sdhci-msm.c |   2 +
>  include/linux/mmc/card.h     |   4 +
>  include/linux/mmc/host.h     |   2 +
>  4 files changed, 162 insertions(+), 9 deletions(-)

Would mind resending this version and while doing that, please add
some version information to each patch in the series. This helps while
reviewing.

Kind regards
Uffe

[PATCH V3 0/3] mmc: Add partial initialization support

[Sarthak Garg]

Add the ability to partially initialize the MMC device by
using device sleep/awake sequence (CMD5).
Device will be sent to sleep state during mmc runtime/system suspend
and will be woken up during mmc runtime/system resume.
By using this sequence the device doesn't need full initialization
which gives 25% time reduction in system/runtime resume path.
Also enable this feature along with mmc runtime PM for qualcomm
controllers.

Sarthak Garg (3):
  mmc: core: Add partial initialization support
  mmc: sdhci-msm: Enable MMC_CAP_AGGRESSIVE_PM for Qualcomm controllers
  mmc: sdhci-msm: Enable MMC_CAP2_SLEEP_AWAKE for Qualcomm controllers

 drivers/mmc/core/mmc.c       | 163 +++++++++++++++++++++++++++++++++--
 drivers/mmc/host/sdhci-msm.c |   2 +
 include/linux/mmc/card.h     |   4 +
 include/linux/mmc/host.h     |   2 +
 4 files changed, 162 insertions(+), 9 deletions(-)

-- 
2.17.1