ARM: debug: use kconfig choice for selecting DEBUG_LL UART

Now that the DEBUG_LL UART can be selected by a Kconfig choice, convert the Samsung UART selection to use a set of bools rather than an int.

Signed-off-by Will Deacon <[email protected]>

---
?arch/arm/Kconfig.debug ? ? ? ?| ? 45 ++++++++++++++++++++++++++++++----------- ?arch/arm/plat-samsung/Kconfig | ? ?7 ++++++
?2 files changed, 40 insertions(+), 12 deletions(-)

arch/arm/Kconfig.debug +0 -0

index 11604c9..2f80564 100644
@@ -94,6 +94,39 @@ choice

? ? ? ? ? ? ? ? ?Saying N will cause the debug messages to appear on the first ? ? ? ? ? ? ? ? ?serial port.

+ ? ? ? config DEBUG_S3C_UART0
+ ? ? ? ? ? ? ? depends on PLAT_SAMSUNG
+ ? ? ? ? ? ? ? bool "Use S3C UART 0 for low-level debug"
+ ? ? ? ? ? ? ? help
+ ? ? ? ? ? ? ? ? Say Y here if you want the debug print routines to direct
+ ? ? ? ? ? ? ? ? their output to UART 0. The port must have been initialised
+ ? ? ? ? ? ? ? ? by the boot-loader before use.

arch/arm/Kconfig.debug +26 -0

+
+ ? ? ? ? ? ? ? ? The uncompressor code port configuration is now handled
+ ? ? ? ? ? ? ? ? by CONFIG_S3C_LOWLEVEL_UART_PORT.
+
+ ? ? ? config DEBUG_S3C_UART1
+ ? ? ? ? ? ? ? depends on PLAT_SAMSUNG
+ ? ? ? ? ? ? ? bool "Use S3C UART 1 for low-level debug"
+ ? ? ? ? ? ? ? help
+ ? ? ? ? ? ? ? ? Say Y here if you want the debug print routines to direct
+ ? ? ? ? ? ? ? ? their output to UART 1. The port must have been initialised
+ ? ? ? ? ? ? ? ? by the boot-loader before use.
+
+ ? ? ? ? ? ? ? ? The uncompressor code port configuration is now handled
+ ? ? ? ? ? ? ? ? by CONFIG_S3C_LOWLEVEL_UART_PORT.
+
+ ? ? ? config DEBUG_S3C_UART2
+ ? ? ? ? ? ? ? depends on PLAT_SAMSUNG
+ ? ? ? ? ? ? ? bool "Use S3C UART 2 for low-level debug"
+ ? ? ? ? ? ? ? help
+ ? ? ? ? ? ? ? ? Say Y here if you want the debug print routines to direct
+ ? ? ? ? ? ? ? ? their output to UART 2. The port must have been initialised
+ ? ? ? ? ? ? ? ? by the boot-loader before use.
+
+ ? ? ? ? ? ? ? ? The uncompressor code port configuration is now handled
+ ? ? ? ? ? ? ? ? by CONFIG_S3C_LOWLEVEL_UART_PORT.
+

?endchoice

?config EARLY_PRINTK

arch/arm/Kconfig.debug +0 -0

@@ -124,16 +157,4 @@ config OC_ETM

? ? ? ? ?buffer driver that will allow you to collect traces of the ? ? ? ? ?kernel code.

arch/arm/Kconfig.debug +0 -12

-config DEBUG_S3C_UART
- ? ? ? depends on PLAT_SAMSUNG
- ? ? ? int "S3C UART to use for low-level debug"
- ? ? ? default "0"
- ? ? ? help
- ? ? ? ? Choice for UART for kernel low-level using S3C UARTS,
- ? ? ? ? should be between zero and two. The port must have been
- ? ? ? ? initialised by the boot-loader before use.
-
- ? ? ? ? The uncompressor code port configuration is now handled
- ? ? ? ? by CONFIG_S3C_LOWLEVEL_UART_PORT.
-

?endmenu

arch/arm/plat-samsung/Kconfig +0 -0

index b3e1065..49b14b1 100644
@@ -367,4 +367,11 @@ config SAMSUNG_PD

? ? ? ?help
? ? ? ? ?Say Y here if you want to control Power Domain by Runtime PM.

arch/arm/plat-samsung/Kconfig +7 -0

+config DEBUG_S3C_UART
+ ? ? ? depends on PLAT_SAMSUNG
+ ? ? ? int
+ ? ? ? default "0" if DEBUG_S3C_UART0
+ ? ? ? default "1" if DEBUG_S3C_UART1
+ ? ? ? default "2" if DEBUG_S3C_UART2
+

?endif
--
1.7.0.4

Hi Will,

What is your opinion about the following diff instead of the above one?

arch/arm/Kconfig.debug +9 -0

index 65cf8c6..035f5cd 100644
@@ -120,6 +120,15 @@ choice
 		  Say Y here if you want the debug print routines to direct
 		  their output to the second serial port on these devices.
 
+	config DEBUG_SAMSUNG_UART
+		bool "Kernel low-level debugging messages via samsung serial port"
+		depends on PLAT_SAMSUNG
+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the serial port for Samsung platforms. Choose
+		  the uart port with the "S3C UART to use for low-level debug"
+		  config option.
+
 endchoice
 
 config EARLY_PRINTK

Quoted 1 line · Thomas Abraham

Hi Will,

Hi Thomas,

Quoted 27 lines

What is your opinion about the following diff instead of the above one?

arch/arm/Kconfig.debug +0 -0

index 65cf8c6..035f5cd 100644
@@ -120,6 +120,15 @@ choice

? ? ? ? ? ? ? ? Say Y here if you want the debug print routines to direct ? ? ? ? ? ? ? ? their output to the second serial port on these devices.

+ ? ? config DEBUG_SAMSUNG_UART
+ ? ? ? ? ? ? bool "Kernel low-level debugging messages via samsung serial port"
+ ? ? ? ? ? ? depends on PLAT_SAMSUNG
+ ? ? ? ? ? ? help
+ ? ? ? ? ? ? ? Say Y here if you want the debug print routines to direct
+ ? ? ? ? ? ? ? their output to the serial port for Samsung platforms. Choose
+ ? ? ? ? ? ? ? the uart port with the "S3C UART to use for low-level debug"
+ ? ? ? ? ? ? ? config option.
+
?endchoice

?config EARLY_PRINTK

arch/arm/Kconfig.debug +0 -0

@@ -139,7 +148,7 @@ config OC_ETM

? ? ? ? kernel code.

Quoted 6 lines

?config DEBUG_S3C_UART
- ? ? depends on PLAT_SAMSUNG
+ ? ? depends on DEBUG_SAMSUNG_UART
? ? ? int "S3C UART to use for low-level debug"
? ? ? default "0"
? ? ? help

Well, it's smaller so that's always a plus. However, I don't think the extra level of indirection helps (that is, the "Kernel low-level debugging UART") choice should be where the UART is specified, rather than pointing you at another (platform-specific) option).

Did your suggestion come purely out of aesthetics or are you running into difficulties with the original patch?

Will

There are no difficulties with the original patch. But that was not how Samsung boards have been selecting the low level debug uart port number. The proposed patch tried to maintain the old style.

Another point is that Samsung's Exynos4 (and few other SoC's) has a fourth UART port as well. Not that it is used as a debug port currently, but there is no technical limitation in using that as a console port. So that might need 'DEBUG_S3C_UART3' as well.

Quoted 4 lines · Thomas Abraham

There are no difficulties with the original patch. But that was not how Samsung boards have been selecting the low level debug uart port number. The proposed patch tried to maintain the old style.

Quoted 4 lines

Another point is that Samsung's Exynos4 (and few other SoC's) has a fourth UART port as well. Not that it is used as a debug port currently, but there is no technical limitation in using that as a console port. So that might need 'DEBUG_S3C_UART3' as well.

Tell you what then: let's stick with the original patch for now and then you can refactor the Samsung bit in the future if/when you add support for the fourth UART. Does that sound OK?

Will

Enabling CONFIG_DEBUG_LL (which is required for earlyprintk) hardwires the debug UART address into the kernel, so that we can print before the platform is initialised.

If the user inadvertently selects multiple platforms with DEBUG_LL enabled, the UART address may not be correct and will likely cause the kernel to hang in the very early stages of boot.

This patch, based on a skeleton from Russell, uses a Kconfig choice for selecting the DEBUG_LL UART, therefore allowing the user to make a choice about the supported platform when DEBUG_LL is enabled.

Cc Linus Walleij <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/Kconfig.debug |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

arch/arm/Kconfig.debug +24 -17

index 81cbe40..11604c9 100644
@@ -72,6 +72,30 @@ config DEBUG_LL
 	  in the kernel.  This is helpful if you are debugging code that
 	  executes before the console is initialized.
 
+choice
+	prompt "Kernel low-level debugging port"
+	depends on DEBUG_LL
+
+	config DEBUG_DC21285_PORT
+		bool "Kernel low-level debugging messages via footbridge serial port"
+		depends on FOOTBRIDGE
+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the serial port in the DC21285 (Footbridge).
+		  Saying N will cause the debug messages to appear on the first
+		  16550 serial port.
+
+	config DEBUG_CLPS711X_UART2
+		bool "Kernel low-level debugging messages via UART2"
+		depends on ARCH_CLPS711X
+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the second serial port on these devices.
+		  Saying N will cause the debug messages to appear on the first
+		  serial port.
+
+endchoice
+
 config EARLY_PRINTK
 	bool "Early printk"
 	depends on DEBUG_LL

@@ -100,23 +124,6 @@ config OC_ETM
 	  buffer driver that will allow you to collect traces of the
 	  kernel code.
 
-config DEBUG_DC21285_PORT
-	bool "Kernel low-level debugging messages via footbridge serial port"
-	depends on DEBUG_LL && FOOTBRIDGE
-	help
-	  Say Y here if you want the debug print routines to direct their
-	  output to the serial port in the DC21285 (Footbridge). Saying N
-	  will cause the debug messages to appear on the first 16550
-	  serial port.
-
-config DEBUG_CLPS711X_UART2
-	bool "Kernel low-level debugging messages via UART2"
-	depends on DEBUG_LL && ARCH_CLPS711X
-	help
-	  Say Y here if you want the debug print routines to direct their
-	  output to the second serial port on these devices.  Saying N will
-	  cause the debug messages to appear on the first serial port.
-
 config DEBUG_S3C_UART
 	depends on PLAT_SAMSUNG
 	int "S3C UART to use for low-level debug"

Signature · 7 lines

1.7.0.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 12 lines · Will Deacon

Enabling CONFIG_DEBUG_LL (which is required for earlyprintk) hardwires the debug UART address into the kernel, so that we can print before the platform is initialised.

If the user inadvertently selects multiple platforms with DEBUG_LL enabled, the UART address may not be correct and will likely cause the kernel to hang in the very early stages of boot.

This patch, based on a skeleton from Russell, uses a Kconfig choice for selecting the DEBUG_LL UART, therefore allowing the user to make a choice about the supported platform when DEBUG_LL is enabled.

Quoted 2 lines

Cc Linus Walleij <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Acked-by Nicolas Pitre <[email protected]>

(ditto for the other 2 patches)

Enabling CONFIG_DEBUG_LL (which is required for earlyprintk) hardwires the debug UART address into the kernel, so that we can print before the platform is initialised.

If the user inadvertently selects multiple platforms with DEBUG_LL enabled, the UART address may not be correct and will likely cause the kernel to hang in the very early stages of boot.

This patch, based on a skeleton from Russell, uses a Kconfig choice for selecting the DEBUG_LL UART, therefore allowing the user to make a choice about the supported platform when DEBUG_LL is enabled.

Cc Linus Walleij <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/Kconfig.debug |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

arch/arm/Kconfig.debug +4 -0

index 81cbe40..11604c9 100644
@@ -72,6 +72,30 @@ config DEBUG_LL
 	  in the kernel.  This is helpful if you are debugging code that
 	  executes before the console is initialized.
 
+choice
+	prompt "Kernel low-level debugging port"
+	depends on DEBUG_LL
+

arch/arm/Kconfig.debug +10 -17

+		bool "Kernel low-level debugging messages via UART2"
+		depends on ARCH_CLPS711X
+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the second serial port on these devices.
+		  Saying N will cause the debug messages to appear on the first
+		  serial port.
+
+endchoice
+
 config EARLY_PRINTK
 	bool "Early printk"
 	depends on DEBUG_LL

@@ -100,23 +124,6 @@ config OC_ETM
 	  buffer driver that will allow you to collect traces of the
 	  kernel code.
 
-config DEBUG_DC21285_PORT
-	bool "Kernel low-level debugging messages via footbridge serial port"
-	depends on DEBUG_LL && FOOTBRIDGE
-	help
-	  Say Y here if you want the debug print routines to direct their
-	  output to the serial port in the DC21285 (Footbridge). Saying N
-	  will cause the debug messages to appear on the first 16550
-	  serial port.
-
-config DEBUG_CLPS711X_UART2
-	bool "Kernel low-level debugging messages via UART2"
-	depends on DEBUG_LL && ARCH_CLPS711X
-	help
-	  Say Y here if you want the debug print routines to direct their
-	  output to the second serial port on these devices.  Saying N will
-	  cause the debug messages to appear on the first serial port.
-
 config DEBUG_S3C_UART
 	depends on PLAT_SAMSUNG
 	int "S3C UART to use for low-level debug"

Signature · 1 line

1.7.0.4

Quoted 36 lines · Will Deacon

Enabling CONFIG_DEBUG_LL (which is required for earlyprintk) hardwires the debug UART address into the kernel, so that we can print before the platform is initialised.

If the user inadvertently selects multiple platforms with DEBUG_LL enabled, the UART address may not be correct and will likely cause the kernel to hang in the very early stages of boot.

This patch, based on a skeleton from Russell, uses a Kconfig choice for selecting the DEBUG_LL UART, therefore allowing the user to make a choice about the supported platform when DEBUG_LL is enabled.

Cc Linus Walleij <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/Kconfig.debug |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

arch/arm/Kconfig.debug +10 -0

index 81cbe40..11604c9 100644
@@ -72,6 +72,30 @@ config DEBUG_LL
 	  in the kernel.  This is helpful if you are debugging code that
 	  executes before the console is initialized.
 
+choice
+	prompt "Kernel low-level debugging port"
+	depends on DEBUG_LL
+
+	config DEBUG_DC21285_PORT
+		bool "Kernel low-level debugging messages via footbridge serial port"
+		depends on FOOTBRIDGE
+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the serial port in the DC21285 (Footbridge).

arch/arm/Kconfig.debug config DEBUG_LL · line 88

Quoted 4 lines

arch/arm/Kconfig.debug +4 -0

+		  Saying N will cause the debug messages to appear on the first
+		  16550 serial port.
+
+	config DEBUG_CLPS711X_UART2

I would expect this will become a long list when other platforms start adding CONFIG symbol here. It may be a good idea to sort this list in symbol from the beginning.

Hi Shawn,

Quoted 37 lines · Shawn Guo

Quoted 36 lines · Will Deacon

Enabling CONFIG_DEBUG_LL (which is required for earlyprintk) hardwires the debug UART address into the kernel, so that we can print before the platform is initialised.

If the user inadvertently selects multiple platforms with DEBUG_LL enabled, the UART address may not be correct and will likely cause the kernel to hang in the very early stages of boot.

This patch, based on a skeleton from Russell, uses a Kconfig choice for selecting the DEBUG_LL UART, therefore allowing the user to make a choice about the supported platform when DEBUG_LL is enabled.

Cc Linus Walleij <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/Kconfig.debug |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

arch/arm/Kconfig.debug +10 -0

index 81cbe40..11604c9 100644
@@ -72,6 +72,30 @@ config DEBUG_LL
 	  in the kernel.  This is helpful if you are debugging code that
 	  executes before the console is initialized.
 
+choice
+	prompt "Kernel low-level debugging port"
+	depends on DEBUG_LL
+
+	config DEBUG_DC21285_PORT
+		bool "Kernel low-level debugging messages via footbridge serial port"
+		depends on FOOTBRIDGE
+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the serial port in the DC21285 (Footbridge).

Quoted 8 lines

arch/arm/Kconfig.debug config DEBUG_LL · line 88

Quoted 4 lines

arch/arm/Kconfig.debug +4 -0

+		  Saying N will cause the debug messages to appear on the first
+		  16550 serial port.
+
+	config DEBUG_CLPS711X_UART2

I would expect this will become a long list when other platforms start adding CONFIG symbol here. It may be a good idea to sort this list in symbol from the beginning.

I'm not sure that sorting this list in alphabetical order is a good idea. This is a Kconfig choice, so the default value is the first one in the list that has its dependencies satisfied. Therefore, the ordering has a direct impact on the default UART selection.

Quoted 5 lines · Will Deacon

I'm not sure that sorting this list in alphabetical order is a good idea. This is a Kconfig choice, so the default value is the first one in the list that has its dependencies satisfied. Therefore, the ordering has a direct impact on the default UART selection.

I'm unsure that the default UART selection makes much sense here. When we build so many platforms into single image, it's hard to say which one should be the default. People anyway need to check if the the UART selection matches the platform they are debugging on.

BTW, I just posted one patch for imx based on your series. Are you interested in folding it into yours?

Quoted 6 lines · Shawn Guo

Quoted 5 lines · Will Deacon

I'm not sure that sorting this list in alphabetical order is a good idea. This is a Kconfig choice, so the default value is the first one in the list that has its dependencies satisfied. Therefore, the ordering has a direct impact on the default UART selection.

Quoted 4 lines

I'm unsure that the default UART selection makes much sense here. When we build so many platforms into single image, it's hard to say which one should be the default. People anyway need to check if the the UART selection matches the platform they are debugging on.

Ok, how about having the default choice as a dummy option which doesn't correspond to a UART?:

arch/arm/Kconfig.debug +4 -0

index f23975a..455bc8c 100644
@@ -65,8 +65,12 @@ config DEBUG_USER
 
 # These options are only for real kernel hackers who want to get their hands dirty.
 config DEBUG_LL
+       bool
+
+config DEBUG_LL_UART
        bool "Kernel low-level debugging functions"
        depends on DEBUG_KERNEL
+       select DEBUG_LL if !DEBUG_UART_NONE
        help
          Say Y here to include definitions of printascii, printch, printhex
          in the kernel.  This is helpful if you are debugging code that

arch/arm/Kconfig.debug +4 -0

+       default DEBUG_UART_NONE
+
+       config DEBUG_UART_NONE
+               bool "No UART selected (default)"
 
        config DEBUG_DC21285_PORT
                bool "Kernel low-level debugging messages via footbridge serial port"

That way, you really have to select the UART for DEBUG_LL to be enabled.

Quoted 2 lines

BTW, I just posted one patch for imx based on your series. Are you interested in folding it into yours?

Yep, I'll pick it up once Sascha is happy with it. If we go with the above, I'm happy to make the necessary changes.

Quoted 12 lines · Will Deacon

Quoted 10 lines · Shawn Guo

Quoted 5 lines · Will Deacon

I'm not sure that sorting this list in alphabetical order is a good idea. This is a Kconfig choice, so the default value is the first one in the list that has its dependencies satisfied. Therefore, the ordering has a direct impact on the default UART selection.

I'm unsure that the default UART selection makes much sense here. When we build so many platforms into single image, it's hard to say which one should be the default. People anyway need to check if the the UART selection matches the platform they are debugging on.

Quoted 3 lines

Ok, how about having the default choice as a dummy option which doesn't correspond to a UART?:

To me, it is a little bit overkilled. Can we just sort them in alphabetical order and let the first one be the winner? We can take this as a reward to the good naming :)

Quoted 19 lines

arch/arm/Kconfig.debug +4 -0

index f23975a..455bc8c 100644
@@ -65,8 +65,12 @@ config DEBUG_USER
 
 # These options are only for real kernel hackers who want to get their hands dirty.
 config DEBUG_LL
+       bool
+
+config DEBUG_LL_UART
        bool "Kernel low-level debugging functions"
        depends on DEBUG_KERNEL
+       select DEBUG_LL if !DEBUG_UART_NONE
        help
          Say Y here to include definitions of printascii, printch, printhex
          in the kernel.  This is helpful if you are debugging code that

@@ -74,7 +78,12 @@ config DEBUG_LL
 

arch/arm/Kconfig.debug config DEBUG_LL · line 82

Quoted 5 lines

arch/arm/Kconfig.debug +2 -1

 choice
        prompt "Kernel low-level debugging port"
-       depends on DEBUG_LL
+       depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \
+                                    PLAT_SAMSUNG || ARCH_REALVIEW)

We will have to list a lot of ARCH/PLAT symbols here. This is what I meant overkilled actually.

Quoted 11 lines · Shawn Guo

arch/arm/Kconfig.debug config DEBUG_LL · line 82

Quoted 7 lines · Will Deacon

arch/arm/Kconfig.debug +2 -1

@@ -74,7 +78,12 @@ config DEBUG_LL
 
 choice
        prompt "Kernel low-level debugging port"
-       depends on DEBUG_LL
+       depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \
+                                    PLAT_SAMSUNG || ARCH_REALVIEW)

We will have to list a lot of ARCH/PLAT symbols here. This is what I meant overkilled actually.

Ultimately, we will want to have all the platforms using this mechanism so this list of symbols could eventually be removed. I take your point though; so I'll leave the patch series as it is for now.

Quoted 13 lines · Will Deacon

Quoted 11 lines · Shawn Guo

arch/arm/Kconfig.debug config DEBUG_LL · line 82

Quoted 7 lines · Will Deacon

arch/arm/Kconfig.debug +2 -1

@@ -74,7 +78,12 @@ config DEBUG_LL
 
 choice
        prompt "Kernel low-level debugging port"
-       depends on DEBUG_LL
+       depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \
+                                    PLAT_SAMSUNG || ARCH_REALVIEW)

We will have to list a lot of ARCH/PLAT symbols here. This is what I meant overkilled actually.

Quoted 3 lines

Ultimately, we will want to have all the platforms using this mechanism so this list of symbols could eventually be removed. I take your point though; so I'll leave the patch series as it is for now.

Well, ultimately I'd like to see something even better than that. We should have the bootloader provide the required information to the kernel for it to be able to send bytes to a debug device. It should be rather simple, especially for a serial UART. All we need in that case is:

- physical address of the transmit FIFO register

- physical address of the register indicating "FIFO full" with a corresponding bit mask

- physical address of the register indicating "FIFO empty" with a corresponding bit mask

Quoted 29 lines · Nicolas Pitre

Quoted 13 lines · Will Deacon

Quoted 11 lines · Shawn Guo

arch/arm/Kconfig.debug config DEBUG_LL · line 82

Quoted 7 lines · Will Deacon

arch/arm/Kconfig.debug +2 -1

@@ -74,7 +78,12 @@ config DEBUG_LL
 
 choice
        prompt "Kernel low-level debugging port"
-       depends on DEBUG_LL
+       depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \
+                                    PLAT_SAMSUNG || ARCH_REALVIEW)

We will have to list a lot of ARCH/PLAT symbols here. This is what I meant overkilled actually.

Quoted 3 lines

Ultimately, we will want to have all the platforms using this mechanism so this list of symbols could eventually be removed. I take your point though; so I'll leave the patch series as it is for now.

Well, ultimately I'd like to see something even better than that. We should have the bootloader provide the required information to the kernel for it to be able to send bytes to a debug device. It should be rather simple, especially for a serial UART. All we need in that case is:

- physical address of the transmit FIFO register

- physical address of the register indicating "FIFO full" with a corresponding bit mask

Quoted 2 lines

- physical address of the register indicating "FIFO empty" with a corresponding bit mask

You're missing something there. It's not always about FIFO full and FIFO empty. On some platforms, we want _reliable_ debugging, so the debug stuff waits while CTS is inactive.

Plus you need the virtual address too, because the LL debug stuff is there to debug around places like the initial assembly, before C code is setup.

Plus its not just about bitmasks, but also about size of access, and polarity of bits. So actually you'd need:

- virtual and physical address and size of transmit register
- virtual and physical address, mask and value of transmit status
- virtual and physical address, mask and value of CTS status

but then, we have debugging which doesn't have a physical address associated with it (the jtag stuff) so this doesn't cover the full story either.

Quoted 3 lines · Russell King - ARM Linux

Plus you need the virtual address too, because the LL debug stuff is there to debug around places like the initial assembly, before C code is setup.

The kernel should determine or set up the virtual address by itself. This is obviously not something we want the bootloader to provide. For the same reason, I've discarded the idea that the bootloader could simply have provided via a DT node a small segment of code (it's only a few assembly instructions after all) to drive the serial port because it would require a stable mapping to match that code's idea of the register locations.

I'm not trying to cover the full story. This is mainly for 98% of those cases where a plain serial port is used for both the DEBUG_LL _and_ the early output from the decompressor.

Those people with a JTAG debugger and the knowledge to use it really don't need any generic infrastructure to get some early debugging information out. They can reconfigure their kernel and even hack the source to suit their needs. But the people who are going to be the main consumers of a multi-SOC single-binary kernel won't be the ones recompiling their kernel just to provide us with debugging info.

Quoted 5 lines · Nicolas Pitre

Quoted 3 lines · Russell King - ARM Linux

Plus you need the virtual address too, because the LL debug stuff is there to debug around places like the initial assembly, before C code is setup.

Quoted 7 lines

The kernel should determine or set up the virtual address by itself. This is obviously not something we want the bootloader to provide. For the same reason, I've discarded the idea that the bootloader could simply have provided via a DT node a small segment of code (it's only a few assembly instructions after all) to drive the serial port because it would require a stable mapping to match that code's idea of the register locations.

Yes, and the virtual and physical base addresses are set via the existing macros.

To take the virtual address out of that means that we then have to find some way of storing that data - which can't be inside the normal kernel bss or data sections. BSS has not been zeroed at the point where we want working DEBUG_LL stuff.

Defining a offset-fixed memory location from the kernel is fragile, and will end up wasting the entire page - which would have to be permanently reserved.

This is just getting _idiotic_. There are times when "no" is the word which has to be used, and this is one of them.

Quoted 3 lines

I'm not trying to cover the full story. This is mainly for 98% of those cases where a plain serial port is used for both the DEBUG_LL _and_ the early output from the decompressor.

So yet again we end up with another half baked "solution", which will result in "end-users" being confused because it'll work on some stuff but not on other stuff.

Quoted 6 lines

Those people with a JTAG debugger and the knowledge to use it really don't need any generic infrastructure to get some early debugging information out. They can reconfigure their kernel and even hack the source to suit their needs. But the people who are going to be the main consumers of a multi-SOC single-binary kernel won't be the ones recompiling their kernel just to provide us with debugging info.

That is a stupid argument. The people who need the early console are those bringing up a new board. By the act of being involved in bringing up a new board, they are a developer. They are not a user. They will be having to rebuild the kernel.

There is no technical problem here. It's just entirely conceptual, and one of trying to use stuff inappropriately.

Obviously the way to go is to use a .data variable.

And it can be referenced from assembly code in a position independent manner:

.macro get_ptr_pic, ptr, symbol
b 199f
.align

198:    .long   .
        .long   \symbol
199:    adr     \ptr, 198b              @ get relative address of 198b
        ldr     ip, [\ptr]              @ get absolute address of 198b
        sub     ip, ip, \ptr            @ offset between abs and rel
        ldr     \ptr, [\ptr, #4]        @ absolute address of \symbol
        sub     \ptr, \ptr, ip          @ make it relative

.endm

Quoted 2 lines

This is just getting _idiotic_. There are times when "no" is the word which has to be used, and this is one of them.

You are saying "no" to the possibility of getting rid of the zImage decompressor serial output.

And now you're saying "no" to any possibility of keeping it alive in the context of a multi-SOC kernel.

Are you also saying "no" to the multi-SOC kernel? If so you'll have to convince many more people than just me.

But, just like on x86, booting a kernel binary (say, from a packaged distro) on a new machine might not exactly boot satisfactorily. And this is likely to be the users who notice it first, in which case having functional early serial output might be pretty handy.

Quoted 8 lines · Nicolas Pitre

Obviously the way to go is to use a .data variable.

And it can be referenced from assembly code in a position independent manner:

.macro get_ptr_pic, ptr, symbol
b 199f
.align

Quoted 8 lines

198:    .long   .
        .long   \symbol
199:    adr     \ptr, 198b              @ get relative address of 198b
        ldr     ip, [\ptr]              @ get absolute address of 198b
        sub     ip, ip, \ptr            @ offset between abs and rel
        ldr     \ptr, [\ptr, #4]        @ absolute address of \symbol
        sub     \ptr, \ptr, ip          @ make it relative

.endm

And have you checked that you can crap over 'ip' in the early assembly code? There are some (small) places where that register is used, which means you accidentally corrupt that register.

And further to this, I'll point out that the debugging functions are *explicitly* designed to avoid corrupting any more than just r0-r3 and lr. That's not just the IO functions but also the hex and string printing functions.

And the head*.S code is explicitly written to expect r0-r3 to be corrupted - which basically means that no long-term values are held in those registers.

Quoted 5 lines · Russell King - ARM Linux

And further to this, I'll point out that the debugging functions are *explicitly* designed to avoid corrupting any more than just r0-r3 and lr. That's not just the IO functions but also the hex and string printing functions.

Quoted 3 lines

And the head*.S code is explicitly written to expect r0-r3 to be corrupted - which basically means that no long-term values are held in those registers.

Well, not exactly. I actually have a patch to that effect I made a while ago so all the early code could be unaffected by inserted function call, but held on to it because nothing yet justified its need. Here it is for reference:

Quoted 11 lines · Nicolas Pitre

Quoted 8 lines · Russell King - ARM Linux

And further to this, I'll point out that the debugging functions are *explicitly* designed to avoid corrupting any more than just r0-r3 and lr. That's not just the IO functions but also the hex and string printing functions.

And the head*.S code is explicitly written to expect r0-r3 to be corrupted - which basically means that no long-term values are held in those registers.

Quoted 4 lines

Well, not exactly. I actually have a patch to that effect I made a while ago so all the early code could be unaffected by inserted function call, but held on to it because nothing yet justified its need. Here it is for reference:

And so this buggers up the ability to insert calls to the debugging code by placing values into r0-r3. So that patch will get a nak too.

Quoted 17 lines · Russell King - ARM Linux

Quoted 15 lines · Nicolas Pitre

Quoted 8 lines · Russell King - ARM Linux

And further to this, I'll point out that the debugging functions are *explicitly* designed to avoid corrupting any more than just r0-r3 and lr. That's not just the IO functions but also the hex and string printing functions.

And the head*.S code is explicitly written to expect r0-r3 to be corrupted - which basically means that no long-term values are held in those registers.

Well, not exactly. I actually have a patch to that effect I made a while ago so all the early code could be unaffected by inserted function call, but held on to it because nothing yet justified its need. Here it is for reference:

Quoted 2 lines

And so this buggers up the ability to insert calls to the debugging code by placing values into r0-r3. So that patch will get a nak too.

What? Please look again at the patch and tell me what is wrong with it. Because I can't make sense of your last sentence.

Quoted 19 lines · Nicolas Pitre

Quoted 17 lines · Russell King - ARM Linux

Quoted 15 lines · Nicolas Pitre

Quoted 8 lines · Russell King - ARM Linux

And further to this, I'll point out that the debugging functions are *explicitly* designed to avoid corrupting any more than just r0-r3 and lr. That's not just the IO functions but also the hex and string printing functions.

And the head*.S code is explicitly written to expect r0-r3 to be corrupted - which basically means that no long-term values are held in those registers.

Well, not exactly. I actually have a patch to that effect I made a while ago so all the early code could be unaffected by inserted function call, but held on to it because nothing yet justified its need. Here it is for reference:

Quoted 5 lines

Quoted 2 lines

And so this buggers up the ability to insert calls to the debugging code by placing values into r0-r3. So that patch will get a nak too.

What? Please look again at the patch and tell me what is wrong with it. Because I can't make sense of your last sentence.

Have you not been reading what I've been saying.

Point 1: the code explicitly _avoids_ using r0-r3 except in small short code sequences.

Point 2: the debugging macros explicitly use r0-r3 because they know that these registers aren't going to be used in the assembly code except in small short code sequences.

So, changing all the assembly to use r0-r3 is going to bugger up the ability to use the debugging macros. Therefore this is a change with a net reduction in facility. Therefore I don't want it.

Quoted 8 lines · Nicolas Pitre

Obviously the way to go is to use a .data variable.

And it can be referenced from assembly code in a position independent manner:

.macro get_ptr_pic, ptr, symbol
b 199f
.align

Quoted 8 lines

198:    .long   .
        .long   \symbol
199:    adr     \ptr, 198b              @ get relative address of 198b
        ldr     ip, [\ptr]              @ get absolute address of 198b
        sub     ip, ip, \ptr            @ offset between abs and rel
        ldr     \ptr, [\ptr, #4]        @ absolute address of \symbol
        sub     \ptr, \ptr, ip          @ make it relative

.endm

And have you checked that you can crap over 'ip' in the early assembly code? There are some (small) places where that register is used, which means you accidentally corrupt that register.

Quoted 8 lines

Quoted 2 lines

This is just getting _idiotic_. There are times when "no" is the word which has to be used, and this is one of them.

You are saying "no" to the possibility of getting rid of the zImage decompressor serial output.

And now you're saying "no" to any possibility of keeping it alive in the context of a multi-SOC kernel.

The decompressor has nothing to do with this.

Quoted 2 lines

Are you also saying "no" to the multi-SOC kernel? If so you'll have to convince many more people than just me.

No, I am saying no to the low level debugging code which *developers* rely upon working being fucked up and rendered useless - and so requiring a new implementation to be written - in the name of multi-SoC support. That is what I'm *strongly* objecting to.

Quoted 2 lines · Russell King - ARM Linux

The decompressor has nothing to do with this.

Sure it does, since it relies on early serial output capabilities, just like earlyprintk, or possibly even the senduart code. Solving this once for them all, *appropriately*, would certainly be nice. I would be glad to see this need addressed without making further ugly abuses like the OMAP people did with their OMAP_UART_INFO and the arch/arm/plat-omap/include/plat/uncompress.h abomination.

Probably the best long term solution is to set up the debug uart based on DT data and initialize it in the uncompress code. Anybody debugging lower level stuff can certainly patch in the ucompress debug_ll code.

Quoted 3 lines · Tony Lindgren

Probably the best long term solution is to set up the debug uart based on DT data and initialize it in the uncompress code. Anybody debugging lower level stuff can certainly patch in the ucompress debug_ll code.

How do you do that before the MMU has been setup? Are you going to write a DT data parser in pure assembly to extract this information?

Quoted 5 lines · Russell King - ARM Linux

Quoted 3 lines · Tony Lindgren

Probably the best long term solution is to set up the debug uart based on DT data and initialize it in the uncompress code. Anybody debugging lower level stuff can certainly patch in the ucompress debug_ll code.

Quoted 2 lines

How do you do that before the MMU has been setup? Are you going to write a DT data parser in pure assembly to extract this information?

Don't we already have that with ATAGs to DT support in the DT append patches? At least it calls fdt_setprop so calling fdt_getprop should also work. The patch I'm talking about is:

http://permalink.gmane.org/gmane.linux.drivers.devicetree/5938

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Russell King - ARM Linux

Quoted 3 lines · Tony Lindgren

Probably the best long term solution is to set up the debug uart based on DT data and initialize it in the uncompress code. Anybody debugging lower level stuff can certainly patch in the ucompress debug_ll code.

Quoted 8 lines

Quoted 2 lines

How do you do that before the MMU has been setup? Are you going to write a DT data parser in pure assembly to extract this information?

Don't we already have that with ATAGs to DT support in the DT append patches? At least it calls fdt_setprop so calling fdt_getprop should also work. The patch I'm talking about is:

http://permalink.gmane.org/gmane.linux.drivers.devicetree/5938

Let me reiterate. How are you going to do that before the MMU has been setup.

I'll give you a hint: in the kernel (not the boot loader) you can't call *ANY* C code until you have the MMU enabled. One of the points of the *LOW LEVEL* debug code is so you can debug the low level assembly in the head*.S files before the MMU has been enabled.

That means you can't call the C function "fdt_getprop" to get the parameters.

So, should we have a chunk of assembly code to enable the MMU so that we can run fdt_getprop() to get the parameters for the LL debug code, turn the MMU off, and then run through the head.S code to enable the MMU for the kernel proper? If you think that's silly, you're starting to get the picture I've had for years about all this bastardization of the LL debug code to solve problems beyond what it was originally intended for - which, again, was to debug the head.S code.

Personally, I think this whole thing is getting impractical for the use which people are trying to stretch it too - it's trying to be used to solve the "how do we get console output before the console is up" problem as well as the "how do we debug the low level assembly code".

With the advent of multi-platform kernels, the two requirements have been well proving to be mutually exclusive.

Yes I agree. As an alternative, how about we drop all the debug_ll code and allow people to patch it in as needed?

Quoted 2 lines · Tony Lindgren

Yes I agree. As an alternative, how about we drop all the debug_ll code and allow people to patch it in as needed?

Given all this argument over it, I'm mindful to rip out all the debug_ll code and keep it permanently as an offline patch to prevent further abuse.

Quoted 12 lines · Will Deacon

Quoted 10 lines · Shawn Guo

Quoted 5 lines · Will Deacon

I'm not sure that sorting this list in alphabetical order is a good idea. This is a Kconfig choice, so the default value is the first one in the list that has its dependencies satisfied. Therefore, the ordering has a direct impact on the default UART selection.

I'm unsure that the default UART selection makes much sense here. When we build so many platforms into single image, it's hard to say which one should be the default. People anyway need to check if the the UART selection matches the platform they are debugging on.

Quoted 3 lines

Ok, how about having the default choice as a dummy option which doesn't correspond to a UART?:

To me, it is a little bit overkilled. Can we just sort them in alphabetical order and let the first one be the winner? We can take this as a reward to the good naming :)

arch/arm/Kconfig.debug config DEBUG_LL · line 82

Quoted 5 lines

choice
prompt "Kernel low-level debugging port"

arch/arm/Kconfig.debug +2 -1

-       depends on DEBUG_LL
+       depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \
+                                    PLAT_SAMSUNG || ARCH_REALVIEW)

We will have to list a lot of ARCH/PLAT symbols here. This is what I meant overkilled actually.

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

Signed-off-by Shawn Guo <[email protected]>

Cc Sascha Hauer <[email protected]>

---
 arch/arm/Kconfig.debug                       |   56 ++++++++++++++++++++++++++
 arch/arm/mach-mxs/include/mach/debug-macro.S |   12 +-----
 arch/arm/plat-mxc/include/mach/debug-macro.S |   38 +++---------------
 3 files changed, 64 insertions(+), 42 deletions(-)

arch/arm/Kconfig.debug +56 -0

index f23975a..947adef 100644
@@ -94,6 +94,62 @@ choice
 		  Saying N will cause the debug messages to appear on the first
 		  serial port.
 
+	config DEBUG_IMX1_UART
+		bool "i.MX1 Debug UART"
+		depends on SOC_IMX1
+		help
+		  Say Y here if you want kernel low-level debugging support
+		  on i.MX1.
+
+	config DEBUG_IMX23_UART
+		bool "i.MX23 Debug UART"
+		depends on SOC_IMX23
+		help
+		  Say Y here if you want kernel low-level debugging support
+		  on i.MX23.
+
+	config DEBUG_IMX25_UART
+		bool "i.MX25 Debug UART"
+		depends on SOC_IMX25
+		help
+		  Say Y here if you want kernel low-level debugging support
+		  on i.MX25.
+
+	config DEBUG_IMX21_IMX27_UART
+		bool "i.MX21 and i.MX27 Debug UART"
+		depends on SOC_IMX21 || SOC_IMX27
+		help
+		  Say Y here if you want kernel low-level debugging support
+		  on i.MX21 or i.MX27.
+
+	config DEBUG_IMX28_UART
+		bool "i.MX28 Debug UART"
+		depends on SOC_IMX28
+		help
+		  Say Y here if you want kernel low-level debugging support
+		  on i.MX28.
+
+	config DEBUG_IMX31_IMX35_UART
+		bool "i.MX31 and i.MX35 Debug UART"
+		depends on SOC_IMX31 || SOC_IMX35
+		help
+		  Say Y here if you want kernel low-level debugging support
+		  on i.MX31 or i.MX35.
+
+	config DEBUG_IMX51_UART
+		bool "i.MX51 Debug UART"
+		depends on SOC_IMX51
+		help
+		  Say Y here if you want kernel low-level debugging support
+		  on i.MX51.
+
+	config DEBUG_IMX50_IMX53_UART
+		bool "i.MX50 and i.MX53 Debug UART"
+		depends on SOC_IMX50 || SOC_IMX53
+		help
+		  Say Y here if you want kernel low-level debugging support
+		  on i.MX50 or i.MX53.
+
 	config DEBUG_S3C_UART0
 		depends on PLAT_SAMSUNG
 		bool "Use S3C UART 0 for low-level debug"

arch/arm/mach-mxs/include/mach/debug-macro.S +2 -10

index 79650a1..6d98704 100644
@@ -14,17 +14,9 @@
 #include <mach/mx23.h>
 #include <mach/mx28.h>
 
-#ifdef CONFIG_SOC_IMX23
-#ifdef UART_PADDR
-#error "CONFIG_DEBUG_LL is incompatible with multiple archs"
-#endif
+#ifdef CONFIG_DEBUG_IMX23_UART
 #define UART_PADDR	MX23_DUART_BASE_ADDR
-#endif
-
-#ifdef CONFIG_SOC_IMX28
-#ifdef UART_PADDR
-#error "CONFIG_DEBUG_LL is incompatible with multiple archs"
-#endif
+#elif defined (CONFIG_DEBUG_IMX28_UART)
 #define UART_PADDR	MX28_DUART_BASE_ADDR
 #endif
 

arch/arm/plat-mxc/include/mach/debug-macro.S +6 -32

index e4dde91..07cfdbe 100644
@@ -12,43 +12,17 @@
  */
 #include <mach/hardware.h>
 
-#ifdef CONFIG_SOC_IMX1
+#ifdef CONFIG_DEBUG_IMX1_UART
 #define UART_PADDR	MX1_UART1_BASE_ADDR
-#endif
-
-#ifdef CONFIG_SOC_IMX25
-#ifdef UART_PADDR
-#error "CONFIG_DEBUG_LL is incompatible with multiple archs"
-#endif
+#elif defined (CONFIG_DEBUG_IMX25_UART)
 #define UART_PADDR	MX25_UART1_BASE_ADDR
-#endif
-
-#if defined(CONFIG_SOC_IMX21) || defined (CONFIG_SOC_IMX27)
-#ifdef UART_PADDR
-#error "CONFIG_DEBUG_LL is incompatible with multiple archs"
-#endif
+#elif defined (CONFIG_DEBUG_IMX21_IMX27_UART)
 #define UART_PADDR	MX2x_UART1_BASE_ADDR
-#endif
-
-#if defined(CONFIG_SOC_IMX31) || defined(CONFIG_SOC_IMX35)
-#ifdef UART_PADDR
-#error "CONFIG_DEBUG_LL is incompatible with multiple archs"
-#endif
+#elif defined (CONFIG_DEBUG_IMX31_IMX35_UART)
 #define UART_PADDR	MX3x_UART1_BASE_ADDR
-#endif
-
-#ifdef CONFIG_SOC_IMX51
-#ifdef UART_PADDR
-#error "CONFIG_DEBUG_LL is incompatible with multiple archs"
-#endif
+#elif defined (CONFIG_DEBUG_IMX51_UART)
 #define UART_PADDR	MX51_UART1_BASE_ADDR
-#endif
-
-/* iMX50/53 have same addresses, but not iMX51 */
-#if defined(CONFIG_SOC_IMX50) || defined(CONFIG_SOC_IMX53)
-#ifdef UART_PADDR
-#error "CONFIG_DEBUG_LL is incompatible with multiple archs"
-#endif
+#elif defined (CONFIG_DEBUG_IMX50_IMX53_UART)
 #define UART_PADDR	MX53_UART1_BASE_ADDR
 #endif
 

Signature · 1 line

1.7.4.1

Quoted 3 lines · Shawn Guo

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

I'm unsure whether I like this. The ifdeffery does not look very good,

but the Kconfig snippet is not shorter, also it is in generic arm code and not i.MX specific.

The old way also makes sure that we do not
compile in incompatible lowlevel debug code.
At least this should be a choice in Kconfig to make clear that the different low-level debug options are exclusive.

Quoted 3 lines · Shawn Guo

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

I'm unsure whether I like this. The ifdeffery does not look very good, but the Kconfig snippet is not shorter, also it is in generic arm code and not i.MX specific. The old way also makes sure that we do not compile in incompatible lowlevel debug code.

At least this should be a choice in Kconfig to make clear that the different low-level debug options are exclusive.

Sascha,

Quoted 5 lines · Sascha Hauer

Quoted 3 lines · Shawn Guo

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

Quoted 4 lines

I'm unsure whether I like this. The ifdeffery does not look very good, but the Kconfig snippet is not shorter, also it is in generic arm code and not i.MX specific. The old way also makes sure that we do not compile in incompatible lowlevel debug code.

But it's an unfortunate hinderence to a single zImage kernel which we can only solve sensibly in the generic ARM code.

Quoted 3 lines

At least this should be a choice in Kconfig to make clear that the different low-level debug options are exclusive.

As Shawn pointed out, we are using a Kconfig choice so you can only select one UART for low-level debug output.

Quoted 2 lines · Will Deacon

But it's an unfortunate hinderence to a single zImage kernel which we can only solve sensibly in the generic ARM code.

My problem is that if this option is enabled the kernel will not run on any other SoC except the one being selected here, at least when earlyprintk is passed on the command line. One could argue that this option is for people who exactly know what they do only. Still there should pop up a big fat warning somewhere.

The old i.MX way ensured that by refusing to compile the kernel with multi SoC support.

Quoted 6 lines

Quoted 3 lines

At least this should be a choice in Kconfig to make clear that the different low-level debug options are exclusive.

As Shawn pointed out, we are using a Kconfig choice so you can only select one UART for low-level debug output.

Ah, ok. I didn't see that Shawns mail was a reply to your series turning this into a choice.

Quoted 4 lines · Sascha Hauer

Quoted 2 lines · Will Deacon

But it's an unfortunate hinderence to a single zImage kernel which we can only solve sensibly in the generic ARM code.

Quoted 5 lines

My problem is that if this option is enabled the kernel will not run on any other SoC except the one being selected here, at least when earlyprintk is passed on the command line. One could argue that this option is for people who exactly know what they do only. Still there should pop up a big fat warning somewhere.

Could do - but where?

Quoted 3 lines

The old i.MX way ensured that by refusing to compile the kernel with multi SoC support.

That's a worse solution, because now the kernel doesn't run *anywhere* if you enable DEBUG_LL and multiple SoCs.

A use case for the new method is that
you have a kernel .config file for a multi-SoC kernel which is known not to fail early on a given board. You can enable DEBUG_LL for that board and solve the problem. It should never be enabled for a kernel that is designed to be portable!

Quoted 13 lines · Will Deacon

Sascha,

Quoted 9 lines · Sascha Hauer

Quoted 3 lines · Shawn Guo

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

I'm unsure whether I like this. The ifdeffery does not look very good, but the Kconfig snippet is not shorter, also it is in generic arm code and not i.MX specific. The old way also makes sure that we do not compile in incompatible lowlevel debug code.

Quoted 2 lines

But it's an unfortunate hinderence to a single zImage kernel which we can only solve sensibly in the generic ARM code.

My problem is that if this option is enabled the kernel will not run on any other SoC except the one being selected here, at least when earlyprintk is passed on the command line.

One could argue
that this option is for people who exactly know what they do only.

Quoted 2 lines · Sascha Hauer

One could argue
that this option is for people who exactly know what they do only.

It _IS_ there for people who know what they're doing. That's something I keep on saying about the LL debug stuff. It's there to allow people to debug the early startup of the kernel. It's not there for users.

Quoted 14 lines · Sascha Hauer

Quoted 13 lines · Will Deacon

Sascha,

Quoted 9 lines · Sascha Hauer

Quoted 3 lines · Shawn Guo

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

I'm unsure whether I like this. The ifdeffery does not look very good, but the Kconfig snippet is not shorter, also it is in generic arm code and not i.MX specific. The old way also makes sure that we do not compile in incompatible lowlevel debug code.

Quoted 6 lines

Quoted 2 lines

But it's an unfortunate hinderence to a single zImage kernel which we can only solve sensibly in the generic ARM code.

My problem is that if this option is enabled the kernel will not run on any other SoC except the one being selected here, at least when earlyprintk is passed on the command line.

I never liked the idea of coupling this into earlyprintk - and I think I said so at the time. I'll say it again:

The LL DEBUG stuff is there to be able to do low level "it won't boot" debugging. It's not there as a user option. You are supposed to know exactly what you are doing when using the option.

If we're going to start having earlyprintk be an argument against this, I'll simply rip out the earlyprintk coupling to LL debug, and people can go back to patching printk.c to make this work.

Quoted 2 lines

One could argue
that this option is for people who exactly know what they do only.

It _IS_ there for people who know what they're doing. That's something I keep on saying about the LL debug stuff. It's there to allow people to debug the early startup of the kernel. It's not there for users.

Quoted 7 lines · Russell King - ARM Linux

I never liked the idea of coupling this into earlyprintk - and I think I said so at the time. I'll say it again:

The LL DEBUG stuff is there to be able to do low level "it won't boot" debugging. It's not there as a user option. You are supposed to know exactly what you are doing when using the option.

Quoted 3 lines

If we're going to start having earlyprintk be an argument against this, I'll simply rip out the earlyprintk coupling to LL debug, and people can go back to patching printk.c to make this work.

But we need a functional earlyprintk. It has to be usable by simple _users_ who are not developers. ARM is not going to remain this obscur embedded architecture forever.

Quoted 6 lines

Quoted 2 lines

One could argue
that this option is for people who exactly know what they do only.

It _IS_ there for people who know what they're doing. That's something I keep on saying about the LL debug stuff. It's there to allow people to debug the early startup of the kernel. It's not there for users.

That is not sufficient.

Quoted 8 lines · Nicolas Pitre

Quoted 7 lines · Russell King - ARM Linux

I never liked the idea of coupling this into earlyprintk - and I think I said so at the time. I'll say it again:

The LL DEBUG stuff is there to be able to do low level "it won't boot" debugging. It's not there as a user option. You are supposed to know exactly what you are doing when using the option.

Quoted 7 lines

Quoted 3 lines

If we're going to start having earlyprintk be an argument against this, I'll simply rip out the earlyprintk coupling to LL debug, and people can go back to patching printk.c to make this work.

But we need a functional earlyprintk. It has to be usable by simple _users_ who are not developers. ARM is not going to remain this obscur embedded architecture forever.

I repeat: that is not the point of DEBUG_LL. DEBUG_LL is there for *developers* to debug their initial board bringup. Nothing else.

I repeat: I'm not going to have DEBUG_LL buggered up by do-gooders and people wanting BIOS shite and then have to re-invent it so that we can have decent debugging again. That's NOT going to happen. No way at all.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

One could argue
that this option is for people who exactly know what they do only.

It _IS_ there for people who know what they're doing. That's something I keep on saying about the LL debug stuff. It's there to allow people to debug the early startup of the kernel. It's not there for users.

Quoted 1 line

That is not sufficient.

Tough. That's the way it is and it has to be lived with. Early board bring up has absolutely nothing to do with users. If users are doing early board bringup, they are kernel developers by definition.

Quoted 3 lines · Russell King - ARM Linux

Tough. That's the way it is and it has to be lived with. Early board bring up has absolutely nothing to do with users. If users are doing early board bringup, they are kernel developers by definition.

So be it if you insist. We'll then have to invent a better infrastructure in parallel to be able to do early enough serial output for users, both for earlyprintk and the zImage decompressor.

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

Quoted 3 lines · Shawn Guo

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

now that this patch is in Linus' tree as f350b86121c7a004a5f866333fa1d23fe30263a6, this breaks the build when selecting DEBUG_LL_UART_NONE (as our autobuilder did somehow). Maybe that is OK because DEBUG_LL is an expert option? If not, either the autoselection has to return or DEBUG_LL_UART_NONE shouldn't be selectable for imx.

Hi Uwe,

Quoted 10 lines · Uwe Kleine-K?nig

Quoted 3 lines · Shawn Guo

Now that the DEBUG_LL UART can be selected by a Kconfig choice, simplify the #ifdefery in debug-macro.S and add entries to the top-level Kconfig.debug instead.

now that this patch is in Linus' tree as f350b86121c7a004a5f866333fa1d23fe30263a6, this breaks the build when selecting DEBUG_LL_UART_NONE (as our autobuilder did somehow). Maybe that is OK because DEBUG_LL is an expert option? If not, either the autoselection has to return or DEBUG_LL_UART_NONE shouldn't be selectable for imx.

In the end I'd like to remove DEBUG_LL_UART_NONE. It's only there to service the platforms which have not yet added an entry to the choice.

Quoted 2 lines · Will Deacon

In the end I'd like to remove DEBUG_LL_UART_NONE. It's only there to service the platforms which have not yet added an entry to the choice.

Why not remove it already now? If I remove it together with the entry for my machine and modify mach/debug-macro.S to do the right thing for my machine even without DEBUG_IMX25_UART, my build just works.

Quoted 6 lines · Uwe Kleine-K?nig

Quoted 2 lines · Will Deacon

In the end I'd like to remove DEBUG_LL_UART_NONE. It's only there to service the platforms which have not yet added an entry to the choice.

Why not remove it already now? If I remove it together with the entry for my machine and modify mach/debug-macro.S to do the right thing for my machine even without DEBUG_IMX25_UART, my build just works.

I'm fairly sure this will break the build for everybody who doesn't have an entry in Kconfig.debug choice if they try and select DEBUG_LL.

Quoted 4 lines · Will Deacon

Quoted 3 lines · Uwe Kleine-K?nig

Quoted 2 lines · Will Deacon

In the end I'd like to remove DEBUG_LL_UART_NONE. It's only there to service the platforms which have not yet added an entry to the choice.

Quoted 6 lines

Quoted 3 lines

Why not remove it already now? If I remove it together with the entry for my machine and modify mach/debug-macro.S to do the right thing for my machine even without DEBUG_IMX25_UART, my build just works.

I'm fairly sure this will break the build for everybody who doesn't have an entry in Kconfig.debug choice if they try and select DEBUG_LL.

Ah, I see the problem. It's more subtile than just failure to build: it autoselects DEBUG_ICEDCC then.

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 8 lines · Will Deacon

Quoted 6 lines · Uwe Kleine-K?nig

Quoted 2 lines · Will Deacon

In the end I'd like to remove DEBUG_LL_UART_NONE. It's only there to service the platforms which have not yet added an entry to the choice.

Why not remove it already now? If I remove it together with the entry for my machine and modify mach/debug-macro.S to do the right thing for my machine even without DEBUG_IMX25_UART, my build just works.

Quoted 4 lines

Quoted 2 lines

I'm fairly sure this will break the build for everybody who doesn't have an entry in Kconfig.debug choice if they try and select DEBUG_LL.

Ah, I see the problem. It's more subtile than just failure to build: it autoselects DEBUG_ICEDCC then.

Which results in a kernel which builds fine, but when they attempt to boot it appears to lock fairly solidly. So we had to add this option to allow the possibility for unconverted platforms to continue working.

At this point, I think the right answer is to remove the option in linux-next, and tell anyone who complains that they need to convert their platform properly. (We're going to need them converted in this way anyway for the single zImage project.)

Quoted 7 lines · Russell King - ARM Linux

Quoted 2 lines

Ah, I see the problem. It's more subtile than just failure to build: it autoselects DEBUG_ICEDCC then.

At this point, I think the right answer is to remove the option in linux-next, and tell anyone who complains that they need to convert their platform properly. (We're going to need them converted in this way anyway for the single zImage project.)

I agree.

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 7 lines · Russell King - ARM Linux

Quoted 2 lines

Ah, I see the problem. It's more subtile than just failure to build: it autoselects DEBUG_ICEDCC then.

At this point, I think the right answer is to remove the option in linux-next, and tell anyone who complains that they need to convert their platform properly. (We're going to need them converted in this way anyway for the single zImage project.)

Quoted 1 line

I agree.

It'd be nice if things could be arranged so that the build breaks rather than selecting ICEDCC on unconverted platforms - when you run into the problem it's not that easy to diagnose.

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 7 lines · Russell King - ARM Linux

Quoted 2 lines

Ah, I see the problem. It's more subtile than just failure to build: it autoselects DEBUG_ICEDCC then.

At this point, I think the right answer is to remove the option in linux-next, and tell anyone who complains that they need to convert their platform properly. (We're going to need them converted in this way anyway for the single zImage project.)

Quoted 1 line

I agree.

It'd be nice if things could be arranged so that the build breaks rather than selecting ICEDCC on unconverted platforms - when you run into the problem it's not that easy to diagnose.

Quoted 11 lines · Mark Brown

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 7 lines · Russell King - ARM Linux

Quoted 2 lines

Ah, I see the problem. It's more subtile than just failure to build: it autoselects DEBUG_ICEDCC then.

At this point, I think the right answer is to remove the option in linux-next, and tell anyone who complains that they need to convert their platform properly. (We're going to need them converted in this way anyway for the single zImage project.)

Quoted 5 lines

Quoted 1 line

I agree.

It'd be nice if things could be arranged so that the build breaks rather than selecting ICEDCC on unconverted platforms - when you run into the problem it's not that easy to diagnose.

Well, we could leave the choice as is, and make the NONE option cause a #error.

and then selecting DEBUG_LL_LEGACY from all platforms that do their own thing. That would avoid the possible randconfig errors.

Quoted 2 lines · Arnd Bergmann

and then selecting DEBUG_LL_LEGACY from all platforms that do their own thing. That would avoid the possible randconfig errors.

Your fix seems good to me, and avoids causing hassle for randconfig folks, though Russell's will be more likely to encourage people to convert I guess.

arch/arm/Kconfig.debug +11 -0

+choice
+	prompt "Kernel low-level debugging port"
+	depends on DEBUG_LL
+
+	config DEBUG_DC21285_PORT
+		bool "Kernel low-level debugging messages via footbridge serial port"
+		depends on FOOTBRIDGE
+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the serial port in the DC21285 (Footbridge).
+		  Saying N will cause the debug messages to appear on the first

Quoted 16 lines · Will Deacon

arch/arm/Kconfig.debug +16 -0

+choice
+	prompt "Kernel low-level debugging port"
+	depends on DEBUG_LL
+
+	config DEBUG_DC21285_PORT
+		bool "Kernel low-level debugging messages via footbridge serial port"
+		depends on FOOTBRIDGE
+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the serial port in the DC21285 (Footbridge).
+		  Saying N will cause the debug messages to appear on the first
+		  16550 serial port.
+
+	config DEBUG_CLPS711X_UART2
+		bool "Kernel low-level debugging messages via UART2"
+		depends on ARCH_CLPS711X

arch/arm/Kconfig.debug config DEBUG_LL · line 95

Quoted 5 lines

arch/arm/Kconfig.debug +5 -0

+		help
+		  Say Y here if you want the debug print routines to direct
+		  their output to the second serial port on these devices.
+		  Saying N will cause the debug messages to appear on the first
+		  serial port.

How would I unselect this UART2 config to get serial output on UART1? As far as I can tell there isn't an option for NONE in a choice menu.

Ah yes. This will need to be added as part of the platform updates to go via Arnd. It should be easy enough just to have a DEBUG_CLPS711X_UART1 option, for example, and the platform code will fall back to the first UART.

ARM patch 7072/1 (debug: use kconfig choice for selecting DEBUG_LL UART) only allowed CLSP711X_UART2 to be selected because there is no NONE option in a choice menu. Add a UART1 choice so that users can still choose UART1 explicitly.

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

---

Quoted 3 lines · Will Deacon

Ah yes. This will need to be added as part of the platform updates to go via Arnd. It should be easy enough just to have a DEBUG_CLPS711X_UART1 option, for example, and the platform code will fall back to the first UART.

I couldn't find these patches applied in Arnd's tree so I based it off of Russell's for-next branch.

Hi Stephen,

Quoted 12 lines · Stephen Boyd

ARM patch 7072/1 (debug: use kconfig choice for selecting DEBUG_LL UART) only allowed CLSP711X_UART2 to be selected because there is no NONE option in a choice menu. Add a UART1 choice so that users can still choose UART1 explicitly.

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

---

Quoted 3 lines · Will Deacon

Ah yes. This will need to be added as part of the platform updates to go via Arnd. It should be easy enough just to have a DEBUG_CLPS711X_UART1 option, for example, and the platform code will fall back to the first UART.

Quoted 2 lines

I couldn't find these patches applied in Arnd's tree so I based it off of Russell's for-next branch.

Thanks for cooking the patch, it was somewhere on my list of things to do this week! I think we also need to fix the DEBUG_DC21285_PORT option as that has a similar `if not selected then use a different UART' behaviour. It can be fixed in the same way as you have done in this patch.'

Would you like me to take the three patches in with the ones I currently have for other platforms (Realview, Samsung, imx) or would you prefer to handle these separately? I was planning to send all of the platform bits to Arnd once I've got my branches sorted out (been on holiday for the past two weeks).

Quoted 4 lines · Will Deacon

Thanks for cooking the patch, it was somewhere on my list of things to do this week! I think we also need to fix the DEBUG_DC21285_PORT option as that has a similar `if not selected then use a different UART' behaviour. It can be fixed in the same way as you have done in this patch.'

Ah my eyes glossed over the DC21285 one. Here's one on top of patch 2. Feel free to squash, etc.

Quoted 7 lines · Stephen Boyd

Quoted 4 lines · Will Deacon

Thanks for cooking the patch, it was somewhere on my list of things to do this week! I think we also need to fix the DEBUG_DC21285_PORT option as that has a similar `if not selected then use a different UART' behaviour. It can be fixed in the same way as you have done in this patch.'

Ah my eyes glossed over the DC21285 one. Here's one on top of patch 2. Feel free to squash, etc.

Err, the DC21285 only has one serial port.

Some footbridge platforms expose the DC21285 to the outside world, others don't. Some footbridge platforms have a separate 8250 UART at the standard PCI COM1 address, others don't.

What DEBUG_DC21285_PORT is selecting between is whether to use the DC21285 port (when set) or the 8250 at PCI COM1 (when unset).

Ah ok. How about this instead?

Quoted 7 lines · Will Deacon

Would you like me to take the three patches in with the ones I currently have for other platforms (Realview, Samsung, imx) or would you prefer to handle these separately? I was planning to send all of the platform bits to Arnd once I've got my branches sorted out (been on holiday for the past two weeks).

If you want to handle them that sounds fine. I can't figure out who is coordinating the DEBUG_LL changes. It seems that Russell has at least applied the initial DEBUG_LL patches, so his tree will have some breakage without this fixup patch.

I have some more patches on top of this one for MSM DEBUG_LL support too. Hopefully I can send them out in a little bit.