[PATCH 0/2] mmc: core: Implement support for cache ctrl for SD cards

[PATCH 0/2] mmc: core: Implement support for cache ctrl for SD cards

[Ulf Hansson]

In the SD spec v6.x the SD function extension registers for performance
enhancements were introduced. As a part of this an optional internal cache on
the SD card can be used to improve performance.

To let the SD card use the cache, the host needs to enable it and take care of
flushing of the cache. This series implement support for this.

Note that, there are no HW updates needed for the host to support this feature.
This has been tested on 64GB Sandisk Extreme PRO UHS-I A2 card.

The series is based upon another recently posted series [1] that added support
for poweroff notification.

Tests and reviews are of course greatly appreciated!

Kind regards
Ulf Hansson

[1]
https://patchwork.kernel.org/project/linux-mmc/list/?series=476933

Ulf Hansson (2):
  mmc: core: Move eMMC cache flushing to a new bus_ops callback
  mmc: core: Add support for cache ctrl for SD cards

 drivers/mmc/core/block.c   |  2 +-
 drivers/mmc/core/core.h    |  9 ++++
 drivers/mmc/core/mmc.c     | 25 +++++++++-
 drivers/mmc/core/mmc_ops.c | 22 +--------
 drivers/mmc/core/mmc_ops.h |  2 +-
 drivers/mmc/core/sd.c      | 98 ++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/card.h   |  1 +
 7 files changed, 134 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH 1/2] mmc: core: Move eMMC cache flushing to a new bus_ops callback

[Ulf Hansson]

To prepare to add internal cache management for SD cards, let's start by
moving the eMMC specific code into a new ->flush_cache() bus_ops callback.

In this way, it becomes straight forward to add the SD specific parts,
as subsequent changes are about to show.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/block.c   |  2 +-
 drivers/mmc/core/core.h    |  9 +++++++++
 drivers/mmc/core/mmc.c     | 25 +++++++++++++++++++++++--
 drivers/mmc/core/mmc_ops.c | 21 ---------------------
 drivers/mmc/core/mmc_ops.h |  1 -
 5 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 689eb9afeeed..d73d7be1af2f 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1159,7 +1159,7 @@ static void mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
 	struct mmc_card *card = md->queue.card;
 	int ret = 0;
 
-	ret = mmc_flush_cache(card);
+	ret = mmc_flush_cache(card->host);
 	blk_mq_end_request(req, ret ? BLK_STS_IOERR : BLK_STS_OK);
 }
 
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index db3c9c68875d..0c4de2030b3f 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -30,6 +30,7 @@ struct mmc_bus_ops {
 	int (*hw_reset)(struct mmc_host *);
 	int (*sw_reset)(struct mmc_host *);
 	bool (*cache_enabled)(struct mmc_host *);
+	int (*flush_cache)(struct mmc_host *);
 };
 
 void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
@@ -172,4 +173,12 @@ static inline bool mmc_cache_enabled(struct mmc_host *host)
 	return false;
 }
 
+static inline int mmc_flush_cache(struct mmc_host *host)
+{
+	if (host->bus_ops->flush_cache)
+		return host->bus_ops->flush_cache(host);
+
+	return 0;
+}
+
 #endif
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 13074aa1f605..838726b68ff3 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -28,6 +28,7 @@
 
 #define DEFAULT_CMD6_TIMEOUT_MS	500
 #define MIN_CACHE_EN_TIMEOUT_MS 1600
+#define CACHE_FLUSH_TIMEOUT_MS 30000 /* 30s */
 
 static const unsigned int tran_exp[] = {
 	10000,		100000,		1000000,	10000000,
@@ -2036,6 +2037,25 @@ static bool _mmc_cache_enabled(struct mmc_host *host)
 	       host->card->ext_csd.cache_ctrl & 1;
 }
 
+/*
+ * Flush the internal cache of the eMMC to non-volatile storage.
+ */
+static int _mmc_flush_cache(struct mmc_host *host)
+{
+	int err = 0;
+
+	if (_mmc_cache_enabled(host)) {
+		err = mmc_switch(host->card, EXT_CSD_CMD_SET_NORMAL,
+				 EXT_CSD_FLUSH_CACHE, 1,
+				 CACHE_FLUSH_TIMEOUT_MS);
+		if (err)
+			pr_err("%s: cache flush error %d\n",
+			       mmc_hostname(host), err);
+	}
+
+	return err;
+}
+
 static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 {
 	int err = 0;
@@ -2047,7 +2067,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 	if (mmc_card_suspended(host->card))
 		goto out;
 
-	err = mmc_flush_cache(host->card);
+	err = _mmc_flush_cache(host);
 	if (err)
 		goto out;
 
@@ -2188,7 +2208,7 @@ static int _mmc_hw_reset(struct mmc_host *host)
 	 * In the case of recovery, we can't expect flushing the cache to work
 	 * always, but we have a go and ignore errors.
 	 */
-	mmc_flush_cache(host->card);
+	_mmc_flush_cache(host);
 
 	if ((host->caps & MMC_CAP_HW_RESET) && host->ops->hw_reset &&
 	     mmc_can_reset(card)) {
@@ -2216,6 +2236,7 @@ static const struct mmc_bus_ops mmc_ops = {
 	.shutdown = mmc_shutdown,
 	.hw_reset = _mmc_hw_reset,
 	.cache_enabled = _mmc_cache_enabled,
+	.flush_cache = _mmc_flush_cache,
 };
 
 /*
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index b1da8f1950ee..af423acc4c88 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -20,7 +20,6 @@
 #include "mmc_ops.h"
 
 #define MMC_BKOPS_TIMEOUT_MS		(120 * 1000) /* 120s */
-#define MMC_CACHE_FLUSH_TIMEOUT_MS	(30 * 1000) /* 30s */
 #define MMC_SANITIZE_TIMEOUT_MS		(240 * 1000) /* 240s */
 
 static const u8 tuning_blk_pattern_4bit[] = {
@@ -964,26 +963,6 @@ void mmc_run_bkops(struct mmc_card *card)
 }
 EXPORT_SYMBOL(mmc_run_bkops);
 
-/*
- * Flush the cache to the non-volatile storage.
- */
-int mmc_flush_cache(struct mmc_card *card)
-{
-	int err = 0;
-
-	if (mmc_cache_enabled(card->host)) {
-		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-				 EXT_CSD_FLUSH_CACHE, 1,
-				 MMC_CACHE_FLUSH_TIMEOUT_MS);
-		if (err)
-			pr_err("%s: cache flush error %d\n",
-					mmc_hostname(card->host), err);
-	}
-
-	return err;
-}
-EXPORT_SYMBOL(mmc_flush_cache);
-
 static int mmc_cmdq_switch(struct mmc_card *card, bool enable)
 {
 	u8 val = enable ? EXT_CSD_CMDQ_MODE_ENABLED : 0;
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 2b1d730e56bf..c3c1d9c2577e 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -51,7 +51,6 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms);
 void mmc_run_bkops(struct mmc_card *card);
-int mmc_flush_cache(struct mmc_card *card);
 int mmc_cmdq_enable(struct mmc_card *card);
 int mmc_cmdq_disable(struct mmc_card *card);
 int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms);
-- 
2.25.1

[PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards

[Ulf Hansson]

In SD spec v6.x the SD function extension registers for performance
enhancements were introduced. As a part of this an optional internal cache
on the SD card, can be used to improve performance.

The let the SD card use the cache, the host needs to enable it and manage
flushing of the cache, so let's add support for this.

Note that for an SD card supporting the cache it's mandatory for it, to
also support the poweroff notification feature. According to the SD spec,
if the cache has been enabled and a poweroff notification is sent to the
card, that implicitly also means that the card should flush its internal
cache. Therefore, dealing with cache flushing for REQ_OP_FLUSH block
requests is sufficient.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c |  1 +
 drivers/mmc/core/mmc_ops.h |  1 +
 drivers/mmc/core/sd.c      | 98 ++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/card.h   |  1 +
 4 files changed, 101 insertions(+)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index af423acc4c88..3c58f6d0f482 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -456,6 +456,7 @@ static int mmc_busy_cb(void *cb_data, bool *busy)
 		err = R1_STATUS(status) ? -EIO : 0;
 		break;
 	case MMC_BUSY_HPI:
+	case MMC_BUSY_EXTR_SINGLE:
 		break;
 	default:
 		err = -EINVAL;
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index c3c1d9c2577e..41ab4f573a31 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -14,6 +14,7 @@ enum mmc_busy_cmd {
 	MMC_BUSY_CMD6,
 	MMC_BUSY_ERASE,
 	MMC_BUSY_HPI,
+	MMC_BUSY_EXTR_SINGLE,
 };
 
 struct mmc_host;
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 760aa86bd54d..773444853607 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -67,6 +67,7 @@ static const unsigned int sd_au_size[] = {
 	})
 
 #define SD_POWEROFF_NOTIFY_TIMEOUT_MS 2000
+#define SD_WRITE_EXTR_SINGLE_TIMEOUT_MS 1000
 
 struct sd_busy_data {
 	struct mmc_card *card;
@@ -1287,6 +1288,94 @@ static int sd_read_ext_regs(struct mmc_card *card)
 	return err;
 }
 
+static bool sd_cache_enabled(struct mmc_host *host)
+{
+	return host->card->ext_perf.feature_enabled & SD_EXT_PERF_CACHE;
+}
+
+static int sd_flush_cache(struct mmc_host *host)
+{
+	struct mmc_card *card = host->card;
+	u8 *reg_buf, fno, page;
+	u16 offset;
+	int err;
+
+	if (!sd_cache_enabled(host))
+		return 0;
+
+	reg_buf = kzalloc(512, GFP_KERNEL);
+	if (!reg_buf)
+		return -ENOMEM;
+
+	/*
+	 * Set the Flush Cache bit in the performance enhancement register at
+	 * 261 bytes offset.
+	 */
+	fno = card->ext_perf.fno;
+	page = card->ext_perf.page;
+	offset = card->ext_perf.offset + 261;
+
+	err = sd_write_ext_reg(card, fno, page, offset, 0x1);
+	if (err) {
+		pr_warn("%s: error %d writing Cache Flush bit\n",
+			mmc_hostname(host), err);
+		goto out;
+	}
+
+	err = mmc_poll_for_busy(card, SD_WRITE_EXTR_SINGLE_TIMEOUT_MS, false,
+				MMC_BUSY_EXTR_SINGLE);
+	if (err)
+		goto out;
+
+	/*
+	 * Read the Flush Cache bit. The card shall reset it, to confirm that
+	 * it's has completed the flushing of the cache.
+	 */
+	err = sd_read_ext_reg(card, fno, page, offset, 1, reg_buf);
+	if (err) {
+		pr_warn("%s: error %d reading Cache Flush bit\n",
+			mmc_hostname(host), err);
+		goto out;
+	}
+
+	if (reg_buf[0] & 0x1)
+		err = -ETIMEDOUT;
+out:
+	kfree(reg_buf);
+	return err;
+}
+
+static int sd_enable_cache(struct mmc_card *card)
+{
+	u8 *reg_buf;
+	int err;
+
+	reg_buf = kzalloc(512, GFP_KERNEL);
+	if (!reg_buf)
+		return -ENOMEM;
+
+	/*
+	 * Set the Cache Enable bit in the performance enhancement register at
+	 * 260 bytes offset.
+	 */
+	err = sd_write_ext_reg(card, card->ext_perf.fno, card->ext_perf.page,
+			       card->ext_perf.offset + 260, 0x1);
+	if (err) {
+		pr_warn("%s: error %d writing Cache Enable bit\n",
+			mmc_hostname(card->host), err);
+		goto out;
+	}
+
+	err = mmc_poll_for_busy(card, SD_WRITE_EXTR_SINGLE_TIMEOUT_MS, false,
+				MMC_BUSY_EXTR_SINGLE);
+	if (!err)
+		card->ext_perf.feature_enabled |= SD_EXT_PERF_CACHE;
+
+out:
+	kfree(reg_buf);
+	return err;
+}
+
 /*
  * Handle the detection and initialisation of a card.
  *
@@ -1442,6 +1531,13 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 			goto free_card;
 	}
 
+	/* Enable internal SD cache if supported. */
+	if (card->ext_perf.feature_support & SD_EXT_PERF_CACHE) {
+		err = sd_enable_cache(card);
+		if (err)
+			goto free_card;
+	}
+
 	if (host->cqe_ops && !host->cqe_enabled) {
 		err = host->cqe_ops->cqe_enable(host, card);
 		if (!err) {
@@ -1694,6 +1790,8 @@ static const struct mmc_bus_ops mmc_sd_ops = {
 	.alive = mmc_sd_alive,
 	.shutdown = mmc_sd_suspend,
 	.hw_reset = mmc_sd_hw_reset,
+	.cache_enabled = sd_cache_enabled,
+	.flush_cache = sd_flush_cache,
 };
 
 /*
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 2867af0635f8..74e6c0624d27 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -196,6 +196,7 @@ struct sd_ext_reg {
 	u8			page;
 	u16			offset;
 	u8			rev;
+	u8			feature_enabled;
 	u8			feature_support;
 /* Power Management Function. */
 #define SD_EXT_POWER_OFF_NOTIFY	(1<<0)
-- 
2.25.1

RE: [PATCH 1/2] mmc: core: Move eMMC cache flushing to a new bus_ops callback

[Avri Altman]

> 
> To prepare to add internal cache management for SD cards, let's start by
> moving the eMMC specific code into a new ->flush_cache() bus_ops
> callback.
> 
> In this way, it becomes straight forward to add the SD specific parts,
> as subsequent changes are about to show.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Re: [PATCH 1/2] mmc: core: Move eMMC cache flushing to a new bus_ops callback

[Linus Walleij]

On Thu, May 6, 2021 at 4:58 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> To prepare to add internal cache management for SD cards, let's start by
> moving the eMMC specific code into a new ->flush_cache() bus_ops callback.
>
> In this way, it becomes straight forward to add the SD specific parts,
> as subsequent changes are about to show.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards

[Linus Walleij]

On Thu, May 6, 2021 at 4:58 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> In SD spec v6.x the SD function extension registers for performance
> enhancements were introduced. As a part of this an optional internal cache
> on the SD card, can be used to improve performance.
>
> The let the SD card use the cache, the host needs to enable it and manage
> flushing of the cache, so let's add support for this.
>
> Note that for an SD card supporting the cache it's mandatory for it, to
> also support the poweroff notification feature. According to the SD spec,
> if the cache has been enabled and a poweroff notification is sent to the
> card, that implicitly also means that the card should flush its internal
> cache. Therefore, dealing with cache flushing for REQ_OP_FLUSH block
> requests is sufficient.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
(...)

> +       /*
> +        * Set the Flush Cache bit in the performance enhancement register at
> +        * 261 bytes offset.
> +        */
> +       fno = card->ext_perf.fno;
> +       page = card->ext_perf.page;
> +       offset = card->ext_perf.offset + 261;

261 looks a bit magic, can we add a define of some sort?
I guess it has a name in the spec?

> +       err = sd_write_ext_reg(card, fno, page, offset, 0x1);
> +       if (err) {
> +               pr_warn("%s: error %d writing Cache Flush bit\n",
> +                       mmc_hostname(host), err);
> +               goto out;
> +       }

So this offset contains a single bit.

> +       if (reg_buf[0] & 0x1)
> +               err = -ETIMEDOUT;

And that same bit is checked here.

Is it always going to be one bit only or do we want to

#include <linux/bits.h>
#define SD_CACHE_FLUSH_FLAG BIT(0)

Does it have a name in the spec we can use?

> +       /*
> +        * Set the Cache Enable bit in the performance enhancement register at
> +        * 260 bytes offset.
> +        */
> +       err = sd_write_ext_reg(card, card->ext_perf.fno, card->ext_perf.page,
> +                              card->ext_perf.offset + 260, 0x1);

Same here we want to #define 260 to something symbolic,

And here some define for BIT(0) as well. At least with BIT(0)
in the call to sd_write_ext_reg() rather than 0x1 if I can say
something.

With the above nitpicking fixed up (I trust you):
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

RE: [PATCH 0/2] mmc: core: Implement support for cache ctrl for SD cards

[Avri Altman]

> In the SD spec v6.x the SD function extension registers for performance
> enhancements were introduced. As a part of this an optional internal cache
> on
> the SD card can be used to improve performance.
Just to verify that you are aware of that:
In addition to the extension Registers that were defined in the physical core spec,
there was a separate document that was released that suggest a common OS API's to those registers.
It is called "SD specification part A5, SD Extensions API specification".

Thanks,
Avri

RE: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards

[Avri Altman]

> +static int sd_enable_cache(struct mmc_card *card)
> +{
> +       u8 *reg_buf;
> +       int err;
> +
> +       reg_buf = kzalloc(512, GFP_KERNEL);
> +       if (!reg_buf)
> +               return -ENOMEM;
> +
> +       /*
> +        * Set the Cache Enable bit in the performance enhancement register at
> +        * 260 bytes offset.
> +        */
> +       err = sd_write_ext_reg(card, card->ext_perf.fno, card->ext_perf.page,
> +                              card->ext_perf.offset + 260, 0x1);
> +       if (err) {
> +               pr_warn("%s: error %d writing Cache Enable bit\n",
> +                       mmc_hostname(card->host), err);
> +               goto out;
> +       }
> +
> +       err = mmc_poll_for_busy(card,
> SD_WRITE_EXTR_SINGLE_TIMEOUT_MS, false,
> +                               MMC_BUSY_EXTR_SINGLE);
I think 1sec is for flush cache, but I guess it makes sense to use it here as well.

> +       if (!err)
> +               card->ext_perf.feature_enabled |= SD_EXT_PERF_CACHE;
Maybe 
If (err)
    card->ext_perf.feature_enabled &= ~SD_EXT_PERF_CACHE;

and move to out: to catch the sd_write_ext_reg err ?

> +
> +out:
> +       kfree(reg_buf);
> +       return err;
> +}
> +
>  /*
>   * Handle the detection and initialisation of a card.
>   *
> @@ -1442,6 +1531,13 @@ static int mmc_sd_init_card(struct mmc_host
> *host, u32 ocr,
>                         goto free_card;
>         }
> 
> +       /* Enable internal SD cache if supported. */
> +       if (card->ext_perf.feature_support & SD_EXT_PERF_CACHE) {
> +               err = sd_enable_cache(card);
> +               if (err)
> +                       goto free_card;
If cache enablement failed, is it worthwhile to bail out?
Maybe disabling the cache with the appropriate message is enough?

> +       }
> +
>         if (host->cqe_ops && !host->cqe_enabled) {
>                 err = host->cqe_ops->cqe_enable(host, card);
>                 if (!err) {
> @@ -1694,6 +1790,8 @@ static const struct mmc_bus_ops mmc_sd_ops = {
>         .alive = mmc_sd_alive,
>         .shutdown = mmc_sd_suspend,
>         .hw_reset = mmc_sd_hw_reset,
> +       .cache_enabled = sd_cache_enabled,
> +       .flush_cache = sd_flush_cache,
>  };

I would expect 2 more patches in this series:
 - flush cache on power down
 - cache disablement events?

Thanks,
Avri

Re: [PATCH 0/2] mmc: core: Implement support for cache ctrl for SD cards

[Ulf Hansson]

On Mon, 10 May 2021 at 09:40, Avri Altman <Avri.Altman@wdc.com> wrote:
>
> > In the SD spec v6.x the SD function extension registers for performance
> > enhancements were introduced. As a part of this an optional internal cache
> > on
> > the SD card can be used to improve performance.
> Just to verify that you are aware of that:
> In addition to the extension Registers that were defined in the physical core spec,
> there was a separate document that was released that suggest a common OS API's to those registers.
> It is called "SD specification part A5, SD Extensions API specification".

Thanks for the pointer!

I did have a very brief look at this. The conclusion I made is that
this is way over designed to enable support for features like cache
and poweroff notification. However, there may be other use cases that
could benefit from a user space library, along the lines of the SD
Extensions API, but I can't really tell.

Do you have an interest around this that you can share?

Thanks for reviewing!

Kind regards
Uffe

RE: [PATCH 0/2] mmc: core: Implement support for cache ctrl for SD cards

[Avri Altman]

> On Mon, 10 May 2021 at 09:40, Avri Altman <Avri.Altman@wdc.com> wrote:
> >
> > > In the SD spec v6.x the SD function extension registers for performance
> > > enhancements were introduced. As a part of this an optional internal
> cache
> > > on
> > > the SD card can be used to improve performance.
> > Just to verify that you are aware of that:
> > In addition to the extension Registers that were defined in the physical
> core spec,
> > there was a separate document that was released that suggest a common
> OS API's to those registers.
> > It is called "SD specification part A5, SD Extensions API specification".
> 
> Thanks for the pointer!
> 
> I did have a very brief look at this. The conclusion I made is that
> this is way over designed to enable support for features like cache
> and poweroff notification. However, there may be other use cases that
> could benefit from a user space library, along the lines of the SD
> Extensions API, but I can't really tell.
> 
> Do you have an interest around this that you can share?
Not at the moment. Still thinking about this.

Thanks,
Avri

> 
> Thanks for reviewing!
> 
> Kind regards
> Uffe

RE: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards

[Avri Altman]

> > +       if (!err)
> > +               card->ext_perf.feature_enabled |= SD_EXT_PERF_CACHE;
> Maybe
> If (err)
>     card->ext_perf.feature_enabled &= ~SD_EXT_PERF_CACHE;
> 
> and move to out: to catch the sd_write_ext_reg err ?
Please ignore - got mixed up with card->ext_perf.feature_support...

Sorry,
Avri

> 
> > +
> > +out:
> > +       kfree(reg_buf);
> > +       return err;
> > +}
> > +
> >  /*
> >   * Handle the detection and initialisation of a card.
> >   *
> > @@ -1442,6 +1531,13 @@ static int mmc_sd_init_card(struct mmc_host
> > *host, u32 ocr,
> >                         goto free_card;
> >         }
> >
> > +       /* Enable internal SD cache if supported. */
> > +       if (card->ext_perf.feature_support & SD_EXT_PERF_CACHE) {
> > +               err = sd_enable_cache(card);
> > +               if (err)
> > +                       goto free_card;
> If cache enablement failed, is it worthwhile to bail out?
> Maybe disabling the cache with the appropriate message is enough?
> 
> > +       }
> > +
> >         if (host->cqe_ops && !host->cqe_enabled) {
> >                 err = host->cqe_ops->cqe_enable(host, card);
> >                 if (!err) {
> > @@ -1694,6 +1790,8 @@ static const struct mmc_bus_ops mmc_sd_ops =
> {
> >         .alive = mmc_sd_alive,
> >         .shutdown = mmc_sd_suspend,
> >         .hw_reset = mmc_sd_hw_reset,
> > +       .cache_enabled = sd_cache_enabled,
> > +       .flush_cache = sd_flush_cache,
> >  };
> 
> I would expect 2 more patches in this series:
>  - flush cache on power down
>  - cache disablement events?
> 
> Thanks,
> Avri

Re: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards

[Ulf Hansson]

On Sun, 9 May 2021 at 21:01, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, May 6, 2021 at 4:58 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> > In SD spec v6.x the SD function extension registers for performance
> > enhancements were introduced. As a part of this an optional internal cache
> > on the SD card, can be used to improve performance.
> >
> > The let the SD card use the cache, the host needs to enable it and manage
> > flushing of the cache, so let's add support for this.
> >
> > Note that for an SD card supporting the cache it's mandatory for it, to
> > also support the poweroff notification feature. According to the SD spec,
> > if the cache has been enabled and a poweroff notification is sent to the
> > card, that implicitly also means that the card should flush its internal
> > cache. Therefore, dealing with cache flushing for REQ_OP_FLUSH block
> > requests is sufficient.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> (...)
>
> > +       /*
> > +        * Set the Flush Cache bit in the performance enhancement register at
> > +        * 261 bytes offset.
> > +        */
> > +       fno = card->ext_perf.fno;
> > +       page = card->ext_perf.page;
> > +       offset = card->ext_perf.offset + 261;
>
> 261 looks a bit magic, can we add a define of some sort?

We could, but I am not sure it really improves things. At least it
would not be consistent with the way we treat other magic numbers.

I think it's better to look into this as wider cleanup instead.

> I guess it has a name in the spec?

It's called the "Power Management Setting Register".

>
> > +       err = sd_write_ext_reg(card, fno, page, offset, 0x1);
> > +       if (err) {
> > +               pr_warn("%s: error %d writing Cache Flush bit\n",
> > +                       mmc_hostname(host), err);
> > +               goto out;
> > +       }
>
> So this offset contains a single bit.
>
> > +       if (reg_buf[0] & 0x1)
> > +               err = -ETIMEDOUT;
>
> And that same bit is checked here.

Correct.

>
> Is it always going to be one bit only or do we want to
>
> #include <linux/bits.h>
> #define SD_CACHE_FLUSH_FLAG BIT(0)
>
> Does it have a name in the spec we can use?

Well, it just says "Cache Flush" bit.

It seems to be one bit always for these features. The remaining bits
in the same byte are unused/reserved.

Each feature has at least one dedicated byte, so there are no bytes
being shared between features.

>
> > +       /*
> > +        * Set the Cache Enable bit in the performance enhancement register at
> > +        * 260 bytes offset.
> > +        */
> > +       err = sd_write_ext_reg(card, card->ext_perf.fno, card->ext_perf.page,
> > +                              card->ext_perf.offset + 260, 0x1);
>
> Same here we want to #define 260 to something symbolic,
>
> And here some define for BIT(0) as well. At least with BIT(0)
> in the call to sd_write_ext_reg() rather than 0x1 if I can say
> something.

The conversion to BIT(0) in the argument is clearly an improvement. I
do that change when applying, but leave the defines for the other
magics to be considered as a future cleanup.

>
> With the above nitpicking fixed up (I trust you):
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

Kind regards
Uffe

Re: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards

[Ulf Hansson]

On Mon, 10 May 2021 at 11:10, Avri Altman <Avri.Altman@wdc.com> wrote:
>
> > +static int sd_enable_cache(struct mmc_card *card)
> > +{
> > +       u8 *reg_buf;
> > +       int err;
> > +
> > +       reg_buf = kzalloc(512, GFP_KERNEL);
> > +       if (!reg_buf)
> > +               return -ENOMEM;
> > +
> > +       /*
> > +        * Set the Cache Enable bit in the performance enhancement register at
> > +        * 260 bytes offset.
> > +        */
> > +       err = sd_write_ext_reg(card, card->ext_perf.fno, card->ext_perf.page,
> > +                              card->ext_perf.offset + 260, 0x1);
> > +       if (err) {
> > +               pr_warn("%s: error %d writing Cache Enable bit\n",
> > +                       mmc_hostname(card->host), err);
> > +               goto out;
> > +       }
> > +
> > +       err = mmc_poll_for_busy(card,
> > SD_WRITE_EXTR_SINGLE_TIMEOUT_MS, false,
> > +                               MMC_BUSY_EXTR_SINGLE);
> I think 1sec is for flush cache, but I guess it makes sense to use it here as well.

The spec talks about generic busy signaling time for CMD49 of one
second. That's why I added this here.

>
> > +       if (!err)
> > +               card->ext_perf.feature_enabled |= SD_EXT_PERF_CACHE;
> Maybe
> If (err)
>     card->ext_perf.feature_enabled &= ~SD_EXT_PERF_CACHE;
>
> and move to out: to catch the sd_write_ext_reg err ?
>
> > +
> > +out:
> > +       kfree(reg_buf);
> > +       return err;
> > +}
> > +
> >  /*
> >   * Handle the detection and initialisation of a card.
> >   *
> > @@ -1442,6 +1531,13 @@ static int mmc_sd_init_card(struct mmc_host
> > *host, u32 ocr,
> >                         goto free_card;
> >         }
> >
> > +       /* Enable internal SD cache if supported. */
> > +       if (card->ext_perf.feature_support & SD_EXT_PERF_CACHE) {
> > +               err = sd_enable_cache(card);
> > +               if (err)
> > +                       goto free_card;
> If cache enablement failed, is it worthwhile to bail out?
> Maybe disabling the cache with the appropriate message is enough?

Right, good point.

Let me also think about how we best reset the .feature_enabled field
after a power cycle. Theoretically we could fail to enable a feature
after the system has resumed, but then we would still have the
correspond bits set.

>
> > +       }
> > +
> >         if (host->cqe_ops && !host->cqe_enabled) {
> >                 err = host->cqe_ops->cqe_enable(host, card);
> >                 if (!err) {
> > @@ -1694,6 +1790,8 @@ static const struct mmc_bus_ops mmc_sd_ops = {
> >         .alive = mmc_sd_alive,
> >         .shutdown = mmc_sd_suspend,
> >         .hw_reset = mmc_sd_hw_reset,
> > +       .cache_enabled = sd_cache_enabled,
> > +       .flush_cache = sd_flush_cache,
> >  };
>
> I would expect 2 more patches in this series:
>  - flush cache on power down

According to the spec that should not be needed, because that should
be managed internally in the SD card when we send a poweroff
notification.

Did I get that wrong? Do you prefer to send a flush cache as well
before the poweroff notification?

>  - cache disablement events?

This I don't know about. Can you elaborate?

>
> Thanks,
> Avri

Kind regards
Uffe

RE: [PATCH 2/2] mmc: core: Add support for cache ctrl for SD cards

[Avri Altman]

> > I would expect 2 more patches in this series:
> >  - flush cache on power down
> 
> According to the spec that should not be needed, because that should
> be managed internally in the SD card when we send a poweroff
> notification.
> 
> Did I get that wrong? Do you prefer to send a flush cache as well
> before the poweroff notification?
> 
> >  - cache disablement events?
> 
> This I don't know about. Can you elaborate?
As for the above 2 questions - I asked internally - it may take a while for people to get back to me.
Let's leave it for now.

Thanks,
Avri

> 
> >
> > Thanks,
> > Avri
> 
> Kind regards
> Uffe