> -----Original Message----- > From: Tony Lindgren [mailto:tony@xxxxxxxxxxx] > Sent: Tuesday, November 09, 2010 6:11 AM > To: G, Manjunath Kondaiah > Cc: linux-omap@xxxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; 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@xxxxxx> [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-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
