[PATCH 0/3] staging: tidspbridge: fix ioremap() usage

[PATCH 0/3] staging: tidspbridge: fix ioremap() usage

[Felipe Contreras]

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

[PATCH 0/3] staging: tidspbridge: fix ioremap() usage

[Felipe Contreras]

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

[PATCH 1/3] arm: mm: allow boards to fiddle with meminfo

[Felipe Contreras]

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

[PATCH 1/3] arm: mm: allow boards to fiddle with meminfo

[Felipe Contreras]

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

Re: [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo

[Russell King - ARM Linux]

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.

[PATCH 1/3] arm: mm: allow boards to fiddle with meminfo

[Russell King - ARM Linux]

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.

Re: [PATCH 1/3] arm: mm: allow boards to fiddle with meminfo

[Felipe Contreras]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] arm: mm: allow boards to fiddle with meminfo

[Felipe Contreras]

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

[PATCH 2/3] omap: dsp: fix ioremap() usage

[Felipe Contreras]

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

[PATCH 2/3] omap: dsp: fix ioremap() usage

[Felipe Contreras]

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

RE: [PATCH 2/3] omap: dsp: fix ioremap() usage

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@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]
> +
> 

[PATCH 2/3] omap: dsp: fix ioremap() usage

[Premi, Sanjeev]

> -----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]
> +
> 

Re: [PATCH 2/3] omap: dsp: fix ioremap() usage

[Felipe Contreras]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] omap: dsp: fix ioremap() usage

[Felipe Contreras]

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

RE: [PATCH 2/3] omap: dsp: fix ioremap() usage

[Guzman Lugo, Fernando]

 

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@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@vger.kernel.org More majordomo info at  
> http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/3] omap: dsp: fix ioremap() usage

[Guzman Lugo, Fernando]

 

> -----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
> 

Re: [PATCH 2/3] omap: dsp: fix ioremap() usage

[Felipe Contreras]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] omap: dsp: fix ioremap() usage

[Felipe Contreras]

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

[PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list

[Felipe Contreras]

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

[PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list

[Felipe Contreras]

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

Re: [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list

[Arnd Bergmann]

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

[PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list

[Arnd Bergmann]

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

Re: [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list

[Felipe Contreras]

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 what I can see parts of this memory are also used for readl/writel.

> 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

[PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list

[Felipe Contreras]

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

Re: [PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list

[Arnd Bergmann]

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

[PATCH 3/3] staging: tidspbridge: remove memory consistency from TODO list

[Arnd Bergmann]

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