| Message ID | 20171110032610.GJ28152@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 09, 2017 at 07:26:10PM -0800, Tony Lindgren wrote: > * Joonsoo Kim <iamjoonsoo.kim@lge.com> [171110 00:10]: > > On Thu, Nov 09, 2017 at 07:08:54AM -0800, Tony Lindgren wrote: > > > Hmm OK. Does your first patch above now have the initcall issue too? > > > It boots if I make that also subsys_initcall and then I get: > > > > > [ 2.078094] vmalloc_pool_init: DMA: get vmalloc area: d0010000 > > > > Yes, first patch has the initcall issue and it's intentional in order > > to check the theory. I checked following log for this. > > > > - Boot failure > > SRAM_ADDR: omap_map_sram: P: 0x40208000 - 0x4020f000 > > SRAM_ADDR: omap_map_sram: V: 0xd0050000 - 0xd0057000 > > > > - Boot success > > SRAM_ADDR: omap_map_sram: P: 0x40208000 - 0x4020f000 > > SRAM_ADDR: omap_map_sram: V: 0xd0008000 - 0xd000f000 > > > > When failure, virtual address for sram is higher than normal one due > > to vmalloc area allocation in __dma_alloc_remap(). If it is deferred, > > virtual address is the same with success case and then the system work. > > > > So, my next theory is that there is n900 specific assumption that sram > > should have that address. Could you check if any working tree for n900 > > which doesn't have my CMA series work or not with adding > > "arm/dma: vmalloc area allocation"? > > Oh I see, sorry I was not following you earlier. So you mean that > by adding the vmalloc_pool_init() initcall the va mapping for SRAM > changes. Exactly. > > And yes, save_secure_ram_context seems to be doing some sketchy > virt to phys calculation with sram_phy_addr_mask. Here's a small > patch to fix that for your CMA series, maybe you can merge it > with your series to avoid breaking booting for git bisect. > > Then I'll follow up on cleaning up save_secure_ram_context later. Thanks for the patch. However, the patch should be modified. See below. > Regards, > > Tony > > 8< ------------------------- > >From tony Mon Sep 17 00:00:00 2001 > From: Tony Lindgren <tony@atomide.com> > Date: Thu, 9 Nov 2017 17:05:34 -0800 > Subject: [PATCH] ARM: OMAP2+: Add static SRAM mapping for > save_secure_ram_context > > With the CMA changes from Joonsoo Kim <iamjoonsoo.kim@lge.com>, it > was noticed that n900 stopped booting. After investigating it turned > out that n900 save_secure_ram_context does some whacky virtual to > physical address translation for the SRAM data address. > > Let's fix this for CMA changes by adding a static mapping for SRAM > on omap3. Then we can follow up with a patch to clean up the address > translation in save_secure_ram_context later on. > > Debugged-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > arch/arm/mach-omap2/io.c | 6 ++++++ > arch/arm/mach-omap2/iomap.h | 4 ++++ > 2 files changed, 10 insertions(+) > > diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c > --- a/arch/arm/mach-omap2/io.c > +++ b/arch/arm/mach-omap2/io.c > @@ -139,6 +139,12 @@ static struct map_desc omap243x_io_desc[] __initdata = { > > #ifdef CONFIG_ARCH_OMAP3 > static struct map_desc omap34xx_io_desc[] __initdata = { > + { > + .virtual = OMAP34XX_SRAM_VIRT, > + .pfn = __phys_to_pfn(OMAP34XX_SRAM_PHYS), > + .length = OMAP34XX_SRAM_SIZE, > + .type = MT_DEVICE > + }, > { > .virtual = L3_34XX_VIRT, > .pfn = __phys_to_pfn(L3_34XX_PHYS), > diff --git a/arch/arm/mach-omap2/iomap.h b/arch/arm/mach-omap2/iomap.h > --- a/arch/arm/mach-omap2/iomap.h > +++ b/arch/arm/mach-omap2/iomap.h > @@ -123,6 +123,10 @@ > * VPOM3430 was not working for Int controller > */ > > +#define OMAP34XX_SRAM_PHYS 0x40200000 > +#define OMAP34XX_SRAM_VIRT 0xd0010000 > +#define OMAP34XX_SRAM_SIZE 0x10000 For my testing environment, vmalloc address space is started at roughly 0xe0000000 so 0xd0010000 would not be valid. And, PHYS can be different according to the system type. Maybe either OMAP3_SRAM_PUB_PA or OMAP3_SRAM_PA. It seems that SIZE and TYPE should be considered, too. My understanding is correct? Thanks. -- 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
* Joonsoo Kim <iamjoonsoo.kim@lge.com> [171110 06:34]: > On Thu, Nov 09, 2017 at 07:26:10PM -0800, Tony Lindgren wrote: > > +#define OMAP34XX_SRAM_PHYS 0x40200000 > > +#define OMAP34XX_SRAM_VIRT 0xd0010000 > > +#define OMAP34XX_SRAM_SIZE 0x10000 > > For my testing environment, vmalloc address space is started at > roughly 0xe0000000 so 0xd0010000 would not be valid. Well we can map it anywhere we want, got any preferences? Just that the current save_secure_ram_context uses "high_mask" of 0xffff to translate the address. To make this more flexible, we need the save_secure_ram_context changes too. So we might want to do the static mapping and save_secure_ram_context changes as a single patch. > And, PHYS can be different according to the system type. Maybe either > OMAP3_SRAM_PUB_PA or OMAP3_SRAM_PA. It seems that SIZE and TYPE should > be considered, too. My understanding is correct? We can have a static map for the whole SRAM area, see function __arm_ioremap_pfn_caller() for the comment "Try to reuse one of the static mapping whenever possible". So the different public SRAM start addresses and sizes don't matter there. Regards, Tony -- 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
* Tony Lindgren <tony@atomide.com> [171110 07:36]: > * Joonsoo Kim <iamjoonsoo.kim@lge.com> [171110 06:34]: > > On Thu, Nov 09, 2017 at 07:26:10PM -0800, Tony Lindgren wrote: > > > +#define OMAP34XX_SRAM_PHYS 0x40200000 > > > +#define OMAP34XX_SRAM_VIRT 0xd0010000 > > > +#define OMAP34XX_SRAM_SIZE 0x10000 > > > > For my testing environment, vmalloc address space is started at > > roughly 0xe0000000 so 0xd0010000 would not be valid. > > Well we can map it anywhere we want, got any preferences? Hmm and I'm also wondering what you do to make vmalloc space to start at 0xe0000000 instead of 0xd0000000? The reason I'm asking is because I think we can just move all of save_secure_ram_context to run from DDR instead of SRAM. But I'd rather do a minimal patch first that fixes your series and then we can test the further changes with more time. After moving save_secure_ram_context to DDR, we can call save_secure_ram_context directly with something like: args_pa = __pa(omap3_secure_ram_storage); offset = (unsigned long)omap3_secure_ram_storage - args_pa; ret = save_secure_ram_context(args_pa, offset); > Just that the current save_secure_ram_context uses "high_mask" > of 0xffff to translate the address. To make this more flexible, > we need the save_secure_ram_context changes too. So we might > want to do the static mapping and save_secure_ram_context changes > as a single patch. > > > And, PHYS can be different according to the system type. Maybe either > > OMAP3_SRAM_PUB_PA or OMAP3_SRAM_PA. It seems that SIZE and TYPE should > > be considered, too. My understanding is correct? > > We can have a static map for the whole SRAM area, see function > __arm_ioremap_pfn_caller() for the comment "Try to reuse one of the > static mapping whenever possible". So the different public SRAM start > addresses and sizes don't matter there. And then if save_secure_ram_contet runs in DDR, no static map is needed. Regards, Tony -- 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
On Fri, Nov 10, 2017 at 07:36:20AM -0800, Tony Lindgren wrote: > * Joonsoo Kim <iamjoonsoo.kim@lge.com> [171110 06:34]: > > On Thu, Nov 09, 2017 at 07:26:10PM -0800, Tony Lindgren wrote: > > > +#define OMAP34XX_SRAM_PHYS 0x40200000 > > > +#define OMAP34XX_SRAM_VIRT 0xd0010000 > > > +#define OMAP34XX_SRAM_SIZE 0x10000 > > > > For my testing environment, vmalloc address space is started at > > roughly 0xe0000000 so 0xd0010000 would not be valid. > > Well we can map it anywhere we want, got any preferences? My testing environment is a beagle-(xm?) for QEMU. It is configured by CONFIG_VMSPLIT_3G=y so kernel address space is started at 0xc0000000. And, it has 512 MB memory so 0xc0000000 ~ 0xdff00000 is used for direct mapping. See below. [ 0.000000] Memory: 429504K/522240K available (11264K kernel code, 1562K rwdata, 4288K rodata, 2048K init, 405K bss, 27200K reserved, 65536K cma-reserved, 0K highmem) [ 0.000000] Virtual kernel memory layout: [ 0.000000] vector : 0xffff0000 - 0xffff1000 ( 4 kB) [ 0.000000] fixmap : 0xffc00000 - 0xfff00000 (3072 kB) [ 0.000000] vmalloc : 0xe0000000 - 0xff800000 ( 504 MB) [ 0.000000] lowmem : 0xc0000000 - 0xdff00000 ( 511 MB) [ 0.000000] pkmap : 0xbfe00000 - 0xc0000000 ( 2 MB) [ 0.000000] modules : 0xbf000000 - 0xbfe00000 ( 14 MB) [ 0.000000] .text : 0xc0208000 - 0xc0e00000 (12256 kB) [ 0.000000] .init : 0xc1300000 - 0xc1500000 (2048 kB) [ 0.000000] .data : 0xc1500000 - 0xc1686810 (1563 kB) [ 0.000000] .bss : 0xc168fc68 - 0xc16f512c ( 406 kB) Therefore, if OMAP34XX_SRAM_VIRT is 0xd0010000, direct mapping is broken and the system doesn't work. I guess that we should use more stable address like as 0xf0000000. > > Just that the current save_secure_ram_context uses "high_mask" > of 0xffff to translate the address. To make this more flexible, > we need the save_secure_ram_context changes too. So we might > want to do the static mapping and save_secure_ram_context changes > as a single patch. > > > And, PHYS can be different according to the system type. Maybe either > > OMAP3_SRAM_PUB_PA or OMAP3_SRAM_PA. It seems that SIZE and TYPE should > > be considered, too. My understanding is correct? > > We can have a static map for the whole SRAM area, see function > __arm_ioremap_pfn_caller() for the comment "Try to reuse one of the > static mapping whenever possible". So the different public SRAM start > addresses and sizes don't matter there. Okay. Look fine with SRAM start addresses and sizes. However, we need to consider mtype since __arm_ioremap_pfn_caller() doesn't reuse the mapping if mtype is different. mtype can be either MT_MEMORY_RWX or MT_MEMORY_RWX_NONCACHED. Thanks. -- 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
On Mon, Nov 13, 2017 at 01:15:30PM -0800, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [171110 07:36]: > > * Joonsoo Kim <iamjoonsoo.kim@lge.com> [171110 06:34]: > > > On Thu, Nov 09, 2017 at 07:26:10PM -0800, Tony Lindgren wrote: > > > > +#define OMAP34XX_SRAM_PHYS 0x40200000 > > > > +#define OMAP34XX_SRAM_VIRT 0xd0010000 > > > > +#define OMAP34XX_SRAM_SIZE 0x10000 > > > > > > For my testing environment, vmalloc address space is started at > > > roughly 0xe0000000 so 0xd0010000 would not be valid. > > > > Well we can map it anywhere we want, got any preferences? > > Hmm and I'm also wondering what you do to make vmalloc space to > start at 0xe0000000 instead of 0xd0000000? Please see the another reply. > > The reason I'm asking is because I think we can just move all of > save_secure_ram_context to run from DDR instead of SRAM. But I'd > rather do a minimal patch first that fixes your series and then we > can test the further changes with more time. Okay. I agree to make a minimal patch first and then go next step. > After moving save_secure_ram_context to DDR, we can call > save_secure_ram_context directly with something like: > > args_pa = __pa(omap3_secure_ram_storage); > offset = (unsigned long)omap3_secure_ram_storage - args_pa; > ret = save_secure_ram_context(args_pa, offset); > > > Just that the current save_secure_ram_context uses "high_mask" > > of 0xffff to translate the address. To make this more flexible, > > we need the save_secure_ram_context changes too. So we might > > want to do the static mapping and save_secure_ram_context changes > > as a single patch. > > > > > And, PHYS can be different according to the system type. Maybe either > > > OMAP3_SRAM_PUB_PA or OMAP3_SRAM_PA. It seems that SIZE and TYPE should > > > be considered, too. My understanding is correct? > > > > We can have a static map for the whole SRAM area, see function > > __arm_ioremap_pfn_caller() for the comment "Try to reuse one of the > > static mapping whenever possible". So the different public SRAM start > > addresses and sizes don't matter there. > > And then if save_secure_ram_contet runs in DDR, no static map is > needed. Okay. Thanks. -- 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
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c --- a/arch/arm/mach-omap2/io.c +++ b/arch/arm/mach-omap2/io.c @@ -139,6 +139,12 @@ static struct map_desc omap243x_io_desc[] __initdata = { #ifdef CONFIG_ARCH_OMAP3 static struct map_desc omap34xx_io_desc[] __initdata = { + { + .virtual = OMAP34XX_SRAM_VIRT, + .pfn = __phys_to_pfn(OMAP34XX_SRAM_PHYS), + .length = OMAP34XX_SRAM_SIZE, + .type = MT_DEVICE + }, { .virtual = L3_34XX_VIRT, .pfn = __phys_to_pfn(L3_34XX_PHYS), diff --git a/arch/arm/mach-omap2/iomap.h b/arch/arm/mach-omap2/iomap.h --- a/arch/arm/mach-omap2/iomap.h +++ b/arch/arm/mach-omap2/iomap.h @@ -123,6 +123,10 @@ * VPOM3430 was not working for Int controller */ +#define OMAP34XX_SRAM_PHYS 0x40200000 +#define OMAP34XX_SRAM_VIRT 0xd0010000 +#define OMAP34XX_SRAM_SIZE 0x10000 + #define L4_PER_34XX_PHYS L4_PER_34XX_BASE /* 0x49000000 --> 0xfb000000 */ #define L4_PER_34XX_VIRT (L4_PER_34XX_PHYS + OMAP2_L4_IO_OFFSET)