On Fri, Jan 13, 2017 at 08:26:40AM +0100, Marek Szyprowski wrote:
> This patch replaces irq-safe runtime PM with non-irq-safe version based on
> the new approach. Existing, irq-safe runtime PM implementation for PL330 was
> not bringing much benefits of its own - only clocks were enabled/disabled.
>
> Till now non-irq-safe runtime PM implementation was only possible by calling
> pm_runtime_get/put functions from alloc/free_chan_resources. All other DMA
> engine API functions cannot be called from a context, which permits sleeping.
> Such implementation, in practice would result in keeping DMA controller's
> device active almost all the time, because most of the slave device drivers
> (DMA engine clients) acquire DMA channel in their probe() function and
> released it during driver removal.
>
> This patch provides a new, different approach. It is based on an observation
> that there can be only one slave device using each DMA channel. PL330 hardware
> always has dedicated channels for each peripheral device. Using recently
> introduced device dependencies (links) infrastructure one can ensure proper
> runtime PM state of PL330 DMA controller basing on the runtime PM state of
> the slave device.
>
> In this approach in pl330_alloc_chan_resources() function a new dependency
> is being created between PL330 DMA controller device (as a supplier) and
> given slave device (as a consumer). This way PL330 DMA controller device
> runtime active counter is increased when the slave device is resumed and
> decreased the same time when given slave device is put to suspend. This way
> it has been ensured to keep PL330 DMA controller runtime active if there is
> an active used of any of its DMA channels. Slave device pointer is initially
> stored in per-channel data in of_dma_xlate callback. This is similar to what
> has been already implemented in Exynos IOMMU driver in commit 2f5f44f205cc95
> ("iommu/exynos: Use device dependency links to control runtime pm").
>
> If slave device doesn't implement runtime PM or keeps device runtime active
> all the time, then PL330 DMA controller will be runtime active all the time
> when channel is being allocated. The goal is however to have runtime PM
> added to all devices in the system, because it lets respective power
> domains to be turned off, what gives the best results in terms of power
> saving.
>
> If one requests memory-to-memory channel, runtime active counter is
> increased unconditionally. This might be a drawback of this approach, but
> PL330 is not really used for memory-to-memory operations due to poor
> performance in such operations compared to the CPU.
>
> Introducing non-irq-safe runtime power management finally allows to turn off
> audio power domain on Exynos5 SoCs.
>
> Removal of irq-safe runtime PM is based on the revert of the following
> commits:
> 1. commit 5c9e6c2b2ba3 "dmaengine: pl330: fix runtime pm support"
> 2. commit 81cc6edc0870 "dmaengine: pl330: Fix hang on dmaengine_terminate_all
> on certain boards"
> 3. commit ae43b3289186 "ARM: 8202/1: dmaengine: pl330: Add runtime Power
> Management support v12"
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> ---
> drivers/dma/pl330.c | 135 ++++++++++++++++++++++++++++------------------------
> 1 file changed, 72 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index c77a3494659c..a97cb02250ab 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -16,6 +16,7 @@
> #include <linux/init.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/string.h>
> #include <linux/delay.h>
> #include <linux/interrupt.h>
> @@ -268,9 +269,6 @@ enum pl330_byteswap {
>
> #define NR_DEFAULT_DESC 16
>
> -/* Delay for runtime PM autosuspend, ms */
> -#define PL330_AUTOSUSPEND_DELAY 20
> -
> /* Populated by the PL330 core driver for DMA API driver's info */
> struct pl330_config {
> u32 periph_id;
> @@ -449,8 +447,8 @@ struct dma_pl330_chan {
> bool cyclic;
>
> /* for runtime pm tracking */
> - bool active;
> struct device *slave;
> + struct device_link *slave_link;
> };
>
> struct pl330_dmac {
> @@ -465,6 +463,9 @@ struct pl330_dmac {
> /* To protect desc_pool manipulation */
> spinlock_t pool_lock;
>
> + /* For runtime PM management of slave channels */
> + struct mutex rpm_lock;
Isn't synchronization provided by dmaengine core here? The
dma_chan_get/put (which call the alloc/free resources) are called under
dma_list_mutex(). Don't you trust the core in that matter?
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
