| Message ID | 1313729819-30301-1-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. At least this should be a choice in Kconfig to make clear that the different low-level debug options are exclusive. Sascha > > 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(-) > > 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 > > -- > 1.7.4.1 > >
On Fri, Aug 19, 2011 at 08:35:33AM +0200, 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, It does not make the code any worse. Instead, the ifdeffery becomes shorter actually. > but the Kconfig snippet is not shorter, also it is in generic arm code > and not i.MX specific. This comment is on the whole approach proposed by Will, I guess. But I did not see any better alternatives until we can have device tree into such a early boot stage, which is not possible for now. > 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. > It is a choice.
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.
Hello Shawn, 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. 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. Best regards Uwe
Hi Uwe, On Tue, Nov 22, 2011 at 08:58:34AM +0000, Uwe Kleine-König 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. > 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. Will
On Tue, Nov 22, 2011 at 11:50:17AM +0000, Will Deacon wrote: > Hi Uwe, > > On Tue, Nov 22, 2011 at 08:58:34AM +0000, Uwe Kleine-König 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. > > 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. 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. Best regards Uwe
On Tue, Nov 22, 2011 at 01:02:33PM +0000, Uwe Kleine-König wrote: > On Tue, Nov 22, 2011 at 11:50:17AM +0000, Will Deacon wrote: > > 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. Will
On Tue, Nov 22, 2011 at 01:20:55PM +0000, Will Deacon wrote: > On Tue, Nov 22, 2011 at 01:02:33PM +0000, Uwe Kleine-König wrote: > > On Tue, Nov 22, 2011 at 11:50:17AM +0000, Will Deacon wrote: > > > 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. Ah, I see the problem. It's more subtile than just failure to build: it autoselects DEBUG_ICEDCC then. Best regards Uwe
On Tue, Nov 22, 2011 at 02:30:40PM +0100, Uwe Kleine-König wrote: > On Tue, Nov 22, 2011 at 01:20:55PM +0000, Will Deacon wrote: > > On Tue, Nov 22, 2011 at 01:02:33PM +0000, Uwe Kleine-König wrote: > > > On Tue, Nov 22, 2011 at 11:50:17AM +0000, Will Deacon wrote: > > > > 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. > 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.)
On Tue, Nov 22, 2011 at 03:48:38PM +0000, Russell King - ARM Linux wrote: > On Tue, Nov 22, 2011 at 02:30:40PM +0100, Uwe Kleine-König wrote: > > On Tue, Nov 22, 2011 at 01:20:55PM +0000, Will Deacon wrote: > > > On Tue, Nov 22, 2011 at 01:02:33PM +0000, Uwe Kleine-König wrote: > > > > On Tue, Nov 22, 2011 at 11:50:17AM +0000, Will Deacon wrote: > > > > > 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. > > 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. yes. > 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. Best regards Uwe
On Tue, Nov 22, 2011 at 05:38:12PM +0100, Uwe Kleine-K?nig wrote: > On Tue, Nov 22, 2011 at 03:48:38PM +0000, Russell King - ARM Linux wrote: > > > 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. 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.
On Tue, Nov 22, 2011 at 04:47:07PM +0000, Mark Brown wrote: > On Tue, Nov 22, 2011 at 05:38:12PM +0100, Uwe Kleine-K?nig wrote: > > On Tue, Nov 22, 2011 at 03:48:38PM +0000, Russell King - ARM Linux wrote: > > > > > 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. > > 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.
On Tue, Nov 22, 2011 at 10:19:53PM +0100, Arnd Bergmann wrote: > 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.
On Tue, Nov 22, 2011 at 11:00:31PM +0000, Mark Brown wrote: > On Tue, Nov 22, 2011 at 10:19:53PM +0100, Arnd Bergmann wrote: > > > 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. Although Russell's approach is a build breaker for those platforms that have not yet made the switch, that does provide an incentive for the platform maintainers to update their code. If we settle for a legacy solution it will probably sit around forever. Given that we're now past -rc2, it might be best to wait until the next merge window to introduce the #error logic, so the LEGACY solution could be a stopgap for 3.2. Will
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(-)