ARM: tegra: simplify DEBUG_LL UART selection options

Rob, Arnd, Olof, I'd particularly like feedback on whether the following:

#include "../../mach-tegra/iomap.h"

in arch/arm/include/debug/tegra.S is acceptable. I'd really like to continue to #include a header to share the defines to Tegra physical memory layout and virtual based addresses with Tegra's io.c's struct map_desc entries, so they can't get out of sync. So, the include can either use the relative path as quoted above (which I don't think will cause any significant maintenance issue), or Tegra's iomap.h would have to be moved somewhere public so e.g. <tegra-iomap.h> could be included.

Second, is the Kconfig.debug change reasonable? I guess I could remove the second choice menu I added, and just put all the options in the main "Kernel low-level debugging port" choice. However, that would make the "if" statements in "config DEBUG_LL_INCLUDE" rather more unwieldy.

From: Stephen Warren <[email protected]>

Move Tegra's debug-macro.S over to the common debug macro directory.

Move Tegra's debug UART selection menu into ARM's Kconfig.debug, so that all related options are selected in the same place.

Tegra's uncompress.h is left in mach-tegra/include/mach; it will be removed whenever Tegra is converted to multi-platform.

Signed-off-by Stephen Warren <[email protected]>

---
Rob, Arnd, Olof, I'd particularly like feedback on whether the following:

#include "../../mach-tegra/iomap.h"

in arch/arm/include/debug/tegra.S is acceptable. I'd really like to continue to #include a header to share the defines to Tegra physical memory layout and virtual based addresses with Tegra's io.c's struct map_desc entries, so they can't get out of sync. So, the include can either use the relative path as quoted above (which I don't think will cause any significant maintenance issue), or Tegra's iomap.h would have to be moved somewhere public so e.g. <tegra-iomap.h> could be included.

Quoted 16 lines · Stephen Warren

From: Stephen Warren <[email protected]>

Move Tegra's debug-macro.S over to the common debug macro directory.

Move Tegra's debug UART selection menu into ARM's Kconfig.debug, so that all related options are selected in the same place.

Tegra's uncompress.h is left in mach-tegra/include/mach; it will be removed whenever Tegra is converted to multi-platform.

Signed-off-by Stephen Warren <[email protected]>

---
Rob, Arnd, Olof, I'd particularly like feedback on whether the following:

#include "../../mach-tegra/iomap.h"

Quoted 7 lines

in arch/arm/include/debug/tegra.S is acceptable. I'd really like to continue to #include a header to share the defines to Tegra physical memory layout and virtual based addresses with Tegra's io.c's struct map_desc entries, so they can't get out of sync. So, the include can either use the relative path as quoted above (which I don't think will cause any significant maintenance issue), or Tegra's iomap.h would have to be moved somewhere public so e.g. <tegra-iomap.h> could be included.

We already have a way to get the phys and virt addresses at runtime with addruart macro.

Couldn't we wrap this with a proper function and setup the mapping at runtime. This would move it out of the platforms.

I'd also like to make the virtual address the same on all platforms (but different offsets within a 1MB section) and make the phys address a kconfig option. This would also eliminate the need for the platform include and potentially addruart for that matter.

Quoted 25 lines · Rob Herring

Quoted 16 lines · Stephen Warren

From: Stephen Warren <[email protected]>

Move Tegra's debug-macro.S over to the common debug macro directory.

Move Tegra's debug UART selection menu into ARM's Kconfig.debug, so that all related options are selected in the same place.

Tegra's uncompress.h is left in mach-tegra/include/mach; it will be removed whenever Tegra is converted to multi-platform.

Signed-off-by Stephen Warren <[email protected]>

---
Rob, Arnd, Olof, I'd particularly like feedback on whether the following:

#include "../../mach-tegra/iomap.h"

Quoted 7 lines

in arch/arm/include/debug/tegra.S is acceptable. I'd really like to continue to #include a header to share the defines to Tegra physical memory layout and virtual based addresses with Tegra's io.c's struct map_desc entries, so they can't get out of sync. So, the include can either use the relative path as quoted above (which I don't think will cause any significant maintenance issue), or Tegra's iomap.h would have to be moved somewhere public so e.g. <tegra-iomap.h> could be included.

Quoted 2 lines

We already have a way to get the phys and virt addresses at runtime with addruart macro.

So this discussion is mainly about the implementation of addruart.

Quoted 2 lines

Couldn't we wrap this with a proper function and setup the mapping at runtime. This would move it out of the platforms.

So, the mapping already is set up at run-time at least during early boot; __create_page_tables() in arch/arm/kernel/head.S calls addruart and sets up an entry for it.

I suppose the implication here is that the virtual address that addruart returns doesn't have to match anything that the machine later sets up using iotable_init().

If that's true, then Tegra's debug-macro.S only needs to know the UART physical address, and can make up almost any arbitrary virtual address (perhaps even driven by the logic you mention in your next paragraph below) and hence need not rely on Tegra's iomap.h. That said, we'd still have to manually remember not to create conflicting virtual address setups in the two places, which would still be easier with a shared header.

However, I then have two questions:

1) How long do the page tables set up by __create_page_tables() last; do they stick around forever, or at least as long as the macros from debug-macro.S are used, or are they replaced sometime, on the assumption that the machine's .map_io() will call iotable_init() and end up setting up the same mapping?

[    0.000000] Booting Linux on physical CPU 0
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Linux version 3.7.0-rc1-next-20121016-...
[    0.000000] CPU: ARMv7 Processor [411fc090] revision 0 (ARMv7), cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[    0.000000] Machine: nVidia Tegra20 (Flattened Device Tree), model: NVIDIA Tegra2 Harmony evaluation board
[    0.000000] bootconsole [earlycon0] enabled
[    0.000000] Memory policy: ECC disabled, Data cache writealloc

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:

b) Rework debug-macro.S so that it isn't an include file, but rather a regular top-level file.

Quoted 7 lines · Stephen Warren

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:

Why not take the opposite approach. Treat the platform as setting up the addresses for the UART. Then arrange for the debug macros to match that.

Or we define a common virtual address for debug UARTs (which platforms would not be able to use).

Quoted 2 lines

b) Rework debug-macro.S so that it isn't an include file, but rather a regular top-level file.

No, that won't work. It's used in two places - the kernel and the decompressor. Hence why it is an include file (it's not an include file just for the fun of it.)

On 10/17/2012 10:22 AM, Stephen Warren wrote:

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.

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.

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.

Quoted 4 lines · Rob Herring

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.

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.

arch/arm/mm/mmu.c +3 -0

 
+void __init debug_ll_io_init(void)
+{
+	struct map_desc map;

Quoted 25 lines · Rob Herring

Quoted 16 lines · Stephen Warren

From: Stephen Warren <[email protected]>

Move Tegra's debug-macro.S over to the common debug macro directory.

Move Tegra's debug UART selection menu into ARM's Kconfig.debug, so that all related options are selected in the same place.

Tegra's uncompress.h is left in mach-tegra/include/mach; it will be removed whenever Tegra is converted to multi-platform.

Signed-off-by Stephen Warren <[email protected]>

---
Rob, Arnd, Olof, I'd particularly like feedback on whether the following:

#include "../../mach-tegra/iomap.h"

Quoted 7 lines

in arch/arm/include/debug/tegra.S is acceptable. I'd really like to continue to #include a header to share the defines to Tegra physical memory layout and virtual based addresses with Tegra's io.c's struct map_desc entries, so they can't get out of sync. So, the include can either use the relative path as quoted above (which I don't think will cause any significant maintenance issue), or Tegra's iomap.h would have to be moved somewhere public so e.g. <tegra-iomap.h> could be included.

Quoted 2 lines

We already have a way to get the phys and virt addresses at runtime with addruart macro.

So this discussion is mainly about the implementation of addruart.

Quoted 2 lines

Couldn't we wrap this with a proper function and setup the mapping at runtime. This would move it out of the platforms.

So, the mapping already is set up at run-time at least during early boot; __create_page_tables() in arch/arm/kernel/head.S calls addruart and sets up an entry for it.

I suppose the implication here is that the virtual address that addruart returns doesn't have to match anything that the machine later sets up using iotable_init().

If that's true, then Tegra's debug-macro.S only needs to know the UART physical address, and can make up almost any arbitrary virtual address (perhaps even driven by the logic you mention in your next paragraph below) and hence need not rely on Tegra's iomap.h. That said, we'd still have to manually remember not to create conflicting virtual address setups in the two places, which would still be easier with a shared header.

However, I then have two questions:

1) How long do the page tables set up by __create_page_tables() last; do they stick around forever, or at least as long as the macros from debug-macro.S are used, or are they replaced sometime, on the assumption that the machine's .map_io() will call iotable_init() and end up setting up the same mapping?

2) If the virtual address returned by addruart on Tegra is different than any virtual addresses set up by Tegra's .map_io(), and Tegra's .map_io() sets up a very broad mapping that covers all peripherals including the UART, and hence the UART physical registers get mapped into two virtual addresses, will this cause any problems? IIRC, duplicate mappings can cause some issues on ARM, but perhaps that only applies to memory-like mappings, and not completely uncached IO mappings?