[PATCH v4 01/13] OMAP: DMA: Replace read/write macros with functions

[PATCH v4 01/13] OMAP: DMA: Replace read/write macros with functions

[Tony Lindgren]

Hi,

* G, Manjunath Kondaiah <manjugk@ti.com> [101108 05:58]:

> +static u16 reg_map_omap1[] = {
> +	[GCR1]		= 0x400,
> +	[GSCR]		= 0x404,
> +	[GRST]		= 0x408,
...
> +};

The above you should move to mach-omap1/dma.c and pass it in the init
function to plat-omap/dma.c. Let's try to make the common code clean
of omap1/2/3/4 specific data.

> +static u16 reg_map_omap2[] = {
> +	[REVISION]		= 0x00,
> +	[GCR2]			= 0x78,
> +	[IRQSTATUS_L0]		= 0x08,
...
> +};

And the above you should move to mach-omap2/dma.c.

It's OK to do it in a later patch if you prefer that, but in that case
the patch description here should mention it.

>  	if (cpu_class_is_omap2() && dma_trigger) {
>  		u32 val;
>  
> -		val = dma_read(CCR(lch));
> +		l = dma_read(CSDP2, lch);
> +		l &= ~0x03;
> +		l |= data_type;
> +		dma_write(l, CSDP2, lch);
> +
> +		val = dma_read(CCR2, lch);

This seems to be doing more than just change the read function?

Please keep any functional changes separate. Let's first get the
hwmod based initialization done without any functional changes
to avoid breaking things.
  
> @@ -475,11 +654,19 @@ void omap_set_dma_src_data_pack(int lch, int enable)
>  {
>  	u32 l;
>  
> -	l = dma_read(CSDP(lch));
> +	if (cpu_class_is_omap1())
> +		l = dma_read(CSDP1, lch);
> +	else
> +		l = dma_read(CSDP2, lch);
> +
>  	l &= ~(1 << 6);
>  	if (enable)
>  		l |= (1 << 6);
> -	dma_write(l, CSDP(lch));
> +
> +	if (cpu_class_is_omap1())
> +		dma_write(l, CSDP1, lch);
> +	else
> +		dma_write(l, CSDP2, lch);
>  }

Since you now have the register mapping table, why do you still need
to separate between CSDP1 and CSDP2 registers?

You should now be able to call them just CSDP, and then the register
can be mapped to the right offset for omap1 and omap2+.

So this should be just:

void omap_set_dma_src_data_pack(int lch, int enable)
{
	u32 l;

	l = p->dma_read(CSDP, lch);
	l &= ~(1 << 6);
	if (enable)
		l |= (1 << 6);
	p->dma_write(l, CSDP, lch);
}

Please check the other functions for this too, that should
leave out quite a bit of if cpu_class_is_omap1 clutter.

And looking at it more, you should only have one enum for the
registers, not separate enum for both omap1 and omap2+. The
enum should be common for all of them. If needed, the enum
should be separate for common register and channel specific
registers.

Some 32-bit registers for omap1 have the lower and upper registers,
because of the 16-bit register access. So those still need to be
handled separately for omap1. That can be handled automatically
for omap1 (untested):

static inline u32 omap1_dma_read(int reg, int lch)
{
	u32 va, val, ch_offset = 0;

	if (reg > OMAP1_CH_COMMON_START)
		ch_offset = 0x40 * lch;

	va = dma_base + reg_map_omap1[reg] + ch_offset;
	val = __raw_readw(va);

	/* Some channel registers also have an associated upper register */
	if (reg >= CSSA) {
		16 upper = __raw_readw(va + 2);
		val |= (upper << 16);
	}

	return val;
}
...

Regards,

Tony

[PATCH v4 01/13] OMAP: DMA: Replace read/write macros with functions

[G, Manjunath Kondaiah]

> -----Original Message-----
> From: Tony Lindgren [mailto:tony at atomide.com] 
> Sent: Tuesday, November 09, 2010 6:11 AM
> To: G, Manjunath Kondaiah
> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Cousson, Benoit; Kevin 
> Hilman; Shilimkar, Santosh
> Subject: Re: [PATCH v4 01/13] OMAP: DMA: Replace read/write 
> macros with functions
> 
> Hi,
> 
> * G, Manjunath Kondaiah <manjugk@ti.com> [101108 05:58]:
> 
> > +static u16 reg_map_omap1[] = {
> > +	[GCR1]		= 0x400,
> > +	[GSCR]		= 0x404,
> > +	[GRST]		= 0x408,
> ...
> > +};
> 
> The above you should move to mach-omap1/dma.c and pass it in the init
> function to plat-omap/dma.c. Let's try to make the common code clean
> of omap1/2/3/4 specific data.
> 
> > +static u16 reg_map_omap2[] = {
> > +	[REVISION]		= 0x00,
> > +	[GCR2]			= 0x78,
> > +	[IRQSTATUS_L0]		= 0x08,
> ...
> > +};
> 
> And the above you should move to mach-omap2/dma.c.
> 
> It's OK to do it in a later patch if you prefer that, but in that case
> the patch description here should mention it.

It's already taken care in the later patches. I will update the patch
description accordingly.

> 
> >  	if (cpu_class_is_omap2() && dma_trigger) {
> >  		u32 val;
> >  
> > -		val = dma_read(CCR(lch));
> > +		l = dma_read(CSDP2, lch);
> > +		l &= ~0x03;
> > +		l |= data_type;
> > +		dma_write(l, CSDP2, lch);
> > +
> > +		val = dma_read(CCR2, lch);
> 
> This seems to be doing more than just change the read function?
> 
> Please keep any functional changes separate. Let's first get the
> hwmod based initialization done without any functional changes
> to avoid breaking things.
>   
> > @@ -475,11 +654,19 @@ void omap_set_dma_src_data_pack(int 
> lch, int enable)
> >  {
> >  	u32 l;
> >  
> > -	l = dma_read(CSDP(lch));
> > +	if (cpu_class_is_omap1())
> > +		l = dma_read(CSDP1, lch);
> > +	else
> > +		l = dma_read(CSDP2, lch);
> > +
> >  	l &= ~(1 << 6);
> >  	if (enable)
> >  		l |= (1 << 6);
> > -	dma_write(l, CSDP(lch));
> > +
> > +	if (cpu_class_is_omap1())
> > +		dma_write(l, CSDP1, lch);
> > +	else
> > +		dma_write(l, CSDP2, lch);
> >  }
> 
> Since you now have the register mapping table, why do you still need
> to separate between CSDP1 and CSDP2 registers?
> 
> You should now be able to call them just CSDP, and then the register
> can be mapped to the right offset for omap1 and omap2+.
> 
> So this should be just:
> 
> void omap_set_dma_src_data_pack(int lch, int enable)
> {
> 	u32 l;
> 
> 	l = p->dma_read(CSDP, lch);
> 	l &= ~(1 << 6);
> 	if (enable)
> 		l |= (1 << 6);
> 	p->dma_write(l, CSDP, lch);
> }
> 
> Please check the other functions for this too, that should
> leave out quite a bit of if cpu_class_is_omap1 clutter.
> 
> And looking at it more, you should only have one enum for the
> registers, not separate enum for both omap1 and omap2+. The
> enum should be common for all of them. If needed, the enum
> should be separate for common register and channel specific
> registers.

ok. I will use single enum for all omap's in plat/dma.h which
can be accessed by both mach-omap1/dma.c and mach-omap2/dma.c
in later patches.

> 
> Some 32-bit registers for omap1 have the lower and upper registers,
> because of the 16-bit register access. So those still need to be
> handled separately for omap1. That can be handled automatically
> for omap1 (untested):
> 
> static inline u32 omap1_dma_read(int reg, int lch)
> {
> 	u32 va, val, ch_offset = 0;
> 
> 	if (reg > OMAP1_CH_COMMON_START)
> 		ch_offset = 0x40 * lch;
> 
> 	va = dma_base + reg_map_omap1[reg] + ch_offset;
> 	val = __raw_readw(va);
> 
> 	/* Some channel registers also have an associated upper 
> register */
> 	if (reg >= CSSA) {
> 		16 upper = __raw_readw(va + 2);
> 		val |= (upper << 16);
> 	}
> 
> 	return val;
> }

Thanks for the suggestion. It will be taken care with next version 
with some more optimizations such as determining ch_offset between
omap1 and omap2+, reg_map, ch_common_start at init time as suggested
by kevin.

-Manjunath

[PATCH v4 07/13] OMAP4: DMA: hwmod: add system DMA

[Varadarajan, Charulatha]

One minor comment,
 
<<snip>>

> +
>  static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
>  	/* dmm class */
>  	&omap44xx_dmm_hwmod,
> @@ -1077,6 +1174,8 @@ static __initdata struct omap_hwmod 
> *omap44xx_hwmods[] = {
>  	&omap44xx_uart2_hwmod,
>  	&omap44xx_uart3_hwmod,
>  	&omap44xx_uart4_hwmod,
> +	/* dma class */
> +	&omap44xx_dma_system_hwmod,

Add a blank line between each class. See [1]
Same is applicable to patches 4, 5 and 6

[1] https://patchwork.kernel.org/patch/305062/

[PATCH v4 02/13] OMAP: DMA: Introduce errata handling feature

[Tony Lindgren]

* G, Manjunath Kondaiah <manjugk@ti.com> [101108 05:58]:
> Implement errata handling to use flags instead of cpu_is_*
> and cpu_class_* in the code.
> 
> The errata flags are initialized at init time and during runtime
> we are using the errata variable (via the IS_DMA_ERRATA macro)
> to execute the required errata workaround.

Applying the first patch and this patch won't compile on omap1:

arch/arm/plat-omap/built-in.o: In function `omap_init_dma':
omap-pm-noop.c:(.init.text+0x604): undefined reference to `omap_type'

Tony