[PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Wolfram Sang]

Motivation for this series from patch 5:

===
So far, we have been relying on access_end interrupts only to mark DMA
transfers as done implying that DMA end interrupts have occurred by then
anyhow. On some SoCs under some conditions, this turned out to be not
enough. So, we enable DMA interrupts as well and make sure that both
events, DMA irq and access_end irq, have happened before finishing the
DMA transfer.
===

The first two patches are cleanups. For the rest, the basis were BSP
patches, but they have been refactored heavily, so they look very
different now:

* self-contained
  except for the new dma_irq callback which is for the TMIO core, all
  other code is self-contained in renesas_sdhi_internal_dmac now.
* concise
  Less new members for the existing structs, also code duplication was
  avoided by moving code to more suitable locations
* replaced the spinlock with atomic bit operators
* used quirk mechanism for the old INFO1 layout

Tested on a Salvator-X board with M3-W (r8a77960) and a Salvator-XS
board with M3-N (r8a77965). No regression encountered in functionality
and performance. A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/upport_dmaend


Here are excerpts of a log when booting the M3-N with patch 6 applied to
show that all combinations of incoming irqs are handled:

=== DMA first, Access second ===

          <idle>-0       [000] d.h..     0.505454: renesas_sdhi_internal_dmac_dma_irq: DMA end
          <idle>-0       [000] d.h..     0.505496: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
          <idle>-0       [000] ..s..     0.505498: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet

=== Access first, DMA second ===

     kworker/0:2-42      [000] d.h..     0.510603: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
     kworker/0:2-42      [000] d.h..     0.510605: renesas_sdhi_internal_dmac_dma_irq: DMA end
     kworker/0:2-42      [000] ..s..     0.510606: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet

=== Access first, Simulated DMA second ===

          <idle>-0       [000] d.H..     0.510635: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
          <idle>-0       [000] ..s..     0.510637: renesas_sdhi_internal_dmac_issue_tasklet_fn: Simulated DMA end
          <idle>-0       [000] ..s..     0.510638: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet

(I have never seen Simulated DMA (= CMD error happened) first, but it
should be handled like regular DMA end as well(tm).)

=== Access first, no DMA end needed because of DATA error (EILSEQ) ===

          <idle>-0       [000] d.H..     0.510894: renesas_sdhi_internal_dmac_dataend_dma: Access end: -84
          <idle>-0       [000] ..s..     0.510896: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet

===

I think this is as far as I can reach alone. I'd love to get review
comments and further testing. Shimoda-san, can you kindly ask the BSP
team to do further testing?

Thank you everyone and happy hacking,

   Wolfram


Wolfram Sang (6):
  mmc: renesas_sdhi: remove accessor function for internal_dmac
  mmc: renesas_sdhi: improve naming of DMA struct
  mmc: tmio: add callback for dma irq
  mmc: renesas_sdhi: add quirk for broken register layout
  mmc: renesas_sdhi: take DMA end interrupts into account
  DEBUG mmc: renesas_sdhi: debug end_flags for DMA

 drivers/mmc/host/renesas_sdhi.h               | 14 ++-
 drivers/mmc/host/renesas_sdhi_core.c          |  2 +-
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 86 ++++++++++++-------
 drivers/mmc/host/tmio_mmc.h                   |  1 +
 drivers/mmc/host/tmio_mmc_core.c              |  3 +
 5 files changed, 72 insertions(+), 34 deletions(-)

-- 
2.35.1

[PATCH RFC 1/6] mmc: renesas_sdhi: remove accessor function for internal_dmac

[Wolfram Sang]

This accessor function does not help readability but makes it worse.
Because I soon need to read from the registers as well and don't want to
add another function like this, I chose to remove the existing one and
use the accessor directly. I also switch from writeq to writel because
no 64 bit register is actually involved.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 31 +++++--------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 42937596c4c4..48bea0bd75e8 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -279,13 +279,6 @@ static const struct of_device_id renesas_sdhi_internal_dmac_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, renesas_sdhi_internal_dmac_of_match);
 
-static void
-renesas_sdhi_internal_dmac_dm_write(struct tmio_mmc_host *host,
-				    int addr, u64 val)
-{
-	writeq(val, host->ctl + addr);
-}
-
 static void
 renesas_sdhi_internal_dmac_enable_dma(struct tmio_mmc_host *host, bool enable)
 {
@@ -295,8 +288,7 @@ renesas_sdhi_internal_dmac_enable_dma(struct tmio_mmc_host *host, bool enable)
 		return;
 
 	if (!enable)
-		renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO1,
-						    INFO1_CLEAR);
+		writel(INFO1_CLEAR, host->ctl + DM_CM_INFO1);
 
 	if (priv->dma_priv.enable)
 		priv->dma_priv.enable(host, enable);
@@ -309,10 +301,8 @@ renesas_sdhi_internal_dmac_abort_dma(struct tmio_mmc_host *host)
 
 	renesas_sdhi_internal_dmac_enable_dma(host, false);
 
-	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_RST,
-					    RST_RESERVED_BITS & ~val);
-	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_RST,
-					    RST_RESERVED_BITS | val);
+	writel(RST_RESERVED_BITS & ~val, host->ctl + DM_CM_RST);
+	writel(RST_RESERVED_BITS | val, host->ctl + DM_CM_RST);
 
 	clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags);
 
@@ -397,10 +387,8 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 	renesas_sdhi_internal_dmac_enable_dma(host, true);
 
 	/* set dma parameters */
-	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_DTRAN_MODE,
-					    dtran_mode);
-	renesas_sdhi_internal_dmac_dm_write(host, DM_DTRAN_ADDR,
-					    sg_dma_address(sg));
+	writel(dtran_mode, host->ctl + DM_CM_DTRAN_MODE);
+	writel(sg_dma_address(sg), host->ctl + DM_DTRAN_ADDR);
 
 	host->dma_on = true;
 
@@ -420,8 +408,7 @@ static void renesas_sdhi_internal_dmac_issue_tasklet_fn(unsigned long arg)
 	tmio_mmc_enable_mmc_irqs(host, TMIO_STAT_DATAEND);
 
 	/* start the DMAC */
-	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_DTRAN_CTRL,
-					    DTRAN_CTRL_DM_START);
+	writel(DTRAN_CTRL_DM_START, host->ctl + DM_CM_DTRAN_CTRL);
 }
 
 static bool renesas_sdhi_internal_dmac_complete(struct tmio_mmc_host *host)
@@ -502,10 +489,8 @@ renesas_sdhi_internal_dmac_request_dma(struct tmio_mmc_host *host,
 	struct renesas_sdhi *priv = host_to_priv(host);
 
 	/* Disable DMAC interrupts, we don't use them */
-	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO1_MASK,
-					    INFO1_MASK_CLEAR);
-	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_INFO2_MASK,
-					    INFO2_MASK_CLEAR);
+	writel(INFO1_MASK_CLEAR, host->ctl + DM_CM_INFO1_MASK);
+	writel(INFO2_MASK_CLEAR, host->ctl + DM_CM_INFO2_MASK);
 
 	/* Each value is set to non-zero to assume "enabling" each DMA */
 	host->chan_rx = host->chan_tx = (void *)0xdeadbeaf;
-- 
2.35.1

[PATCH RFC 2/6] mmc: renesas_sdhi: improve naming of DMA struct

[Wolfram Sang]

Commit 058db2868cd8 ("mmc: tmio, renesas_sdhi: move struct tmio_mmc_dma
to renesas_sdhi.h") is correct. The DMA struct should be prefixed with
'renesas_sdhi' to avoid confusion about is namespace. Fix some
indentation while here.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi.h      | 8 ++++----
 drivers/mmc/host/renesas_sdhi_core.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index c4abfee1ebae..b661892847f6 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -53,12 +53,12 @@ struct renesas_sdhi_of_data_with_quirks {
 	const struct renesas_sdhi_quirks *quirks;
 };
 
-struct tmio_mmc_dma {
+struct renesas_sdhi_dma {
 	enum dma_slave_buswidth dma_buswidth;
 	bool (*filter)(struct dma_chan *chan, void *arg);
 	void (*enable)(struct tmio_mmc_host *host, bool enable);
-	struct completion	dma_dataend;
-	struct tasklet_struct	dma_complete;
+	struct completion dma_dataend;
+	struct tasklet_struct dma_complete;
 };
 
 struct renesas_sdhi {
@@ -66,7 +66,7 @@ struct renesas_sdhi {
 	struct clk *clkh;
 	struct clk *clk_cd;
 	struct tmio_mmc_data mmc_data;
-	struct tmio_mmc_dma dma_priv;
+	struct renesas_sdhi_dma dma_priv;
 	const struct renesas_sdhi_quirks *quirks;
 	struct pinctrl *pinctrl;
 	struct pinctrl_state *pins_default, *pins_uhs;
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index b970699743e0..0733951325e9 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -908,7 +908,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 {
 	struct tmio_mmc_data *mmd = pdev->dev.platform_data;
 	struct tmio_mmc_data *mmc_data;
-	struct tmio_mmc_dma *dma_priv;
+	struct renesas_sdhi_dma *dma_priv;
 	struct tmio_mmc_host *host;
 	struct renesas_sdhi *priv;
 	int num_irqs, irq, ret, i;
-- 
2.35.1

[PATCH RFC 3/6] mmc: tmio: add callback for dma irq

[Wolfram Sang]

We don't want to rely only on the access_end irq in the future, so
implement a callback for dma irqs.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc.h      | 1 +
 drivers/mmc/host/tmio_mmc_core.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 501613c74406..873a06a179c8 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -128,6 +128,7 @@ struct tmio_mmc_dma_ops {
 
 	/* optional */
 	void (*end)(struct tmio_mmc_host *host);	/* held host->lock */
+	bool (*dma_irq)(struct tmio_mmc_host *host);
 };
 
 struct tmio_mmc_host {
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 437048bb8027..a73422382a57 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -670,6 +670,9 @@ static bool __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host, int ireg,
 		return true;
 	}
 
+	if (host->dma_ops && host->dma_ops->dma_irq && host->dma_ops->dma_irq(host))
+		return true;
+
 	return false;
 }
 
-- 
2.35.1

[PATCH RFC 4/6] mmc: renesas_sdhi: add quirk for broken register layout

[Wolfram Sang]

Some early Gen3 SoCs have the DTRANEND1 bit at a different location than
all later SoCs. Because we need the bit soon, add a quirk so we know
which bit to use.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi.h               | 1 +
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index b661892847f6..fa88b721364c 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -44,6 +44,7 @@ struct renesas_sdhi_quirks {
 	bool fixed_addr_mode;
 	bool dma_one_rx_only;
 	bool manual_tap_correction;
+	bool old_info1_layout;
 	u32 hs400_bad_taps;
 	const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
 };
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 48bea0bd75e8..630ec1e785d6 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -49,7 +49,8 @@
 /* DM_CM_INFO1 and DM_CM_INFO1_MASK */
 #define INFO1_CLEAR		0
 #define INFO1_MASK_CLEAR	GENMASK_ULL(31, 0)
-#define INFO1_DTRANEND1		BIT(17)
+#define INFO1_DTRANEND1		BIT(20)
+#define INFO1_DTRANEND1_OLD	BIT(17)
 #define INFO1_DTRANEND0		BIT(16)
 
 /* DM_CM_INFO2 and DM_CM_INFO2_MASK */
@@ -165,6 +166,7 @@ static const struct renesas_sdhi_quirks sdhi_quirks_4tap_nohs400_one_rx = {
 	.hs400_disabled = true,
 	.hs400_4taps = true,
 	.dma_one_rx_only = true,
+	.old_info1_layout = true,
 };
 
 static const struct renesas_sdhi_quirks sdhi_quirks_4tap = {
-- 
2.35.1

[PATCH RFC 5/6] mmc: renesas_sdhi: take DMA end interrupts into account

[Wolfram Sang]

So far, we have been relying on access_end interrupts only to mark DMA
transfers as done implying that DMA end interrupts have occurred by then
anyhow. On some SoCs under some conditions, this turned out to be not
enough. So, we enable DMA interrupts as well and make sure that both
events, DMA irq and access_end irq, have happened before finishing the
DMA transfer.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi.h               |  5 ++
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 51 ++++++++++++++++---
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index fa88b721364c..8f96457c9739 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -54,7 +54,12 @@ struct renesas_sdhi_of_data_with_quirks {
 	const struct renesas_sdhi_quirks *quirks;
 };
 
+/* We want both end_flags to be set before we mark DMA as finished */
+#define SDHI_DMA_END_FLAG_DMA		BIT(0)
+#define SDHI_DMA_END_FLAG_ACCESS	BIT(1)
+
 struct renesas_sdhi_dma {
+	unsigned long end_flags;
 	enum dma_slave_buswidth dma_buswidth;
 	bool (*filter)(struct dma_chan *chan, void *arg);
 	void (*enable)(struct tmio_mmc_host *host, bool enable);
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 630ec1e785d6..f6d1e04a627f 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -47,7 +47,6 @@
 #define RST_RESERVED_BITS	GENMASK_ULL(31, 0)
 
 /* DM_CM_INFO1 and DM_CM_INFO1_MASK */
-#define INFO1_CLEAR		0
 #define INFO1_MASK_CLEAR	GENMASK_ULL(31, 0)
 #define INFO1_DTRANEND1		BIT(20)
 #define INFO1_DTRANEND1_OLD	BIT(17)
@@ -285,12 +284,14 @@ static void
 renesas_sdhi_internal_dmac_enable_dma(struct tmio_mmc_host *host, bool enable)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
+	u32 dma_irqs = INFO1_DTRANEND0 |
+			(priv->quirks && priv->quirks->old_info1_layout ?
+			INFO1_DTRANEND1_OLD : INFO1_DTRANEND1);
 
 	if (!host->chan_tx || !host->chan_rx)
 		return;
 
-	if (!enable)
-		writel(INFO1_CLEAR, host->ctl + DM_CM_INFO1);
+	writel(enable ? ~dma_irqs : INFO1_MASK_CLEAR, host->ctl + DM_CM_INFO1_MASK);
 
 	if (priv->dma_priv.enable)
 		priv->dma_priv.enable(host, enable);
@@ -311,12 +312,36 @@ renesas_sdhi_internal_dmac_abort_dma(struct tmio_mmc_host *host)
 	renesas_sdhi_internal_dmac_enable_dma(host, true);
 }
 
+static bool renesas_sdhi_internal_dmac_dma_irq(struct tmio_mmc_host *host)
+{
+	struct renesas_sdhi *priv = host_to_priv(host);
+	struct renesas_sdhi_dma *dma_priv = &priv->dma_priv;
+
+	u32 dma_irqs = INFO1_DTRANEND0 |
+			(priv->quirks && priv->quirks->old_info1_layout ?
+			INFO1_DTRANEND1_OLD : INFO1_DTRANEND1);
+	u32 status = readl(host->ctl + DM_CM_INFO1);
+
+	if (status & dma_irqs) {
+		writel(status ^ dma_irqs, host->ctl + DM_CM_INFO1);
+		set_bit(SDHI_DMA_END_FLAG_DMA, &dma_priv->end_flags);
+		if (test_bit(SDHI_DMA_END_FLAG_ACCESS, &dma_priv->end_flags))
+			tasklet_schedule(&dma_priv->dma_complete);
+	}
+
+	return status & dma_irqs;
+}
+
 static void
 renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host)
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
+	struct renesas_sdhi_dma *dma_priv = &priv->dma_priv;
 
-	tasklet_schedule(&priv->dma_priv.dma_complete);
+	set_bit(SDHI_DMA_END_FLAG_ACCESS, &dma_priv->end_flags);
+	if (test_bit(SDHI_DMA_END_FLAG_DMA, &dma_priv->end_flags) ||
+	    host->data->error)
+		tasklet_schedule(&dma_priv->dma_complete);
 }
 
 /*
@@ -386,6 +411,7 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 		dtran_mode |= DTRAN_MODE_CH_NUM_CH0;
 	}
 
+	priv->dma_priv.end_flags = 0;
 	renesas_sdhi_internal_dmac_enable_dma(host, true);
 
 	/* set dma parameters */
@@ -406,11 +432,19 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 static void renesas_sdhi_internal_dmac_issue_tasklet_fn(unsigned long arg)
 {
 	struct tmio_mmc_host *host = (struct tmio_mmc_host *)arg;
+	struct renesas_sdhi *priv = host_to_priv(host);
 
 	tmio_mmc_enable_mmc_irqs(host, TMIO_STAT_DATAEND);
 
-	/* start the DMAC */
-	writel(DTRAN_CTRL_DM_START, host->ctl + DM_CM_DTRAN_CTRL);
+	if (!host->cmd->error) {
+		/* start the DMAC */
+		writel(DTRAN_CTRL_DM_START, host->ctl + DM_CM_DTRAN_CTRL);
+	} else {
+		/* on CMD errors, simulate DMA end immediately */
+		set_bit(SDHI_DMA_END_FLAG_DMA, &priv->dma_priv.end_flags);
+		if (test_bit(SDHI_DMA_END_FLAG_ACCESS, &priv->dma_priv.end_flags))
+			tasklet_schedule(&priv->dma_priv.dma_complete);
+	}
 }
 
 static bool renesas_sdhi_internal_dmac_complete(struct tmio_mmc_host *host)
@@ -490,9 +524,11 @@ renesas_sdhi_internal_dmac_request_dma(struct tmio_mmc_host *host,
 {
 	struct renesas_sdhi *priv = host_to_priv(host);
 
-	/* Disable DMAC interrupts, we don't use them */
+	/* Disable DMAC interrupts initially */
 	writel(INFO1_MASK_CLEAR, host->ctl + DM_CM_INFO1_MASK);
 	writel(INFO2_MASK_CLEAR, host->ctl + DM_CM_INFO2_MASK);
+	writel(0, host->ctl + DM_CM_INFO1);
+	writel(0, host->ctl + DM_CM_INFO2);
 
 	/* Each value is set to non-zero to assume "enabling" each DMA */
 	host->chan_rx = host->chan_tx = (void *)0xdeadbeaf;
@@ -524,6 +560,7 @@ static const struct tmio_mmc_dma_ops renesas_sdhi_internal_dmac_dma_ops = {
 	.abort = renesas_sdhi_internal_dmac_abort_dma,
 	.dataend = renesas_sdhi_internal_dmac_dataend_dma,
 	.end = renesas_sdhi_internal_dmac_end_dma,
+	.dma_irq = renesas_sdhi_internal_dmac_dma_irq,
 };
 
 static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev)
-- 
2.35.1

Re: [PATCH RFC 5/6] mmc: renesas_sdhi: take DMA end interrupts into account

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Oct 6, 2022 at 9:06 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> So far, we have been relying on access_end interrupts only to mark DMA
> transfers as done implying that DMA end interrupts have occurred by then
> anyhow. On some SoCs under some conditions, this turned out to be not
> enough. So, we enable DMA interrupts as well and make sure that both
> events, DMA irq and access_end irq, have happened before finishing the
> DMA transfer.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c

> @@ -285,12 +284,14 @@ static void
>  renesas_sdhi_internal_dmac_enable_dma(struct tmio_mmc_host *host, bool enable)
>  {
>         struct renesas_sdhi *priv = host_to_priv(host);
> +       u32 dma_irqs = INFO1_DTRANEND0 |
> +                       (priv->quirks && priv->quirks->old_info1_layout ?
> +                       INFO1_DTRANEND1_OLD : INFO1_DTRANEND1);

Perhaps it makes sense to store the dma_irqs mask in priv->quirks,
or even in priv, to simplify this code (repeated below)?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC 5/6] mmc: renesas_sdhi: take DMA end interrupts into account

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

Hi Geert,

> > +       u32 dma_irqs = INFO1_DTRANEND0 |
> > +                       (priv->quirks && priv->quirks->old_info1_layout ?
> > +                       INFO1_DTRANEND1_OLD : INFO1_DTRANEND1);
> 
> Perhaps it makes sense to store the dma_irqs mask in priv->quirks,
> or even in priv, to simplify this code (repeated below)?

I tried that, yet didn't find it prettier. I am not strict here, though,
so can change if desired.

Thanks for the review!

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 6/6] DEBUG mmc: renesas_sdhi: debug end_flags for DMA

[Wolfram Sang]

Not for upstream! With these trace_printks, one can verify that DMA is
only finished once both end_flags are set. The order in which they are
set does not matter. Also, cases when CMD or DATA have errors still work

Not-Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index f6d1e04a627f..ac3983aca275 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -325,6 +325,7 @@ static bool renesas_sdhi_internal_dmac_dma_irq(struct tmio_mmc_host *host)
 	if (status & dma_irqs) {
 		writel(status ^ dma_irqs, host->ctl + DM_CM_INFO1);
 		set_bit(SDHI_DMA_END_FLAG_DMA, &dma_priv->end_flags);
+trace_printk("DMA end\n");
 		if (test_bit(SDHI_DMA_END_FLAG_ACCESS, &dma_priv->end_flags))
 			tasklet_schedule(&dma_priv->dma_complete);
 	}
@@ -339,6 +340,7 @@ renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host)
 	struct renesas_sdhi_dma *dma_priv = &priv->dma_priv;
 
 	set_bit(SDHI_DMA_END_FLAG_ACCESS, &dma_priv->end_flags);
+trace_printk("Access end: %d\n", host->data->error);
 	if (test_bit(SDHI_DMA_END_FLAG_DMA, &dma_priv->end_flags) ||
 	    host->data->error)
 		tasklet_schedule(&dma_priv->dma_complete);
@@ -442,6 +444,7 @@ static void renesas_sdhi_internal_dmac_issue_tasklet_fn(unsigned long arg)
 	} else {
 		/* on CMD errors, simulate DMA end immediately */
 		set_bit(SDHI_DMA_END_FLAG_DMA, &priv->dma_priv.end_flags);
+trace_printk("Simulated DMA end\n");
 		if (test_bit(SDHI_DMA_END_FLAG_ACCESS, &priv->dma_priv.end_flags))
 			tasklet_schedule(&priv->dma_priv.dma_complete);
 	}
@@ -477,6 +480,7 @@ static void renesas_sdhi_internal_dmac_complete_tasklet_fn(unsigned long arg)
 {
 	struct tmio_mmc_host *host = (struct tmio_mmc_host *)arg;
 
+trace_printk("Tasklet\n");
 	spin_lock_irq(&host->lock);
 	if (!renesas_sdhi_internal_dmac_complete(host))
 		goto out;
-- 
2.35.1

Re: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Ulf Hansson]

Hi Wolfram,

On Thu, 6 Oct 2022 at 21:05, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Motivation for this series from patch 5:
>
> ===
> So far, we have been relying on access_end interrupts only to mark DMA
> transfers as done implying that DMA end interrupts have occurred by then
> anyhow. On some SoCs under some conditions, this turned out to be not
> enough. So, we enable DMA interrupts as well and make sure that both
> events, DMA irq and access_end irq, have happened before finishing the
> DMA transfer.

The tmio/sdhi core still relies on using tasklets. I think we should
strive to move away from tasklets for all mmc host drivers and to use
threaded irqs instead.

That said, I am worried that it might be harder to move away from
tasklets beyond $subject series, for tmio/sdhi, but I might be wrong?
So, I am wondering if it perhaps would be better to make that
modernization/conversion as the first step instead?

Kind regards
Uffe


> ===
>
> The first two patches are cleanups. For the rest, the basis were BSP
> patches, but they have been refactored heavily, so they look very
> different now:
>
> * self-contained
>   except for the new dma_irq callback which is for the TMIO core, all
>   other code is self-contained in renesas_sdhi_internal_dmac now.
> * concise
>   Less new members for the existing structs, also code duplication was
>   avoided by moving code to more suitable locations
> * replaced the spinlock with atomic bit operators
> * used quirk mechanism for the old INFO1 layout
>
> Tested on a Salvator-X board with M3-W (r8a77960) and a Salvator-XS
> board with M3-N (r8a77965). No regression encountered in functionality
> and performance. A branch can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/upport_dmaend
>
>
> Here are excerpts of a log when booting the M3-N with patch 6 applied to
> show that all combinations of incoming irqs are handled:
>
> === DMA first, Access second ===
>
>           <idle>-0       [000] d.h..     0.505454: renesas_sdhi_internal_dmac_dma_irq: DMA end
>           <idle>-0       [000] d.h..     0.505496: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
>           <idle>-0       [000] ..s..     0.505498: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet
>
> === Access first, DMA second ===
>
>      kworker/0:2-42      [000] d.h..     0.510603: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
>      kworker/0:2-42      [000] d.h..     0.510605: renesas_sdhi_internal_dmac_dma_irq: DMA end
>      kworker/0:2-42      [000] ..s..     0.510606: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet
>
> === Access first, Simulated DMA second ===
>
>           <idle>-0       [000] d.H..     0.510635: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
>           <idle>-0       [000] ..s..     0.510637: renesas_sdhi_internal_dmac_issue_tasklet_fn: Simulated DMA end
>           <idle>-0       [000] ..s..     0.510638: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet
>
> (I have never seen Simulated DMA (= CMD error happened) first, but it
> should be handled like regular DMA end as well(tm).)
>
> === Access first, no DMA end needed because of DATA error (EILSEQ) ===
>
>           <idle>-0       [000] d.H..     0.510894: renesas_sdhi_internal_dmac_dataend_dma: Access end: -84
>           <idle>-0       [000] ..s..     0.510896: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet
>
> ===
>
> I think this is as far as I can reach alone. I'd love to get review
> comments and further testing. Shimoda-san, can you kindly ask the BSP
> team to do further testing?
>
> Thank you everyone and happy hacking,
>
>    Wolfram
>
>
> Wolfram Sang (6):
>   mmc: renesas_sdhi: remove accessor function for internal_dmac
>   mmc: renesas_sdhi: improve naming of DMA struct
>   mmc: tmio: add callback for dma irq
>   mmc: renesas_sdhi: add quirk for broken register layout
>   mmc: renesas_sdhi: take DMA end interrupts into account
>   DEBUG mmc: renesas_sdhi: debug end_flags for DMA
>
>  drivers/mmc/host/renesas_sdhi.h               | 14 ++-
>  drivers/mmc/host/renesas_sdhi_core.c          |  2 +-
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 86 ++++++++++++-------
>  drivers/mmc/host/tmio_mmc.h                   |  1 +
>  drivers/mmc/host/tmio_mmc_core.c              |  3 +
>  5 files changed, 72 insertions(+), 34 deletions(-)
>
> --
> 2.35.1
>

Re: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Arnd Bergmann]

On Tue, Oct 25, 2022, at 12:51, Ulf Hansson wrote:
> Hi Wolfram,
>
> On Thu, 6 Oct 2022 at 21:05, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>>
>> Motivation for this series from patch 5:
>>
>> ===
>> So far, we have been relying on access_end interrupts only to mark DMA
>> transfers as done implying that DMA end interrupts have occurred by then
>> anyhow. On some SoCs under some conditions, this turned out to be not
>> enough. So, we enable DMA interrupts as well and make sure that both
>> events, DMA irq and access_end irq, have happened before finishing the
>> DMA transfer.
>
> The tmio/sdhi core still relies on using tasklets. I think we should
> strive to move away from tasklets for all mmc host drivers and to use
> threaded irqs instead.
>
> That said, I am worried that it might be harder to move away from
> tasklets beyond $subject series, for tmio/sdhi, but I might be wrong?
> So, I am wondering if it perhaps would be better to make that
> modernization/conversion as the first step instead?

Moving away from tasklets is probably a good idea overall, but I'm
not sure that MMC actually needs a custom IRQ deferral mechanism
in addition to the existing BLOCK_SOFTIRQ. I would expect that block
drivers usually operate in the context of the blk_mq caller, and
adding in another thread context can add substantial latency.

    Arnd

Re: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Ulf Hansson]

On Tue, 25 Oct 2022 at 13:04, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Oct 25, 2022, at 12:51, Ulf Hansson wrote:
> > Hi Wolfram,
> >
> > On Thu, 6 Oct 2022 at 21:05, Wolfram Sang
> > <wsa+renesas@sang-engineering.com> wrote:
> >>
> >> Motivation for this series from patch 5:
> >>
> >> ===
> >> So far, we have been relying on access_end interrupts only to mark DMA
> >> transfers as done implying that DMA end interrupts have occurred by then
> >> anyhow. On some SoCs under some conditions, this turned out to be not
> >> enough. So, we enable DMA interrupts as well and make sure that both
> >> events, DMA irq and access_end irq, have happened before finishing the
> >> DMA transfer.
> >
> > The tmio/sdhi core still relies on using tasklets. I think we should
> > strive to move away from tasklets for all mmc host drivers and to use
> > threaded irqs instead.
> >
> > That said, I am worried that it might be harder to move away from
> > tasklets beyond $subject series, for tmio/sdhi, but I might be wrong?
> > So, I am wondering if it perhaps would be better to make that
> > modernization/conversion as the first step instead?
>
> Moving away from tasklets is probably a good idea overall, but I'm
> not sure that MMC actually needs a custom IRQ deferral mechanism
> in addition to the existing BLOCK_SOFTIRQ. I would expect that block
> drivers usually operate in the context of the blk_mq caller, and
> adding in another thread context can add substantial latency.

Well, it's not that simple as it depends on what the MMC controller
supports too (and whether the MMC/SD card really conforms to the
specs). We can't poll for busy completions in IRQ context, for
example. And in some cases, we simply need to poll with a CMD13.

Don't get me wrong, I am not promoting a custom deferral mechanism for
MMC, but just a regular threaded IRQ handler (per host driver of
course) when that is needed. And most mmc host drivers already have
that today.

For more sophisticated HWs, we have the mmc hsq interface, that host
drivers can hook into, to potentially avoid some unnecessary context
switchings.

>
>     Arnd

Kind regards
Uffe

Re: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

Hi Ulf,

> The tmio/sdhi core still relies on using tasklets. I think we should
> strive to move away from tasklets for all mmc host drivers and to use
> threaded irqs instead.

Ooookay, noted. I'll put it on my todo-list. But frankly, I can't give
this priority for a while :/ SDHI is used on a lot of platforms, even on
different architectures. Regression testing will be a big one here.

> That said, I am worried that it might be harder to move away from
> tasklets beyond $subject series, for tmio/sdhi, but I might be wrong?
> So, I am wondering if it perhaps would be better to make that
> modernization/conversion as the first step instead?

As said above, I can't do this in the foreseeable future. However, this
series fixes an issue we see. So, my vote goes for applying this now.
Even if it costs some extra cycles on the tasklet-removal-work.

D'accord?

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Ulf Hansson]

On Wed, 2 Nov 2022 at 20:14, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Ulf,
>
> > The tmio/sdhi core still relies on using tasklets. I think we should
> > strive to move away from tasklets for all mmc host drivers and to use
> > threaded irqs instead.
>
> Ooookay, noted. I'll put it on my todo-list. But frankly, I can't give
> this priority for a while :/ SDHI is used on a lot of platforms, even on
> different architectures. Regression testing will be a big one here.
>
> > That said, I am worried that it might be harder to move away from
> > tasklets beyond $subject series, for tmio/sdhi, but I might be wrong?
> > So, I am wondering if it perhaps would be better to make that
> > modernization/conversion as the first step instead?
>
> As said above, I can't do this in the foreseeable future. However, this
> series fixes an issue we see. So, my vote goes for applying this now.
> Even if it costs some extra cycles on the tasklet-removal-work.
>
> D'accord?

Sure, I am fine with applying this. I just wanted to point out my long
term thoughts around using the tasklets.

Do you intend to send a new version to drop the RFC? Or can I apply as is?

Kind regards
Uffe

Re: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]


> Sure, I am fine with applying this. I just wanted to point out my long
> term thoughts around using the tasklets.

Yes, I made an action item for that.

> Do you intend to send a new version to drop the RFC? Or can I apply as is?

It should have actually been named RFT. I'd like the BSP team to test it
if they have time. I will ping them.

Thanks, Ulf!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Friday, November 4, 2022 3:24 AM
> 
> > Do you intend to send a new version to drop the RFC? Or can I apply as is?
> 
> It should have actually been named RFT. I'd like the BSP team to test it
> if they have time. I will ping them.

I'm sorry for delayed the response.
Our test team tested this patch series on R-Car E3 board and they didn't observe
any regression. So,

Tested-by: Duy Nguyen <duy.nguyen.rh@renesas.com>

And, I tested this on R-Car H3 board. So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

> Thanks, Ulf!

Re: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]


> Our test team tested this patch series on R-Car E3 board and they didn't observe
> any regression. So,
> 
> Tested-by: Duy Nguyen <duy.nguyen.rh@renesas.com>
> 
> And, I tested this on R-Car H3 board. So,
> 
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thank you very much, Shimoda-san! Have a nice weekend.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 0/6] mmc: renesas_sdhi: add support for DMA end irqs

[Ulf Hansson]

On Thu, 6 Oct 2022 at 21:05, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Motivation for this series from patch 5:
>
> ===
> So far, we have been relying on access_end interrupts only to mark DMA
> transfers as done implying that DMA end interrupts have occurred by then
> anyhow. On some SoCs under some conditions, this turned out to be not
> enough. So, we enable DMA interrupts as well and make sure that both
> events, DMA irq and access_end irq, have happened before finishing the
> DMA transfer.
> ===
>
> The first two patches are cleanups. For the rest, the basis were BSP
> patches, but they have been refactored heavily, so they look very
> different now:
>
> * self-contained
>   except for the new dma_irq callback which is for the TMIO core, all
>   other code is self-contained in renesas_sdhi_internal_dmac now.
> * concise
>   Less new members for the existing structs, also code duplication was
>   avoided by moving code to more suitable locations
> * replaced the spinlock with atomic bit operators
> * used quirk mechanism for the old INFO1 layout
>
> Tested on a Salvator-X board with M3-W (r8a77960) and a Salvator-XS
> board with M3-N (r8a77965). No regression encountered in functionality
> and performance. A branch can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/upport_dmaend
>
>
> Here are excerpts of a log when booting the M3-N with patch 6 applied to
> show that all combinations of incoming irqs are handled:
>
> === DMA first, Access second ===
>
>           <idle>-0       [000] d.h..     0.505454: renesas_sdhi_internal_dmac_dma_irq: DMA end
>           <idle>-0       [000] d.h..     0.505496: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
>           <idle>-0       [000] ..s..     0.505498: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet
>
> === Access first, DMA second ===
>
>      kworker/0:2-42      [000] d.h..     0.510603: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
>      kworker/0:2-42      [000] d.h..     0.510605: renesas_sdhi_internal_dmac_dma_irq: DMA end
>      kworker/0:2-42      [000] ..s..     0.510606: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet
>
> === Access first, Simulated DMA second ===
>
>           <idle>-0       [000] d.H..     0.510635: renesas_sdhi_internal_dmac_dataend_dma: Access end: 0
>           <idle>-0       [000] ..s..     0.510637: renesas_sdhi_internal_dmac_issue_tasklet_fn: Simulated DMA end
>           <idle>-0       [000] ..s..     0.510638: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet
>
> (I have never seen Simulated DMA (= CMD error happened) first, but it
> should be handled like regular DMA end as well(tm).)
>
> === Access first, no DMA end needed because of DATA error (EILSEQ) ===
>
>           <idle>-0       [000] d.H..     0.510894: renesas_sdhi_internal_dmac_dataend_dma: Access end: -84
>           <idle>-0       [000] ..s..     0.510896: renesas_sdhi_internal_dmac_complete_tasklet_fn: Tasklet
>
> ===
>
> I think this is as far as I can reach alone. I'd love to get review
> comments and further testing. Shimoda-san, can you kindly ask the BSP
> team to do further testing?
>
> Thank you everyone and happy hacking,
>
>    Wolfram
>
>
> Wolfram Sang (6):
>   mmc: renesas_sdhi: remove accessor function for internal_dmac
>   mmc: renesas_sdhi: improve naming of DMA struct
>   mmc: tmio: add callback for dma irq
>   mmc: renesas_sdhi: add quirk for broken register layout
>   mmc: renesas_sdhi: take DMA end interrupts into account
>   DEBUG mmc: renesas_sdhi: debug end_flags for DMA
>
>  drivers/mmc/host/renesas_sdhi.h               | 14 ++-
>  drivers/mmc/host/renesas_sdhi_core.c          |  2 +-
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 86 ++++++++++++-------
>  drivers/mmc/host/tmio_mmc.h                   |  1 +
>  drivers/mmc/host/tmio_mmc_core.c              |  3 +
>  5 files changed, 72 insertions(+), 34 deletions(-)
>

Patch 1->5 applied for next, thanks!

Kind regards
Uffe