| Message ID | bc59bbd3709a7af1723d37ab846bf44c6867c00e.1312190881.git.viresh.kumar@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 01, 2011 at 03:07:18PM +0530, Viresh Kumar wrote: > @@ -1993,6 +2002,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id) > dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n", > amba_part(adev), amba_rev(adev), > (unsigned long long)adev->res.start, adev->irq[0]); > + > + pm_runtime_suspend(&adev->dev); Having read the runtime pm documentation, devices are assumed to be suspended at probe time, and there should be a call to pm_runtime_enable() in here. See Documentation/power/runtime_pm.txt chapter 5. However, this is complicated by the core managing the peripheral clock, which starts off in the enabled state. So there's only one sane solution, which is to tell the runtime PM that the device is already active. So I think a primecell's probe function should look like this: primecell_probe() { ret = amba_request_regions(adev, NULL); if (ret) return ret; ... allocate stuff, don't access primecell though ... pm_runtime_set_active(&adev->dev); pm_runtime_enable(&adev->dev); ... get clocks and enable them, do rest of init ... pm_runtime_put_sync(&adev->dev); return 0; }
On 08/01/2011 03:56 PM, Russell King - ARM Linux wrote: > I think a primecell's probe function should look like this: > > primecell_probe() > { > ret = amba_request_regions(adev, NULL); > if (ret) > return ret; > > ... allocate stuff, don't access primecell though ... > > pm_runtime_set_active(&adev->dev); > pm_runtime_enable(&adev->dev); > > ... get clocks and enable them, do rest of init ... > > pm_runtime_put_sync(&adev->dev); > return 0; > } Ok. I didn't had much knowhow of pm_runtime* framework. I will send V3 for this patch alone, please see if that one is fine or still needs some updation. Thanks for your help.
On Mon, Aug 01, 2011 at 03:07:18PM +0530, Viresh Kumar wrote: > Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in > to action where used. This will also handle enabling/disabling of interface > clock (Code will be added in amba/bus.c by Russell King). I don't think this is correct... > @@ -879,11 +880,19 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x, > */ > static int pl08x_alloc_chan_resources(struct dma_chan *chan) > { > + struct pl08x_dma_chan *plchan = to_pl08x_chan(chan); > + struct pl08x_driver_data *pl08x = plchan->host; > + > + pm_runtime_resume(&pl08x->adev->dev); Shouldn't this be pm_runtime_get_sync() ? > return 0; > } > > static void pl08x_free_chan_resources(struct dma_chan *chan) > { > + struct pl08x_dma_chan *plchan = to_pl08x_chan(chan); > + struct pl08x_driver_data *pl08x = plchan->host; > + > + pm_runtime_suspend(&pl08x->adev->dev); And pm_runtime_put() ? > } > > /* > @@ -1993,6 +2002,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id) > dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n", > amba_part(adev), amba_rev(adev), > (unsigned long long)adev->res.start, adev->irq[0]); > + > + pm_runtime_suspend(&adev->dev); And pm_runtime_put() ? We don't want to call pm_runtime_suspend/resume directly because that could have bad consequences if more than one channel is in use. The enabling and disabling of runtime PM for AMBA devices will be handled in the core code... Lastly, we may want to make this even tighter to the actual period that the DMA is being used, rather than just the period that a driver has been allocated a channel. I'd rather have it done that way now (and tested) so that we achieve maximal effect from runtime PM, rather than having something which needs to be revisited again. IOW, if this is worth doing, then its worth doing properly first time.
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 3fbaf0e..428a67b 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -84,6 +84,7 @@ #include <linux/init.h> #include <linux/interrupt.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #include <linux/seq_file.h> #include <linux/slab.h> #include <asm/hardware/pl080.h> @@ -879,11 +880,19 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x, */ static int pl08x_alloc_chan_resources(struct dma_chan *chan) { + struct pl08x_dma_chan *plchan = to_pl08x_chan(chan); + struct pl08x_driver_data *pl08x = plchan->host; + + pm_runtime_resume(&pl08x->adev->dev); return 0; } static void pl08x_free_chan_resources(struct dma_chan *chan) { + struct pl08x_dma_chan *plchan = to_pl08x_chan(chan); + struct pl08x_driver_data *pl08x = plchan->host; + + pm_runtime_suspend(&pl08x->adev->dev); } /* @@ -1993,6 +2002,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id) dev_info(&pl08x->adev->dev, "DMA: PL%03x rev%u at 0x%08llx irq %d\n", amba_part(adev), amba_rev(adev), (unsigned long long)adev->res.start, adev->irq[0]); + + pm_runtime_suspend(&adev->dev); return 0; out_no_slave_reg:
Insert notifiers for the runtime PM API. With this the runtime PM layer kicks in to action where used. This will also handle enabling/disabling of interface clock (Code will be added in amba/bus.c by Russell King). Signed-off-by: Viresh Kumar <viresh.kumar@st.com> --- drivers/dma/amba-pl08x.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)