Skip to contentSkip to conversation

mfd: arizona: Fix initialisation of the PM runtime

PATCH RESEND In review Charles Keepax 2 participants 5 messages Aug 11, 2015 – Aug 12, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH RESEND 1/1 mfd: arizona: Fix initialisation of the PM runtime

The PM runtime core by default assumes a chip is suspended when runtime PM is enabled. Currently the arizona driver enables runtime PM when the chip is fully active and then disables the DCVDD regulator at the end of arizona_dev_init. This however has several problems, firstly the if we reach the end of arizona_dev_init, we did not properly follow all the proceedures for shutting down the chip, and most notably we never marked the chip as cache only so any writes occuring between then and the next PM runtime resume will be lost. Secondly, if we are already resumed when we reach the end of dev_init, then at best we get unbalanced regulator enable/disables at work we lose DCVDD whilst we need it.

Additionally, since the commit 4f0216409f7c ("mfd: arizona: Add better support for system suspend"), the PM runtime operations may disable/enable the IRQ, so the IRQs must now be enabled before we call any PM operations.

This patch adds a call to pm_runtime_set_active to inform the PM core that the device is starting up active and moves the PM enabling to around the IRQ initialisation to avoid any PM callbacks happening until the IRQs are initialised.

---

This patch was marked as applied on the 24th of June but doesn't seem to have shown up in your tree yet. Just resending just in case it got missed as it is a fairly important bug fix.

Thanks,
Charles

 drivers/mfd/arizona-core.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)
drivers/mfd/arizona-core.c +6 -8
index bc814b0..375b954 100644
@@ -1201,10 +1201,6 @@ int arizona_dev_init(struct arizona *arizona)
 			     arizona->pdata.gpio_defaults[i]);
 	}
 
-	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
-	pm_runtime_use_autosuspend(arizona->dev);
-	pm_runtime_enable(arizona->dev);
-
 	/* Chip default */
 	if (!arizona->pdata.clk32k_src)
 		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
@@ -1329,11 +1325,17 @@ int arizona_dev_init(struct arizona *arizona)
 					   arizona->pdata.spk_fmt[i]);
 	}
 
+	pm_runtime_set_active(arizona->dev);
+	pm_runtime_enable(arizona->dev);
+
 	/* Set up for interrupts */
 	ret = arizona_irq_init(arizona);
 	if (ret != 0)
 		goto err_reset;
 
+	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
+	pm_runtime_use_autosuspend(arizona->dev);
+
 	arizona_request_irq(arizona, ARIZONA_IRQ_CLKGEN_ERR, "CLKGEN error",
 			    arizona_clkgen_err, arizona);
 	arizona_request_irq(arizona, ARIZONA_IRQ_OVERCLOCKED, "Overclocked",
@@ -1367,10 +1369,6 @@ int arizona_dev_init(struct arizona *arizona)
 		goto err_irq;
 	}
 
-#ifdef CONFIG_PM
-	regulator_disable(arizona->dcvdd);
-#endif
-
 	return 0;
 
 err_irq:
Signature · 1 line
1.7.2.5
#1 lore raw
Quoted 29 lines · Charles Keepax

The PM runtime core by default assumes a chip is suspended when runtime PM is enabled. Currently the arizona driver enables runtime PM when the chip is fully active and then disables the DCVDD regulator at the end of arizona_dev_init. This however has several problems, firstly the if we reach the end of arizona_dev_init, we did not properly follow all the proceedures for shutting down the chip, and most notably we never marked the chip as cache only so any writes occuring between then and the next PM runtime resume will be lost. Secondly, if we are already resumed when we reach the end of dev_init, then at best we get unbalanced regulator enable/disables at work we lose DCVDD whilst we need it.

Additionally, since the commit 4f0216409f7c ("mfd: arizona: Add better support for system suspend"), the PM runtime operations may disable/enable the IRQ, so the IRQs must now be enabled before we call any PM operations.

This patch adds a call to pm_runtime_set_active to inform the PM core that the device is starting up active and moves the PM enabling to around the IRQ initialisation to avoid any PM callbacks happening until the IRQs are initialised.

---

This patch was marked as applied on the 24th of June but doesn't seem to have shown up in your tree yet. Just resending just in case it got missed as it is a fairly important bug fix.

Quoted 5 lines

Thanks,
Charles

 drivers/mfd/arizona-core.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

Why are you re-sending this?

Quoted 44 lines
drivers/mfd/arizona-core.c +6 -8
index bc814b0..375b954 100644
@@ -1201,10 +1201,6 @@ int arizona_dev_init(struct arizona *arizona)
 			     arizona->pdata.gpio_defaults[i]);
 	}
 
-	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
-	pm_runtime_use_autosuspend(arizona->dev);
-	pm_runtime_enable(arizona->dev);
-
 	/* Chip default */
 	if (!arizona->pdata.clk32k_src)
 		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
@@ -1329,11 +1325,17 @@ int arizona_dev_init(struct arizona *arizona)
 					   arizona->pdata.spk_fmt[i]);
 	}
 
+	pm_runtime_set_active(arizona->dev);
+	pm_runtime_enable(arizona->dev);
+
 	/* Set up for interrupts */
 	ret = arizona_irq_init(arizona);
 	if (ret != 0)
 		goto err_reset;
 
+	pm_runtime_set_autosuspend_delay(arizona->dev, 100);
+	pm_runtime_use_autosuspend(arizona->dev);
+
 	arizona_request_irq(arizona, ARIZONA_IRQ_CLKGEN_ERR, "CLKGEN error",
 			    arizona_clkgen_err, arizona);
 	arizona_request_irq(arizona, ARIZONA_IRQ_OVERCLOCKED, "Overclocked",
@@ -1367,10 +1369,6 @@ int arizona_dev_init(struct arizona *arizona)
 		goto err_irq;
 	}
 
-#ifdef CONFIG_PM
-	regulator_disable(arizona->dcvdd);
-#endif
-
 	return 0;
 
 err_irq:
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
#2 lore raw
Quoted 31 lines · Lee Jones
Quoted 29 lines · Charles Keepax

The PM runtime core by default assumes a chip is suspended when runtime PM is enabled. Currently the arizona driver enables runtime PM when the chip is fully active and then disables the DCVDD regulator at the end of arizona_dev_init. This however has several problems, firstly the if we reach the end of arizona_dev_init, we did not properly follow all the proceedures for shutting down the chip, and most notably we never marked the chip as cache only so any writes occuring between then and the next PM runtime resume will be lost. Secondly, if we are already resumed when we reach the end of dev_init, then at best we get unbalanced regulator enable/disables at work we lose DCVDD whilst we need it.

Additionally, since the commit 4f0216409f7c ("mfd: arizona: Add better support for system suspend"), the PM runtime operations may disable/enable the IRQ, so the IRQs must now be enabled before we call any PM operations.

This patch adds a call to pm_runtime_set_active to inform the PM core that the device is starting up active and moves the PM enabling to around the IRQ initialisation to avoid any PM callbacks happening until the IRQs are initialised.

---

This patch was marked as applied on the 24th of June but doesn't seem to have shown up in your tree yet. Just resending just in case it got missed as it is a fairly important bug fix.

Quoted 7 lines
Quoted 5 lines

Thanks,
Charles

 drivers/mfd/arizona-core.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

Why are you re-sending this?

Ah apologies my bad just realised I over looked the patch on your fixes branch. I thought it had been missed, sorry for the noise.

Thanks,
Charles

#3 lore raw
Quoted 35 lines · Charles Keepax
Quoted 34 lines · Lee Jones
Quoted 32 lines · Charles Keepax

The PM runtime core by default assumes a chip is suspended when runtime PM is enabled. Currently the arizona driver enables runtime PM when the chip is fully active and then disables the DCVDD regulator at the end of arizona_dev_init. This however has several problems, firstly the if we reach the end of arizona_dev_init, we did not properly follow all the proceedures for shutting down the chip, and most notably we never marked the chip as cache only so any writes occuring between then and the next PM runtime resume will be lost. Secondly, if we are already resumed when we reach the end of dev_init, then at best we get unbalanced regulator enable/disables at work we lose DCVDD whilst we need it.

Additionally, since the commit 4f0216409f7c ("mfd: arizona: Add better support for system suspend"), the PM runtime operations may disable/enable the IRQ, so the IRQs must now be enabled before we call any PM operations.

This patch adds a call to pm_runtime_set_active to inform the PM core that the device is starting up active and moves the PM enabling to around the IRQ initialisation to avoid any PM callbacks happening until the IRQs are initialised.

---

This patch was marked as applied on the 24th of June but doesn't seem to have shown up in your tree yet. Just resending just in case it got missed as it is a fairly important bug fix.

Thanks,
Charles

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
 drivers/mfd/arizona-core.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

Why are you re-sending this?

Ah apologies my bad just realised I over looked the patch on your fixes branch. I thought it had been missed, sorry for the noise.

If I've sent an "applied, thanks" message and the patch has still not shown up, it's better to reply directly to it than to send a RESEND.

Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
#4 lore raw
Quoted 11 lines · Lee Jones
Quoted 6 lines · Charles Keepax
Quoted 2 lines · Lee Jones

On Tue, 11 Aug 2015, Charles Keepax wrote:
Why are you re-sending this?

Ah apologies my bad just realised I over looked the patch on your fixes branch. I thought it had been missed, sorry for the noise.

If I've sent an "applied, thanks" message and the patch has still not shown up, it's better to reply directly to it than to send a RESEND.

Sorry I will do that should the situation arise again.

Thanks,
Charles