arm/imx: use Kconfig choice for low-level debug UART selection
diff mbox

Message ID 1313729819-30301-1-git-send-email-shawn.guo@linaro.org
State New, archived
Headers show

Commit Message

Shawn Guo Aug. 19, 2011, 4:56 a.m. UTC
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 <shawn.guo@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
---
 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(-)

Comments

Will Deacon Aug. 19, 2011, 11:09 a.m. UTC | #1
Sascha,

On Fri, Aug 19, 2011 at 07:35:33AM +0100, Sascha Hauer wrote:
> On Fri, Aug 19, 2011 at 12:56:59PM +0800, Shawn Guo wrote:
> > 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.

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

> 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.

Will
Sascha Hauer Aug. 19, 2011, 11:39 a.m. UTC | #2
On Fri, Aug 19, 2011 at 12:09:41PM +0100, Will Deacon wrote:
> Sascha,
> 
> On Fri, Aug 19, 2011 at 07:35:33AM +0100, Sascha Hauer wrote:
> > On Fri, Aug 19, 2011 at 12:56:59PM +0800, Shawn Guo wrote:
> > > 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.
> 
> 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.

> 
> > 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.

Sascha
Will Deacon Aug. 19, 2011, 12:35 p.m. UTC | #3
On Fri, Aug 19, 2011 at 12:39:37PM +0100, Sascha Hauer wrote:
> On Fri, Aug 19, 2011 at 12:09:41PM +0100, Will Deacon wrote:
> > 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.

Could do - but where?

> 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!

> > 
> > > 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.

Yup - the idea is to force a single UART definition when DEBUG_LL is
enabled.

Will
Sascha Hauer Aug. 19, 2011, 5:15 p.m. UTC | #4
On Fri, Aug 19, 2011 at 01:35:43PM +0100, Will Deacon wrote:
> On Fri, Aug 19, 2011 at 12:39:37PM +0100, Sascha Hauer wrote:
> > On Fri, Aug 19, 2011 at 12:09:41PM +0100, Will Deacon wrote:
> > > 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.
> 
> Could do - but where?

Good question, I don't know.

> 
> > 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.

Better a kernel that does not compile than a kernel that compiles and
does not run.

> 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!

Indeed.

How about the something like the following?

config DEBUG_LL
        bool "Kernel low-level debugging functions (read help!)"
        depends on DEBUG_KERNEL
        help
          Say Y here to include definitions of printascii, printch,printhex
          in the kernel.  This is helpful if you are debugging code that
          executes before the console is initialized.
	  The UART lowlevel functions will limit the kernel to exactly
	  the system specified below. Trying to use these on any other
	  system will lock up the kernel or even worse.

Sascha
Russell King - ARM Linux Aug. 21, 2011, 9:18 a.m. UTC | #5
On Fri, Aug 19, 2011 at 01:39:37PM +0200, Sascha Hauer wrote:
> On Fri, Aug 19, 2011 at 12:09:41PM +0100, Will Deacon wrote:
> > Sascha,
> > 
> > On Fri, Aug 19, 2011 at 07:35:33AM +0100, Sascha Hauer wrote:
> > > On Fri, Aug 19, 2011 at 12:56:59PM +0800, Shawn Guo wrote:
> > > > 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.
> > 
> > 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.

> 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.
Will Deacon Aug. 21, 2011, 11:25 a.m. UTC | #6
On Sun, Aug 21, 2011 at 10:18:51AM +0100, Russell King - ARM Linux wrote:
> On Fri, Aug 19, 2011 at 01:39:37PM +0200, Sascha Hauer wrote:
> > 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.

Understood. I don't think it hurts to make this explicit in the Kconfig help
entry for DEBUG_LL (as Sascha suggested) so I'll do that for v2 of this
series.

Will
Nicolas Pitre Aug. 21, 2011, 5:59 p.m. UTC | #7
On Sun, 21 Aug 2011, Russell King - ARM Linux wrote:

> On Fri, Aug 19, 2011 at 01:39:37PM +0200, Sascha Hauer wrote:
> > On Fri, Aug 19, 2011 at 12:09:41PM +0100, Will Deacon wrote:
> > > Sascha,
> > > 
> > > On Fri, Aug 19, 2011 at 07:35:33AM +0100, Sascha Hauer wrote:
> > > > On Fri, Aug 19, 2011 at 12:56:59PM +0800, Shawn Guo wrote:
> > > > > 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.
> > > 
> > > 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.

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.

> > 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.


Nicolas
Russell King - ARM Linux Aug. 21, 2011, 6:17 p.m. UTC | #8
On Sun, Aug 21, 2011 at 01:59:44PM -0400, Nicolas Pitre wrote:
> On Sun, 21 Aug 2011, Russell King - ARM Linux wrote:
> > 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.
> 
> 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.

> > > 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.

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.
Nicolas Pitre Aug. 21, 2011, 6:28 p.m. UTC | #9
On Sun, 21 Aug 2011, Russell King - ARM Linux wrote:

> On Sun, Aug 21, 2011 at 01:59:44PM -0400, Nicolas Pitre wrote:
> > On Sun, 21 Aug 2011, Russell King - ARM Linux wrote:
> > > 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.
> > 
> > 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.
> 
> > > > 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.
> 
> 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.


Nicolas
Russell King - ARM Linux Aug. 21, 2011, 6:33 p.m. UTC | #10
On Sun, Aug 21, 2011 at 02:28:46PM -0400, Nicolas Pitre wrote:
> On Sun, 21 Aug 2011, Russell King - ARM Linux wrote:
> > 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.

"Users" have nothing to do with it.

In any case, that will probably be _much_ better because it can be
independent of the debugging code to be used for sort out the *assembly*
part of the kernel boot.

Patch
diff mbox

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index f23975a..947adef 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -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"
diff --git a/arch/arm/mach-mxs/include/mach/debug-macro.S b/arch/arm/mach-mxs/include/mach/debug-macro.S
index 79650a1..6d98704 100644
--- a/arch/arm/mach-mxs/include/mach/debug-macro.S
+++ b/arch/arm/mach-mxs/include/mach/debug-macro.S
@@ -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
 
diff --git a/arch/arm/plat-mxc/include/mach/debug-macro.S b/arch/arm/plat-mxc/include/mach/debug-macro.S
index e4dde91..07cfdbe 100644
--- a/arch/arm/plat-mxc/include/mach/debug-macro.S
+++ b/arch/arm/plat-mxc/include/mach/debug-macro.S
@@ -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