[PATCH v2 0/4] mmc: introduce multi-block read gap tuning

[PATCH v2 0/4] mmc: introduce multi-block read gap tuning

[Benoît Monin]

This patchset implements the multi-block read gap tuning for the SDHCI 
cadence driver. To do so, a generic function (mmc_read_blocks) to read 
blocks from the MMC is added to mmc_ops in the first patch.

The second patch is the implementation proper in the driver which try 
to do a multi-block read and increase the gap delay until it succeeds.
The read gap is done by the controller when its internal fifo is full
when reading multiple blocks. That is why we cannot use mmc_get_ext_csd
to do the tuning since it only read one block.

The implementation of the mmc_read_blocks also brings a helper to check 
for CMD23 support by MMC card, similar to mmc_host_can_cmd23 for host,
so the last two patches uses it instead of local implementation in
mmc_test.c and block.c.

v1 -> v2:
Split the code between the core and the driver by adding a generic 
function to read blocks from the MMC.

Link to v1:
https://lore.kernel.org/linux-mmc/2a43386ffef4012530ca2421ad81ad21c36c8a25.1750943549.git.benoit.monin@bootlin.com/

Benoît Monin (4):
  mmc: core: add mmc_read_blocks to mmc_ops
  mmc: sdhci-cadence: implement multi-block read gap tuning
  mmc: mmc_test: use mmc_card_can_cmd23
  mmc: block: use mmc_card_can_cmd23

 drivers/mmc/core/block.c         | 12 ++----
 drivers/mmc/core/card.h          | 10 +++++
 drivers/mmc/core/mmc_ops.c       | 69 ++++++++++++++++++++++++++++++
 drivers/mmc/core/mmc_test.c      |  9 +---
 drivers/mmc/host/sdhci-cadence.c | 72 +++++++++++++++++++++++++++++++-
 include/linux/mmc/host.h         |  3 ++
 6 files changed, 157 insertions(+), 18 deletions(-)

[PATCH v2 1/4] mmc: core: add mmc_read_blocks to mmc_ops

[Benoît Monin]

Add a generic function to read some blocks of data from the MMC, to be
used by drivers as part of their tuning.

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
 drivers/mmc/core/card.h    | 10 ++++++
 drivers/mmc/core/mmc_ops.c | 69 ++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h   |  3 ++
 3 files changed, 82 insertions(+)

diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index 9cbdd240c3a7d..93fd502c1f5fc 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -11,6 +11,7 @@
 #define _MMC_CORE_CARD_H
 
 #include <linux/mmc/card.h>
+#include <linux/mmc/mmc.h>
 
 #define mmc_card_name(c)	((c)->cid.prod_name)
 #define mmc_card_id(c)		(dev_name(&(c)->dev))
@@ -300,4 +301,13 @@ static inline int mmc_card_no_uhs_ddr50_tuning(const struct mmc_card *c)
 	return c->quirks & MMC_QUIRK_NO_UHS_DDR50_TUNING;
 }
 
+static inline bool mmc_card_can_cmd23(struct mmc_card *card)
+{
+	return ((mmc_card_mmc(card) &&
+		 card->csd.mmca_vsn >= CSD_SPEC_VER_3) ||
+		(mmc_card_sd(card) && !mmc_card_ult_capacity(card) &&
+		 card->scr.cmds & SD_SCR_CMD23_SUPPORT)) &&
+		!(card->quirks & MMC_QUIRK_BLK_NO_CMD23);
+}
+
 #endif
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 66283825513cb..848d8aa3ff2b5 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -1077,3 +1077,72 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
 	return err;
 }
 EXPORT_SYMBOL_GPL(mmc_sanitize);
+
+/**
+ * mmc_read_blocks() - read data blocks from the mmc
+ * @card: mmc card to read from, can be NULL
+ * @host: mmc host doing the read
+ * @blksz: data block size
+ * @blocks: number of blocks to read
+ * @blk_addr: first block address
+ * @buf: output buffer
+ * @len: size of the buffer
+ *
+ * Read one or more blocks of data from the mmc. This is a low-level helper for
+ * tuning operation. If card is NULL, it is assumed that CMD23 can be used for
+ * multi-block read.
+ *
+ * Return: 0 in case of success, otherwise -EIO
+ */
+int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
+		    unsigned int blksz, unsigned int blocks,
+		    unsigned int blk_addr, void *buf, unsigned int len)
+{
+	struct mmc_request mrq = {};
+	struct mmc_command sbc = {};
+	struct mmc_command cmd = {};
+	struct mmc_command stop = {};
+	struct mmc_data data = {};
+	struct scatterlist sg;
+
+	if (blocks > 1) {
+		if (mmc_host_can_cmd23(host) &&
+		    (!card || mmc_card_can_cmd23(card))) {
+			mrq.sbc = &sbc;
+			sbc.opcode = MMC_SET_BLOCK_COUNT;
+			sbc.arg = blocks;
+			sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
+		}
+		cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
+		mrq.stop = &stop;
+		stop.opcode = MMC_STOP_TRANSMISSION;
+		stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
+	} else {
+		cmd.opcode = MMC_READ_SINGLE_BLOCK;
+	}
+
+	mrq.cmd = &cmd;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+
+	mrq.data = &data;
+	data.flags = MMC_DATA_READ;
+	data.blksz = blksz;
+	data.blocks = blocks;
+	data.blk_addr = blk_addr;
+	data.sg = &sg;
+	data.sg_len = 1;
+	if (card)
+		mmc_set_data_timeout(&data, card);
+	else
+		data.timeout_ns = 1000000000;
+
+	sg_init_one(&sg, buf, len);
+
+	mmc_wait_for_req(host, &mrq);
+
+	if (sbc.error || cmd.error || data.error)
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mmc_read_blocks);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 68f09a955a902..72196817a6f0f 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -743,5 +743,8 @@ int mmc_send_status(struct mmc_card *card, u32 *status);
 int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
 int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
 int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
+int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
+		    unsigned int blksz, unsigned int blocks,
+		    unsigned int blk_addr, void *buf, unsigned int len);
 
 #endif /* LINUX_MMC_HOST_H */

[PATCH v2 2/4] mmc: sdhci-cadence: implement multi-block read gap tuning

[Benoît Monin]

The controller suspends the clock between blocks when reading from the
MMC as part of its flow-control, called read block gap. At higher clock
speed and with IO delay between the controller and the MMC, this clock
pause can happen too late, during the read of the next block and
trigger a read error.

To prevent this, the delay can be programmed for each mode via the pair
of registers HRS37/38. This delay is obtained during tuning, by trying
a multi-block read and increasing the delay until the read succeeds.

For now, the tuning is only done in HS200, as the read error has only
been observed at that speed.

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
 drivers/mmc/host/sdhci-cadence.c | 72 +++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 2d823e158c598..2199100f3d4a7 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -36,6 +36,24 @@
 #define   SDHCI_CDNS_HRS06_MODE_MMC_HS400	0x5
 #define   SDHCI_CDNS_HRS06_MODE_MMC_HS400ES	0x6
 
+/* Read block gap */
+#define SDHCI_CDNS_HRS37		0x94	/* interface mode select */
+#define   SDHCI_CDNS_HRS37_MODE_DS		0x0
+#define   SDHCI_CDNS_HRS37_MODE_HS		0x1
+#define   SDHCI_CDNS_HRS37_MODE_UDS_SDR12	0x8
+#define   SDHCI_CDNS_HRS37_MODE_UDS_SDR25	0x9
+#define   SDHCI_CDNS_HRS37_MODE_UDS_SDR50	0xa
+#define   SDHCI_CDNS_HRS37_MODE_UDS_SDR104	0xb
+#define   SDHCI_CDNS_HRS37_MODE_UDS_DDR50	0xc
+#define   SDHCI_CDNS_HRS37_MODE_MMC_LEGACY	0x20
+#define   SDHCI_CDNS_HRS37_MODE_MMC_SDR		0x21
+#define   SDHCI_CDNS_HRS37_MODE_MMC_DDR		0x22
+#define   SDHCI_CDNS_HRS37_MODE_MMC_HS200	0x23
+#define   SDHCI_CDNS_HRS37_MODE_MMC_HS400	0x24
+#define   SDHCI_CDNS_HRS37_MODE_MMC_HS400ES	0x25
+#define SDHCI_CDNS_HRS38		0x98	/* Read block gap coefficient */
+#define   SDHCI_CDNS_HRS38_BLKGAP_MAX		0xf
+
 /* SRS - Slot Register Set (SDHCI-compatible) */
 #define SDHCI_CDNS_SRS_BASE		0x200
 
@@ -251,6 +269,52 @@ static int sdhci_cdns_set_tune_val(struct sdhci_host *host, unsigned int val)
 	return 0;
 }
 
+/**
+ * sdhci_cdns_tune_blkgap() - tune multi-block read gap
+ * @mmc: MMC host
+ *
+ * Tune delay used in multi block read. To do so,
+ * try sending multi-block read command with incremented gap, unless
+ * it succeeds.
+ *
+ * Return: error code
+ */
+static int sdhci_cdns_tune_blkgap(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_cdns_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	void __iomem *hrs37_reg = priv->hrs_addr + SDHCI_CDNS_HRS37;
+	void __iomem *hrs38_reg = priv->hrs_addr + SDHCI_CDNS_HRS38;
+	int ret;
+	u32 gap;
+	u32 hrs37_mode;
+	void *buf;
+
+	switch (host->timing) {
+	case MMC_TIMING_MMC_HS200:
+		hrs37_mode = SDHCI_CDNS_HRS37_MODE_MMC_HS200;
+		break;
+	default:
+		return 0; /* no tuning in this mode */
+	}
+
+	writel(hrs37_mode, hrs37_reg);
+
+	buf = kmalloc(512 * 32, GFP_KERNEL);
+	for (gap = 0; gap <= SDHCI_CDNS_HRS38_BLKGAP_MAX; gap++) {
+		writel(gap, hrs38_reg);
+		ret = mmc_read_blocks(NULL, mmc, 512, 32, 0, buf, 512 * 32);
+		if (ret == 0)
+			break;
+	}
+	kfree(buf);
+
+	dev_dbg(mmc_dev(mmc), "read block gap tune %s, gap %d\n",
+		ret == 0 ? "OK" : "failed", gap);
+	return ret;
+}
+
 /*
  * In SD mode, software must not use the hardware tuning and instead perform
  * an almost identical procedure to eMMC.
@@ -261,6 +325,7 @@ static int sdhci_cdns_execute_tuning(struct sdhci_host *host, u32 opcode)
 	int max_streak = 0;
 	int end_of_streak = 0;
 	int i;
+	int ret;
 
 	/*
 	 * Do not execute tuning for UHS_SDR50 or UHS_DDR50.
@@ -288,7 +353,12 @@ static int sdhci_cdns_execute_tuning(struct sdhci_host *host, u32 opcode)
 		return -EIO;
 	}
 
-	return sdhci_cdns_set_tune_val(host, end_of_streak - max_streak / 2);
+	ret = sdhci_cdns_set_tune_val(host, end_of_streak - max_streak / 2);
+
+	if (!ret)
+		ret = sdhci_cdns_tune_blkgap(host->mmc);
+
+	return ret;
 }
 
 static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,

[PATCH v2 3/4] mmc: mmc_test: use mmc_card_can_cmd23

[Benoît Monin]

Use the helper instead of using a local implementation.

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
 drivers/mmc/core/mmc_test.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/mmc/core/mmc_test.c b/drivers/mmc/core/mmc_test.c
index 80e5d87a5e50b..58413af22517b 100644
--- a/drivers/mmc/core/mmc_test.c
+++ b/drivers/mmc/core/mmc_test.c
@@ -180,20 +180,13 @@ static int mmc_test_set_blksize(struct mmc_test_card *test, unsigned size)
 	return mmc_set_blocklen(test->card, size);
 }
 
-static bool mmc_test_card_cmd23(struct mmc_card *card)
-{
-	return mmc_card_mmc(card) ||
-	       (mmc_card_sd(card) && card->scr.cmds & SD_SCR_CMD23_SUPPORT);
-}
-
 static void mmc_test_prepare_sbc(struct mmc_test_card *test,
 				 struct mmc_request *mrq, unsigned int blocks)
 {
 	struct mmc_card *card = test->card;
 
 	if (!mrq->sbc || !mmc_host_can_cmd23(card->host) ||
-	    !mmc_test_card_cmd23(card) || !mmc_op_multi(mrq->cmd->opcode) ||
-	    (card->quirks & MMC_QUIRK_BLK_NO_CMD23)) {
+	    !mmc_card_can_cmd23(card) || !mmc_op_multi(mrq->cmd->opcode)) {
 		mrq->sbc = NULL;
 		return;
 	}

[PATCH v2 4/4] mmc: block: use mmc_card_can_cmd23

[Benoît Monin]

Use the helper to test for CMD23 card support. Remove the check on the
NO_CMD23 quirk as it is already done in the helper.

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
 drivers/mmc/core/block.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 9cc47bf94804b..f67f5ff97f896 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1768,8 +1768,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 	 * these, while retaining features like reliable writes.
 	 */
 	if ((md->flags & MMC_BLK_CMD23) && mmc_op_multi(brq->cmd.opcode) &&
-	    (do_rel_wr || !(card->quirks & MMC_QUIRK_BLK_NO_CMD23) ||
-	     do_data_tag)) {
+	    (do_rel_wr || do_data_tag)) {
 		brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
 		brq->sbc.arg = brq->data.blocks |
 			(do_rel_wr ? (1 << 31) : 0) |
@@ -2618,13 +2617,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 	 */
 	md->read_only = mmc_blk_readonly(card);
 
-	if (mmc_host_can_cmd23(card->host)) {
-		if ((mmc_card_mmc(card) &&
-		     card->csd.mmca_vsn >= CSD_SPEC_VER_3) ||
-		    (mmc_card_sd(card) && !mmc_card_ult_capacity(card) &&
-		     card->scr.cmds & SD_SCR_CMD23_SUPPORT))
-			md->flags |= MMC_BLK_CMD23;
-	}
+	if (mmc_host_can_cmd23(card->host) && mmc_card_can_cmd23(card))
+		md->flags |= MMC_BLK_CMD23;
 
 	if (md->flags & MMC_BLK_CMD23 &&
 	    ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||

Re: [PATCH v2 1/4] mmc: core: add mmc_read_blocks to mmc_ops

[Ulf Hansson]

On Mon, 7 Jul 2025 at 17:24, Benoît Monin <benoit.monin@bootlin.com> wrote:
>
> Add a generic function to read some blocks of data from the MMC, to be
> used by drivers as part of their tuning.
>
> Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> ---
>  drivers/mmc/core/card.h    | 10 ++++++
>  drivers/mmc/core/mmc_ops.c | 69 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h   |  3 ++
>  3 files changed, 82 insertions(+)
>
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 9cbdd240c3a7d..93fd502c1f5fc 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -11,6 +11,7 @@
>  #define _MMC_CORE_CARD_H
>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/mmc.h>
>
>  #define mmc_card_name(c)       ((c)->cid.prod_name)
>  #define mmc_card_id(c)         (dev_name(&(c)->dev))
> @@ -300,4 +301,13 @@ static inline int mmc_card_no_uhs_ddr50_tuning(const struct mmc_card *c)
>         return c->quirks & MMC_QUIRK_NO_UHS_DDR50_TUNING;
>  }
>
> +static inline bool mmc_card_can_cmd23(struct mmc_card *card)
> +{
> +       return ((mmc_card_mmc(card) &&
> +                card->csd.mmca_vsn >= CSD_SPEC_VER_3) ||
> +               (mmc_card_sd(card) && !mmc_card_ult_capacity(card) &&
> +                card->scr.cmds & SD_SCR_CMD23_SUPPORT)) &&
> +               !(card->quirks & MMC_QUIRK_BLK_NO_CMD23);

First, please make the above part a separate patch. It makes sense to
add a helper for this, as you show in patch3 and patch4. I also
recommend that these patches should also be re-ordered so they come
first in the series.

Second, I don't think we should mix mmc_card_can* functions with the
card-quirks. Better to have two separate helpers, especially since
CMD23 is used for other things too, like RPMB and reliable writes, for
example. Thus I suggest we add:

mmc_card_can_cmd23() - which looks at what the card supports, similar
to above without MMC_QUIRK_BLK_NO_CMD23. Put the definition in
drivers/mmc/core/core.h and export the symbols, similar to what we do
for mmc_card_can_erase() and friends.

mmc_card_broken_blk_cmd23() - which should only check
MMC_QUIRK_BLK_NO_CMD23. This belongs in drivers/mmc/core/card.h.

> +}
> +
>  #endif
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 66283825513cb..848d8aa3ff2b5 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -1077,3 +1077,72 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
>         return err;
>  }
>  EXPORT_SYMBOL_GPL(mmc_sanitize);
> +
> +/**
> + * mmc_read_blocks() - read data blocks from the mmc
> + * @card: mmc card to read from, can be NULL
> + * @host: mmc host doing the read
> + * @blksz: data block size
> + * @blocks: number of blocks to read
> + * @blk_addr: first block address
> + * @buf: output buffer
> + * @len: size of the buffer
> + *
> + * Read one or more blocks of data from the mmc. This is a low-level helper for
> + * tuning operation. If card is NULL, it is assumed that CMD23 can be used for
> + * multi-block read.
> + *
> + * Return: 0 in case of success, otherwise -EIO
> + */
> +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> +                   unsigned int blksz, unsigned int blocks,
> +                   unsigned int blk_addr, void *buf, unsigned int len)
> +{
> +       struct mmc_request mrq = {};
> +       struct mmc_command sbc = {};
> +       struct mmc_command cmd = {};
> +       struct mmc_command stop = {};
> +       struct mmc_data data = {};
> +       struct scatterlist sg;
> +
> +       if (blocks > 1) {
> +               if (mmc_host_can_cmd23(host) &&
> +                   (!card || mmc_card_can_cmd23(card))) {
> +                       mrq.sbc = &sbc;
> +                       sbc.opcode = MMC_SET_BLOCK_COUNT;
> +                       sbc.arg = blocks;
> +                       sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +               }
> +               cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> +               mrq.stop = &stop;
> +               stop.opcode = MMC_STOP_TRANSMISSION;
> +               stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> +       } else {
> +               cmd.opcode = MMC_READ_SINGLE_BLOCK;
> +       }
> +
> +       mrq.cmd = &cmd;
> +       cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +       mrq.data = &data;
> +       data.flags = MMC_DATA_READ;
> +       data.blksz = blksz;
> +       data.blocks = blocks;
> +       data.blk_addr = blk_addr;
> +       data.sg = &sg;
> +       data.sg_len = 1;
> +       if (card)
> +               mmc_set_data_timeout(&data, card);
> +       else
> +               data.timeout_ns = 1000000000;
> +
> +       sg_init_one(&sg, buf, len);
> +
> +       mmc_wait_for_req(host, &mrq);
> +
> +       if (sbc.error || cmd.error || data.error)
> +               return -EIO;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_read_blocks);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 68f09a955a902..72196817a6f0f 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -743,5 +743,8 @@ int mmc_send_status(struct mmc_card *card, u32 *status);
>  int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
>  int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
>  int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
> +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> +                   unsigned int blksz, unsigned int blocks,
> +                   unsigned int blk_addr, void *buf, unsigned int len);

I really think we must avoid exporting such a generic function. This
becomes visible outside the mmc subsystem and I am worried that it
will be abused.

Can we perhaps make it harder to integrate with the tuning support on
the core, somehow? I haven't thought much about it, but maybe you can
propose something along those lines - otherwise I will try to think of
another way to do it.

>
>  #endif /* LINUX_MMC_HOST_H */

Kind regards
Uffe

Re: [PATCH v2 1/4] mmc: core: add mmc_read_blocks to mmc_ops

[Benoît Monin]

Hi Ulf,

Thanks for the review.

On Wednesday, 9 July 2025 at 16:46:45 CEST, Ulf Hansson wrote:
> On Mon, 7 Jul 2025 at 17:24, Benoît Monin <benoit.monin@bootlin.com> wrote:
> >
> > Add a generic function to read some blocks of data from the MMC, to be
> > used by drivers as part of their tuning.
> >
> > Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> > ---
> >  drivers/mmc/core/card.h    | 10 ++++++
> >  drivers/mmc/core/mmc_ops.c | 69 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/mmc/host.h   |  3 ++
> >  3 files changed, 82 insertions(+)
> >
> > diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> > index 9cbdd240c3a7d..93fd502c1f5fc 100644
> > --- a/drivers/mmc/core/card.h
> > +++ b/drivers/mmc/core/card.h
> > @@ -11,6 +11,7 @@
> >  #define _MMC_CORE_CARD_H
> >
> >  #include <linux/mmc/card.h>
> > +#include <linux/mmc/mmc.h>
> >
> >  #define mmc_card_name(c)       ((c)->cid.prod_name)
> >  #define mmc_card_id(c)         (dev_name(&(c)->dev))
> > @@ -300,4 +301,13 @@ static inline int mmc_card_no_uhs_ddr50_tuning(const struct mmc_card *c)
> >         return c->quirks & MMC_QUIRK_NO_UHS_DDR50_TUNING;
> >  }
> >
> > +static inline bool mmc_card_can_cmd23(struct mmc_card *card)
> > +{
> > +       return ((mmc_card_mmc(card) &&
> > +                card->csd.mmca_vsn >= CSD_SPEC_VER_3) ||
> > +               (mmc_card_sd(card) && !mmc_card_ult_capacity(card) &&
> > +                card->scr.cmds & SD_SCR_CMD23_SUPPORT)) &&
> > +               !(card->quirks & MMC_QUIRK_BLK_NO_CMD23);
> 
> First, please make the above part a separate patch. It makes sense to
> add a helper for this, as you show in patch3 and patch4. I also
> recommend that these patches should also be re-ordered so they come
> first in the series.
> 
> Second, I don't think we should mix mmc_card_can* functions with the
> card-quirks. Better to have two separate helpers, especially since
> CMD23 is used for other things too, like RPMB and reliable writes, for
> example. Thus I suggest we add:
> 
> mmc_card_can_cmd23() - which looks at what the card supports, similar
> to above without MMC_QUIRK_BLK_NO_CMD23. Put the definition in
> drivers/mmc/core/core.h and export the symbols, similar to what we do
> for mmc_card_can_erase() and friends.
> 
> mmc_card_broken_blk_cmd23() - which should only check
> MMC_QUIRK_BLK_NO_CMD23. This belongs in drivers/mmc/core/card.h.
> 
Ok, I will do that.

> > +}
> > +
> >  #endif
> > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> > index 66283825513cb..848d8aa3ff2b5 100644
> > --- a/drivers/mmc/core/mmc_ops.c
> > +++ b/drivers/mmc/core/mmc_ops.c
> > @@ -1077,3 +1077,72 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
> >         return err;
> >  }
> >  EXPORT_SYMBOL_GPL(mmc_sanitize);
> > +
> > +/**
> > + * mmc_read_blocks() - read data blocks from the mmc
> > + * @card: mmc card to read from, can be NULL
> > + * @host: mmc host doing the read
> > + * @blksz: data block size
> > + * @blocks: number of blocks to read
> > + * @blk_addr: first block address
> > + * @buf: output buffer
> > + * @len: size of the buffer
> > + *
> > + * Read one or more blocks of data from the mmc. This is a low-level helper for
> > + * tuning operation. If card is NULL, it is assumed that CMD23 can be used for
> > + * multi-block read.
> > + *
> > + * Return: 0 in case of success, otherwise -EIO
> > + */
> > +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> > +                   unsigned int blksz, unsigned int blocks,
> > +                   unsigned int blk_addr, void *buf, unsigned int len)
> > +{
> > +       struct mmc_request mrq = {};
> > +       struct mmc_command sbc = {};
> > +       struct mmc_command cmd = {};
> > +       struct mmc_command stop = {};
> > +       struct mmc_data data = {};
> > +       struct scatterlist sg;
> > +
> > +       if (blocks > 1) {
> > +               if (mmc_host_can_cmd23(host) &&
> > +                   (!card || mmc_card_can_cmd23(card))) {
> > +                       mrq.sbc = &sbc;
> > +                       sbc.opcode = MMC_SET_BLOCK_COUNT;
> > +                       sbc.arg = blocks;
> > +                       sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> > +               }
> > +               cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> > +               mrq.stop = &stop;
> > +               stop.opcode = MMC_STOP_TRANSMISSION;
> > +               stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> > +       } else {
> > +               cmd.opcode = MMC_READ_SINGLE_BLOCK;
> > +       }
> > +
> > +       mrq.cmd = &cmd;
> > +       cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> > +
> > +       mrq.data = &data;
> > +       data.flags = MMC_DATA_READ;
> > +       data.blksz = blksz;
> > +       data.blocks = blocks;
> > +       data.blk_addr = blk_addr;
> > +       data.sg = &sg;
> > +       data.sg_len = 1;
> > +       if (card)
> > +               mmc_set_data_timeout(&data, card);
> > +       else
> > +               data.timeout_ns = 1000000000;
> > +
> > +       sg_init_one(&sg, buf, len);
> > +
> > +       mmc_wait_for_req(host, &mrq);
> > +
> > +       if (sbc.error || cmd.error || data.error)
> > +               return -EIO;
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mmc_read_blocks);
> > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > index 68f09a955a902..72196817a6f0f 100644
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -743,5 +743,8 @@ int mmc_send_status(struct mmc_card *card, u32 *status);
> >  int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
> >  int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
> >  int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
> > +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> > +                   unsigned int blksz, unsigned int blocks,
> > +                   unsigned int blk_addr, void *buf, unsigned int len);
> 
> I really think we must avoid exporting such a generic function. This
> becomes visible outside the mmc subsystem and I am worried that it
> will be abused.
> 
> Can we perhaps make it harder to integrate with the tuning support on
> the core, somehow? I haven't thought much about it, but maybe you can
> propose something along those lines - otherwise I will try to think of
> another way to do it.
> 
I agree that the function might be too generic now. Here are some of
the ideas I have to make less appealing for abuse:

* Rename it to mention tuning (mmc_tuning_read?)
* Drop some parameters:
  * blk_addr: Reading from 0 should be all that is needed for tuning
  * other?
* Move its declaration to a header private to drivers/mmc (where?)

Let me know what you think.

> >
> >  #endif /* LINUX_MMC_HOST_H */
> 
> Kind regards
> Uffe
> 

Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 1/4] mmc: core: add mmc_read_blocks to mmc_ops

[Ulf Hansson]

> > > +}
> > > +
> > >  #endif
> > > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> > > index 66283825513cb..848d8aa3ff2b5 100644
> > > --- a/drivers/mmc/core/mmc_ops.c
> > > +++ b/drivers/mmc/core/mmc_ops.c
> > > @@ -1077,3 +1077,72 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
> > >         return err;
> > >  }
> > >  EXPORT_SYMBOL_GPL(mmc_sanitize);
> > > +
> > > +/**
> > > + * mmc_read_blocks() - read data blocks from the mmc
> > > + * @card: mmc card to read from, can be NULL
> > > + * @host: mmc host doing the read
> > > + * @blksz: data block size
> > > + * @blocks: number of blocks to read
> > > + * @blk_addr: first block address
> > > + * @buf: output buffer
> > > + * @len: size of the buffer
> > > + *
> > > + * Read one or more blocks of data from the mmc. This is a low-level helper for
> > > + * tuning operation. If card is NULL, it is assumed that CMD23 can be used for
> > > + * multi-block read.
> > > + *
> > > + * Return: 0 in case of success, otherwise -EIO
> > > + */
> > > +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> > > +                   unsigned int blksz, unsigned int blocks,
> > > +                   unsigned int blk_addr, void *buf, unsigned int len)
> > > +{
> > > +       struct mmc_request mrq = {};
> > > +       struct mmc_command sbc = {};
> > > +       struct mmc_command cmd = {};
> > > +       struct mmc_command stop = {};
> > > +       struct mmc_data data = {};
> > > +       struct scatterlist sg;
> > > +
> > > +       if (blocks > 1) {
> > > +               if (mmc_host_can_cmd23(host) &&
> > > +                   (!card || mmc_card_can_cmd23(card))) {
> > > +                       mrq.sbc = &sbc;
> > > +                       sbc.opcode = MMC_SET_BLOCK_COUNT;
> > > +                       sbc.arg = blocks;
> > > +                       sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> > > +               }
> > > +               cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> > > +               mrq.stop = &stop;
> > > +               stop.opcode = MMC_STOP_TRANSMISSION;
> > > +               stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> > > +       } else {
> > > +               cmd.opcode = MMC_READ_SINGLE_BLOCK;
> > > +       }
> > > +
> > > +       mrq.cmd = &cmd;
> > > +       cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> > > +
> > > +       mrq.data = &data;
> > > +       data.flags = MMC_DATA_READ;
> > > +       data.blksz = blksz;
> > > +       data.blocks = blocks;
> > > +       data.blk_addr = blk_addr;
> > > +       data.sg = &sg;
> > > +       data.sg_len = 1;
> > > +       if (card)
> > > +               mmc_set_data_timeout(&data, card);
> > > +       else
> > > +               data.timeout_ns = 1000000000;
> > > +
> > > +       sg_init_one(&sg, buf, len);
> > > +
> > > +       mmc_wait_for_req(host, &mrq);
> > > +
> > > +       if (sbc.error || cmd.error || data.error)
> > > +               return -EIO;
> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(mmc_read_blocks);
> > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > > index 68f09a955a902..72196817a6f0f 100644
> > > --- a/include/linux/mmc/host.h
> > > +++ b/include/linux/mmc/host.h
> > > @@ -743,5 +743,8 @@ int mmc_send_status(struct mmc_card *card, u32 *status);
> > >  int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
> > >  int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
> > >  int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
> > > +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> > > +                   unsigned int blksz, unsigned int blocks,
> > > +                   unsigned int blk_addr, void *buf, unsigned int len);
> >
> > I really think we must avoid exporting such a generic function. This
> > becomes visible outside the mmc subsystem and I am worried that it
> > will be abused.
> >
> > Can we perhaps make it harder to integrate with the tuning support on
> > the core, somehow? I haven't thought much about it, but maybe you can
> > propose something along those lines - otherwise I will try to think of
> > another way to do it.
> >
> I agree that the function might be too generic now. Here are some of
> the ideas I have to make less appealing for abuse:
>
> * Rename it to mention tuning (mmc_tuning_read?)

Yes, something like that or possibly "mmc_read_tuning".

> * Drop some parameters:
>   * blk_addr: Reading from 0 should be all that is needed for tuning
>   * other?

Yes, I like that. If we can make it useful only for the tuning case,
that would be great.

I think we should drop struct mmc_card* too. The ->execute_tuning()
host ops takes a struct mmc_host* and during initialization when the
callback is invoked from the core, host->card has not yet been set. In
other words, there are no "card" available for the host to use.

Moreover, do we really need to pass along the data that we have read?
Can't we just alloc the buffer, read the data and free the buffer. The
important part is to provide an error code to the caller, letting it
know whether we succeeded in reading the data or whether it failed,
right?

> * Move its declaration to a header private to drivers/mmc (where?)

Unfortunately not, we currently don't have any other suitable place,
but next to mmc_get_ext_csd().

>
> Let me know what you think.
>

Kind regards
Uffe