Re: [RFC PATCH v3 04/16] ARM: edma: add DT and runtime PM support for AM33XX

[Matt Porter <mporter@ti.com>]

On Sun, Oct 28, 2012 at 04:33:39PM +0530, Sekhar Nori wrote:
> On 10/18/2012 6:56 PM, Matt Porter wrote:
> > Adds support for parsing the TI EDMA DT data into the required
> > EDMA private API platform data.
> > 
> > Calls runtime PM API only in the DT case in order to unidle the
> > associated hwmods on AM33XX.
> 
> Runtime PM is supported on DaVinci now, so if that was the reason for
> this choice, then it doesn't need to be that way.

Thanks, fixed this for v4.

> > 
> > Signed-off-by: Matt Porter <mporter@ti.com>
> > ---
> >  arch/arm/common/edma.c                      |  255 +++++++++++++++++++++++++--
> >  arch/arm/mach-davinci/board-da830-evm.c     |    4 +-
> >  arch/arm/mach-davinci/board-da850-evm.c     |    8 +-
> >  arch/arm/mach-davinci/board-dm646x-evm.c    |    4 +-
> >  arch/arm/mach-davinci/board-omapl138-hawk.c |    8 +-
> >  arch/arm/mach-davinci/devices-da8xx.c       |    8 +-
> >  arch/arm/mach-davinci/devices-tnetv107x.c   |    4 +-
> >  arch/arm/mach-davinci/dm355.c               |    4 +-
> >  arch/arm/mach-davinci/dm365.c               |    4 +-
> >  arch/arm/mach-davinci/dm644x.c              |    4 +-
> >  arch/arm/mach-davinci/dm646x.c              |    4 +-
> >  include/linux/platform_data/edma.h          |    8 +-
> >  12 files changed, 272 insertions(+), 43 deletions(-)
> > 
> > diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> > index a3d189d..6d2a590 100644
> > --- a/arch/arm/common/edma.c
> > +++ b/arch/arm/common/edma.c
> > @@ -24,6 +24,13 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/io.h>
> >  #include <linux/slab.h>
> > +#include <linux/edma.h>
> > +#include <linux/err.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_dma.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #include <linux/platform_data/edma.h>
> >  
> > @@ -1366,31 +1373,237 @@ void edma_clear_event(unsigned channel)
> >  EXPORT_SYMBOL(edma_clear_event);
> >  
> >  /*-----------------------------------------------------------------------*/
> > +static int edma_of_read_u32_to_s8_array(const struct device_node *np,
> > +					 const char *propname, s8 *out_values,
> > +					 size_t sz)
> > +{
> > +	struct property *prop = of_find_property(np, propname, NULL);
> > +	const __be32 *val;
> > +
> > +	if (!prop)
> > +		return -EINVAL;
> > +	if (!prop->value)
> > +		return -ENODATA;
> > +	if ((sz * sizeof(u32)) > prop->length)
> > +		return -EOVERFLOW;
> > +
> > +	val = prop->value;
> > +
> > +	while (sz--)
> > +		*out_values++ = (s8)(be32_to_cpup(val++) & 0xff);
> > +
> > +	/* Terminate it */
> > +	*out_values++ = -1;
> > +	*out_values++ = -1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int edma_of_read_u32_to_s16_array(const struct device_node *np,
> > +					 const char *propname, s16 *out_values,
> > +					 size_t sz)
> > +{
> > +	struct property *prop = of_find_property(np, propname, NULL);
> > +	const __be32 *val;
> > +
> > +	if (!prop)
> > +		return -EINVAL;
> > +	if (!prop->value)
> > +		return -ENODATA;
> > +	if ((sz * sizeof(u32)) > prop->length)
> > +		return -EOVERFLOW;
> > +
> > +	val = prop->value;
> > +
> > +	while (sz--)
> > +		*out_values++ = (s16)(be32_to_cpup(val++) & 0xffff);
> > +
> > +	/* Terminate it */
> > +	*out_values++ = -1;
> > +	*out_values++ = -1;
> > +
> > +	return 0;
> > +}
> 
> I think these helper functions will have some general use beyond EDMA
> and can be kept in drivers/of/base.c. Grant/Rob need to agree though.

I expect these to go away before too long as I expect to rewrite all of
these data structures when the private edma api code is folded into
drivers/dma/edma.c. When that happens, I would like to use some data
structures that lend themselves to the DT property value. Given that,
let's wait until there is another user to move these helpers to
drivers/of/.

> > diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
> > index 95b5e10..ffcbec1 100644
> > --- a/arch/arm/mach-davinci/board-da830-evm.c
> > +++ b/arch/arm/mach-davinci/board-da830-evm.c
> > @@ -512,7 +512,7 @@ static struct davinci_i2c_platform_data da830_evm_i2c_0_pdata = {
> >   * example: Timer, GPIO, UART events etc) on da830/omap-l137 EVM, hence
> >   * they are being reserved for codecs on the DSP side.
> >   */
> > -static const s16 da830_dma_rsv_chans[][2] = {
> > +static s16 da830_dma_rsv_chans[][2] = {
> 
> I wonder why you had to remove const here and in other places. You seem
> to be allocating new memory for DT case anyway. Its also not a good idea
> to modify the passed platform data.

Good catch. Although I was never modifying pdata itself, at one point
the DT parsing code was modifying pointers that were const and I removed
the const. I've since fixed that issue since we'd like them to be const
as you point out. As a result, I've removed all these changes from v4 so
all those files aren't touched.

-Matt