[PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Mark Brown]

Every device that implements runtime power management for DAIs is doing
it in pretty much the same way: in the startup callback they take a
runtime PM reference and then in the shutdown callback they release that
reference, keeping the device active while the DAI is active. Given the
frequency with which this is done and the obviousness of the need to keep
the device active in this period factor the code out into the core, taking
references on the device for each CPU DAI, CODEC DAI and DMA device in the
core.

As runtime PM is reference counted this shouldn't interfere with any
other reference holding by the drivers, and since (in common with the
existing implementations) we don't check for errors on enabling it
shouldn't matter if the device actually has runtime PM enabled or not.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/soc-pcm.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 49aa71e..8aa7cec 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/delay.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
 #include <sound/core.h>
@@ -77,6 +78,10 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	struct snd_soc_dai_driver *codec_dai_drv = codec_dai->driver;
 	int ret = 0;
 
+	pm_runtime_get_sync(cpu_dai->dev);
+	pm_runtime_get_sync(codec_dai->dev);
+	pm_runtime_get_sync(platform->dev);
+
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
 	/* startup the audio subsystem */
@@ -233,6 +238,11 @@ platform_err:
 		cpu_dai->driver->ops->shutdown(substream, cpu_dai);
 out:
 	mutex_unlock(&rtd->pcm_mutex);
+
+	pm_runtime_put(platform->dev);
+	pm_runtime_put(codec_dai->dev);
+	pm_runtime_put(cpu_dai->dev);
+
 	return ret;
 }
 
@@ -339,6 +349,11 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	}
 
 	mutex_unlock(&rtd->pcm_mutex);
+
+	pm_runtime_put(platform->dev);
+	pm_runtime_put(codec_dai->dev);
+	pm_runtime_put(cpu_dai->dev);
+
 	return 0;
 }
 
-- 
1.7.7.3

[PATCH 2/5] ASoC: Use core pm_runtime callbacks for omap-dmic

[Mark Brown]

Now that the core holds a pm_runtime reference to the device while the
link is active there is no need for the driver to do so.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/omap/omap-dmic.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/sound/soc/omap/omap-dmic.c b/sound/soc/omap/omap-dmic.c
index 9c73c0c..0855c1c 100644
--- a/sound/soc/omap/omap-dmic.c
+++ b/sound/soc/omap/omap-dmic.c
@@ -114,7 +114,6 @@ static int omap_dmic_dai_startup(struct snd_pcm_substream *substream,
 	mutex_lock(&dmic->mutex);
 
 	if (!dai->active) {
-		pm_runtime_get_sync(dmic->dev);
 		snd_pcm_hw_constraint_msbits(substream->runtime, 0, 32, 24);
 		dmic->active = 1;
 	} else {
@@ -133,10 +132,8 @@ static void omap_dmic_dai_shutdown(struct snd_pcm_substream *substream,
 
 	mutex_lock(&dmic->mutex);
 
-	if (!dai->active) {
-		pm_runtime_put_sync(dmic->dev);
+	if (!dai->active)
 		dmic->active = 0;
-	}
 
 	mutex_unlock(&dmic->mutex);
 }
-- 
1.7.7.3

Re: [PATCH 2/5] ASoC: Use core pm_runtime callbacks for omap-dmic

[Peter Ujfalusi]

On 12/05/2011 02:01 AM, Mark Brown wrote:
> Now that the core holds a pm_runtime reference to the device while the
> link is active there is no need for the driver to do so.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

[PATCH 3/5] ASoC: Use core pm_runtime callbacks for omap-mcpdm

[Mark Brown]

Now that the core holds a pm_runtime reference to the device while the
link is active there is no need for the driver to do so.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/omap/omap-mcpdm.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/sound/soc/omap/omap-mcpdm.c b/sound/soc/omap/omap-mcpdm.c
index b50ac60..0e25df4f 100644
--- a/sound/soc/omap/omap-mcpdm.c
+++ b/sound/soc/omap/omap-mcpdm.c
@@ -266,8 +266,6 @@ static int omap_mcpdm_dai_startup(struct snd_pcm_substream *substream,
 	mutex_lock(&mcpdm->mutex);
 
 	if (!dai->active) {
-		pm_runtime_get_sync(mcpdm->dev);
-
 		/* Enable watch dog for ES above ES 1.0 to avoid saturation */
 		if (omap_rev() != OMAP4430_REV_ES1_0) {
 			u32 ctrl = omap_mcpdm_read(mcpdm, MCPDM_REG_CTRL);
@@ -295,9 +293,6 @@ static void omap_mcpdm_dai_shutdown(struct snd_pcm_substream *substream,
 			omap_mcpdm_stop(mcpdm);
 			omap_mcpdm_close_streams(mcpdm);
 		}
-
-		if (!omap_mcpdm_active(mcpdm))
-			pm_runtime_put_sync(mcpdm->dev);
 	}
 
 	mutex_unlock(&mcpdm->mutex);
-- 
1.7.7.3

Re: [PATCH 3/5] ASoC: Use core pm_runtime callbacks for omap-mcpdm

[Peter Ujfalusi]

On 12/05/2011 02:01 AM, Mark Brown wrote:
> Now that the core holds a pm_runtime reference to the device while the
> link is active there is no need for the driver to do so.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

[PATCH 4/5] ASoC: Use core pm_runtime callbacks for siu_dai

[Mark Brown]

Now that the core holds a pm_runtime reference to the device while the
link is active there is no need for the driver to do so.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/sh/siu_dai.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/sound/soc/sh/siu_dai.c b/sound/soc/sh/siu_dai.c
index 11c6085..52d4c17 100644
--- a/sound/soc/sh/siu_dai.c
+++ b/sound/soc/sh/siu_dai.c
@@ -112,9 +112,6 @@ static void siu_dai_start(struct siu_port *port_info)
 
 	dev_dbg(port_info->pcm->card->dev, "%s\n", __func__);
 
-	/* Turn on SIU clock */
-	pm_runtime_get_sync(info->dev);
-
 	/* Issue software reset to siu */
 	siu_write32(base + SIU_SRCTL, 0);
 
@@ -158,9 +155,6 @@ static void siu_dai_stop(struct siu_port *port_info)
 
 	/* SIU software reset */
 	siu_write32(base + SIU_SRCTL, 0);
-
-	/* Turn off SIU clock */
-	pm_runtime_put_sync(info->dev);
 }
 
 static void siu_dai_spbAselect(struct siu_port *port_info)
-- 
1.7.7.3

[PATCH 5/5] ASoC: Use core pm_runtime callbacks for fsi

[Mark Brown]

Now that the core holds a pm_runtime reference to the device while the
link is active there is no need for the driver to do so.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/sh/fsi.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index a27c306..db6c89a 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -893,8 +893,6 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
 	u32 flags = fsi_get_info_flags(fsi);
 	u32 data = 0;
 
-	pm_runtime_get_sync(dev);
-
 	/* clock setting */
 	if (fsi_is_clk_master(fsi))
 		data = DIMD | DOMD;
@@ -951,8 +949,6 @@ static void fsi_hw_shutdown(struct fsi_priv *fsi,
 {
 	if (fsi_is_clk_master(fsi))
 		fsi_set_master_clk(dev, fsi, fsi->rate, 0);
-
-	pm_runtime_put_sync(dev);
 }
 
 static int fsi_dai_startup(struct snd_pcm_substream *substream,
-- 
1.7.7.3

Re: [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Peter Ujfalusi]

On 12/05/2011 02:01 AM, Mark Brown wrote:
> Every device that implements runtime power management for DAIs is doing
> it in pretty much the same way: in the startup callback they take a
> runtime PM reference and then in the shutdown callback they release that
> reference, keeping the device active while the DAI is active. Given the
> frequency with which this is done and the obviousness of the need to keep
> the device active in this period factor the code out into the core, taking
> references on the device for each CPU DAI, CODEC DAI and DMA device in the
> core.
> 
> As runtime PM is reference counted this shouldn't interfere with any
> other reference holding by the drivers, and since (in common with the
> existing implementations) we don't check for errors on enabling it
> shouldn't matter if the device actually has runtime PM enabled or not.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  sound/soc/soc-pcm.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 49aa71e..8aa7cec 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -19,6 +19,7 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/delay.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/workqueue.h>
>  #include <sound/core.h>
> @@ -77,6 +78,10 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
>  	struct snd_soc_dai_driver *codec_dai_drv = codec_dai->driver;
>  	int ret = 0;
>  
> +	pm_runtime_get_sync(cpu_dai->dev);
> +	pm_runtime_get_sync(codec_dai->dev);
> +	pm_runtime_get_sync(platform->dev);
> +
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);

I think it is better to move the pm_runtime_get_sync calls after the
mutex_lock_nested() to be really safe (and to not change the way DAI
drivers were handling the pm_runtime).

-- 
Péter

Re: [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Mark Brown]

On Mon, Dec 05, 2011 at 10:01:07AM +0200, Peter Ujfalusi wrote:
> On 12/05/2011 02:01 AM, Mark Brown wrote:

> > +	pm_runtime_get_sync(cpu_dai->dev);
> > +	pm_runtime_get_sync(codec_dai->dev);
> > +	pm_runtime_get_sync(platform->dev);
> > +
> >  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);

> I think it is better to move the pm_runtime_get_sync calls after the
> mutex_lock_nested() to be really safe (and to not change the way DAI
> drivers were handling the pm_runtime).

This increases the amount of time we are sitting with the pcm_mutex held
and I can't see a reason why we should need to do that.  If there is a
problem here I'd rather improve the drivers so that they can cope with
having the power management happening after the lock, the main reason
the drivers were doing this previously is that the lock happened to be
held by the caller.

Re: [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Peter Ujfalusi]

On 12/05/2011 01:39 PM, Mark Brown wrote:
> This increases the amount of time we are sitting with the pcm_mutex held
> and I can't see a reason why we should need to do that.  If there is a
> problem here I'd rather improve the drivers so that they can cope with
> having the power management happening after the lock, the main reason
> the drivers were doing this previously is that the lock happened to be
> held by the caller.

It is just that the pm_runtime calls might need the same protection as
the other operations for dais/codecs/driver.
I just thought about this series, and it might broke the omap-mcpdm
driver as soon as we have the BIAS_OFF support for the twl6040 codec
driver. The functional clock is coming from the external codec. If we
power down the external fclk we will see external abort crash, since the
McPDM IP does not have the needed clocks at the time of the
pm_runtime_put_sync call (which will try to configure some McPDM registers).
Need to check this. Can we hold this series back for a while?

-- 
Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Mark Brown]

On Mon, Dec 05, 2011 at 03:32:19PM +0200, Peter Ujfalusi wrote:
> On 12/05/2011 01:39 PM, Mark Brown wrote:

> > This increases the amount of time we are sitting with the pcm_mutex held
> > and I can't see a reason why we should need to do that.  If there is a
> > problem here I'd rather improve the drivers so that they can cope with
> > having the power management happening after the lock, the main reason
> > the drivers were doing this previously is that the lock happened to be
> > held by the caller.

> It is just that the pm_runtime calls might need the same protection as
> the other operations for dais/codecs/driver.

Well, the pcm_mutex doesn't cover all of those anyway (trigger and
pointer in particular) and we've no guarantee that anything will
actually happen at the point where the core does the calls as there may
be other things holding the device active.

> I just thought about this series, and it might broke the omap-mcpdm
> driver as soon as we have the BIAS_OFF support for the twl6040 codec
> driver. The functional clock is coming from the external codec. If we
> power down the external fclk we will see external abort crash, since the
> McPDM IP does not have the needed clocks at the time of the
> pm_runtime_put_sync call (which will try to configure some McPDM registers).
> Need to check this. Can we hold this series back for a while?

I don't understand how this could make the situation any worse than it
is already - if nothing else this series will only make the region where
we've got the device active slightly wider.  There's definitely an issue
there, but it seems like it already exists and is orthogonal to this
refactoring.  The McPDM needs to hold a reference on the CODEC somehow
while it is active it seems, either via DAPM or via the runtime_pm APIs.

Also note that as this is a reference counting API there's nothing
stopping any of the devices from being kept active independantly of
this, your use case would seem to be a good example of the sort of
situation where this might be useful.

Re: [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Peter Ujfalusi]

On 12/05/2011 03:46 PM, Mark Brown wrote:
> Well, the pcm_mutex doesn't cover all of those anyway (trigger and
> pointer in particular) and we've no guarantee that anything will
> actually happen at the point where the core does the calls as there may
> be other things holding the device active.

It covers the soc_pcm_open, and soc_pcm_close sequences.

> I don't understand how this could make the situation any worse than it
> is already - if nothing else this series will only make the region where
> we've got the device active slightly wider.

The ordering of the pm_runtime_get/put will be different.
We will have the pm_runtime_put after all other parts of the audio
system has been closed, turned off.

> There's definitely an issue
> there, but it seems like it already exists and is orthogonal to this
> refactoring.  The McPDM needs to hold a reference on the CODEC somehow
> while it is active it seems, either via DAPM or via the runtime_pm APIs.

Yes, this is the reason we do not have the BIAS_OFF support in twl6040
still. I'm working on to integrate the external fclk (bit clock from
twl6040) for McPDM with pm_runtime so we are not going to have issues
with missing clocks.
But as I said at this time we do not have issues since the fclk for
McPDM is always present.
This change actually gives more incentive to do the pm_runtime support
for the McPDM external fclk ;)

-- 
Péter

Re: [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Mark Brown]

On Mon, Dec 05, 2011 at 04:53:00PM +0200, Peter Ujfalusi wrote:
> On 12/05/2011 03:46 PM, Mark Brown wrote:

> > Well, the pcm_mutex doesn't cover all of those anyway (trigger and
> > pointer in particular) and we've no guarantee that anything will
> > actually happen at the point where the core does the calls as there may
> > be other things holding the device active.

> It covers the soc_pcm_open, and soc_pcm_close sequences.

It's a pretty limited subset (and of course it won't cover the cases
where one DAI is in two PCMs).

> > I don't understand how this could make the situation any worse than it
> > is already - if nothing else this series will only make the region where
> > we've got the device active slightly wider.

> The ordering of the pm_runtime_get/put will be different.
> We will have the pm_runtime_put after all other parts of the audio
> system has been closed, turned off.

Hrm, yes.  But that's much wider than just the issue with moving inside
the pcm_lock - for example, the shutdown calls already come before the
DAPM teardowns.

> > There's definitely an issue
> > there, but it seems like it already exists and is orthogonal to this
> > refactoring.  The McPDM needs to hold a reference on the CODEC somehow
> > while it is active it seems, either via DAPM or via the runtime_pm APIs.
> 
> Yes, this is the reason we do not have the BIAS_OFF support in twl6040
> still. I'm working on to integrate the external fclk (bit clock from
> twl6040) for McPDM with pm_runtime so we are not going to have issues
> with missing clocks.
> But as I said at this time we do not have issues since the fclk for
> McPDM is always present.
> This change actually gives more incentive to do the pm_runtime support
> for the McPDM external fclk ;)

So it sounds like things are OK with the proposed patch then, though
there's still some larger issues to work through?

Re: [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Peter Ujfalusi]

On 12/05/2011 05:07 PM, Mark Brown wrote:
>> The ordering of the pm_runtime_get/put will be different.
>> We will have the pm_runtime_put after all other parts of the audio
>> system has been closed, turned off.
> 
> Hrm, yes.  But that's much wider than just the issue with moving inside
> the pcm_lock - for example, the shutdown calls already come before the
> DAPM teardowns.

Sure, ideally I would pair the pm_runtime calls with the corresponding
startup/shutdown callback.

> So it sounds like things are OK with the proposed patch then, though
> there's still some larger issues to work through?

Yes, correct. I need to have pm_runtime support for the external
functional clock for the McPDM (coming as bit clock from twl6040 codec).

-- 
Péter

Re: [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs

[Peter Ujfalusi]

On 12/05/2011 02:01 AM, Mark Brown wrote:
> Every device that implements runtime power management for DAIs is doing
> it in pretty much the same way: in the startup callback they take a
> runtime PM reference and then in the shutdown callback they release that
> reference, keeping the device active while the DAI is active. Given the
> frequency with which this is done and the obviousness of the need to keep
> the device active in this period factor the code out into the core, taking
> references on the device for each CPU DAI, CODEC DAI and DMA device in the
> core.
> 
> As runtime PM is reference counted this shouldn't interfere with any
> other reference holding by the drivers, and since (in common with the
> existing implementations) we don't check for errors on enabling it
> shouldn't matter if the device actually has runtime PM enabled or not.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>