Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231019054612.9192-1-quic_sartgarg@quicinc.com> <20231019054612.9192-2-quic_sartgarg@quicinc.com>
In-Reply-To: <20231019054612.9192-2-quic_sartgarg@quicinc.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 19 Oct 2023 17:00:30 +0200
Message-ID: <CAPDyKFofS16AsQeTVNiDi_PHUatGoQ3no-1+Azo+yqG0SPTe4Q@mail.gmail.com>
Subject: Re: [PATCH V3 1/3] mmc: core: Add partial initialization support
To:     Sarthak Garg <quic_sartgarg@quicinc.com>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, quic_cang@quicinc.com,
        quic_nguyenb@quicinc.com, quic_rampraka@quicinc.com,
        quic_pragalla@quicinc.com, quic_sayalil@quicinc.com,
        quic_nitirawa@quicinc.com, quic_sachgupt@quicinc.com,
        quic_bhaskarv@quicinc.com, quic_narepall@quicinc.com,
        kernel@quicinc.com,
        Veerabhadrarao Badiganti <quic_vbadigan@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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