* [PATCH 0/3] staging: tidspbridge: fix ioremap() usage
@ 2010-10-10 17:40 Felipe Contreras
2010-10-10 17:40 ` [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo Felipe Contreras
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Felipe Contreras @ 2010-10-10 17:40 UTC (permalink / raw)
To: linux-arm-kernel
Now we truly reserve the memory so that the kernel doesn't map it.
Unfortunately, the ARM architecture code doesn't provide a good way to do this,
so I had to modify it a bit.
Felipe Contreras (3):
arm: mm: allow boards to fiddle with meminfo
omap: dsp: fix ioremap() usage
staging: tidspbridge: remove memory consistency from TODO list
arch/arm/include/asm/mach/arch.h | 2 +-
arch/arm/mm/init.c | 7 +++--
arch/arm/plat-omap/common.c | 43 +++++++++++++++++++++++++++--
arch/arm/plat-omap/devices.c | 30 ---------------------
arch/arm/plat-omap/include/plat/common.h | 3 +-
arch/arm/plat-omap/include/plat/dsp.h | 6 ----
drivers/staging/tidspbridge/TODO | 1 -
7 files changed, 47 insertions(+), 45 deletions(-)
--
1.7.3.1.2.g7fe2b
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo 2010-10-10 17:40 [PATCH 0/3] staging: tidspbridge: fix ioremap() usage Felipe Contreras @ 2010-10-10 17:40 ` Felipe Contreras 2010-10-10 19:03 ` Russell King - ARM Linux 2010-10-10 17:40 ` [PATCH 2/3] omap: dsp: fix ioremap() usage Felipe Contreras 2010-10-10 17:40 ` [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list Felipe Contreras 2 siblings, 1 reply; 13+ messages in thread From: Felipe Contreras @ 2010-10-10 17:40 UTC (permalink / raw) To: linux-arm-kernel So that they can reserve some memory. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- arch/arm/include/asm/mach/arch.h | 2 +- arch/arm/mm/init.c | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h index 8a0dd18..408e4d5 100644 --- a/arch/arm/include/asm/mach/arch.h +++ b/arch/arm/include/asm/mach/arch.h @@ -38,7 +38,7 @@ struct machine_desc { void (*fixup)(struct machine_desc *, struct tag *, char **, struct meminfo *); - void (*reserve)(void);/* reserve mem blocks */ + void (*reserve)(struct meminfo *);/* reserve mem blocks */ void (*map_io)(void);/* IO mapping function */ void (*init_irq)(void); struct sys_timer *timer; /* system tick timer */ diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c index 7185b00..4b5c117 100644 --- a/arch/arm/mm/init.c +++ b/arch/arm/mm/init.c @@ -272,8 +272,6 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc) int i; memblock_init(); - for (i = 0; i < mi->nr_banks; i++) - memblock_add(mi->bank[i].start, mi->bank[i].size); /* Register the kernel text, kernel data and initrd with memblock. */ #ifdef CONFIG_XIP_KERNEL @@ -295,7 +293,10 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc) /* reserve any platform specific memblock areas */ if (mdesc->reserve) - mdesc->reserve(); + mdesc->reserve(mi); + + for (i = 0; i < mi->nr_banks; i++) + memblock_add(mi->bank[i].start, mi->bank[i].size); memblock_analyze(); memblock_dump_all(); -- 1.7.3.1.2.g7fe2b ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo 2010-10-10 17:40 ` [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo Felipe Contreras @ 2010-10-10 19:03 ` Russell King - ARM Linux 2010-10-10 19:37 ` Felipe Contreras 0 siblings, 1 reply; 13+ messages in thread From: Russell King - ARM Linux @ 2010-10-10 19:03 UTC (permalink / raw) To: linux-arm-kernel On Sun, Oct 10, 2010 at 08:40:38PM +0300, Felipe Contreras wrote: > So that they can reserve some memory. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > arch/arm/include/asm/mach/arch.h | 2 +- > arch/arm/mm/init.c | 7 ++++--- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h > index 8a0dd18..408e4d5 100644 > --- a/arch/arm/include/asm/mach/arch.h > +++ b/arch/arm/include/asm/mach/arch.h > @@ -38,7 +38,7 @@ struct machine_desc { > void (*fixup)(struct machine_desc *, > struct tag *, char **, > struct meminfo *); > - void (*reserve)(void);/* reserve mem blocks */ > + void (*reserve)(struct meminfo *);/* reserve mem blocks */ > void (*map_io)(void);/* IO mapping function */ > void (*init_irq)(void); > struct sys_timer *timer; /* system tick timer */ > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c > index 7185b00..4b5c117 100644 > --- a/arch/arm/mm/init.c > +++ b/arch/arm/mm/init.c > @@ -272,8 +272,6 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc) > int i; > > memblock_init(); > - for (i = 0; i < mi->nr_banks; i++) > - memblock_add(mi->bank[i].start, mi->bank[i].size); > > /* Register the kernel text, kernel data and initrd with memblock. */ > #ifdef CONFIG_XIP_KERNEL > @@ -295,7 +293,10 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc) > > /* reserve any platform specific memblock areas */ > if (mdesc->reserve) > - mdesc->reserve(); > + mdesc->reserve(mi); > + > + for (i = 0; i < mi->nr_banks; i++) > + memblock_add(mi->bank[i].start, mi->bank[i].size); It is not a good idea to change the ordering here, as we'll now be adding the memory blocks _after_ we've started to make reservations into memblock. At least the omapfb code wants there to be memory present in memblock when ->reserve is called. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo 2010-10-10 19:03 ` Russell King - ARM Linux @ 2010-10-10 19:37 ` Felipe Contreras 0 siblings, 0 replies; 13+ messages in thread From: Felipe Contreras @ 2010-10-10 19:37 UTC (permalink / raw) To: linux-arm-kernel On Sun, Oct 10, 2010 at 10:03 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Sun, Oct 10, 2010 at 08:40:38PM +0300, Felipe Contreras wrote: >> So that they can reserve some memory. >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> >> --- >> ?arch/arm/include/asm/mach/arch.h | ? ?2 +- >> ?arch/arm/mm/init.c ? ? ? ? ? ? ? | ? ?7 ++++--- >> ?2 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h >> index 8a0dd18..408e4d5 100644 >> --- a/arch/arm/include/asm/mach/arch.h >> +++ b/arch/arm/include/asm/mach/arch.h >> @@ -38,7 +38,7 @@ struct machine_desc { >> ? ? ? void ? ? ? ? ? ? ? ? ? ?(*fixup)(struct machine_desc *, >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct tag *, char **, >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct meminfo *); >> - ? ? void ? ? ? ? ? ? ? ? ? ?(*reserve)(void);/* reserve mem blocks ?*/ >> + ? ? void ? ? ? ? ? ? ? ? ? ?(*reserve)(struct meminfo *);/* reserve mem blocks ? ? ?*/ >> ? ? ? void ? ? ? ? ? ? ? ? ? ?(*map_io)(void);/* IO mapping function ?*/ >> ? ? ? void ? ? ? ? ? ? ? ? ? ?(*init_irq)(void); >> ? ? ? struct sys_timer ? ? ? ?*timer; ? ? ? ? /* system tick timer ? ?*/ >> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c >> index 7185b00..4b5c117 100644 >> --- a/arch/arm/mm/init.c >> +++ b/arch/arm/mm/init.c >> @@ -272,8 +272,6 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc) >> ? ? ? int i; >> >> ? ? ? memblock_init(); >> - ? ? for (i = 0; i < mi->nr_banks; i++) >> - ? ? ? ? ? ? memblock_add(mi->bank[i].start, mi->bank[i].size); >> >> ? ? ? /* Register the kernel text, kernel data and initrd with memblock. */ >> ?#ifdef CONFIG_XIP_KERNEL >> @@ -295,7 +293,10 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc) >> >> ? ? ? /* reserve any platform specific memblock areas */ >> ? ? ? if (mdesc->reserve) >> - ? ? ? ? ? ? mdesc->reserve(); >> + ? ? ? ? ? ? mdesc->reserve(mi); >> + >> + ? ? for (i = 0; i < mi->nr_banks; i++) >> + ? ? ? ? ? ? memblock_add(mi->bank[i].start, mi->bank[i].size); > > It is not a good idea to change the ordering here, as we'll now be > adding the memory blocks _after_ we've started to make reservations > into memblock. > > At least the omapfb code wants there to be memory present in memblock > when ->reserve is called. True. I initially added a new callback called reserve_mem(), but I thought it would be nicer to avoid adding a new function almost identical to an existing one. Anyway, what is your suggestion? Can you come up with something else? -- Felipe Contreras ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] omap: dsp: fix ioremap() usage 2010-10-10 17:40 [PATCH 0/3] staging: tidspbridge: fix ioremap() usage Felipe Contreras 2010-10-10 17:40 ` [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo Felipe Contreras @ 2010-10-10 17:40 ` Felipe Contreras 2010-10-10 20:17 ` Premi, Sanjeev 2010-10-11 15:15 ` Guzman Lugo, Fernando 2010-10-10 17:40 ` [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list Felipe Contreras 2 siblings, 2 replies; 13+ messages in thread From: Felipe Contreras @ 2010-10-10 17:40 UTC (permalink / raw) To: linux-arm-kernel On commit 309caa9 doing ioremap() became forbidden due tue architectural limitations. Only a single mapping is allowed now, so the mempool must not be part of the memory managed by the kernel. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- arch/arm/plat-omap/common.c | 43 +++++++++++++++++++++++++++-- arch/arm/plat-omap/devices.c | 30 --------------------- arch/arm/plat-omap/include/plat/common.h | 3 +- arch/arm/plat-omap/include/plat/dsp.h | 6 ---- 4 files changed, 42 insertions(+), 40 deletions(-) diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c index 57205a4..3fee3ca 100644 --- a/arch/arm/plat-omap/common.c +++ b/arch/arm/plat-omap/common.c @@ -37,7 +37,6 @@ #include <plat/fpga.h> #include <plat/serial.h> #include <plat/vram.h> -#include <plat/dsp.h> #include <plat/clock.h> @@ -84,11 +83,49 @@ const void *omap_get_var_config(u16 tag, size_t *len) } EXPORT_SYMBOL(omap_get_var_config); -void __init omap_reserve(void) +#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) + +static phys_addr_t omap_dsp_mempool_base; + +static void __init omap_dsp_reserve_mem(struct meminfo *mi) +{ + phys_addr_t size = CONFIG_TIDSPBRIDGE_MEMPOOL_SIZE; + phys_addr_t addr = ~0; + int i; + + if (!size) + return; + + for (i = mi->nr_banks - 1; i >= 0; i--) + if (mi->bank[i].size >= size) { + mi->bank[i].size -= size; + addr = mi->bank[i].start + mi->bank[i].size; + break; + } + + if (addr == ~0) { + pr_err("%s: failed to reserve 0x%x bytes\n", + __func__, size); + return; + } + + omap_dsp_mempool_base = addr; +} + +phys_addr_t omap_dsp_get_mempool_base(void) +{ + return omap_dsp_mempool_base; +} +EXPORT_SYMBOL(omap_dsp_get_mempool_base); +#else +static inline void omap_dsp_reserve_mem(struct meminfo *mi) { } +#endif + +void __init omap_reserve(struct meminfo *mi) { omapfb_reserve_sdram_memblock(); omap_vram_reserve_sdram_memblock(); - omap_dsp_reserve_sdram_memblock(); + omap_dsp_reserve_mem(mi); } /* diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c index 4c8f9b9..d1920be 100644 --- a/arch/arm/plat-omap/devices.c +++ b/arch/arm/plat-omap/devices.c @@ -15,7 +15,6 @@ #include <linux/platform_device.h> #include <linux/io.h> #include <linux/slab.h> -#include <linux/memblock.h> #include <mach/hardware.h> #include <asm/mach-types.h> @@ -273,35 +272,6 @@ static void omap_init_wdt(void) static inline void omap_init_wdt(void) {} #endif -#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) - -static phys_addr_t omap_dsp_phys_mempool_base; - -void __init omap_dsp_reserve_sdram_memblock(void) -{ - phys_addr_t size = CONFIG_TIDSPBRIDGE_MEMPOOL_SIZE; - phys_addr_t paddr; - - if (!size) - return; - - paddr = __memblock_alloc_base(size, SZ_1M, MEMBLOCK_REAL_LIMIT); - if (!paddr) { - pr_err("%s: failed to reserve %x bytes\n", - __func__, size); - return; - } - - omap_dsp_phys_mempool_base = paddr; -} - -phys_addr_t omap_dsp_get_mempool_base(void) -{ - return omap_dsp_phys_mempool_base; -} -EXPORT_SYMBOL(omap_dsp_get_mempool_base); -#endif - /* * This gets called after board-specific INIT_MACHINE, and initializes most * on-chip peripherals accessible on this board (except for few like USB): diff --git a/arch/arm/plat-omap/include/plat/common.h b/arch/arm/plat-omap/include/plat/common.h index 9776b41..3675492 100644 --- a/arch/arm/plat-omap/include/plat/common.h +++ b/arch/arm/plat-omap/include/plat/common.h @@ -30,11 +30,12 @@ #include <plat/i2c.h> struct sys_timer; +struct meminfo; extern void omap_map_common_io(void); extern struct sys_timer omap_timer; -extern void omap_reserve(void); +extern void omap_reserve(struct meminfo *mi); /* * IO bases for various OMAP processors diff --git a/arch/arm/plat-omap/include/plat/dsp.h b/arch/arm/plat-omap/include/plat/dsp.h index 9c604b3..4e53687 100644 --- a/arch/arm/plat-omap/include/plat/dsp.h +++ b/arch/arm/plat-omap/include/plat/dsp.h @@ -22,10 +22,4 @@ struct omap_dsp_platform_data { phys_addr_t phys_mempool_size; }; -#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) -extern void omap_dsp_reserve_sdram_memblock(void); -#else -static inline void omap_dsp_reserve_sdram_memblock(void) { } -#endif - #endif -- 1.7.3.1.2.g7fe2b ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] omap: dsp: fix ioremap() usage 2010-10-10 17:40 ` [PATCH 2/3] omap: dsp: fix ioremap() usage Felipe Contreras @ 2010-10-10 20:17 ` Premi, Sanjeev 2010-10-10 20:39 ` Felipe Contreras 2010-10-11 15:15 ` Guzman Lugo, Fernando 1 sibling, 1 reply; 13+ messages in thread From: Premi, Sanjeev @ 2010-10-10 20:17 UTC (permalink / raw) To: linux-arm-kernel > -----Original Message----- > From: linux-omap-owner at vger.kernel.org > [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of Felipe > Contreras > Sent: Sunday, October 10, 2010 11:11 PM > To: linux-arm; linux-omap; Greg KH > Cc: Ramirez Luna, Omar; Russell King; Felipe Contreras > Subject: [PATCH 2/3] omap: dsp: fix ioremap() usage > > On commit 309caa9 doing ioremap() became forbidden due tue > architectural > limitations. Only a single mapping is allowed now, so the mempool must > not be part of the memory managed by the kernel. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > arch/arm/plat-omap/common.c | 43 > +++++++++++++++++++++++++++-- > arch/arm/plat-omap/devices.c | 30 --------------------- > arch/arm/plat-omap/include/plat/common.h | 3 +- > arch/arm/plat-omap/include/plat/dsp.h | 6 ---- > 4 files changed, 42 insertions(+), 40 deletions(-) > > diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c > index 57205a4..3fee3ca 100644 > --- a/arch/arm/plat-omap/common.c > +++ b/arch/arm/plat-omap/common.c > @@ -37,7 +37,6 @@ > #include <plat/fpga.h> > #include <plat/serial.h> > #include <plat/vram.h> > -#include <plat/dsp.h> > > #include <plat/clock.h> > > @@ -84,11 +83,49 @@ const void *omap_get_var_config(u16 tag, > size_t *len) > } > EXPORT_SYMBOL(omap_get_var_config); > > -void __init omap_reserve(void) > +#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) [sp] I do understand that dspbridge is the only driver accessing dsp in the linux-omap; but there are other known drivers - which would need this feature. Can we use a more generic config option viz. CONFIG_RESERVE_DSPMEM (or something similar) so that code is easily (re)usable. ~sanjeev [snip]...[snip] > + > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] omap: dsp: fix ioremap() usage 2010-10-10 20:17 ` Premi, Sanjeev @ 2010-10-10 20:39 ` Felipe Contreras 0 siblings, 0 replies; 13+ messages in thread From: Felipe Contreras @ 2010-10-10 20:39 UTC (permalink / raw) To: linux-arm-kernel Hi, Please remove the chunks you are not replying to. On Sun, Oct 10, 2010 at 11:17 PM, Premi, Sanjeev <premi@ti.com> wrote: >> -void __init omap_reserve(void) >> +#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) > > [sp] I do understand that dspbridge is the only driver accessing dsp in > ? ? the linux-omap; but there are other known drivers - which would need > ? ? this feature. Are you talking about linux-omap the tree? If so, no, the tidspbridge is part of staging, not linux-omap. If you are trying to say there are other DSP drivers intended for OMAP, then I guess you are referring to dsp-link, which uses it's own tricks for contiguous memory allocation (CMEM). So, who and how would benefit from this? > ? ? Can we use a more generic config option viz. CONFIG_RESERVE_DSPMEM > ? ? (or something similar) so that code is easily (re)usable. How about CONFIG_OMAP_DSP, and CONFIG_OMAP_DSP_RESERVE_SIZE? -- Felipe Contreras ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] omap: dsp: fix ioremap() usage 2010-10-10 17:40 ` [PATCH 2/3] omap: dsp: fix ioremap() usage Felipe Contreras 2010-10-10 20:17 ` Premi, Sanjeev @ 2010-10-11 15:15 ` Guzman Lugo, Fernando 2010-10-11 19:05 ` Felipe Contreras 1 sibling, 1 reply; 13+ messages in thread From: Guzman Lugo, Fernando @ 2010-10-11 15:15 UTC (permalink / raw) To: linux-arm-kernel > -----Original Message----- > From: linux-omap-owner at vger.kernel.org > [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of Felipe > Contreras > Sent: Sunday, October 10, 2010 12:41 PM > To: linux-arm; linux-omap; Greg KH > Cc: Ramirez Luna, Omar; Russell King; Felipe Contreras > Subject: [PATCH 2/3] omap: dsp: fix ioremap() usage > > On commit 309caa9 doing ioremap() became forbidden due tue > architectural limitations. Only a single mapping is allowed > now, so the mempool must not be part of the memory managed by > the kernel. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > arch/arm/plat-omap/common.c | 43 > +++++++++++++++++++++++++++-- > arch/arm/plat-omap/devices.c | 30 --------------------- > arch/arm/plat-omap/include/plat/common.h | 3 +- > arch/arm/plat-omap/include/plat/dsp.h | 6 ---- > 4 files changed, 42 insertions(+), 40 deletions(-) > > diff --git a/arch/arm/plat-omap/common.c > b/arch/arm/plat-omap/common.c index 57205a4..3fee3ca 100644 > --- a/arch/arm/plat-omap/common.c > +++ b/arch/arm/plat-omap/common.c > @@ -37,7 +37,6 @@ > #include <plat/fpga.h> > #include <plat/serial.h> > #include <plat/vram.h> > -#include <plat/dsp.h> > > #include <plat/clock.h> > > @@ -84,11 +83,49 @@ const void *omap_get_var_config(u16 tag, > size_t *len) } EXPORT_SYMBOL(omap_get_var_config); > > -void __init omap_reserve(void) > +#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) > + > +static phys_addr_t omap_dsp_mempool_base; > + > +static void __init omap_dsp_reserve_mem(struct meminfo *mi) { > + phys_addr_t size = CONFIG_TIDSPBRIDGE_MEMPOOL_SIZE; > + phys_addr_t addr = ~0; > + int i; > + > + if (!size) > + return; > + > + for (i = mi->nr_banks - 1; i >= 0; i--) > + if (mi->bank[i].size >= size) { > + mi->bank[i].size -= size; > + addr = mi->bank[i].start + mi->bank[i].size; > + break; > + } Missing {} in "for" lopp. Even tough you are checking for success inside For loop why check again outside? And also not need to define addr. What do you think about this: static void __init omap_dsp_reserve_mem(struct meminfo *mi) { phys_addr_t size = CONFIG_TIDSPBRIDGE_MEMPOOL_SIZE; int i; if (!size) return; for (i = mi->nr_banks - 1; i >= 0; i--) { if (mi->bank[i].size >= size) { mi->bank[i].size -= size; omap_dsp_mempool_base = mi->bank[i].start + mi->bank[i].size; return; } } pr_err("%s: failed to reserve 0x%x bytes\n", __func__, size); } Regards, Fernando. > + > + if (addr == ~0) { > + pr_err("%s: failed to reserve 0x%x bytes\n", > + __func__, size); > + return; > + } > + > + omap_dsp_mempool_base = addr; > +} > + > +phys_addr_t omap_dsp_get_mempool_base(void) { > + return omap_dsp_mempool_base; > +} > +EXPORT_SYMBOL(omap_dsp_get_mempool_base); > +#else > +static inline void omap_dsp_reserve_mem(struct meminfo *mi) > { } #endif > + > +void __init omap_reserve(struct meminfo *mi) > { > omapfb_reserve_sdram_memblock(); > omap_vram_reserve_sdram_memblock(); > - omap_dsp_reserve_sdram_memblock(); > + omap_dsp_reserve_mem(mi); > } > > /* > diff --git a/arch/arm/plat-omap/devices.c > b/arch/arm/plat-omap/devices.c index 4c8f9b9..d1920be 100644 > --- a/arch/arm/plat-omap/devices.c > +++ b/arch/arm/plat-omap/devices.c > @@ -15,7 +15,6 @@ > #include <linux/platform_device.h> > #include <linux/io.h> > #include <linux/slab.h> > -#include <linux/memblock.h> > > #include <mach/hardware.h> > #include <asm/mach-types.h> > @@ -273,35 +272,6 @@ static void omap_init_wdt(void) static > inline void omap_init_wdt(void) {} #endif > > -#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) > - > -static phys_addr_t omap_dsp_phys_mempool_base; > - > -void __init omap_dsp_reserve_sdram_memblock(void) > -{ > - phys_addr_t size = CONFIG_TIDSPBRIDGE_MEMPOOL_SIZE; > - phys_addr_t paddr; > - > - if (!size) > - return; > - > - paddr = __memblock_alloc_base(size, SZ_1M, MEMBLOCK_REAL_LIMIT); > - if (!paddr) { > - pr_err("%s: failed to reserve %x bytes\n", > - __func__, size); > - return; > - } > - > - omap_dsp_phys_mempool_base = paddr; > -} > - > -phys_addr_t omap_dsp_get_mempool_base(void) -{ > - return omap_dsp_phys_mempool_base; > -} > -EXPORT_SYMBOL(omap_dsp_get_mempool_base); > -#endif > - > /* > * This gets called after board-specific INIT_MACHINE, and > initializes most > * on-chip peripherals accessible on this board (except for > few like USB): > diff --git a/arch/arm/plat-omap/include/plat/common.h > b/arch/arm/plat-omap/include/plat/common.h > index 9776b41..3675492 100644 > --- a/arch/arm/plat-omap/include/plat/common.h > +++ b/arch/arm/plat-omap/include/plat/common.h > @@ -30,11 +30,12 @@ > #include <plat/i2c.h> > > struct sys_timer; > +struct meminfo; > > extern void omap_map_common_io(void); > extern struct sys_timer omap_timer; > > -extern void omap_reserve(void); > +extern void omap_reserve(struct meminfo *mi); > > /* > * IO bases for various OMAP processors diff --git > a/arch/arm/plat-omap/include/plat/dsp.h > b/arch/arm/plat-omap/include/plat/dsp.h > index 9c604b3..4e53687 100644 > --- a/arch/arm/plat-omap/include/plat/dsp.h > +++ b/arch/arm/plat-omap/include/plat/dsp.h > @@ -22,10 +22,4 @@ struct omap_dsp_platform_data { > phys_addr_t phys_mempool_size; > }; > > -#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) > -extern void omap_dsp_reserve_sdram_memblock(void); > -#else > -static inline void omap_dsp_reserve_sdram_memblock(void) { } -#endif > - > #endif > -- > 1.7.3.1.2.g7fe2b > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-omap" in the body of a message to > majordomo at vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] omap: dsp: fix ioremap() usage 2010-10-11 15:15 ` Guzman Lugo, Fernando @ 2010-10-11 19:05 ` Felipe Contreras 0 siblings, 0 replies; 13+ messages in thread From: Felipe Contreras @ 2010-10-11 19:05 UTC (permalink / raw) To: linux-arm-kernel On Mon, Oct 11, 2010 at 6:15 PM, Guzman Lugo, Fernando <fernando.lugo@ti.com> wrote: >> +#if defined(CONFIG_TIDSPBRIDGE) || defined(CONFIG_TIDSPBRIDGE_MODULE) >> + >> +static phys_addr_t omap_dsp_mempool_base; >> + >> +static void __init omap_dsp_reserve_mem(struct meminfo *mi) { >> + ? ? phys_addr_t size = CONFIG_TIDSPBRIDGE_MEMPOOL_SIZE; >> + ? ? phys_addr_t addr = ~0; >> + ? ? int i; >> + >> + ? ? if (!size) >> + ? ? ? ? ? ? return; >> + >> + ? ? for (i = mi->nr_banks - 1; i >= 0; i--) >> + ? ? ? ? ? ? if (mi->bank[i].size >= size) { >> + ? ? ? ? ? ? ? ? ? ? mi->bank[i].size -= size; >> + ? ? ? ? ? ? ? ? ? ? addr = mi->bank[i].start + mi->bank[i].size; >> + ? ? ? ? ? ? ? ? ? ? break; >> + ? ? ? ? ? ? } > > Missing {} in "for" lopp. Even tough you are checking for success inside > For loop why check again outside? And also not need to define addr. > What do you think about this: It comes directly from Russell's code: http://article.gmane.org/gmane.linux.kernel/1046606 But I do prefer to add the braces. Your proposed patch is ok, although I prefer to have the important code at the end of the function, and the error check before that. Anyway, Russell has come with a different approach, so I have to try that instead. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list 2010-10-10 17:40 [PATCH 0/3] staging: tidspbridge: fix ioremap() usage Felipe Contreras 2010-10-10 17:40 ` [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo Felipe Contreras 2010-10-10 17:40 ` [PATCH 2/3] omap: dsp: fix ioremap() usage Felipe Contreras @ 2010-10-10 17:40 ` Felipe Contreras 2010-10-11 10:40 ` Arnd Bergmann 2 siblings, 1 reply; 13+ messages in thread From: Felipe Contreras @ 2010-10-10 17:40 UTC (permalink / raw) To: linux-arm-kernel The mempool area is not handled by the kernel any more. Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- drivers/staging/tidspbridge/TODO | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/staging/tidspbridge/TODO b/drivers/staging/tidspbridge/TODO index 187363f..54f4a29 100644 --- a/drivers/staging/tidspbridge/TODO +++ b/drivers/staging/tidspbridge/TODO @@ -13,7 +13,6 @@ * Audit and clean up header files folder * Use kernel coding style * checkpatch.pl fixes -* allocate ext_mem_pool from consistent memory instead of using ioremap Please send any patches to Greg Kroah-Hartman <greg@kroah.com> and Omar Ramirez Luna <omar.ramirez@ti.com>. -- 1.7.3.1.2.g7fe2b ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list 2010-10-10 17:40 ` [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list Felipe Contreras @ 2010-10-11 10:40 ` Arnd Bergmann 2010-10-11 11:16 ` Felipe Contreras 0 siblings, 1 reply; 13+ messages in thread From: Arnd Bergmann @ 2010-10-11 10:40 UTC (permalink / raw) To: linux-arm-kernel On Sunday 10 October 2010, Felipe Contreras wrote: > The mempool area is not handled by the kernel any more. But tidspbridge still uses ioremap to set up the mapping for RAM, even though it now is outside of the kernel linar mapping. You should really only use ioremap on MMIO registers, nothing else. These registers are marked as __iomem pointers and can only be passed into functions that talk to the hardware like iowrite32 or writel, but not used like memory. Please have a look at "sparse", which will warn about address space violations among other things. The tidspbridge driver is full of them, and you should fix the code that sparse warns about, which will also show you all the places where ioremap is used incorrectly. Arnd ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list 2010-10-11 10:40 ` Arnd Bergmann @ 2010-10-11 11:16 ` Felipe Contreras 2010-10-11 14:02 ` Arnd Bergmann 0 siblings, 1 reply; 13+ messages in thread From: Felipe Contreras @ 2010-10-11 11:16 UTC (permalink / raw) To: linux-arm-kernel On Mon, Oct 11, 2010 at 1:40 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Sunday 10 October 2010, Felipe Contreras wrote: >> The mempool area is not handled by the kernel any more. > > But tidspbridge still uses ioremap to set up the mapping for RAM, > even though it now is outside of the kernel linar mapping. Which is what ioremap() complained about, and how Russell King suggested to solve the issue. > You should really only use ioremap on MMIO registers, nothing > else. These registers are marked as __iomem pointers and can only > be passed into functions that talk to the hardware like iowrite32 > or writel, but not used like memory. >From bogus@does.not.exist.com Sat Oct 9 07:11:57 2010 From: bogus@does.not.exist.com () Date: Sat, 09 Oct 2010 11:11:57 -0000 Subject: No subject Message-ID: <mailman.1.1286795819.18522.linux-arm-kernel@lists.infradead.org> > Please have a look at "sparse", which will warn about address space > violations among other things. The tidspbridge driver is full of them, > and you should fix the code that sparse warns about, which will > also show you all the places where ioremap is used incorrectly. In one of my branches I moved ioremap() to arch/arm/mach-omap2/dsp.c and if I use sparse there, it gives no warning. I would prefer to map the memory some other way and make it non-cacheable, but I don't know any other way. Then, if readl/writel are still needed, only ioremap() that area. And finally, and hopefully, do cache flushes instead of requiring consistent memory. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list 2010-10-11 11:16 ` Felipe Contreras @ 2010-10-11 14:02 ` Arnd Bergmann 0 siblings, 0 replies; 13+ messages in thread From: Arnd Bergmann @ 2010-10-11 14:02 UTC (permalink / raw) To: linux-arm-kernel On Monday 11 October 2010, Felipe Contreras wrote: > On Mon, Oct 11, 2010 at 1:40 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Sunday 10 October 2010, Felipe Contreras wrote: > >> The mempool area is not handled by the kernel any more. > > > > But tidspbridge still uses ioremap to set up the mapping for RAM, > > even though it now is outside of the kernel linar mapping. > > Which is what ioremap() complained about, and how Russell King > suggested to solve the issue. You are right that this is what Russell asked about, having a single mapping for memory means that you avoid the problems that the warning was put there for and you no longer risk memory corruption. That is good and the changes you did are the important ones. What I'm arguing is that you still don't use the interface in the way it's designed and things might break again in the future. For instance, I've seen platforms where readl/writel is not a pointer dereference but a hypercall or goes through an indirect index/data register pair. I hope we don't ever get something like this on ARM, but it would still be good to write the code in a more robust way that doesn't mix __iomem tokens with kernel pointers. > > You should really only use ioremap on MMIO registers, nothing > > else. These registers are marked as __iomem pointers and can only > > be passed into functions that talk to the hardware like iowrite32 > > or writel, but not used like memory. > > From what I can see parts of this memory are also used for readl/writel. In that case, it's worse than I thought ;-) If you use readl(), it needs to be an __iomem pointer, if you use it by direct dereferences, it must not be __iomem. Obviously, you need to use ioremap to target the device registers, but I don't see how it could make sense for a communication area in memory. > > Please have a look at "sparse", which will warn about address space > > violations among other things. The tidspbridge driver is full of them, > > and you should fix the code that sparse warns about, which will > > also show you all the places where ioremap is used incorrectly. > > In one of my branches I moved ioremap() to arch/arm/mach-omap2/dsp.c > and if I use sparse there, it gives no warning. I don't see how moving the code around would get rid of an address space warning, unless you play dirty tricks like using __force casts or passing pointers around as integers. > I would prefer to map the memory some other way and make it > non-cacheable, but I don't know any other way. Then, if readl/writel > are still needed, only ioremap() that area. And finally, and > hopefully, do cache flushes instead of requiring consistent memory. Yes, that sounds reasonable. Can you use a chunk of regular kernel memory with dma_map_single/dma_sync_single_for_{cpu,device} for this, like normal drivers do? Arnd ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-10-11 19:05 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-10-10 17:40 [PATCH 0/3] staging: tidspbridge: fix ioremap() usage Felipe Contreras 2010-10-10 17:40 ` [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo Felipe Contreras 2010-10-10 19:03 ` Russell King - ARM Linux 2010-10-10 19:37 ` Felipe Contreras 2010-10-10 17:40 ` [PATCH 2/3] omap: dsp: fix ioremap() usage Felipe Contreras 2010-10-10 20:17 ` Premi, Sanjeev 2010-10-10 20:39 ` Felipe Contreras 2010-10-11 15:15 ` Guzman Lugo, Fernando 2010-10-11 19:05 ` Felipe Contreras 2010-10-10 17:40 ` [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list Felipe Contreras 2010-10-11 10:40 ` Arnd Bergmann 2010-10-11 11:16 ` Felipe Contreras 2010-10-11 14:02 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).