dma: ste_dma40: Maintain spinlock order while handling pause

PATCH New Ulf Hansson 4 participants 12 messages Apr 23, 2014 – May 7, 2014

Subsystems (2)

dmaengine Arm/nomadik/Ux500 architectures

lore

Share & tools

lore

Ulf Hansson

author

2014-04-23 19:52 UTC #0 lore raw

PATCH 1/4 dma: ste_dma40: Maintain spinlock order while handling pause

The runtime PM resume callback needs to be executed while holding the spinlock, make sure to maintain this for the pause operation as well.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/dma/ste_dma40.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dma/ste_dma40.c +1 -1

index bf18c78..6e97cf6 100644
@@ -1495,8 +1495,8 @@ static int d40_pause(struct d40_chan *d40c)
 	if (!d40c->busy)
 		return 0;
 
-	pm_runtime_get_sync(d40c->base->dev);
 	spin_lock_irqsave(&d40c->lock, flags);
+	pm_runtime_get_sync(d40c->base->dev);
 
 	res = d40_channel_execute_command(d40c, D40_DMA_SUSPEND_REQ);

Signature · 1 line

1.7.9.5

Ulf Hansson

author

2014-04-23 19:52 UTC #1 lore raw

PATCH 2/4 dma: ste_dma40: Don't require CONFIG_PM_RUNTIME

While probing, don't rely on CONFIG_PM_RUNTIME to be configured. Instead, let's power up the device and make it fully operational. Update the runtime PM status to reflect the active state.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/dma/ste_dma40.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

drivers/dma/ste_dma40.c +10 ~~-11~~

index 6e97cf6..45e809f 100644
@@ -556,7 +556,6 @@ struct d40_gen_dmac {
  * later
  * @reg_val_backup_chan: Backup data for standard channel parameter registers.
  * @gcc_pwr_off_mask: Mask to maintain the channels that can be turned off.
- * @initialized: true if the dma has been initialized
  * @gen_dmac: the struct for generic registers values to represent u8500/8540
  * DMA controller
  */

@@ -594,7 +593,6 @@ struct d40_base {
 	u32				  reg_val_backup_v4[BACKUP_REGS_SZ_MAX];
 	u32				 *reg_val_backup_chan;
 	u16				  gcc_pwr_off_mask;
-	bool				  initialized;
 	struct d40_gen_dmac		  gen_dmac;
 };

@@ -3030,8 +3028,7 @@ static int dma40_runtime_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct d40_base *base = platform_get_drvdata(pdev);
 
-	if (base->initialized)
-		d40_save_restore_registers(base, false);
+	d40_save_restore_registers(base, false);
 
 	writel_relaxed(D40_DREG_GCC_ENABLE_ALL,
 		       base->virtbase + D40_DREG_GCC);

@@ -3645,12 +3642,6 @@ static int __init d40_probe(struct platform_device *pdev)
 		goto failure;
 	}
 
-	pm_runtime_irq_safe(base->dev);
-	pm_runtime_set_autosuspend_delay(base->dev, DMA40_AUTOSUSPEND_DELAY);
-	pm_runtime_use_autosuspend(base->dev);
-	pm_runtime_enable(base->dev);
-	pm_runtime_resume(base->dev);
-
 	if (base->plat_data->use_esram_lcla) {
 
 		base->lcpa_regulator = regulator_get(base->dev, "lcla_esram");

@@ -3671,7 +3662,15 @@ static int __init d40_probe(struct platform_device *pdev)
 		}
 	}
 
-	base->initialized = true;
+	writel_relaxed(D40_DREG_GCC_ENABLE_ALL, base->virtbase + D40_DREG_GCC);
+
+	pm_runtime_irq_safe(base->dev);
+	pm_runtime_set_autosuspend_delay(base->dev, DMA40_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(base->dev);
+	pm_runtime_mark_last_busy(base->dev);
+	pm_runtime_set_active(base->dev);
+	pm_runtime_enable(base->dev);
+
 	ret = d40_dmaengine_init(base, num_reserved_chans);
 	if (ret)
 		goto failure;

Signature · 1 line

1.7.9.5

Linus Walleij

2014-04-24 12:54 UTC #2 lore raw

PATCH 2/4 dma: ste_dma40: Don't require CONFIG_PM_RUNTIME

Quoted 5 lines · Ulf Hansson

While probing, don't rely on CONFIG_PM_RUNTIME to be configured. Instead, let's power up the device and make it fully operational. Update the runtime PM status to reflect the active state.
Signed-off-by Ulf Hansson <[email protected]>

Acked-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Ulf Hansson

author

2014-04-23 19:52 UTC #3 lore raw

PATCH 3/4 dma: ste_dma40: Convert to PM macros while providing the PM callbacks

In reply to Ulf Hansson

Converting to the PM macros makes us simplify and remove some code.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/dma/ste_dma40.c |  150 ++++++++++++++++++++++-------------------------
 1 file changed, 71 insertions(+), 79 deletions(-)

drivers/dma/ste_dma40.c +71 ~~-79~~

index 45e809f..0b29af3 100644
@@ -1054,62 +1054,6 @@ static int d40_sg_2_dmalen(struct scatterlist *sgl, int sg_len,
 	return len;
 }
 
-
-#ifdef CONFIG_PM
-static void dma40_backup(void __iomem *baseaddr, u32 *backup,
-			 u32 *regaddr, int num, bool save)
-{
-	int i;
-
-	for (i = 0; i < num; i++) {
-		void __iomem *addr = baseaddr + regaddr[i];
-
-		if (save)
-			backup[i] = readl_relaxed(addr);
-		else
-			writel_relaxed(backup[i], addr);
-	}
-}
-
-static void d40_save_restore_registers(struct d40_base *base, bool save)
-{
-	int i;
-
-	/* Save/Restore channel specific registers */
-	for (i = 0; i < base->num_phy_chans; i++) {
-		void __iomem *addr;
-		int idx;
-
-		if (base->phy_res[i].reserved)
-			continue;
-
-		addr = base->virtbase + D40_DREG_PCBASE + i * D40_DREG_PCDELTA;
-		idx = i * ARRAY_SIZE(d40_backup_regs_chan);
-
-		dma40_backup(addr, &base->reg_val_backup_chan[idx],
-			     d40_backup_regs_chan,
-			     ARRAY_SIZE(d40_backup_regs_chan),
-			     save);
-	}
-
-	/* Save/Restore global registers */
-	dma40_backup(base->virtbase, base->reg_val_backup,
-		     d40_backup_regs, ARRAY_SIZE(d40_backup_regs),
-		     save);
-
-	/* Save/Restore registers only existing on dma40 v3 and later */
-	if (base->gen_dmac.backup)
-		dma40_backup(base->virtbase, base->reg_val_backup_v4,
-			     base->gen_dmac.backup,
-			base->gen_dmac.backup_size,
-			save);
-}
-#else
-static void d40_save_restore_registers(struct d40_base *base, bool save)
-{
-}
-#endif
-
 static int __d40_execute_command_phy(struct d40_chan *d40c,
 				     enum d40_command command)
 {

@@ -2996,8 +2940,8 @@ failure1:
 }
 
 /* Suspend resume functionality */
-#ifdef CONFIG_PM
-static int dma40_pm_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int dma40_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct d40_base *base = platform_get_drvdata(pdev);

@@ -3008,6 +2952,69 @@ static int dma40_pm_suspend(struct device *dev)
 	return ret;
 }
 
+static int dma40_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct d40_base *base = platform_get_drvdata(pdev);
+	int ret = 0;
+
+	if (base->lcpa_regulator)
+		ret = regulator_enable(base->lcpa_regulator);
+
+	return ret;
+}
+#endif
+
+#ifdef CONFIG_PM
+static void dma40_backup(void __iomem *baseaddr, u32 *backup,
+			 u32 *regaddr, int num, bool save)
+{
+	int i;
+
+	for (i = 0; i < num; i++) {
+		void __iomem *addr = baseaddr + regaddr[i];
+
+		if (save)
+			backup[i] = readl_relaxed(addr);
+		else
+			writel_relaxed(backup[i], addr);
+	}
+}
+
+static void d40_save_restore_registers(struct d40_base *base, bool save)
+{
+	int i;
+
+	/* Save/Restore channel specific registers */
+	for (i = 0; i < base->num_phy_chans; i++) {
+		void __iomem *addr;
+		int idx;
+
+		if (base->phy_res[i].reserved)
+			continue;
+
+		addr = base->virtbase + D40_DREG_PCBASE + i * D40_DREG_PCDELTA;
+		idx = i * ARRAY_SIZE(d40_backup_regs_chan);
+
+		dma40_backup(addr, &base->reg_val_backup_chan[idx],
+			     d40_backup_regs_chan,
+			     ARRAY_SIZE(d40_backup_regs_chan),
+			     save);
+	}
+
+	/* Save/Restore global registers */
+	dma40_backup(base->virtbase, base->reg_val_backup,
+		     d40_backup_regs, ARRAY_SIZE(d40_backup_regs),
+		     save);
+
+	/* Save/Restore registers only existing on dma40 v3 and later */
+	if (base->gen_dmac.backup)
+		dma40_backup(base->virtbase, base->reg_val_backup_v4,
+			     base->gen_dmac.backup,
+			base->gen_dmac.backup_size,
+			save);
+}
+
 static int dma40_runtime_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);

@@ -3034,29 +3041,14 @@ static int dma40_runtime_resume(struct device *dev)
 		       base->virtbase + D40_DREG_GCC);
 	return 0;
 }
-
-static int dma40_resume(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct d40_base *base = platform_get_drvdata(pdev);
-	int ret = 0;
-
-	if (base->lcpa_regulator)
-		ret = regulator_enable(base->lcpa_regulator);
-
-	return ret;
-}
+#endif
 
 static const struct dev_pm_ops dma40_pm_ops = {
-	.suspend		= dma40_pm_suspend,
-	.runtime_suspend	= dma40_runtime_suspend,
-	.runtime_resume		= dma40_runtime_resume,
-	.resume			= dma40_resume,
+	SET_SYSTEM_SLEEP_PM_OPS(dma40_suspend, dma40_resume)
+	SET_PM_RUNTIME_PM_OPS(dma40_runtime_suspend,
+				dma40_runtime_resume,
+				NULL)
 };
-#define DMA40_PM_OPS	(&dma40_pm_ops)
-#else
-#define DMA40_PM_OPS	NULL
-#endif
 
 /* Initialization functions. */

@@ -3753,7 +3745,7 @@ static struct platform_driver d40_driver = {
 	.driver = {
 		.owner = THIS_MODULE,
 		.name  = D40_NAME,
-		.pm = DMA40_PM_OPS,
+		.pm = &dma40_pm_ops,
 		.of_match_table = d40_match,
 	},
 };

Signature · 1 line

1.7.9.5

Linus Walleij

2014-04-24 12:55 UTC #4 lore raw

PATCH 3/4 dma: ste_dma40: Convert to PM macros while providing the PM callbacks

Quoted 3 lines · Ulf Hansson

Converting to the PM macros makes us simplify and remove some code.
Signed-off-by Ulf Hansson <[email protected]>

Acked-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Ulf Hansson

author

2014-04-23 19:52 UTC #5 lore raw

PATCH 4/4 dma: ste_dma40: Fixup system suspend/resume

In reply to Ulf Hansson

Make sure to handle register context save/restore when needed from system PM callbacks.

Previously we solely trusted the device to reside in in-active state while the system suspend callback were invoked, which is just too optimistic.

Signed-off-by Ulf Hansson <[email protected]>

---
 drivers/dma/ste_dma40.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

drivers/dma/ste_dma40.c +10 -3

index 0b29af3..b539fc9 100644
@@ -2945,7 +2945,11 @@ static int dma40_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct d40_base *base = platform_get_drvdata(pdev);
-	int ret = 0;
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
 
 	if (base->lcpa_regulator)
 		ret = regulator_disable(base->lcpa_regulator);

@@ -2958,10 +2962,13 @@ static int dma40_resume(struct device *dev)
 	struct d40_base *base = platform_get_drvdata(pdev);
 	int ret = 0;
 
-	if (base->lcpa_regulator)
+	if (base->lcpa_regulator) {
 		ret = regulator_enable(base->lcpa_regulator);
+		if (ret)
+			return ret;
+	}
 
-	return ret;
+	return pm_runtime_force_resume(dev);
 }
 #endif

Signature · 1 line

1.7.9.5

Linus Walleij

2014-04-24 12:55 UTC #6 lore raw

PATCH 4/4 dma: ste_dma40: Fixup system suspend/resume

Quoted 8 lines · Ulf Hansson

Make sure to handle register context save/restore when needed from system PM callbacks.
Previously we solely trusted the device to reside in in-active state while the system suspend callback were invoked, which is just too optimistic.
Signed-off-by Ulf Hansson <[email protected]>

Acked-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Sergei Shtylyov

2014-04-23 20:39 UTC #7 lore raw

PATCH 1/4 dma: ste_dma40: Maintain spinlock order while handling pause

In reply to Ulf Hansson

Quoted 13 lines · Ulf Hansson

The runtime PM resume callback needs to be executed while holding the spinlock, make sure to maintain this for the pause operation as well.
Signed-off-by Ulf Hansson <[email protected]>
---
  drivers/dma/ste_dma40.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma/ste_dma40.c +0 -0
index bf18c78..6e97cf6 100644

drivers/dma/ste_dma40.c static int d40_pause(struct d40_chan *d40c) · line 1499

Quoted 6 lines

drivers/dma/ste_dma40.c +1 -1

@@ -1495,8 +1495,8 @@ static int d40_pause(struct d40_chan *d40c)
  	if (!d40c->busy)
  		return 0;
 
-	pm_runtime_get_sync(d40c->base->dev);
  	spin_lock_irqsave(&d40c->lock, flags);
+	pm_runtime_get_sync(d40c->base->dev);

That function may sleep AFAIK, so you can't really call it with a spinlock
held. Or do I miss something?

WBR, Sergei

Ulf Hansson

author

2014-04-24 09:11 UTC #8 lore raw

PATCH 1/4 dma: ste_dma40: Maintain spinlock order while handling pause

Quoted 20 lines · Sergei Shtylyov

Quoted 18 lines · Ulf Hansson
The runtime PM resume callback needs to be executed while holding the spinlock, make sure to maintain this for the pause operation as well.
Signed-off-by Ulf Hansson <[email protected]>
---
  drivers/dma/ste_dma40.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma/ste_dma40.c +0 -0
index bf18c78..6e97cf6 100644
@@ -1495,8 +1495,8 @@ static int d40_pause(struct d40_chan *d40c)
        if (!d40c->busy)
                return 0;
 

Quoted 7 lines

drivers/dma/ste_dma40.c static int d40_pause(struct d40_chan *d40c) · line 1499
Quoted 3 lines
drivers/dma/ste_dma40.c +1 -1
-       pm_runtime_get_sync(d40c->base->dev);
        spin_lock_irqsave(&d40c->lock, flags);
+       pm_runtime_get_sync(d40c->base->dev);
That function may sleep AFAIK, so you can't really call it with a spinlock
held. Or do I miss something?

That's the default behaviour from the runtime PM core.

But, since we have invoked "pm_runtime_irq_safe()" at ->probe(), that means the pm_runtime_get_sync() function won't sleep.

Kind regards
Ulf Hansson

Quoted 3 lines

WBR, Sergei

Linus Walleij

2014-04-24 13:25 UTC #9 lore raw

PATCH 1/4 dma: ste_dma40: Maintain spinlock order while handling pause

In reply to Ulf Hansson

Quoted 4 lines · Ulf Hansson

The runtime PM resume callback needs to be executed while holding the spinlock, make sure to maintain this for the pause operation as well.
Signed-off-by Ulf Hansson <[email protected]>

Acked-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Vinod Koul

Applied

2014-05-07 06:22 UTC #10 lore raw

PATCH 1/4 dma: ste_dma40: Maintain spinlock order while handling pause

In reply to Ulf Hansson

Quoted 2 lines · Ulf Hansson

The runtime PM resume callback needs to be executed while holding the spinlock, make sure to maintain this for the pause operation as well.

Applied, all thanks.

Though we need to change the driver to use SET_LATE_SYSTEM_SLEEP_PM_OPS as all dmaengine drivers should suspend late as clients can be active while suspending

Signature · 1 line

~Vinod

Ulf Hansson

author

2014-05-07 08:41 UTC #11 lore raw

PATCH 1/4 dma: ste_dma40: Maintain spinlock order while handling pause

Quoted 9 lines · Vinod Koul

Quoted 2 lines · Ulf Hansson
The runtime PM resume callback needs to be executed while holding the spinlock, make sure to maintain this for the pause operation as well.
Applied, all thanks.
Though we need to change the driver to use SET_LATE_SYSTEM_SLEEP_PM_OPS as all dmaengine drivers should suspend late as clients can be active while suspending

Right, I suspected that as well - even if I did ran some tests to verify this not to happen in practice for ux500.

Anyway, I will send a patch which converts to SET_LATE_SYSTEM_SLEEP_PM_OPS, since for sure it makes sense.

Kind regards
Ulf Hansson

Quoted 4 lines

--
~Vinod