[PATCH RFC 0/2] mmc: jz4740: Use the integrated DMA controller on the X1600

[PATCH RFC 0/2] mmc: jz4740: Use the integrated DMA controller on the X1600

[Olivier Louvignes]

The X1600's MSCs have no request lines to the system DMA controller, so
jz4740_mmc runs them in PIO. This series uses the MSC's own descriptor
DMA there instead. On a HiBy R1 (X1600, SDHC, 4-bit, 50 MHz), reads go
from 17.3 to 22.5 MB/s and writes from 13.9 to 18.2 MB/s, with the CPU
free.

It is an RFC because of how a read is known to be finished. On the
X1600:

- DMA_DATA_DONE (IFLG bit 31) never came in our tests, masked or
  unmasked. Rockbox's driver for the X1000 and X1600 waits for it, with
  the FIFO thresholds (CMDAT RTRG/TTRG) at 32; we tried those thresholds
  only with the flags masked.
- DMAEND (IFLG bit 16) is only flagged while it is unmasked, as the
  manual says. Unmasked, it came for every read we watched, including
  the 8- and 64-byte register reads at card setup; MSC_STAT's DMAEND
  stayed clear for the 64-byte one.
- Waiting for the DMAEND interrupt made reads very slow in a first
  attempt (minutes to list a directory). We haven't found why.

So this version polls: a read is done once DMADA has reached the end of
the last segment with DMALEN at 0, or on DMAEND. Is there a completion
signal on this controller that we have missed, or is this acceptable?

Second, reads under a block (the SCR and SD status at card setup)
sometimes never raised DATA_TRAN_DONE through the DMA, and the card
setup timed out, so they stay on PIO.

Third, SDIO writes that brcmfmac glued together from several frames, in
segments that end inside a block, timed out through the DMA (CMD53,
-ETIMEDOUT) and left the bus dead, while the same writes went through PIO
cleanly. Transfers with such a segment stay on PIO.

Fourth, the X1600 manual's two descriptions of the descriptor's LINK bit
disagree. The code follows the MSC_DMACMD register table, which the
hardware matches: reads spanning several descriptors work.

The R1's Wi-Fi (brcmfmac over SDIO on the other MSC) goes through it
too: its firmware download of about 420 KB in CMD53 transfers read back
correctly, and with the rule above, 10 rounds of DNS, HTTP and ping and
three Wi-Fi off/on cycles went through cleanly.

Only the new ingenic,x1600-mmc compatible turns this on, so the other
SoCs keep their external DMA or PIO. Not tested: error paths (CRC
errors, a card pulled mid-transfer), suspend and resume.

Olivier Louvignes (2):
  dt-bindings: mmc: ingenic: Add the X1600
  mmc: jz4740: Use the integrated DMA controller on the X1600

 .../devicetree/bindings/mmc/ingenic,mmc.yaml  |   3 +
 drivers/mmc/host/jz4740_mmc.c                 | 182 ++++++++++++++++--
 2 files changed, 174 insertions(+), 11 deletions(-)


base-commit: fd9af27f6c2319713bb23832066585f8a59a1771
-- 
2.39.5

[PATCH RFC 1/2] dt-bindings: mmc: ingenic: Add the X1600

[Olivier Louvignes]

The X1600's MSC has the X1000's registers, so the X1000 is its fallback.
Unlike the X1000's, its MSCs have no request lines to the system DMA
controller: the driver uses their own DMA controller instead, which needs
the X1600 compatible to tell them apart.

Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
 Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml b/Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml
index bb4e0be0c893..6b26a17982f3 100644
--- a/Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml
+++ b/Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml
@@ -25,6 +25,9 @@ properties:
       - items:
           - const: ingenic,jz4770-mmc
           - const: ingenic,jz4760-mmc
+      - items:
+          - const: ingenic,x1600-mmc
+          - const: ingenic,x1000-mmc
 
   reg:
     maxItems: 1

base-commit: fd9af27f6c2319713bb23832066585f8a59a1771
-- 
2.39.5

Re: [PATCH RFC 1/2] dt-bindings: mmc: ingenic: Add the X1600

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: mmc: ingenic: dmas and dma-names required for ingenic,x1600-mmc
--

commit 43640170e1ca344d80f3c0f4831c11fa293a023e
Author: Olivier Louvignes <olivier@mgcrea.io>

dt-bindings: mmc: ingenic: Add the X1600

This commit adds the ingenic,x1600-mmc compatible string with
ingenic,x1000-mmc as a fallback to the Ingenic MMC device tree binding
schema.

> diff --git a/Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml b/Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml
> index bb4e0be0c8935..6b26a17982f37 100644
> --- a/Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml
> +++ b/Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml
> @@ -25,6 +25,9 @@ properties:
>        - items:
>            - const: ingenic,jz4770-mmc
>            - const: ingenic,jz4760-mmc
> +      - items:
> +          - const: ingenic,x1600-mmc
> +          - const: ingenic,x1000-mmc

[Severity: Medium]
The commit message notes that the X1600 MSCs have no request lines to the
system DMA controller and use their own integrated DMA controller instead.

However, in Documentation/devicetree/bindings/mmc/ingenic,mmc.yaml, the
top-level required list still unconditionally includes dmas and dma-names:

required:
  - compatible
  - reg
  - interrupts
  - clocks
  - clock-names
  - dmas
  - dma-names

Won't dtbs_check fail when validating X1600 MMC device tree nodes that
omit dmas and dma-names? Should dmas and dma-names be moved to a
conditional block under allOf so that they are disallowed (or not
required) when compatible contains ingenic,x1600-mmc?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025140325.73204-1-olivier@mgcrea.io?part=1

[PATCH RFC 2/2] mmc: jz4740: Use the integrated DMA controller on the X1600

[Olivier Louvignes]

From the JZ4780 on, the MSC has a DMA controller of its own, which walks
a chain of descriptors (next, address, length, command) and is selected
with DMA_SEL clear in DMAC. The X1600 has no request lines from its MSCs
to the system DMA controller (Ingenic's own kernel uses the integrated
controller), so the driver fell back to PIO there, which takes the CPU
for every word.

Use the integrated controller on the X1600 when the device tree gives no
DMA channels: one descriptor per scatterlist segment, up to 64 KiB each,
started right after the command. Buffers that aren't word-aligned,
transfers under a block and segments that end inside a block still go
through PIO.

A write is complete once the transfer is done, as before. A read needs
its last words in memory, and on the X1600 no flag the driver polls says
so: DMA_DATA_DONE never came, even unmasked, and DMAEND is only flagged
while unmasked. So a read is done when the DMA address has reached the
end of the last segment with nothing left of it, or on DMAEND.

On the HiBy R1 (X1600, an SDHC card at 50 MHz, 4-bit), reads go from 17.3
to 22.5 MB/s and writes from 13.9 to 18.2 MB/s, with the CPU free; as a
USB mass storage gadget, writes from a computer from 4.4 to 10.7 MB/s.

Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
 drivers/mmc/host/jz4740_mmc.c | 182 ++++++++++++++++++++++++++++++++--
 1 file changed, 171 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 6a3c26b7c82d..300ad2a01b17 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -24,6 +24,7 @@
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/scatterlist.h>
+#include <linux/sizes.h>
 
 #include <asm/cacheflush.h>
 
@@ -45,6 +46,9 @@
 #define JZ_REG_MMC_TXFIFO	0x3C
 #define JZ_REG_MMC_LPM		0x40
 #define JZ_REG_MMC_DMAC		0x44
+#define JZ_REG_MMC_DMANDA	0x48
+#define JZ_REG_MMC_DMADA	0x4C
+#define JZ_REG_MMC_DMALEN	0x50
 
 #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
 #define JZ_MMC_STRPCL_EXIT_TRANSFER BIT(6)
@@ -93,6 +97,7 @@
 #define JZ_MMC_CMDAT_RSP_R2 2
 #define JZ_MMC_CMDAT_RSP_R3 3
 
+#define JZ_MMC_IRQ_DMAEND BIT(16)
 #define JZ_MMC_IRQ_SDIO BIT(7)
 #define JZ_MMC_IRQ_TXFIFO_WR_REQ BIT(6)
 #define JZ_MMC_IRQ_RXFIFO_RD_REQ BIT(5)
@@ -103,6 +108,15 @@
 #define JZ_MMC_DMAC_DMA_SEL BIT(1)
 #define JZ_MMC_DMAC_DMA_EN BIT(0)
 
+/*
+ * A descriptor of the integrated DMA controller; dcmd bits. LINK set fetches
+ * the next descriptor, as the X1600 manual's MSC_DMACMD table says (its
+ * descriptor section says the opposite).
+ */
+#define JZ_MMC_DESC_LINK BIT(0)
+#define JZ_MMC_DESC_ENDI BIT(1)
+#define JZ_MMC_DESC_MAX_LEN SZ_64K
+
 #define	JZ_MMC_LPM_DRV_RISING BIT(31)
 #define	JZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY BIT(31)
 #define	JZ_MMC_LPM_DRV_RISING_1NS_DLY BIT(30)
@@ -118,6 +132,7 @@ enum jz4740_mmc_version {
 	JZ_MMC_JZ4760,
 	JZ_MMC_JZ4780,
 	JZ_MMC_X1000,
+	JZ_MMC_X1600,
 };
 
 enum jz4740_mmc_state {
@@ -140,6 +155,13 @@ enum jz4740_mmc_state {
  * COOKIE_MAPPED: the request was mapped in the irq handler,
  * and should be unmapped before mmc_request_done is called..
  */
+struct jz4740_mmc_desc {
+	__le32 next;
+	__le32 addr;
+	__le32 len;
+	__le32 cmd;
+};
+
 enum jz4780_cookie {
 	COOKIE_UNMAPPED = 0,
 	COOKIE_PREMAPPED,
@@ -179,6 +201,18 @@ struct jz4740_mmc_host {
 	struct dma_chan *dma_tx;
 	bool use_dma;
 
+	/*
+	 * The integrated DMA controller, used on the X1600, whose MSCs have no
+	 * request lines to the system DMA controller; idma_active is set for a
+	 * request it carries.
+	 */
+	bool use_idma;
+	bool idma_active;
+	struct jz4740_mmc_desc *descs;
+	dma_addr_t descs_dma;
+	/* Where the integrated DMA's address ends up when it is done */
+	u32 idma_end;
+
 /* The DMA trigger level is 8 words, that is to say, the DMA read
  * trigger is when data words in MSC_RXFIFO is >= 8 and the DMA write
  * trigger is when data words in MSC_TXFIFO is < 8.
@@ -283,13 +317,22 @@ static inline struct dma_chan *jz4740_mmc_get_dma_chan(struct jz4740_mmc_host *h
 		return host->dma_tx;
 }
 
+/* The device that maps buffers: the DMA engine's, or ours for the integrated one */
+static struct device *jz4740_mmc_dma_dev(struct jz4740_mmc_host *host,
+					 struct mmc_data *data)
+{
+	if (host->use_idma)
+		return mmc_dev(host->mmc);
+
+	return jz4740_mmc_get_dma_chan(host, data)->device->dev;
+}
+
 static void jz4740_mmc_dma_unmap(struct jz4740_mmc_host *host,
 				 struct mmc_data *data)
 {
-	struct dma_chan *chan = jz4740_mmc_get_dma_chan(host, data);
 	enum dma_data_direction dir = mmc_get_dma_dir(data);
 
-	dma_unmap_sg(chan->device->dev, data->sg, data->sg_len, dir);
+	dma_unmap_sg(jz4740_mmc_dma_dev(host, data), data->sg, data->sg_len, dir);
 	data->host_cookie = COOKIE_UNMAPPED;
 }
 
@@ -300,17 +343,14 @@ static int jz4740_mmc_prepare_dma_data(struct jz4740_mmc_host *host,
 				       struct mmc_data *data,
 				       int cookie)
 {
-	struct dma_chan *chan = jz4740_mmc_get_dma_chan(host, data);
 	enum dma_data_direction dir = mmc_get_dma_dir(data);
 	unsigned int sg_count;
 
 	if (data->host_cookie == COOKIE_PREMAPPED)
 		return data->sg_count;
 
-	sg_count = dma_map_sg(chan->device->dev,
-			data->sg,
-			data->sg_len,
-			dir);
+	sg_count = dma_map_sg(jz4740_mmc_dma_dev(host, data), data->sg,
+			      data->sg_len, dir);
 
 	if (!sg_count) {
 		dev_err(mmc_dev(host->mmc),
@@ -371,13 +411,101 @@ static int jz4740_mmc_start_dma_transfer(struct jz4740_mmc_host *host,
 	return -ENOMEM;
 }
 
+/*
+ * The integrated DMA controller moves whole words from word-aligned
+ * addresses; other buffers (none the block layer hands us) go through PIO.
+ * So do transfers under a block, which are the card's registers read while
+ * it is set up: on the X1600 such a read through the DMA sometimes never
+ * raised DATA_TRAN_DONE although all of it had been moved, and the setup
+ * started over after each 5 s timeout. And so do segments that end inside
+ * a block: SDIO writes that brcmfmac glued together from several frames
+ * timed out and left the bus dead, while the same writes went through PIO.
+ */
+static bool jz4740_mmc_idma_can(struct jz4740_mmc_host *host,
+				struct mmc_data *data)
+{
+	struct scatterlist *sg;
+	int i;
+
+	if (!host->use_idma || data->blocks * data->blksz < 512)
+		return false;
+
+	for_each_sg(data->sg, sg, data->sg_len, i)
+		if ((sg->offset | sg->length) & 3 || sg->length % data->blksz)
+			return false;
+
+	return true;
+}
+
+/* Maps the request's buffers and chains one descriptor per segment */
+static int jz4740_mmc_idma_prepare(struct jz4740_mmc_host *host,
+				   struct mmc_data *data)
+{
+	struct scatterlist *sg;
+	int i, sg_count;
+
+	sg_count = jz4740_mmc_prepare_dma_data(host, data, COOKIE_MAPPED);
+	if (sg_count < 0)
+		return sg_count;
+
+	for_each_sg(data->sg, sg, sg_count, i) {
+		struct jz4740_mmc_desc *desc = &host->descs[i];
+		bool last = i == sg_count - 1;
+
+		desc->next = cpu_to_le32(last ? 0 : host->descs_dma + (i + 1) * sizeof(*desc));
+		desc->addr = cpu_to_le32(sg_dma_address(sg));
+		desc->len = cpu_to_le32(sg_dma_len(sg));
+		desc->cmd = cpu_to_le32(last ? JZ_MMC_DESC_ENDI : JZ_MMC_DESC_LINK);
+		if (last)
+			host->idma_end = sg_dma_address(sg) + sg_dma_len(sg);
+	}
+	/* The descriptors are coherent memory; order them before the start */
+	wmb();
+
+	return 0;
+}
+
+/*
+ * The card's data is in (DATA_TRAN_DONE); wait for the integrated DMA to
+ * have written the last of it to memory. On the X1600 DMA_DATA_DONE (bit
+ * 31) didn't come, even unmasked, and DMAEND is only flagged while
+ * unmasked, which the polling here doesn't do. So it is done when the
+ * address has reached the end of the last segment with nothing left of it,
+ * or on DMAEND.
+ */
+static bool jz4740_mmc_idma_read_done(struct jz4740_mmc_host *host)
+{
+	if (jz4740_mmc_read_irq_reg(host) & JZ_MMC_IRQ_DMAEND)
+		return true;
+
+	return readl(host->base + JZ_REG_MMC_DMADA) == host->idma_end &&
+	       readl(host->base + JZ_REG_MMC_DMALEN) == 0;
+}
+
+static void jz4740_mmc_idma_wait_read(struct jz4740_mmc_host *host,
+				      struct mmc_data *data)
+{
+	unsigned int timeout = 10000;
+
+	while (!jz4740_mmc_idma_read_done(host) && --timeout)
+		udelay(1);
+
+	if (!timeout && !data->error) {
+		dev_err(mmc_dev(host->mmc), "DMA did not finish a read\n");
+		data->error = -EIO;
+	}
+}
+
 static void jz4740_mmc_pre_request(struct mmc_host *mmc,
 				   struct mmc_request *mrq)
 {
 	struct jz4740_mmc_host *host = mmc_priv(mmc);
 	struct mmc_data *data = mrq->data;
 
-	if (!host->use_dma)
+	if (host->use_idma && !jz4740_mmc_idma_can(host, data))
+		return;
+
+	if (!host->use_dma && !host->use_idma)
 		return;
 
 	data->host_cookie = COOKIE_UNMAPPED;
@@ -395,7 +523,7 @@ static void jz4740_mmc_post_request(struct mmc_host *mmc,
 	if (data && data->host_cookie != COOKIE_UNMAPPED)
 		jz4740_mmc_dma_unmap(host, data);
 
-	if (err) {
+	if (err && host->use_dma) {
 		struct dma_chan *chan = jz4740_mmc_get_dma_chan(host, data);
 
 		dmaengine_terminate_all(chan);
@@ -462,6 +590,11 @@ static void jz4740_mmc_request_done(struct jz4740_mmc_host *host)
 	data = req->data;
 	host->req = NULL;
 
+	if (host->idma_active) {
+		writel(0, host->base + JZ_REG_MMC_DMAC);
+		host->idma_active = false;
+	}
+
 	if (data && data->host_cookie == COOKIE_MAPPED)
 		jz4740_mmc_dma_unmap(host, data);
 	mmc_request_done(host->mmc, req);
@@ -680,6 +813,7 @@ static void jz4740_mmc_send_command(struct jz4740_mmc_host *host,
 	struct mmc_command *cmd)
 {
 	uint32_t cmdat = host->cmdat;
+	bool idma = false;
 
 	host->cmdat &= ~JZ_MMC_CMDAT_INIT;
 	jz4740_mmc_clock_disable(host);
@@ -726,6 +860,11 @@ static void jz4740_mmc_send_command(struct jz4740_mmc_host *host,
 			}
 		} else if (host->version >= JZ_MMC_JZ4780) {
 			writel(0, host->base + JZ_REG_MMC_DMAC);
+			idma = jz4740_mmc_idma_can(host, cmd->data) &&
+			       !jz4740_mmc_idma_prepare(host, cmd->data);
+			if (idma)
+				writel(host->descs_dma, host->base + JZ_REG_MMC_DMANDA);
+			host->idma_active = idma;
 		}
 
 		writew(cmd->data->blksz, host->base + JZ_REG_MMC_BLKLEN);
@@ -737,6 +876,10 @@ static void jz4740_mmc_send_command(struct jz4740_mmc_host *host,
 	writel(cmdat, host->base + JZ_REG_MMC_CMDAT);
 
 	jz4740_mmc_clock_enable(host, 1);
+
+	/* Started after the command, as Ingenic's own drivers do */
+	if (idma)
+		writel(JZ_MMC_DMAC_DMA_EN, host->base + JZ_REG_MMC_DMAC);
 }
 
 static void jz_mmc_prepare_data_transfer(struct jz4740_mmc_host *host)
@@ -786,6 +929,9 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
 			 */
 			timeout = jz4740_mmc_start_dma_transfer(host, data);
 			data->bytes_xfered = data->blocks * data->blksz;
+		} else if (host->idma_active) {
+			/* Running since the command started */
+			data->bytes_xfered = data->blocks * data->blksz;
 		} else if (data->flags & MMC_DATA_READ)
 			/* Use PIO if DMA is not enabled.
 			 * Data transfer direction was defined before
@@ -812,6 +958,9 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
 		fallthrough;
 
 	case JZ4740_MMC_STATE_SEND_STOP:
+		if (host->idma_active && (data->flags & MMC_DATA_READ))
+			jz4740_mmc_idma_wait_read(host, data);
+
 		if (!req->stop)
 			break;
 
@@ -850,7 +999,8 @@ static irqreturn_t jz_mmc_irq(int irq, void *devid)
 	irq_reg &= ~host->irq_mask;
 
 	tmp &= ~(JZ_MMC_IRQ_TXFIFO_WR_REQ | JZ_MMC_IRQ_RXFIFO_RD_REQ |
-		JZ_MMC_IRQ_PRG_DONE | JZ_MMC_IRQ_DATA_TRAN_DONE);
+		JZ_MMC_IRQ_PRG_DONE | JZ_MMC_IRQ_DATA_TRAN_DONE |
+		JZ_MMC_IRQ_DMAEND);
 
 	if (tmp != irq_reg)
 		jz4740_mmc_write_irq_reg(host, tmp & ~irq_reg);
@@ -1033,6 +1183,7 @@ static const struct of_device_id jz4740_mmc_of_match[] = {
 	{ .compatible = "ingenic,jz4775-mmc", .data = (void *) JZ_MMC_JZ4780 },
 	{ .compatible = "ingenic,jz4780-mmc", .data = (void *) JZ_MMC_JZ4780 },
 	{ .compatible = "ingenic,x1000-mmc", .data = (void *) JZ_MMC_X1000 },
+	{ .compatible = "ingenic,x1600-mmc", .data = (void *)JZ_MMC_X1600 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, jz4740_mmc_of_match);
@@ -1123,6 +1274,15 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 		goto err_free_irq;
 	host->use_dma = !ret;
 
+	if (!host->use_dma && host->version >= JZ_MMC_X1600) {
+		host->descs = dmam_alloc_coherent(&pdev->dev,
+						  mmc->max_segs * sizeof(*host->descs),
+						  &host->descs_dma, GFP_KERNEL);
+		host->use_idma = !!host->descs;
+		if (host->use_idma)
+			mmc->max_seg_size = JZ_MMC_DESC_MAX_LEN;
+	}
+
 	platform_set_drvdata(pdev, host);
 	ret = mmc_add_host(mmc);
 
@@ -1133,7 +1293,7 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
 	dev_info(&pdev->dev, "Ingenic SD/MMC card driver registered\n");
 
 	dev_info(&pdev->dev, "Using %s, %d-bit mode\n",
-		 host->use_dma ? "DMA" : "PIO",
+		 host->use_dma ? "DMA" : host->use_idma ? "integrated DMA" : "PIO",
 		 (mmc->caps & MMC_CAP_8_BIT_DATA) ? 8 :
 		 ((mmc->caps & MMC_CAP_4_BIT_DATA) ? 4 : 1));
 
-- 
2.39.5

Re: [PATCH RFC 2/2] mmc: jz4740: Use the integrated DMA controller on the X1600

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mmc: jz4740: unreset bytes_xfered on idma read error masks I/O failure
--

commit 03cdfaf8519d89524e6b08dadbfa32014a811a97
Author: Olivier Louvignes <olivier@mgcrea.io>

mmc: jz4740: Use the integrated DMA controller on the X1600

This commit adds support for the MSC's integrated descriptor-based DMA
controller on the Ingenic X1600 when external DMA channels are not
provided in the device tree. It falls back to PIO for unaligned buffers,
transfers smaller than a block, or segments not aligned to the block size.

> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> index 6a3c26b7c82d8..300ad2a01b177 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c

[ ... ]

> @@ -371,13 +411,101 @@ static int jz4740_mmc_start_dma_transfer(struct jz4740_mmc_host *host,

[ ... ]

> +static void jz4740_mmc_idma_wait_read(struct jz4740_mmc_host *host,
> +				      struct mmc_data *data)
> +{
> +	unsigned int timeout = 10000;
> +
> +	while (!jz4740_mmc_idma_read_done(host) && --timeout)
> +		udelay(1);
> +
> +	if (!timeout && !data->error) {
> +		dev_err(mmc_dev(host->mmc), "DMA did not finish a read\n");
> +		data->error = -EIO;
> +	}
> +}

[ ... ]

> @@ -786,6 +929,9 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
>  			 */
>  			timeout = jz4740_mmc_start_dma_transfer(host, data);
>  			data->bytes_xfered = data->blocks * data->blksz;
> +		} else if (host->idma_active) {
> +			/* Running since the command started */
> +			data->bytes_xfered = data->blocks * data->blksz;
>  		} else if (data->flags & MMC_DATA_READ)
>  			/* Use PIO if DMA is not enabled.
>  			 * Data transfer direction was defined before
> @@ -812,6 +958,9 @@ static irqreturn_t jz_mmc_irq_worker(int irq, void *devid)
>  		fallthrough;
>  
>  	case JZ4740_MMC_STATE_SEND_STOP:
> +		if (host->idma_active && (data->flags & MMC_DATA_READ))
> +			jz4740_mmc_idma_wait_read(host, data);

[Severity: High]
Can a failed or timed-out integrated DMA read complete with BLK_STS_OK and
return stale buffer contents to the caller?

In jz_mmc_irq_worker() under JZ4740_MMC_STATE_TRANSFER_DATA, when
host->idma_active is true, data->bytes_xfered is set to
data->blocks * data->blksz before JZ_MMC_IRQ_DATA_TRAN_DONE is polled and
before jz4740_mmc_idma_wait_read() verifies that the DMA engine has drained
the read data to memory.

If jz4740_mmc_idma_wait_read() times out in JZ4740_MMC_STATE_SEND_STOP, it
sets data->error = -EIO, but leaves data->bytes_xfered set to the full
request size (and if data->error was already set earlier by
jz4740_mmc_transfer_check_state() or jz_mmc_irq(), jz4740_mmc_idma_wait_read()
still spins for up to 10 ms while leaving data->bytes_xfered untouched).

When the request completes, mmc_blk_mq_poll_completion() enters
mmc_blk_mq_rw_recovery():

drivers/mmc/core/block.c:mmc_blk_mq_rw_recovery() {
    ...
	err = __mmc_send_status(card, &status, 0);
	if (err || mmc_blk_status_error(req, status))
		brq->data.bytes_xfered = 0;
    ...
	if (brq->data.bytes_xfered)
		return;
    ...
}

Because the bus transfer from the card finished cleanly and the failure
occurred in the host's integrated DMA controller, __mmc_send_status()
succeeds without card status error bits, so brq->data.bytes_xfered is not
cleared. Then mmc_blk_mq_complete_rq() sees nr_bytes equal to the full
request length and finishes the block request with BLK_STS_OK:

drivers/mmc/core/block.c:mmc_blk_mq_complete_rq() {
	struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
	unsigned int nr_bytes = mqrq->brq.data.bytes_xfered;

	if (nr_bytes) {
		if (blk_update_request(req, BLK_STS_OK, nr_bytes))
			blk_mq_requeue_request(req, true);
		else
			__blk_mq_end_request(req, BLK_STS_OK);
	} else if (!blk_rq_bytes(req)) {
    ...
}

Should data->bytes_xfered only be set after the transfer and
jz4740_mmc_idma_wait_read() succeed without data->error (or be reset to 0 on
error), and should jz4740_mmc_idma_wait_read() skip waiting when data->error
is already set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025140325.73204-1-olivier@mgcrea.io?part=2

Re: [PATCH RFC 0/2] mmc: jz4740: Use the integrated DMA controller on the X1600

[Olivier Louvignes]

Hi Ulf,

Please drop this RFC, both its premise and its question turned out to
be wrong.

The X1600's MSCs do have request lines to the system DMA controller:
the programming manual lists PDMA request types for both (0x34 to 0x37),
and MSC_DMAC.DMASEL picks between the two DMA paths. With dmas in the
device tree, the existing PDMA path works on the HiBy R1, card setup
included, and reads as fast as the integrated DMA did (23.0 to 23.5 MB/s
against 23.5 MB/s with dd, the CPU about 20% busy for both).

And DMA_DATA_DONE does come. jz_mmc_irq() clears the flags of masked
interrupts, DMA_DATA_DONE among them, and on the X1600 writing that bit
also clears DATA_TRAN_DONE and DMAEND, which most likely is why reads
looked unfinished.

So the X1600 needs no driver change here, only dmas in its device tree
like the other Ingenic SoCs.

Sorry for the noise,
Olivier