Re: [PATCH v8 3/3] dmaengine: pl330: Don't require irq-safe runtime PM

[Vinod Koul <vinod.koul@intel.com>]

On Thu, Feb 09, 2017 at 03:22:51PM +0100, Marek Szyprowski wrote:
  
> +static int pl330_set_slave(struct dma_chan *chan, struct device *slave)
> +{
> +	struct dma_pl330_chan *pch = to_pchan(chan);
> +	struct pl330_dmac *pl330 = pch->dmac;
> +	int i;
> +
> +	mutex_lock(&pl330->rpm_lock);
> +
> +	for (i = 0; i < pl330->num_peripherals; i++) {
> +		if (pl330->peripherals[i].chan.slave == slave &&
> +		    pl330->peripherals[i].slave_link) {
> +			pch->slave_link = pl330->peripherals[i].slave_link;
> +			goto done;
> +		}
> +	}
> +
> +	pch->slave_link = device_link_add(slave, pl330->ddma.dev,
> +				       DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);

So you are going to add the link on channel allocation and tear down on the
freeup. I am not sure I really like the idea here.

First, these thing shouldn't be handled in the drivers. These things should
be set in core and each driver setting the links doesn't sound great to me.

Second, should the link be always there and we only mange the state? Here it
seems that we have link being created and destroyed, so why not mark it
ACTIVE and DORMANT instead...

Lastly, looking at th description of the issue here, am perceiving (maybe my
understanding is not quite right here) that you have an IP block in SoC
which has multiple things and share common stuff and doing right PM is a
challenge for you, right?

-- 
~Vinod