[PATCH 0/4] dmaengine: ti: cppi41: Fix suspend/resume issues

[PATCH 0/4] dmaengine: ti: cppi41: Fix suspend/resume issues

[Markus Schneider-Pargmann (TI)]

Hi,

there are some issues with cppi41 regarding suspend/resume. This series
fixes suspend/resume when the device is currently runtime suspended. It
also fixes an incomplete restore of QMGR_MEMCTRL during resume. To make
the restore on resume more reliable the last patch unifies the register
writes for init and resume.

I tested this series with am335x-evm

Best,
Markus

Signed-off-by: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
---
Markus Schneider-Pargmann (TI) (4):
      dmaengine: ti: cppi41: Use td_queue to restore DMA_TDFDQ
      dmaengine: ti: cppi41: Fix register access in suspend_late
      dmaengine: ti: cppi41: Restore QMGR_MEMCTRL in resume
      dmaengine: ti: cppi41: Share register setup with resume

 drivers/dma/ti/cppi41.c | 74 ++++++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 31 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261030-v7-3-topic-am335lpm-cppi41-b180a2d62987

Best regards,
--  
Markus Schneider-Pargmann (TI) <msp@baylibre.com>

[PATCH 1/4] dmaengine: ti: cppi41: Use td_queue to restore DMA_TDFDQ

[Markus Schneider-Pargmann (TI)]

In init_cppi41() td_queue.submit is used to setup DMA_TDFDQ. In
suspend/resume the same value is read, stored and restored.

To get rid of the suspend register access, use td_queue.submit in resume
as well to restore the value.

Cc: stable@vger.kernel.org
Signed-off-by: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
---
 drivers/dma/ti/cppi41.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c
index 88756dccd62c..458d20b12462 100644
--- a/drivers/dma/ti/cppi41.c
+++ b/drivers/dma/ti/cppi41.c
@@ -146,9 +146,6 @@ struct cppi41_dd {
 	struct list_head pending;	/* Pending queued transfers */
 	spinlock_t lock;		/* Lock for pending list */
 
-	/* context for suspend/resume */
-	unsigned int dma_tdfdq;
-
 	bool is_suspended;
 };
 
@@ -1174,7 +1171,6 @@ static int __maybe_unused cppi41_suspend(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
 
-	cdd->dma_tdfdq = cppi_readl(cdd->ctrl_mem + DMA_TDFDQ);
 	disable_sched(cdd);
 
 	return 0;
@@ -1195,7 +1191,7 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 
 	init_sched(cdd);
 
-	cppi_writel(cdd->dma_tdfdq, cdd->ctrl_mem + DMA_TDFDQ);
+	cppi_writel(cdd->td_queue.submit, cdd->ctrl_mem + DMA_TDFDQ);
 	cppi_writel(cdd->scratch_phys, cdd->qmgr_mem + QMGR_LRAM0_BASE);
 	cppi_writel(QMGR_SCRATCH_SIZE, cdd->qmgr_mem + QMGR_LRAM_SIZE);
 	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM1_BASE);

-- 
2.55.0

[PATCH 2/4] dmaengine: ti: cppi41: Fix register access in suspend_late

[Markus Schneider-Pargmann (TI)]

cppi41_suspend() and cppi41_resume() access registers. However the
device may be runtime suspended in which case it is not allowed to
access the registers.

On suspend do not run disable_sched() when it is runtime suspended. This
avoids accessing the registers and crashing the kernel during suspend.

Resume when runtime suspended is delayed until cppi41_runtime_resume()
is called using the variable ctx_lost. This keeps any access while the
device is runtime suspended.

Fixes: fdea2d09b997 ("dmaengine: cppi41: Add basic PM runtime support")
Cc: stable@vger.kernel.org
Signed-off-by: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
---
 drivers/dma/ti/cppi41.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c
index 458d20b12462..f5a824418fe7 100644
--- a/drivers/dma/ti/cppi41.c
+++ b/drivers/dma/ti/cppi41.c
@@ -146,6 +146,9 @@ struct cppi41_dd {
 	struct list_head pending;	/* Pending queued transfers */
 	spinlock_t lock;		/* Lock for pending list */
 
+	/* Register context may have been lost during system suspend */
+	bool ctx_lost;
+
 	bool is_suspended;
 };
 
@@ -1171,7 +1174,10 @@ static int __maybe_unused cppi41_suspend(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
 
-	disable_sched(cdd);
+	if (!pm_runtime_status_suspended(dev))
+		disable_sched(cdd);
+
+	cdd->ctx_lost = true;
 
 	return 0;
 }
@@ -1182,6 +1188,9 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 	struct cppi41_channel *c;
 	int i;
 
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
 	for (i = 0; i < DESCS_AREAS; i++)
 		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
 
@@ -1196,6 +1205,8 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 	cppi_writel(QMGR_SCRATCH_SIZE, cdd->qmgr_mem + QMGR_LRAM_SIZE);
 	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM1_BASE);
 
+	cdd->ctx_lost = false;
+
 	return 0;
 }
 
@@ -1217,6 +1228,9 @@ static int __maybe_unused cppi41_runtime_resume(struct device *dev)
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
 	unsigned long flags;
 
+	if (cdd->ctx_lost)
+		cppi41_resume(dev);
+
 	spin_lock_irqsave(&cdd->lock, flags);
 	cdd->is_suspended = false;
 	cppi41_run_queue(cdd);

-- 
2.55.0

[PATCH 3/4] dmaengine: ti: cppi41: Restore QMGR_MEMCTRL in resume

[Markus Schneider-Pargmann (TI)]

Currently only QMGR_MEMBASE is restored in resume, QMGR_MEMCTRL is left
at its default values.

Add a restore for QMGR_MEMCTRL. This introduces a helper function
cppi41_memctrl() to not duplicate this calcuation in init and resume.

Fixes: f97b98d6d02e ("dma: cppi41: add support for suspend and resume")
Cc: stable@vger.kernel.org
Signed-off-by: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
---
 drivers/dma/ti/cppi41.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c
index f5a824418fe7..e2a089066c6d 100644
--- a/drivers/dma/ti/cppi41.c
+++ b/drivers/dma/ti/cppi41.c
@@ -842,13 +842,22 @@ static void deinit_cppi41(struct device *dev, struct cppi41_dd *cdd)
 			cdd->scratch_phys);
 }
 
+static u32 cppi41_memctrl(int area)
+{
+	u32 reg;
+
+	reg = (area * ALLOC_DECS_NUM) << QMGR_MEMCTRL_IDX_SH;
+	reg |= (ilog2(sizeof(struct cppi41_desc)) - 5) << QMGR_MEMCTRL_DESC_SH;
+	reg |= ilog2(ALLOC_DECS_NUM) - 5;
+
+	return reg;
+}
+
 static int init_descs(struct device *dev, struct cppi41_dd *cdd)
 {
 	unsigned int desc_size;
 	unsigned int mem_decs;
 	int i;
-	u32 reg;
-	u32 idx;
 
 	BUILD_BUG_ON(sizeof(struct cppi41_desc) &
 			(sizeof(struct cppi41_desc) - 1));
@@ -858,13 +867,7 @@ static int init_descs(struct device *dev, struct cppi41_dd *cdd)
 	desc_size = sizeof(struct cppi41_desc);
 	mem_decs = ALLOC_DECS_NUM * desc_size;
 
-	idx = 0;
 	for (i = 0; i < DESCS_AREAS; i++) {
-
-		reg = idx << QMGR_MEMCTRL_IDX_SH;
-		reg |= (ilog2(desc_size) - 5) << QMGR_MEMCTRL_DESC_SH;
-		reg |= ilog2(ALLOC_DECS_NUM) - 5;
-
 		BUILD_BUG_ON(DESCS_AREAS != 1);
 		cdd->cd = dma_alloc_coherent(dev, mem_decs,
 				&cdd->descs_phys, GFP_KERNEL);
@@ -872,9 +875,7 @@ static int init_descs(struct device *dev, struct cppi41_dd *cdd)
 			return -ENOMEM;
 
 		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
-		cppi_writel(reg, cdd->qmgr_mem + QMGR_MEMCTRL(i));
-
-		idx += ALLOC_DECS_NUM;
+		cppi_writel(cppi41_memctrl(i), cdd->qmgr_mem + QMGR_MEMCTRL(i));
 	}
 	return 0;
 }
@@ -1191,8 +1192,10 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 	if (pm_runtime_status_suspended(dev))
 		return 0;
 
-	for (i = 0; i < DESCS_AREAS; i++)
+	for (i = 0; i < DESCS_AREAS; i++) {
 		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
+		cppi_writel(cppi41_memctrl(i), cdd->qmgr_mem + QMGR_MEMCTRL(i));
+	}
 
 	list_for_each_entry(c, &cdd->ddev.channels, chan.device_node)
 		if (!c->is_tx)

-- 
2.55.0

[PATCH 4/4] dmaengine: ti: cppi41: Share register setup with resume

[Markus Schneider-Pargmann (TI)]

Both probe and resume basically do the same thing and setup the
registers. But resume has its own copy of the register writes. This will
always lead to differences when changes are made.

Create a separate function that is used by both probe and resume.
Note that the ordering is slightly different but I didn't see or found
any issues with the new order while testing.

Signed-off-by: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
---
 drivers/dma/ti/cppi41.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c
index e2a089066c6d..b62a26c2659d 100644
--- a/drivers/dma/ti/cppi41.c
+++ b/drivers/dma/ti/cppi41.c
@@ -873,9 +873,6 @@ static int init_descs(struct device *dev, struct cppi41_dd *cdd)
 				&cdd->descs_phys, GFP_KERNEL);
 		if (!cdd->cd)
 			return -ENOMEM;
-
-		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
-		cppi_writel(cppi41_memctrl(i), cdd->qmgr_mem + QMGR_MEMCTRL(i));
 	}
 	return 0;
 }
@@ -903,6 +900,24 @@ static void init_sched(struct cppi41_dd *cdd)
 	cppi_writel(reg, cdd->sched_mem + DMA_SCHED_CTRL);
 }
 
+/* Program the registers, also used to restore the context on resume */
+static void cppi41_init_hw(struct cppi41_dd *cdd)
+{
+	int i;
+
+	cppi_writel(cdd->scratch_phys, cdd->qmgr_mem + QMGR_LRAM0_BASE);
+	cppi_writel(TOTAL_DESCS_NUM, cdd->qmgr_mem + QMGR_LRAM_SIZE);
+	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM1_BASE);
+
+	for (i = 0; i < DESCS_AREAS; i++) {
+		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
+		cppi_writel(cppi41_memctrl(i), cdd->qmgr_mem + QMGR_MEMCTRL(i));
+	}
+
+	cppi_writel(cdd->td_queue.submit, cdd->ctrl_mem + DMA_TDFDQ);
+	init_sched(cdd);
+}
+
 static int init_cppi41(struct device *dev, struct cppi41_dd *cdd)
 {
 	int ret;
@@ -913,16 +928,11 @@ static int init_cppi41(struct device *dev, struct cppi41_dd *cdd)
 	if (!cdd->qmgr_scratch)
 		return -ENOMEM;
 
-	cppi_writel(cdd->scratch_phys, cdd->qmgr_mem + QMGR_LRAM0_BASE);
-	cppi_writel(TOTAL_DESCS_NUM, cdd->qmgr_mem + QMGR_LRAM_SIZE);
-	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM1_BASE);
-
 	ret = init_descs(dev, cdd);
 	if (ret)
 		goto err_td;
 
-	cppi_writel(cdd->td_queue.submit, cdd->ctrl_mem + DMA_TDFDQ);
-	init_sched(cdd);
+	cppi41_init_hw(cdd);
 
 	return 0;
 err_td:
@@ -1187,26 +1197,15 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 {
 	struct cppi41_dd *cdd = dev_get_drvdata(dev);
 	struct cppi41_channel *c;
-	int i;
 
 	if (pm_runtime_status_suspended(dev))
 		return 0;
 
-	for (i = 0; i < DESCS_AREAS; i++) {
-		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
-		cppi_writel(cppi41_memctrl(i), cdd->qmgr_mem + QMGR_MEMCTRL(i));
-	}
-
 	list_for_each_entry(c, &cdd->ddev.channels, chan.device_node)
 		if (!c->is_tx)
 			cppi_writel(c->q_num, c->gcr_reg + RXHPCRA0);
 
-	init_sched(cdd);
-
-	cppi_writel(cdd->td_queue.submit, cdd->ctrl_mem + DMA_TDFDQ);
-	cppi_writel(cdd->scratch_phys, cdd->qmgr_mem + QMGR_LRAM0_BASE);
-	cppi_writel(QMGR_SCRATCH_SIZE, cdd->qmgr_mem + QMGR_LRAM_SIZE);
-	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM1_BASE);
+	cppi41_init_hw(cdd);
 
 	cdd->ctx_lost = false;
 

-- 
2.55.0