diff mbox

sh: migor: Reserve memory block for CEU

Message ID 1510164346-21134-1-git-send-email-jacopo+renesas@jmondi.org (mailing list archive)
State Not Applicable
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Jacopo Mondi Nov. 8, 2017, 6:05 p.m. UTC 
A memory region for CEU video buffer has to be reserved during machine
initialization.

Originally, it was allocated through DMA API helpers and stored in the
second IORESOURCE_MEM entry, to be later remapped by the CEU driver with
a call to 'dma_declare_coherent_memory()'

As Linux does not allow anymore to remap system RAM regions with
'memremap' function, sh_mobile_ceu driver fails when trying to remap the
memory area:

WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n",
	  &offset, (unsigned long) size)

from 'memremap()' function in kernel/memremap.c

To avoid hitting that WARN_ONCE() and have memory successfully remapped,
reserve a region using '.mv_mem_reserve' member of SH's 'struct
sh_machine_vector' to make sure memory is reserved early enough, and
removed from the available system RAM.

This is similar to what happens on ARM architecture with
'arm_memblock_steal()' function.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/sh/boards/mach-migor/setup.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

--
2.7.4

Comments

Geert Uytterhoeven Nov. 8, 2017, 6:31 p.m. UTC | #1 
Hi Jacopo,

On Wed, Nov 8, 2017 at 7:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> A memory region for CEU video buffer has to be reserved during machine
> initialization.
>
> Originally, it was allocated through DMA API helpers and stored in the
> second IORESOURCE_MEM entry, to be later remapped by the CEU driver with
> a call to 'dma_declare_coherent_memory()'
>
> As Linux does not allow anymore to remap system RAM regions with
> 'memremap' function, sh_mobile_ceu driver fails when trying to remap the
> memory area:
>
> WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n",
>           &offset, (unsigned long) size)
>
> from 'memremap()' function in kernel/memremap.c
>
> To avoid hitting that WARN_ONCE() and have memory successfully remapped,
> reserve a region using '.mv_mem_reserve' member of SH's 'struct
> sh_machine_vector' to make sure memory is reserved early enough, and
> removed from the available system RAM.
>
> This is similar to what happens on ARM architecture with
> 'arm_memblock_steal()' function.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I assume this failure isnot limited to CEU on Migo-R, but applies to
the other 24
callers of platform_resource_setup_memory(), too?

Can platform_resource_setup_memory() be fixed instead, or is that difficult
due to the need to reserve the memory very early in the boot process?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Laurent Pinchart Nov. 9, 2017, 1:32 a.m. UTC | #2 
Hi Geert,

On Wednesday, 8 November 2017 20:31:22 EET Geert Uytterhoeven wrote:
> On Wed, Nov 8, 2017 at 7:05 PM, Jacopo Mondi wrote:
> > A memory region for CEU video buffer has to be reserved during machine
> > initialization.
> > 
> > Originally, it was allocated through DMA API helpers and stored in the
> > second IORESOURCE_MEM entry, to be later remapped by the CEU driver with
> > a call to 'dma_declare_coherent_memory()'
> > 
> > As Linux does not allow anymore to remap system RAM regions with
> > 'memremap' function, sh_mobile_ceu driver fails when trying to remap the
> > memory area:
> > 
> > WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n",
> > 
> >           &offset, (unsigned long) size)
> > 
> > from 'memremap()' function in kernel/memremap.c
> > 
> > To avoid hitting that WARN_ONCE() and have memory successfully remapped,
> > reserve a region using '.mv_mem_reserve' member of SH's 'struct
> > sh_machine_vector' to make sure memory is reserved early enough, and
> > removed from the available system RAM.
> > 
> > This is similar to what happens on ARM architecture with
> > 'arm_memblock_steal()' function.
> > 
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> I assume this failure isnot limited to CEU on Migo-R, but applies to
> the other 24 callers of platform_resource_setup_memory(), too?
> 
> Can platform_resource_setup_memory() be fixed instead, or is that difficult
> due to the need to reserve the memory very early in the boot process?

That's exactly the problem, memory needs to be reserved early at boot, earlier 
than the current call sites of platform_resource_setup_memory().

The real fix in my opinion would be to use CMA, but implementing CMA for SH is 
out of scope of Jacopo's work on the CEU driver. And that would still require 
patching all the 24 callers of platform_resource_setup_memory() anyway.
Laurent Pinchart Nov. 9, 2017, 1:39 a.m. UTC | #3 
Hi Jacopo,

Thank you for the patch.

On Wednesday, 8 November 2017 20:05:46 EET Jacopo Mondi wrote:
> A memory region for CEU video buffer has to be reserved during machine
> initialization.
> 
> Originally, it was allocated through DMA API helpers and stored in the
> second IORESOURCE_MEM entry, to be later remapped by the CEU driver with
> a call to 'dma_declare_coherent_memory()'
> 
> As Linux does not allow anymore to remap system RAM regions with
> 'memremap' function, sh_mobile_ceu driver fails when trying to remap the
> memory area:
> 
> WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n",
> 	  &offset, (unsigned long) size)
> 
> from 'memremap()' function in kernel/memremap.c
> 
> To avoid hitting that WARN_ONCE() and have memory successfully remapped,
> reserve a region using '.mv_mem_reserve' member of SH's 'struct
> sh_machine_vector' to make sure memory is reserved early enough, and
> removed from the available system RAM.

I'd phrase it the other way around. The important point here is that the 
reserved memory block must be removed from system memory in order to be 
remapable. This requires usage of the memblock API, and as this API is only 
usable very early during the boot process, we have to use the .mv_mem_reserve 
operation.

> This is similar to what happens on ARM architecture with
> 'arm_memblock_steal()' function.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/sh/boards/mach-migor/setup.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/sh/boards/mach-migor/setup.c
> b/arch/sh/boards/mach-migor/setup.c index 0bcbe58..080627c 100644
> --- a/arch/sh/boards/mach-migor/setup.c
> +++ b/arch/sh/boards/mach-migor/setup.c
> @@ -12,6 +12,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/input.h>
>  #include <linux/input/sh_keysc.h>
> +#include <linux/memblock.h>
>  #include <linux/mmc/host.h>
>  #include <linux/mtd/physmap.h>
>  #include <linux/mfd/tmio.h>
> @@ -45,6 +46,9 @@
>   * 0x18000000       8GB    8   NAND Flash (K9K8G08U0A)
>   */
> 
> +#define CEU_BUFFER_MEMORY_SIZE		(4 << 20)
> +static phys_addr_t ceu_dma_membase;
> +
>  static struct smc91x_platdata smc91x_info = {
>  	.flags = SMC91X_USE_16BIT | SMC91X_NOWAIT,
>  };
> @@ -501,6 +505,8 @@ extern char migor_sdram_leave_end;
> 
>  static int __init migor_devices_setup(void)
>  {
> +	struct resource *r = &migor_ceu_device.resource[2];
> +
>  	/* register board specific self-refresh code */
>  	sh_mobile_register_self_refresh(SUSP_SH_STANDBY | SUSP_SH_SF,
>  					&migor_sdram_enter_start,
> @@ -632,8 +638,6 @@ static int __init migor_devices_setup(void)
>  #endif
>  	__raw_writew(__raw_readw(PORT_MSELCRB) | 0x2000, PORT_MSELCRB); /* D15->D8
> */
> 
> -	platform_resource_setup_memory(&migor_ceu_device, "ceu", 4 << 20);
> -
>  	/* SIU: Port B */
>  	gpio_request(GPIO_FN_SIUBOLR, NULL);
>  	gpio_request(GPIO_FN_SIUBOBT, NULL);
> @@ -647,6 +651,12 @@ static int __init migor_devices_setup(void)
>  	 */
>  	__raw_writew(__raw_readw(PORT_MSELCRA) | 1, PORT_MSELCRA);
> 
> +	/* Setup additional memory resources for Migo-R */

I would initialize the r variable here instead of at declaration time to make 
the code more readable. Otherwise the reader will have to scroll back to find 
out what r points to.

Other than that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	r->flags = IORESOURCE_MEM;
> +	r->start = ceu_dma_membase;
> +	r->end = r->start + CEU_BUFFER_MEMORY_SIZE - 1;
> +	r->name = "ceu";
> +
>  	i2c_register_board_info(0, migor_i2c_devices,
>  				ARRAY_SIZE(migor_i2c_devices));
> 
> @@ -665,10 +675,24 @@ static int migor_mode_pins(void)
>  	return MODE_PIN0 | MODE_PIN1 | MODE_PIN5;
>  }
> 
> +/* Reserve a portion of memory for CEU buffers */
> +static void __init migor_mv_mem_reserve(void)
> +{
> +	phys_addr_t phys;
> +	phys_addr_t size = CEU_BUFFER_MEMORY_SIZE;
> +
> +	phys = memblock_alloc_base(size, PAGE_SIZE, MEMBLOCK_ALLOC_ANYWHERE);
> +	memblock_free(phys, size);
> +	memblock_remove(phys, size);
> +
> +	ceu_dma_membase = phys;
> +}
> +
>  /*
>   * The Machine Vector
>   */
>  static struct sh_machine_vector mv_migor __initmv = {
>  	.mv_name		= "Migo-R",
>  	.mv_mode_pins		= migor_mode_pins,
> +	.mv_mem_reserve		= migor_mv_mem_reserve,
>  };
Jacopo Mondi Nov. 14, 2017, 4:59 p.m. UTC | #4 
Hi Laurent, Geert,

On Thu, Nov 09, 2017 at 03:39:42AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.

I'm sending a v2 of this, with an additional patch to postpone page to
pfn mapping, as it hangs the CPU on SH4 with SPARSEMEM enabled.

Laurent: as you have suggested that patch, and I was under the
impression you wanted to submit a proper fix for that, please feel
free to reply to that pointing this out, if that's the case.

I need these two patch sent out because:
1) the sh_mobile_ceu driver on Migo-R mainline is broken without them
2) I'm basing my patches to move Migo-R to use the new CEU driver on
   top of them.

Thanks
  j

>
> On Wednesday, 8 November 2017 20:05:46 EET Jacopo Mondi wrote:
> > A memory region for CEU video buffer has to be reserved during machine
> > initialization.
> >
> > Originally, it was allocated through DMA API helpers and stored in the
> > second IORESOURCE_MEM entry, to be later remapped by the CEU driver with
> > a call to 'dma_declare_coherent_memory()'
> >
> > As Linux does not allow anymore to remap system RAM regions with
> > 'memremap' function, sh_mobile_ceu driver fails when trying to remap the
> > memory area:
> >
> > WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n",
> > 	  &offset, (unsigned long) size)
> >
> > from 'memremap()' function in kernel/memremap.c
> >
> > To avoid hitting that WARN_ONCE() and have memory successfully remapped,
> > reserve a region using '.mv_mem_reserve' member of SH's 'struct
> > sh_machine_vector' to make sure memory is reserved early enough, and
> > removed from the available system RAM.
>
> I'd phrase it the other way around. The important point here is that the
> reserved memory block must be removed from system memory in order to be
> remapable. This requires usage of the memblock API, and as this API is only
> usable very early during the boot process, we have to use the .mv_mem_reserve
> operation.
>
> > This is similar to what happens on ARM architecture with
> > 'arm_memblock_steal()' function.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  arch/sh/boards/mach-migor/setup.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/sh/boards/mach-migor/setup.c
> > b/arch/sh/boards/mach-migor/setup.c index 0bcbe58..080627c 100644
> > --- a/arch/sh/boards/mach-migor/setup.c
> > +++ b/arch/sh/boards/mach-migor/setup.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/input.h>
> >  #include <linux/input/sh_keysc.h>
> > +#include <linux/memblock.h>
> >  #include <linux/mmc/host.h>
> >  #include <linux/mtd/physmap.h>
> >  #include <linux/mfd/tmio.h>
> > @@ -45,6 +46,9 @@
> >   * 0x18000000       8GB    8   NAND Flash (K9K8G08U0A)
> >   */
> >
> > +#define CEU_BUFFER_MEMORY_SIZE		(4 << 20)
> > +static phys_addr_t ceu_dma_membase;
> > +
> >  static struct smc91x_platdata smc91x_info = {
> >  	.flags = SMC91X_USE_16BIT | SMC91X_NOWAIT,
> >  };
> > @@ -501,6 +505,8 @@ extern char migor_sdram_leave_end;
> >
> >  static int __init migor_devices_setup(void)
> >  {
> > +	struct resource *r = &migor_ceu_device.resource[2];
> > +
> >  	/* register board specific self-refresh code */
> >  	sh_mobile_register_self_refresh(SUSP_SH_STANDBY | SUSP_SH_SF,
> >  					&migor_sdram_enter_start,
> > @@ -632,8 +638,6 @@ static int __init migor_devices_setup(void)
> >  #endif
> >  	__raw_writew(__raw_readw(PORT_MSELCRB) | 0x2000, PORT_MSELCRB); /* D15->D8
> > */
> >
> > -	platform_resource_setup_memory(&migor_ceu_device, "ceu", 4 << 20);
> > -
> >  	/* SIU: Port B */
> >  	gpio_request(GPIO_FN_SIUBOLR, NULL);
> >  	gpio_request(GPIO_FN_SIUBOBT, NULL);
> > @@ -647,6 +651,12 @@ static int __init migor_devices_setup(void)
> >  	 */
> >  	__raw_writew(__raw_readw(PORT_MSELCRA) | 1, PORT_MSELCRA);
> >
> > +	/* Setup additional memory resources for Migo-R */
>
> I would initialize the r variable here instead of at declaration time to make
> the code more readable. Otherwise the reader will have to scroll back to find
> out what r points to.
>
> Other than that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +	r->flags = IORESOURCE_MEM;
> > +	r->start = ceu_dma_membase;
> > +	r->end = r->start + CEU_BUFFER_MEMORY_SIZE - 1;
> > +	r->name = "ceu";
> > +
> >  	i2c_register_board_info(0, migor_i2c_devices,
> >  				ARRAY_SIZE(migor_i2c_devices));
> >
> > @@ -665,10 +675,24 @@ static int migor_mode_pins(void)
> >  	return MODE_PIN0 | MODE_PIN1 | MODE_PIN5;
> >  }
> >
> > +/* Reserve a portion of memory for CEU buffers */
> > +static void __init migor_mv_mem_reserve(void)
> > +{
> > +	phys_addr_t phys;
> > +	phys_addr_t size = CEU_BUFFER_MEMORY_SIZE;
> > +
> > +	phys = memblock_alloc_base(size, PAGE_SIZE, MEMBLOCK_ALLOC_ANYWHERE);
> > +	memblock_free(phys, size);
> > +	memblock_remove(phys, size);
> > +
> > +	ceu_dma_membase = phys;
> > +}
> > +
> >  /*
> >   * The Machine Vector
> >   */
> >  static struct sh_machine_vector mv_migor __initmv = {
> >  	.mv_name		= "Migo-R",
> >  	.mv_mode_pins		= migor_mode_pins,
> > +	.mv_mem_reserve		= migor_mv_mem_reserve,
> >  };
>
> --
> Regards,
>
> Laurent Pinchart
>
diff mbox

Patch

diff --git a/arch/sh/boards/mach-migor/setup.c b/arch/sh/boards/mach-migor/setup.c
index 0bcbe58..080627c 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -12,6 +12,7 @@ 
 #include <linux/interrupt.h>
 #include <linux/input.h>
 #include <linux/input/sh_keysc.h>
+#include <linux/memblock.h>
 #include <linux/mmc/host.h>
 #include <linux/mtd/physmap.h>
 #include <linux/mfd/tmio.h>
@@ -45,6 +46,9 @@ 
  * 0x18000000       8GB    8   NAND Flash (K9K8G08U0A)
  */

+#define CEU_BUFFER_MEMORY_SIZE		(4 << 20)
+static phys_addr_t ceu_dma_membase;
+
 static struct smc91x_platdata smc91x_info = {
 	.flags = SMC91X_USE_16BIT | SMC91X_NOWAIT,
 };
@@ -501,6 +505,8 @@  extern char migor_sdram_leave_end;

 static int __init migor_devices_setup(void)
 {
+	struct resource *r = &migor_ceu_device.resource[2];
+
 	/* register board specific self-refresh code */
 	sh_mobile_register_self_refresh(SUSP_SH_STANDBY | SUSP_SH_SF,
 					&migor_sdram_enter_start,
@@ -632,8 +638,6 @@  static int __init migor_devices_setup(void)
 #endif
 	__raw_writew(__raw_readw(PORT_MSELCRB) | 0x2000, PORT_MSELCRB); /* D15->D8 */

-	platform_resource_setup_memory(&migor_ceu_device, "ceu", 4 << 20);
-
 	/* SIU: Port B */
 	gpio_request(GPIO_FN_SIUBOLR, NULL);
 	gpio_request(GPIO_FN_SIUBOBT, NULL);
@@ -647,6 +651,12 @@  static int __init migor_devices_setup(void)
 	 */
 	__raw_writew(__raw_readw(PORT_MSELCRA) | 1, PORT_MSELCRA);

+	/* Setup additional memory resources for Migo-R */
+	r->flags = IORESOURCE_MEM;
+	r->start = ceu_dma_membase;
+	r->end = r->start + CEU_BUFFER_MEMORY_SIZE - 1;
+	r->name = "ceu";
+
 	i2c_register_board_info(0, migor_i2c_devices,
 				ARRAY_SIZE(migor_i2c_devices));

@@ -665,10 +675,24 @@  static int migor_mode_pins(void)
 	return MODE_PIN0 | MODE_PIN1 | MODE_PIN5;
 }

+/* Reserve a portion of memory for CEU buffers */
+static void __init migor_mv_mem_reserve(void)
+{
+	phys_addr_t phys;
+	phys_addr_t size = CEU_BUFFER_MEMORY_SIZE;
+
+	phys = memblock_alloc_base(size, PAGE_SIZE, MEMBLOCK_ALLOC_ANYWHERE);
+	memblock_free(phys, size);
+	memblock_remove(phys, size);
+
+	ceu_dma_membase = phys;
+}
+
 /*
  * The Machine Vector
  */
 static struct sh_machine_vector mv_migor __initmv = {
 	.mv_name		= "Migo-R",
 	.mv_mode_pins		= migor_mode_pins,
+	.mv_mem_reserve		= migor_mv_mem_reserve,
 };