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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: G, Manjunath Kondaiah
> Sent: Tuesday, October 26, 2010 10:55 PM
[..]

> > [...]
> > >
> > > diff --git a/arch/arm/plat-omap/dma.c
> > b/arch/arm/plat-omap/dma.c index
> > > f5c5b8d..77241e2 100644
> > > --- a/arch/arm/plat-omap/dma.c
> > > +++ b/arch/arm/plat-omap/dma.c
> > > @@ -40,6 +40,147 @@
> > >
> [...]
> > > +static u16 reg_map_omap2[] = {
> > > +       [REVISION]              = 0x00,
> > > +       [GCR2]                  = 0x78,
> > > +       [IRQSTATUS_L0]          = 0x08,
> > > +       [IRQSTATUS_L1]          = 0x0c,
> > [..]
> > > +       /* OMAP4 specific registers */
> > > +       [CDP]                   = 0xd0,
> > > +       [CNDP]                  = 0xd4,
> > > +       [CCDN]                  = 0xd8,
> > > +};
> > > +
> > dumb question: any reason why a struct wont do?
> > struct reg_map_omap2 {
> > u16 revision;
> > ...
> > ...
> > }
> 
> This approach is as per i2c driver as suggested by kevin.
> http://www.spinics.net/lists/linux-omap/msg36446.html
Thanks for pointing this out. I2c has what 18 registers while dma has over 40 registers :( patch 11 [1] now I understand this step -> this merges 
together at later patchset - it starts to make sense now. It becomes reg_map. Thanks - looks like a good change in the eventual code.


[...]
> > > +static inline void dma_write(u32 val, int reg, int lch) {
> > > +       if (cpu_class_is_omap1()) {
> > > +               if (reg > OMAP1_CH_COMMON_START)
> > > +                       __raw_writew(val, dma_base +
> > > +                               (reg_map_omap1[reg] + 0x40 * lch));
> > > +               else
> > > +                       __raw_writew(val, dma_base +
> > reg_map_omap1[reg]);
> > > +       } else {
> > > +               if (reg > OMAP2_CH_COMMON_START)
> > > +                       __raw_writel(val, dma_base +
> > > +                               (reg_map_omap2[reg] + 0x60 * lch));
> > > +               else
> > > +                       __raw_writel(val, dma_base +
> > reg_map_omap2[reg]);
> > > +       }
> > > +}
Eventual code looks like this:

62 static inline void dma_write(u32 val, int reg, int lch)
63 {
64         if (d->dev_caps & IS_WORD_16) {
65                 if (reg >= CH_COMMON_START)
66                         __raw_writew(val, dma_base +
67                                 (reg_map[reg] + 0x40 * lch));
68                 else
69                         __raw_writew(val, dma_base + reg_map[reg]);
70         } else {
71                 if (reg > CH_COMMON_START)
72                         __raw_writel(val, dma_base +
73                                 (reg_map[reg] + 0x60 * lch));
74                 else
75                         __raw_writel(val, dma_base + reg_map[reg]);
76         }
77 }
I don't really see how inline will really help here!

> > > +
> > > +static inline u32 dma_read(int reg, int lch) {
> > > +       u32 val;
> > > +
> > > +       if (cpu_class_is_omap1()) {
> > > +               if (reg > OMAP1_CH_COMMON_START)
> > > +                       val = __raw_readw(dma_base +
> > > +                                       (reg_map_omap1[reg]
> > + 0x40 * lch));
> > > +               else
> > > +                       val = __raw_readw(dma_base +
> > reg_map_omap1[reg]);
> > > +       } else {
> > > +               if (reg > OMAP2_CH_COMMON_START)
> > > +                       val = __raw_readl(dma_base +
> > > +                                       (reg_map_omap2[reg]
> > + 0x60 * lch));
> > > +               else
> > > +                       val = __raw_readl(dma_base +
> > reg_map_omap2[reg]);
> > > +       }
> > > +       return val;
> > > +}
> > What is the benefit of using inline function here? would'nt
> > we increase code size? cant we use a function pointer
> > initialized to class1 or rest?
> > Quote from CodingStyle (15):
> > "A reasonable rule of thumb is to not put inline at functions
> > that have more than 3 lines of code in them. An exception to
> > this rule are the cases where a parameter is known to be a
> > compiletime constant, and as a result of this constantness
> > you *know* the compiler will be able to optimize most of your
> > function away at compile time. For a good example of this
> > later case, see the kmalloc() inline function.
> > "
> > is the expectation that cpu_class_is_omap1 compile time
> > constant hence the actual compiled in code is smaller
> > -candidate for inline?
> 
> Detailed discussion and alignment can be found at:
> http://www.spinics.net/lists/linux-omap/thrd6.html
Better link:
http://marc.info/?t=128264802300006&r=1&w=2

> 
> Search for:
> [PATCH v2 07/11] OMAP2/3/4: DMA: HWMOD: Device registration

http://marc.info/?l=linux-omap&m=128464661906497&w=2
my question is slightly different here - debate of suggestion to use inline
is based on the size of code involved, the discussion in the thread 
discussed around 3 lines of code, which made sense, unfortunately, the 
thread does not answer my question unfortunately for *this* specific patch
- OR do you wish to point me to some specific link which answers this?

> 
> 
> >
> > >
> > >  #ifdef CONFIG_ARCH_OMAP15XX
> > >  /* Returns 1 if the DMA module is in OMAP1510-compatible mode, 0
> > >         if (cpu_class_is_omap1()) {
> [...]
> > >                 u16 ccr;
> > >
> > > -               ccr = dma_read(CCR(lch));
> > > +               l = dma_read(CSDP1, lch);
> > > +               l &= ~0x03;
> > > +               l |= data_type;
> > > +               dma_write(l, CSDP1, lch);
> > > +
> > > +               ccr = dma_read(CCR1, lch);
> > >                 ccr &= ~(1 << 5);
> > >                 if (sync_mode == OMAP_DMA_SYNC_FRAME)
> > >                         ccr |= 1 << 5;
> > > -               dma_write(ccr, CCR(lch));
> > > +               dma_write(ccr, CCR1, lch);
> > >
> > > -               ccr = dma_read(CCR2(lch));
> > > +               ccr = dma_read(CCR1_2, lch);
> > >                 ccr &= ~(1 << 2);
> > >                 if (sync_mode == OMAP_DMA_SYNC_BLOCK)
> > >                         ccr |= 1 << 2;
> > > -               dma_write(ccr, CCR2(lch));
> > > +               dma_write(ccr, CCR1_2, lch);
> > >         }
> > >
> > >         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);
> > This seems to me to be a replication mess :( compared to the original
> > CSDP(lch) - which mapped to class1 or class2 as needed - same
> > elsewhere.
> > Code is becoming pretty much needing a refactoring as a result :(
> >
> > [...]
> 
> This is interim patch towards hwmod and driver cleanup. As mentioned in
> the
> patch description, objective is to replace lowlevel read/write macros with
> inline functions. For few registers(between omap1 and omap2+), the offset
> is
> different. Earlier, this offset difference was handled through macros.
> Now, it is replaced with inline functions, the code should be seperated
> for
> handling between omap1 and omap2+. It will get cleaned up once the code is
> moved respective mach-omapx directories.
> 
> You can have look at: PATCH 11/13

Ok looks like it never made to ML :( [1] as you refered to in a later patch 
Makes sense to me now. Thanks.


[1] http://dev.omapzoom.org/?p=manju/kernel-omap3-dev.git;a=commitdiff;h=7457a6b4248772aaa52dfb13a170f891596a512a
Regards,
Nishanth Menon
--
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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux