| Message ID | 1313729819-30301-1-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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.
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
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
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.
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
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.
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
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(-)