[alsa-devel] [PATCH 1/2 v2] ASoC: sh: fsi: Add runtime PM support
This patch add support runtime PM. Driver callbacks for Runtime PM are empty because the device registers are always re-initialized after pm_runtime_get_sync(). The Runtime PM functions replaces the clock framework module stop bit handling in this driver.
Signed-off-by: Kuninori Morimoto morimoto.kuninori@renesas.com --- v1 -> v2
o add pm_runtime_disable if failed
sound/soc/sh/fsi.c | 40 ++++++++++++++++++++++++++-------------- 1 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index e1a3d1a..e2f70ac 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -17,7 +17,7 @@ #include <linux/platform_device.h> #include <linux/delay.h> #include <linux/list.h> -#include <linux/clk.h> +#include <linux/pm_runtime.h> #include <linux/io.h> #include <sound/core.h> #include <sound/pcm.h> @@ -105,7 +105,6 @@ struct fsi_priv { struct fsi_master { void __iomem *base; int irq; - struct clk *clk; struct fsi_priv fsia; struct fsi_priv fsib; struct sh_fsi_platform_info *info; @@ -559,7 +558,7 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream, int is_master; int ret = 0;
- clk_enable(master->clk); + pm_runtime_get_sync(dai->dev);
/* CKG1 */ data = is_play ? (1 << 0) : (1 << 4); @@ -674,7 +673,7 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream, fsi_irq_disable(fsi, is_play); fsi_clk_ctrl(fsi, 0);
- clk_disable(master->clk); + pm_runtime_put_sync(dai->dev); }
static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd, @@ -872,7 +871,6 @@ EXPORT_SYMBOL_GPL(fsi_soc_platform); static int fsi_probe(struct platform_device *pdev) { struct resource *res; - char clk_name[8]; unsigned int irq; int ret;
@@ -903,14 +901,9 @@ static int fsi_probe(struct platform_device *pdev) master->fsia.base = master->base; master->fsib.base = master->base + 0x40;
- /* FSI is based on SPU mstp */ - snprintf(clk_name, sizeof(clk_name), "spu%d", pdev->id); - master->clk = clk_get(NULL, clk_name); - if (IS_ERR(master->clk)) { - dev_err(&pdev->dev, "cannot get %s mstp\n", clk_name); - ret = -EIO; - goto exit_iounmap; - } + pm_suspend_ignore_children(&pdev->dev, true); + pm_runtime_enable(&pdev->dev); + pm_runtime_resume(&pdev->dev);
fsi_soc_dai[0].dev = &pdev->dev; fsi_soc_dai[1].dev = &pdev->dev; @@ -935,6 +928,7 @@ exit_free_irq: free_irq(irq, master); exit_iounmap: iounmap(master->base); + pm_runtime_disable(&pdev->dev); exit_kfree: kfree(master); master = NULL; @@ -947,7 +941,7 @@ static int fsi_remove(struct platform_device *pdev) snd_soc_unregister_dais(fsi_soc_dai, ARRAY_SIZE(fsi_soc_dai)); snd_soc_unregister_platform(&fsi_soc_platform);
- clk_put(master->clk); + pm_runtime_disable(&pdev->dev);
free_irq(master->irq, master);
@@ -957,9 +951,27 @@ static int fsi_remove(struct platform_device *pdev) return 0; }
+static int fsi_runtime_nop(struct device *dev) +{ + /* Runtime PM callback shared between ->runtime_suspend() + * and ->runtime_resume(). Simply returns success. + * + * This driver re-initializes all registers after + * pm_runtime_get_sync() anyway so there is no need + * to save and restore registers here. + */ + return 0; +} + +static struct dev_pm_ops fsi_pm_ops = { + .runtime_suspend = fsi_runtime_nop, + .runtime_resume = fsi_runtime_nop, +}; + static struct platform_driver fsi_driver = { .driver = { .name = "sh_fsi", + .pm = &fsi_pm_ops, }, .probe = fsi_probe, .remove = fsi_remove,
On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
This patch add support runtime PM. Driver callbacks for Runtime PM are empty because the device registers are always re-initialized after pm_runtime_get_sync(). The Runtime PM functions replaces the clock framework module stop bit handling in this driver.
Signed-off-by: Kuninori Morimoto morimoto.kuninori@renesas.com
Hrm. I'll have to see how this plays with ASoC core pm_runtime support when that appears. It should be OK as-is, I think.
- pm_suspend_ignore_children(&pdev->dev, true);
- pm_runtime_enable(&pdev->dev);
- pm_runtime_resume(&pdev->dev);
Why pm_suspend_ignore_all_children()? I'd not expect the device to have any children and if it did it doesn't seem like an entirely safe assumption.
+static int fsi_runtime_nop(struct device *dev) +{
- /* Runtime PM callback shared between ->runtime_suspend()
* and ->runtime_resume(). Simply returns success.** This driver re-initializes all registers after* pm_runtime_get_sync() anyway so there is no need* to save and restore registers here.*/- return 0;
+}
This sets off alarm bells but it's perfectly reasonable, especially with platforms able to put things into a low power state with no explicit driver code now they can do power domain style things like SH. I've CCed in the PM folks since this seems like a perfectly reasonable use case which ought to be handled more nicely.
On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
Adding in an address for Magnus which hopefully won't bounce and also the linux-pm list which get_maintainers didn't find.
On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
This patch add support runtime PM. Driver callbacks for Runtime PM are empty because the device registers are always re-initialized after pm_runtime_get_sync(). The Runtime PM functions replaces the clock framework module stop bit handling in this driver.
Signed-off-by: Kuninori Morimoto morimoto.kuninori@renesas.com
Hrm. I'll have to see how this plays with ASoC core pm_runtime support when that appears. It should be OK as-is, I think.
- pm_suspend_ignore_children(&pdev->dev, true);
- pm_runtime_enable(&pdev->dev);
- pm_runtime_resume(&pdev->dev);
Why pm_suspend_ignore_all_children()? I'd not expect the device to have any children and if it did it doesn't seem like an entirely safe assumption.
+static int fsi_runtime_nop(struct device *dev) +{
- /* Runtime PM callback shared between ->runtime_suspend()
* and ->runtime_resume(). Simply returns success.** This driver re-initializes all registers after* pm_runtime_get_sync() anyway so there is no need* to save and restore registers here.*/- return 0;
+}
This sets off alarm bells but it's perfectly reasonable, especially with platforms able to put things into a low power state with no explicit driver code now they can do power domain style things like SH. I've CCed in the PM folks since this seems like a perfectly reasonable use case which ought to be handled more nicely. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, Nov 09, 2009 at 01:31:36PM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
Any chance someone from the PM side could comment on the issue below?
+static int fsi_runtime_nop(struct device *dev) +{
- /* Runtime PM callback shared between ->runtime_suspend()
* and ->runtime_resume(). Simply returns success.** This driver re-initializes all registers after* pm_runtime_get_sync() anyway so there is no need* to save and restore registers here.*/- return 0;
+}
This sets off alarm bells but it's perfectly reasonable, especially with platforms able to put things into a low power state with no explicit driver code now they can do power domain style things like SH. I've CCed in the PM folks since this seems like a perfectly reasonable use case which ought to be handled more nicely.
On Monday 16 November 2009, Mark Brown wrote:
On Mon, Nov 09, 2009 at 01:31:36PM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
Any chance someone from the PM side could comment on the issue below?
There is. I haven't had a chance to look at it yet, will do shortly.
+static int fsi_runtime_nop(struct device *dev) +{
- /* Runtime PM callback shared between ->runtime_suspend()
* and ->runtime_resume(). Simply returns success.** This driver re-initializes all registers after* pm_runtime_get_sync() anyway so there is no need* to save and restore registers here.*/- return 0;
+}
This sets off alarm bells but it's perfectly reasonable, especially with platforms able to put things into a low power state with no explicit driver code now they can do power domain style things like SH. I've CCed in the PM folks since this seems like a perfectly reasonable use case which ought to be handled more nicely.
Thanks, Rafael
On Mon 2009-11-16 15:30:00, Mark Brown wrote:
On Mon, Nov 09, 2009 at 01:31:36PM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
Any chance someone from the PM side could comment on the issue below?
+static int fsi_runtime_nop(struct device *dev) +{
- /* Runtime PM callback shared between ->runtime_suspend()
* and ->runtime_resume(). Simply returns success.** This driver re-initializes all registers after* pm_runtime_get_sync() anyway so there is no need* to save and restore registers here.*/- return 0;
+}
This sets off alarm bells but it's perfectly reasonable, especially with platforms able to put things into a low power state with no explicit driver code now they can do power domain style things like SH. I've CCed in the PM folks since this seems like a perfectly reasonable use case which ought to be handled more nicely.
I believe that having few nop functions around the tree should not be huge problem. If it is, you can introduce one shared top function into the core... Pavel
On Tue, Nov 17, 2009 at 12:52:36PM +0100, Pavel Machek wrote:
I believe that having few nop functions around the tree should not be huge problem. If it is, you can introduce one shared top function into the core...
The problem I have with that is that for most APIs noop functions are a big fat warning sign that something is going wrong and the API is being abused. This then creates noise and code review problems in the driver code since you've got something that normally suggests a problem.
On Tue 2009-11-17 12:41:25, Mark Brown wrote:
On Tue, Nov 17, 2009 at 12:52:36PM +0100, Pavel Machek wrote:
I believe that having few nop functions around the tree should not be huge problem. If it is, you can introduce one shared top function into the core...
The problem I have with that is that for most APIs noop functions are a big fat warning sign that something is going wrong and the API is being abused. This then creates noise and code review problems in the driver code since you've got something that normally suggests a problem.
That still sounds like poor reason to add tests to core. But return 0 function for that purpose should be ok (and should make code easy to review, too).
On Wed, Nov 18, 2009 at 05:09:08PM +0100, Pavel Machek wrote:
On Tue 2009-11-17 12:41:25, Mark Brown wrote:
The problem I have with that is that for most APIs noop functions are a big fat warning sign that something is going wrong and the API is being abused. This then creates noise and code review problems in the driver code since you've got something that normally suggests a problem.
That still sounds like poor reason to add tests to core. But return 0 function for that purpose should be ok (and should make code easy to review, too).
What makes you believe that this is a poor reason? The issue isn't that the driver code is complex, the issue is that it's noise in the driver which suggests that the driver isn't doing something it's supposed to do.
Hi Mark,
On Tue, Nov 17, 2009 at 12:30 AM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Nov 09, 2009 at 01:31:36PM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
Any chance someone from the PM side could comment on the issue below?
+static int fsi_runtime_nop(struct device *dev) +{
- /* Runtime PM callback shared between ->runtime_suspend()
- * and ->runtime_resume(). Simply returns success.
- *
- * This driver re-initializes all registers after
- * pm_runtime_get_sync() anyway so there is no need
- * to save and restore registers here.
- */
- return 0;
+}
This sets off alarm bells but it's perfectly reasonable, especially with platforms able to put things into a low power state with no explicit driver code now they can do power domain style things like SH. I've CCed in the PM folks since this seems like a perfectly reasonable use case which ought to be handled more nicely.
On SuperH we have Runtime PM enabled on a few platforms together with a few updated drivers. The latest driver to become more power aware is this FSI driver.
It's all quite simple, in the FSI driver we enable the clock to the FSI hardware using Runtime PM functions, and whenever the clock is turned off the SuperH specific bus code _may_ decide to call ->runtime_suspend() and turn off the power domain. The "_may_" comes from the fact that multiple hardware blocks may be grouped together into a single power domain, and all devices in the power domain must support Runtime PM and have their clocks disabled before we can turn off the power.
At this point the SuperH specific platform bus code requires the callbacks ->runtime_suspend() and ->runtime_resume() to be present. It may be a good idea to allow them to be NULL in the future or maybe having some shared functions, but before starting to break out stuff I'd like to see how other Runtime PM implementations deal with this. So unless people object I prefer to keep it as-is for now.
/ magnus
On Tue, Nov 17, 2009 at 09:46:35PM +0900, Magnus Damm wrote:
On SuperH we have Runtime PM enabled on a few platforms together with a few updated drivers. The latest driver to become more power aware is this FSI driver.
I understand exactly what the runtime PM stuff and the driver are doing here, the issue is the mandatory suspend and resume functions.
At this point the SuperH specific platform bus code requires the callbacks ->runtime_suspend() and ->runtime_resume() to be present. It may be a good idea to allow them to be NULL in the future or maybe having some shared functions, but before starting to break out stuff I'd like to see how other Runtime PM implementations deal with this. So unless people object I prefer to keep it as-is for now.
What is the reason for requiring that the driver provide stub functions? For me the issue is that if it's mandatory for the driver to provide the functions then having stub functions in there makes the driver look like it is abusing the API by not implementing mandatory functionality.
Given that the arch is now dealing with clocking and power for the device using the runtime PM system it seems fairly clear that there are going to be drivers like this one that can at least skip the suspend part and may not need to do anything at resume time either.
On Tuesday 17 November 2009, Mark Brown wrote:
On Tue, Nov 17, 2009 at 09:46:35PM +0900, Magnus Damm wrote:
On SuperH we have Runtime PM enabled on a few platforms together with a few updated drivers. The latest driver to become more power aware is this FSI driver.
I understand exactly what the runtime PM stuff and the driver are doing here, the issue is the mandatory suspend and resume functions.
At this point the SuperH specific platform bus code requires the callbacks ->runtime_suspend() and ->runtime_resume() to be present. It may be a good idea to allow them to be NULL in the future or maybe having some shared functions, but before starting to break out stuff I'd like to see how other Runtime PM implementations deal with this. So unless people object I prefer to keep it as-is for now.
What is the reason for requiring that the driver provide stub functions? For me the issue is that if it's mandatory for the driver to provide the functions then having stub functions in there makes the driver look like it is abusing the API by not implementing mandatory functionality.
In fact, it's not mandatory for bus types, not for drivers. IMO bus types really have to know how to suspend a device and how to resume it, otherwise the core framework won't be useful anyway. What the bus type does about drivers not implementing ->runtime_suspend() or ->runtime_resume(), it's up to the bus type. That's even documented IIRC.
Thanks, Rafael
On Tue, Nov 17, 2009 at 11:14:04PM +0100, Rafael J. Wysocki wrote:
In fact, it's not mandatory for bus types, not for drivers. IMO bus types really have to know how to suspend a device and how to resume it, otherwise the core framework won't be useful anyway. What the bus type does about drivers not implementing ->runtime_suspend() or ->runtime_resume(), it's up to the bus type. That's even documented IIRC.
OK, thanks - I hadn't realised that these were being called directly from the bus type code rather than by the core.
On Tue, Nov 17, 2009 at 9:59 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Tue, Nov 17, 2009 at 09:46:35PM +0900, Magnus Damm wrote:
On SuperH we have Runtime PM enabled on a few platforms together with a few updated drivers. The latest driver to become more power aware is this FSI driver.
I understand exactly what the runtime PM stuff and the driver are doing here, the issue is the mandatory suspend and resume functions.
Cool, but don't you think it makes sense to see how other architectures will deal with this first?
At this point the SuperH specific platform bus code requires the callbacks ->runtime_suspend() and ->runtime_resume() to be present. It may be a good idea to allow them to be NULL in the future or maybe having some shared functions, but before starting to break out stuff I'd like to see how other Runtime PM implementations deal with this. So unless people object I prefer to keep it as-is for now.
What is the reason for requiring that the driver provide stub functions? For me the issue is that if it's mandatory for the driver to provide the functions then having stub functions in there makes the driver look like it is abusing the API by not implementing mandatory functionality.
I see your point, but there is another side to it as well. Having the stubs there is a way of showing that these functions may be called as part of the Runtime PM management. Not having them would confuse people even more IMO.
Given that the arch is now dealing with clocking and power for the device using the runtime PM system it seems fairly clear that there are going to be drivers like this one that can at least skip the suspend part and may not need to do anything at resume time either.
Right. We just need to figure out what the arch-independent solution would be, and we're unfortunately not there yet.
I can spend some time on updating the drivers and removing the nop-stubs if you'd like, but I'd rather not since I suspect that I'll have to update things again when we get Runtime PM for ARM or other non-sh architectures.
Let me know what you think!
/ magnus
On Wed, Nov 18, 2009 at 07:09:14PM +0900, Magnus Damm wrote:
On Tue, Nov 17, 2009 at 9:59 PM, Mark Brown
I understand exactly what the runtime PM stuff and the driver are doing here, the issue is the mandatory suspend and resume functions.
Cool, but don't you think it makes sense to see how other architectures will deal with this first?
Of course, the other way of looking at it would be that it'd be good to provide the best possible implementation in the first architecture so that others can draw inspiration from it.
Looking at what you've got it seems clearly sane as-is modulo this issue and I can see how it maps onto the Samsung/OMAP style hardware so it doesn't set off any alarm bells for me on that front. The only thing I'm a bit unsure about with your implementation is mixing in the clock gating but to be honest that's something that could be changed on a per device basis anyway.
What is the reason for requiring that the driver provide stub functions? For me the issue is that if it's mandatory for the driver to provide the functions then having stub functions in there makes the driver look like it is abusing the API by not implementing mandatory functionality.
I see your point, but there is another side to it as well. Having the stubs there is a way of showing that these functions may be called as part of the Runtime PM management. Not having them would confuse people even more IMO.
Given that they're part of dev_pm_ops which already does such things as routine I'm not sure that it's particularly confusing. To me it's confusing that one might be required to do something at runtime suspend time - since the driver has to explicitly tell the PM core that the device is idle so it seems reasonable to expect that the hardware would already be quiesced and all that'd be needed would be to power down the block.
I can spend some time on updating the drivers and removing the nop-stubs if you'd like, but I'd rather not since I suspect that I'll have to update things again when we get Runtime PM for ARM or other non-sh architectures.
Like I say, the SH doesn't sound fundamentally different to the ARMs here.
On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
- pm_suspend_ignore_children(&pdev->dev, true);
- pm_runtime_enable(&pdev->dev);
- pm_runtime_resume(&pdev->dev);
Why pm_suspend_ignore_all_children()? I'd not expect the device to have any children and if it did it doesn't seem like an entirely safe assumption.
I've not yet seen any response to this query?
participants (5)
-
Kuninori Morimoto -
Magnus Damm -
Mark Brown -
Pavel Machek -
Rafael J. Wysocki