Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
"G, Manjunath Kondaiah" writes:
>> -Original Message-
>> From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
>> Sent: Wednesday, November 10, 2010 9:33 PM
>> To: G, Manjunath Kondaiah
>> Cc: linux-omap@vger.kernel.org;
>> linux-arm-ker...@lists.infradead.org; Cousson, Benoit;
>> Shilimkar, Santosh
>> Subject: Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write
>> macros with functions
>>
>> "G, Manjunath Kondaiah" writes:
>>
>> [...]
>>
>> >> > + 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]);
>> >> > + }
>> >> > +}
>> >>
>> >> The register base offset, register array and the stride (offset
>> >> between instances: 0x40 or 0x60) are detectable at init time, and
>> >> there's no reason to have conditional code for them. IOW, they
>> >> should be init time constants. Doing so would greatly simply these
>> >> functions. In fact the CH_COMMON_START could also be an init time
>> >> constant as well. So, given the following init_time constants:
>> >> dma_ch_common_start, dma_stride, reg_map, the above would
>> be reduced
>> >> to something actually worth inlining, for example (not actually
>> >> tested):
>> >>
>> >> static inline void dma_write(u32 val, int reg, int lch)
>> >> {
>> >> u8 stride = (reg > dma_ch_common_start) ? dma_stride : 0;
>> >>
>> >> __raw_writel(val, dma_base + (reg_map[reg] +
>> stride * lch));
>> >> }
>> >>
>> >> Same applies to dma_read().
>> >
>> > Thanks for the suggestion. It is taken care along with
>> Tony's comment
>> > for handling these read/write's between omap1 and omap2+.
>> >
>> > Here is code snippet for handling both omap1(includes 16 bit
>> > registers) and omap2+
>> >
>> > static inline void dma_write(u32 val, int reg, int lch)
>> > {
>> > u8 stride;
>> > u32 offset;
>> >
>> > stride = (reg >= dma_common_ch_start) ? dma_stride : 0;
>> > offset = reg_map[reg] + (stride * lch);
>> >
>> > if (dma_stride == 0x40) {
>>
>> The use of hard-coded constants still isn't right here.This is
>> bascally the same as a cpu_is_omap1 check. After you separate out the
>> device files, I thought you had separate omap1 and omap2+ versions of
>> these, so I don't follow the need for this.
>
> This code will be moved to respective mach-omapx dma files and this
> conditional check vanishes automatically in PATCH 10/13. Since this patch
> targets replacing read/write macros with inline functions, no functionality
> changes(except change in logic for handling 16bit registers for omap1)
> are done with new patch hence handling omap1 and omap2+ is
> done this way.
>
> I hope having the conditional check in this interim patch is ok.
Personally, I would rather not have an intermediate step, but if it
makes the series smoother, I guess it's OK.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
> -Original Message-
> From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
> Sent: Wednesday, November 10, 2010 9:33 PM
> To: G, Manjunath Kondaiah
> Cc: linux-omap@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; Cousson, Benoit;
> Shilimkar, Santosh
> Subject: Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write
> macros with functions
>
> "G, Manjunath Kondaiah" writes:
>
> [...]
>
> >> > +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]);
> >> > +}
> >> > +}
> >>
> >> The register base offset, register array and the stride (offset
> >> between instances: 0x40 or 0x60) are detectable at init time, and
> >> there's no reason to have conditional code for them. IOW, they
> >> should be init time constants. Doing so would greatly simply these
> >> functions. In fact the CH_COMMON_START could also be an init time
> >> constant as well. So, given the following init_time constants:
> >> dma_ch_common_start, dma_stride, reg_map, the above would
> be reduced
> >> to something actually worth inlining, for example (not actually
> >> tested):
> >>
> >> static inline void dma_write(u32 val, int reg, int lch)
> >> {
> >> u8 stride = (reg > dma_ch_common_start) ? dma_stride : 0;
> >>
> >> __raw_writel(val, dma_base + (reg_map[reg] +
> stride * lch));
> >> }
> >>
> >> Same applies to dma_read().
> >
> > Thanks for the suggestion. It is taken care along with
> Tony's comment
> > for handling these read/write's between omap1 and omap2+.
> >
> > Here is code snippet for handling both omap1(includes 16 bit
> > registers) and omap2+
> >
> > static inline void dma_write(u32 val, int reg, int lch)
> > {
> > u8 stride;
> > u32 offset;
> >
> > stride = (reg >= dma_common_ch_start) ? dma_stride : 0;
> > offset = reg_map[reg] + (stride * lch);
> >
> > if (dma_stride == 0x40) {
>
> The use of hard-coded constants still isn't right here.This is
> bascally the same as a cpu_is_omap1 check. After you separate out the
> device files, I thought you had separate omap1 and omap2+ versions of
> these, so I don't follow the need for this.
This code will be moved to respective mach-omapx dma files and this
conditional check vanishes automatically in PATCH 10/13. Since this patch
targets replacing read/write macros with inline functions, no functionality
changes(except change in logic for handling 16bit registers for omap1)
are done with new patch hence handling omap1 and omap2+ is
done this way.
I hope having the conditional check in this interim patch is ok.
-Manjunath
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
"G, Manjunath Kondaiah" writes:
[...]
>> > + 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]);
>> > + }
>> > +}
>>
>> The register base offset, register array and the stride (offset
>> between instances: 0x40 or 0x60) are detectable at init time, and
>> there's no reason to have conditional code for them. IOW, they
>> should be init time constants. Doing so would greatly simply these
>> functions. In fact the CH_COMMON_START could also be an init time
>> constant as well. So, given the following init_time constants:
>> dma_ch_common_start, dma_stride, reg_map, the above would be reduced
>> to something actually worth inlining, for example (not actually
>> tested):
>>
>> static inline void dma_write(u32 val, int reg, int lch)
>> {
>> u8 stride = (reg > dma_ch_common_start) ? dma_stride : 0;
>>
>> __raw_writel(val, dma_base + (reg_map[reg] + stride * lch));
>> }
>>
>> Same applies to dma_read().
>
> Thanks for the suggestion. It is taken care along with Tony's comment
> for handling these read/write's between omap1 and omap2+.
>
> Here is code snippet for handling both omap1(includes 16 bit
> registers) and omap2+
>
> static inline void dma_write(u32 val, int reg, int lch)
> {
> u8 stride;
> u32 offset;
>
> stride = (reg >= dma_common_ch_start) ? dma_stride : 0;
> offset = reg_map[reg] + (stride * lch);
>
> if (dma_stride == 0x40) {
The use of hard-coded constants still isn't right here.This is
bascally the same as a cpu_is_omap1 check. After you separate out the
device files, I thought you had separate omap1 and omap2+ versions of
these, so I don't follow the need for this.
> __raw_writew(val, omap_dma_base + offset);
This could be moved before the 'if' and the 'else' clause removed.
> if ((reg > CLNK_CTRL && reg < CCR2) ||
> (reg > PCHD_ID && reg < CAPS_2)) {
> u32 offset2 = reg_map[reg] + 2 + (stride * lch);
> __raw_writew(val >> 16, omap_dma_base + offset2);
> }
> } else
> __raw_writel(val, omap_dma_base + offset);
> }
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
> -Original Message-
> From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
> Sent: Wednesday, November 10, 2010 3:08 AM
> To: G, Manjunath Kondaiah
> Cc: linux-omap@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; Cousson, Benoit;
> Shilimkar, Santosh
> Subject: Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write
> macros with functions
>
> "G, Manjunath Kondaiah" writes:
>
> > The low level read/write macros are replaced with static
> inline functions
> > and register offsets are handled through static register
> offset tables
> > mapped through enumeration constants.
> >
> > The objective of this patch is to prepare for omap dma
> driver cleanup
> > and dma hwmod implementation. The code optimization and
> moving machine
> > specific code to respective mach-omapx dma file will be
> handled in the
> > rest of this patch series.
>
> [...]
>
> > -#define dma_write(val, reg)
> \
> > -({
> \
> > - if (cpu_class_is_omap1())
> \
> > - __raw_writew((u16)(val), omap_dma_base +
> OMAP1_DMA_##reg); \
> > - else
> \
> > - __raw_writel((val), omap_dma_base +
> OMAP_DMA4_##reg); \
> > -})
> > +static inline void dma_write(u32 val, int reg, int lch)
> > +{
> > + if (cpu_class_is_omap1()) {
>
> Reminder: any use of cpu_is_* checks outside of init code
> will be NAK'd.
>
> cpu_is_* check do not belong at runtime, especially in a crucial path
> like this.
ok. removed cpu_is_* checks and taken care in init.
>
> > + 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]);
> > + }
> > +}
>
> The register base offset, register array and the stride
> (offset between
> instances: 0x40 or 0x60) are detectable at init time, and there's no
> reason to have conditional code for them. IOW, they should
> be init time
> constants. Doing so would greatly simply these functions.
> In fact the
> CH_COMMON_START could also be an init time constant as well.
> So, given
> the following init_time constants: dma_ch_common_start, dma_stride,
> reg_map, the above would be reduced to something actually worth
> inlining, for example (not actually tested):
>
> static inline void dma_write(u32 val, int reg, int lch)
> {
> u8 stride = (reg > dma_ch_common_start) ? dma_stride : 0;
>
> __raw_writel(val, dma_base + (reg_map[reg] + stride * lch));
> }
>
> Same applies to dma_read().
Thanks for the suggestion. It is taken care along with Tony's comment
for handling these read/write's between omap1 and omap2+.
Here is code snippet for handling both omap1(includes 16 bit
registers) and omap2+
static inline void dma_write(u32 val, int reg, int lch)
{
u8 stride;
u32 offset;
stride = (reg >= dma_common_ch_start) ? dma_stride : 0;
offset = reg_map[reg] + (stride * lch);
if (dma_stride == 0x40) {
__raw_writew(val, omap_dma_base + offset);
if ((reg > CLNK_CTRL && reg < CCR2) ||
(reg > PCHD_ID && reg < CAPS_2)) {
u32 offset2 = reg_map[reg] + 2 + (stride * lch);
__raw_writew(val >> 16, omap_dma_base + offset2);
}
} else
__raw_writel(val, omap_dma_base + offset);
}
static inline u32 dma_read(int reg, int lch)
{
u8 stride;
u32 offset, val;
stride = (reg >= dma_common_ch_start) ? dma_stride : 0;
offset = reg_map[reg] + (stride * lch);
if (dma_stride == 0x40) {
val = __raw_readw(omap_dma_base + offset);
if ((reg > CLNK_CTRL && reg < CCR2) ||
(reg > PCHD_ID && reg < CAPS_2)) {
u16 upper;
u32 offset2 = reg_map[reg] + 2 + (stride * lch);
upper = __raw_readw(omap_dma_base + offset2);
val |= (upper << 16);
}
} else
val = __raw_readl(omap_dma_base + offset);
return val;
}
-Manjunath
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
"G, Manjunath Kondaiah" writes:
> The low level read/write macros are replaced with static inline functions
> and register offsets are handled through static register offset tables
> mapped through enumeration constants.
>
> The objective of this patch is to prepare for omap dma driver cleanup
> and dma hwmod implementation. The code optimization and moving machine
> specific code to respective mach-omapx dma file will be handled in the
> rest of this patch series.
[...]
> -#define dma_write(val, reg) \
> -({ \
> - if (cpu_class_is_omap1()) \
> - __raw_writew((u16)(val), omap_dma_base + OMAP1_DMA_##reg); \
> - else\
> - __raw_writel((val), omap_dma_base + OMAP_DMA4_##reg); \
> -})
> +static inline void dma_write(u32 val, int reg, int lch)
> +{
> + if (cpu_class_is_omap1()) {
Reminder: any use of cpu_is_* checks outside of init code will be NAK'd.
cpu_is_* check do not belong at runtime, especially in a crucial path
like this.
> + 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]);
> + }
> +}
The register base offset, register array and the stride (offset between
instances: 0x40 or 0x60) are detectable at init time, and there's no
reason to have conditional code for them. IOW, they should be init time
constants. Doing so would greatly simply these functions. In fact the
CH_COMMON_START could also be an init time constant as well. So, given
the following init_time constants: dma_ch_common_start, dma_stride,
reg_map, the above would be reduced to something actually worth
inlining, for example (not actually tested):
static inline void dma_write(u32 val, int reg, int lch)
{
u8 stride = (reg > dma_ch_common_start) ? dma_stride : 0;
__raw_writel(val, dma_base + (reg_map[reg] + stride * lch));
}
Same applies to dma_read().
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
> -Original Message-
> From: Menon, Nishanth
> Sent: Wednesday, October 27, 2010 7:56 PM
> To: G, Manjunath Kondaiah
> Cc: linux-omap@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; Cousson, Benoit; Kevin
> Hilman; Shilimkar, Santosh
> Subject: RE: [PATCH v3 01/13] OMAP: DMA: Replace read/write
> macros with functions
>
[...]
> > 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:
>
RE: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
> -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=12826480236&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 *t
RE: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
> -Original Message-
> From: Menon, Nishanth
> Sent: Tuesday, October 26, 2010 8:19 PM
> To: G, Manjunath Kondaiah
> Cc: linux-omap@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; Cousson, Benoit; Kevin
> Hilman; Shilimkar, Santosh
> Subject: Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write
> macros with functions
>
> G, Manjunath Kondaiah had written, on 10/26/2010 08:25 AM,
> the following:
> [...]
> >
> > 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
>
> [..]
> >
> > -#define dma_read(reg)
> \
> > -({
> \
> > - u32 __val;
> \
> > - if (cpu_class_is_omap1())
> \
> > - __val = __raw_readw(omap_dma_base +
> OMAP1_DMA_##reg); \
> > - else
> \
> > - __val = __raw_readl(omap_dma_base +
> OMAP_DMA4_##reg); \
> > - __val;
> \
> > -})
> > -
> > -#define dma_write(val, reg)
> \
> > -({
> \
> > - if (cpu_class_is_omap1())
> \
> > - __raw_writew((u16)(val), omap_dma_base +
> OMAP1_DMA_##reg); \
> > - else
> \
> > - __raw_writel((val), omap_dma_base +
> OMAP_DMA4_##reg); \
> > -})
> > +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]);
> > + }
> > +}
> > +
> > +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 constant
Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions
G, Manjunath Kondaiah had written, on 10/26/2010 08:25 AM, the following:
[...]
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 @@
#undef DEBUG
+enum {
+ GCR1 = 0, GSCR, GRST, HW_ID,
+ PCH2_ID,PCH0_ID,PCH1_ID,PCHG_ID,
+ PCHD_ID,CAPS1_0_U, CAPS1_0_L, CAPS1_1_U,
+ CAPS1_1_L, CAPS1_2,CAPS1_3,CAPS1_4,
+ PCH2_SR,PCH0_SR,PCH1_SR,PCHD_SR,
[...]
+};
+
+static u16 reg_map_omap1[] = {
+ [GCR1] = 0x400,
+ [GSCR] = 0x404,
+ [GRST] = 0x408,
+ [HW_ID] = 0x442,
+ [PCH2_ID] = 0x444,
+ [PCH0_ID] = 0x446,
+ [PCH1_ID] = 0x448,
+ [PCHG_ID] = 0x44a,
+ [PCHD_ID] = 0x44c,
[...]
+ [LCH_CTRL] = 0x2a,
+};
+
+enum {
+ REVISION = 0, GCR2, IRQSTATUS_L0, IRQSTATUS_L1,
+ IRQSTATUS_L2, IRQSTATUS_L3, IRQENABLE_L0, IRQENABLE_L1,
[...]
+
+ /* OMAP4 specific registers */
+ CDP,CNDP, CCDN,
+
+ OMAP2_CH_COMMON_END,
+};
+
+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;
...
...
}
[..]
-#define dma_read(reg) \
-({ \
- u32 __val; \
- if (cpu_class_is_omap1()) \
- __val = __raw_readw(omap_dma_base + OMAP1_DMA_##reg); \
- else\
- __val = __raw_readl(omap_dma_base + OMAP_DMA4_##reg); \
- __val; \
-})
-
-#define dma_write(val, reg)\
-({ \
- if (cpu_class_is_omap1()) \
- __raw_writew((u16)(val), omap_dma_base + OMAP1_DMA_##reg); \
- else\
- __raw_writel((val), omap_dma_base + OMAP_DMA4_##reg); \
-})
+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]);
+ }
+}
+
+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?
#ifdef CONFIG_ARCH_OMAP15XX
/* Returns 1 if the DMA module is in OMAP1510-compatible mode, 0 otherwise */
@@ -209,11 +
