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

login
register
mail settings
Submitter Shawn Guo
Date Aug. 19, 2011, 4:56 a.m.
Message ID <1313729819-30301-1-git-send-email-shawn.guo@linaro.org>
Download mbox | patch
Permalink /patch/110565/
State New
Headers show

Comments

Shawn Guo - Aug. 19, 2011, 4:56 a.m.
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(-)
Sascha Hauer - Aug. 19, 2011, 6:35 a.m.
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
> 
>
Shawn Guo - Aug. 19, 2011, 7 a.m.
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.
Will Deacon - Aug. 19, 2011, 11:09 a.m.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Uwe Kleine-König - Nov. 22, 2011, 8:58 a.m.
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
Will Deacon - Nov. 22, 2011, 11:50 a.m.
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
Uwe Kleine-König - Nov. 22, 2011, 1:02 p.m.
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
Will Deacon - Nov. 22, 2011, 1:20 p.m.
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
Uwe Kleine-König - Nov. 22, 2011, 1:30 p.m.
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
Russell King - ARM Linux - Nov. 22, 2011, 3:48 p.m.
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.)
Uwe Kleine-König - Nov. 22, 2011, 4:38 p.m.
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
Mark Brown - Nov. 22, 2011, 4:47 p.m.
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.
Russell King - ARM Linux - Nov. 22, 2011, 8:24 p.m.
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.
Mark Brown - Nov. 22, 2011, 11 p.m.
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.
Will Deacon - Nov. 23, 2011, 10:57 a.m.
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

Patch

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