| Message ID | 1510164346-21134-1-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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, > };
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, };
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html