[PATCH v4] ARM: omap: edma: add suspend suspend/resume hooks

[Joel Fernandes]

Hi Daniel,

Thanks for your followup patch on this. It looks much better now using existing
functions to save/restore the state.

On 10/30/2013 03:21 PM, Daniel Mack wrote:
> This patch makes the edma driver resume correctly after suspend. Tested
> on an AM33xx platform with cyclic audio streams and omap_hsmmc.
> 
> All information can be reconstructed by already known runtime
> information.
> 
> As we now use some functions that were previously only used from __init
> context, annotations had to be dropped.
> 
> Signed-off-by: Daniel Mack <zonque at gmail.com>
> ---
> There was actually only a v3 ever, I made a mistake when formating the
> first version of this patch. To prevent confusion though, I named this
> one v4.
> 
> v3 -> v4:
> 	* dropped extra allocations, and reconstruct register values
> 	  from already known driver states.
> 
> 
> 
> Hi Joel, Gururaja, Balaji,
> 
> thanks a lot for your feedback. I successfully tested this version with
> davinci mcasp as well as omap_hsmmc. I'd appreciate another round of
> reviews :)
> 
> 
> Thanks,
> Daniel
> 
>  arch/arm/common/edma.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
[..]
> +static int edma_pm_resume(struct device *dev)
> +{
> +	int i, j;
> +
> +	pm_runtime_get_sync(dev);
> +
> +	for (j = 0; j < arch_num_cc; j++) {
> +		struct edma *cc = edma_cc[j];
> +
> +		s8 (*queue_priority_mapping)[2];
> +		s8 (*queue_tc_mapping)[2];
> +
> +		queue_tc_mapping = cc->info->queue_tc_mapping;
> +		queue_priority_mapping = cc->info->queue_priority_mapping;
> +
> +		/* Event queue to TC mapping */
> +		for (i = 0; queue_tc_mapping[i][0] != -1; i++)
> +			map_queue_tc(j, queue_tc_mapping[i][0],
> +					queue_tc_mapping[i][1]);
> +
> +		/* Event queue priority mapping */
> +		for (i = 0; queue_priority_mapping[i][0] != -1; i++)
> +			assign_priority_to_queue(j,
> +						queue_priority_mapping[i][0],
> +						queue_priority_mapping[i][1]);

I know ti,edma-regions property is not currently being used, but we should
future proof this by setting up DRAE for like done in probe:

                for (i = 0; i < info[j]->n_region; i++) {
                        edma_write_array2(j, EDMA_DRAE, i, 0, 0x0);
                        edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
                        edma_write_array(j, EDMA_QRAE, i, 0x0);
                }


> +
> +		/* Map the channel to param entry if channel mapping logic
> +		 * exist
> +		 */
> +		if (edma_read(j, EDMA_CCCFG) & CHMAP_EXIST)
> +			map_dmach_param(j);
> +
> +		for (i = 0; i < cc->num_channels; i++)
> +			if (test_bit(i, cc->edma_inuse)) {
> +				/* ensure access through shadow region 0 */
> +				edma_or_array2(j, EDMA_DRAE, 0, i >> 5,
> +						BIT(i & 0x1f));
> +
> +				setup_dma_interrupt(i,
> +						    cc->intr_data[i].callback,
> +						    cc->intr_data[i].data);
> +			}
> +
> +		enable_irq(cc->irq_res_start);
> +		enable_irq(cc->irq_res_end);
> +	}
> +
> +	pm_runtime_put_sync(dev);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);

I agree with Nishanth here, it is better to do this in .suspend/resume _noirq
stage to rule out any ordering bugs that may show up in the future, since such
an issue already showed up in earlier testing.

I would appreciate it if you can make these 2 changes and post a v5. Thanks for
a lot for all the hardwork.

Acked-by: Joel Fernandes <joelf at ti.com>

regards,

-Joel