[V2,3/3] ARM: tegra: move debug-macro.S to include/debug

Commit Message

Rob Herring Oct. 18, 2012, 1:47 p.m. UTC

On 10/17/2012 04:12 PM, Stephen Warren wrote:
> On 10/17/2012 10:22 AM, Stephen Warren wrote:

snip

> That implies we really do need to keep the two pieces of code completely
> in sync, so a shared header is the right way to go. It also implies that
> having duplicate mappings of the same physical address doesn't cause any
> immediate obvious catastrophic problems.
> 
> Ways we might avoid files in arch/arm/include/debug having to use
> relative include paths to pick up that header are:
> 
> a) Move mach-${mach}/include/mach/iomap.h to iomap-${mach}.h in some
> standard include path.
> 

Your goal should be to get rid of iomap.h though...

> b) Rework debug-macro.S so that it isn't an include file, but rather a
> regular top-level file. In other words, rather than compiling
> arch/arm/kernel/debug.S, and having that #include DEBUG_LL_INCLUDE,
> instead compile DEBUG_LL_SOURCE (i.e. arch/arm/mach-${mach}/debug.S by
> default), and have that #include any common parts (e.g. implementation
> of printhex8). This has benefits of:
> 
> b1) arch/arm/mach-${mach}/debug.S is in the mach directory that owns it,
> rather than having them all dumped into a common location.
> 
> b2) Can use #include "iomap.h", a non-relative include, to pick up the
> shared header
> 
> b3) Perhaps we can simplify the current debug.S e.g. have a common
> debug-semihosting.S that contains the semihosting stuff, and only
> include that from mach-*/debug.S if that machine uses semihosting, or
> similar?
> 
> (b) seems like a lot of work. I don't see any advantage of (a) over just
> using the relative include.

Agreed.

Here is what I mentioned previously. This removes the static mapping from 
the platforms. This is untested and probably breaks on different DEBUG_LL 
options. For now, platforms call debug_ll_io_init, but once all platforms 
are converted, this can be called from devicemaps_init.

Comments

Russell King - ARM Linux Oct. 18, 2012, 1:54 p.m. UTC | #1

On Thu, Oct 18, 2012 at 08:47:56AM -0500, Rob Herring wrote:
> Here is what I mentioned previously. This removes the static mapping from 
> the platforms. This is untested and probably breaks on different DEBUG_LL 
> options. For now, platforms call debug_ll_io_init, but once all platforms 
> are converted, this can be called from devicemaps_init.

There's a problem with this approach.  What if the platform specifically
sets the debug addresses to be within one of it's existing mappings (which
is definitely what you'd want to do with 8250-based UARTs attached to a
PCI bus.)

This isn't going to work in that case unless we split the debug mapping
from the PCI IO space mapping.

Rob Herring Oct. 18, 2012, 2:15 p.m. UTC | #2

On 10/18/2012 08:54 AM, Russell King - ARM Linux wrote:
> On Thu, Oct 18, 2012 at 08:47:56AM -0500, Rob Herring wrote:
>> Here is what I mentioned previously. This removes the static mapping from 
>> the platforms. This is untested and probably breaks on different DEBUG_LL 
>> options. For now, platforms call debug_ll_io_init, but once all platforms 
>> are converted, this can be called from devicemaps_init.
> 
> There's a problem with this approach.  What if the platform specifically
> sets the debug addresses to be within one of it's existing mappings (which
> is definitely what you'd want to do with 8250-based UARTs attached to a
> PCI bus.)
> 
> This isn't going to work in that case unless we split the debug mapping
> from the PCI IO space mapping.

It is opt-in, so we may never get to the last step. We're still better
off than now. Is there a platform that is doing this? If so, I didn't
move any DEBUG_LL users to the fixed i/o space so they would still be
using their old i/o space address. Having 2 mappings would work, right?
You use the debug mapping initially and when PCI bus and the real
console driver are up, you switch to the fixed i/o space.

This isn't just a PCI issue. You could also have a mapping that covers a
block of SOC peripherals including the uart and you don't want a
separate mapping.

Rob

Stephen Warren Oct. 19, 2012, 4:40 p.m. UTC | #3

On 10/18/2012 07:47 AM, Rob Herring wrote:
...
> Here is what I mentioned previously. This removes the static mapping from 
> the platforms. This is untested and probably breaks on different DEBUG_LL 
> options. For now, platforms call debug_ll_io_init, but once all platforms 
> are converted, this can be called from devicemaps_init.

> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c

> +void __init debug_ll_io_init(void)
> +{
> +	struct map_desc map;
> +
> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
> +		return;
> +
> +	debug_ll_addr(&map.pfn, &map.virtual);
> +	map.pfn = __phys_to_pfn(map.pfn);
> +	map.length = PAGE_SIZE;
> +	map.type = MT_DEVICE;
> +	create_mapping(&map);
> +}

OK, so I just call this new function from Tegra's tegra_map_common_io().
That looks pretty neat. I'll give it a try.

Patch

diff --git a/arch/arm/include/asm/mach/map.h b/arch/arm/include/asm/mach/map.h
index 195ac2f..2ece92c 100644
--- a/arch/arm/include/asm/mach/map.h
+++ b/arch/arm/include/asm/mach/map.h
@@ -40,6 +40,9 @@  extern void iotable_init(struct map_desc *, int);
 extern void vm_reserve_area_early(unsigned long addr, unsigned long size,
 				  void *caller);
 
+extern void debug_ll_addr(unsigned long *paddr, unsigned long *vaddr);
+extern void debug_ll_io_init(void);
+
 struct mem_type;
 extern const struct mem_type *get_mem_type(unsigned int type);
 /*
diff --git a/arch/arm/kernel/debug.S b/arch/arm/kernel/debug.S
index 66f711b..93883ed 100644
--- a/arch/arm/kernel/debug.S
+++ b/arch/arm/kernel/debug.S
@@ -100,6 +100,13 @@  ENTRY(printch)
 		b	1b
 ENDPROC(printch)
 
+ENTRY(debug_ll_addr)
+		addruart r2, r3, ip
+		str	r2, [r0]
+		str	r3, [r1]
+		mov	pc, lr
+ENDPROC(debug_ll_addr)
+
 #else
 
 ENTRY(printascii)
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 941dfb9..1c8c7be 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -876,6 +876,20 @@  static void __init pci_reserve_io(void)
 #define pci_reserve_io() do { } while (0)
 #endif
 
+void __init debug_ll_io_init(void)
+{
+	struct map_desc map;
+
+	if (!IS_ENABLED(CONFIG_DEBUG_LL))
+		return;
+
+	debug_ll_addr(&map.pfn, &map.virtual);
+	map.pfn = __phys_to_pfn(map.pfn);
+	map.length = PAGE_SIZE;
+	map.type = MT_DEVICE;
+	create_mapping(&map);
+}
+
 static void * __initdata vmalloc_min =
 	(void *)(VMALLOC_END - (240 << 20) - VMALLOC_OFFSET);