[PATCH] msm: fix debug-macro.S build failure

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

Originally there was an ifdef case to handle when no debug uart
was selected. In commit 0ea1293009826da45e1019f45dfde1e557bb30df
that case was removed which causes the following build failure,

linux-2.6/arch/arm/kernel/debug.S: Assembler messages:
linux-2.6/arch/arm/kernel/debug.S:174: Error: bad instruction `addruart r1,r2'
linux-2.6/arch/arm/kernel/debug.S:176: Error: bad instruction `waituart r2,r3'
linux-2.6/arch/arm/kernel/debug.S:177: Error: bad instruction `senduart r1,r3'
linux-2.6/arch/arm/kernel/debug.S:178: Error: bad instruction `busyuart r2,r3'
linux-2.6/arch/arm/kernel/debug.S:190: Error: bad instruction `addruart r1,r2'

This is a partial revert to add back the case which was removed.

Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Jason Wang <jason77.wang@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Nicolas Pitre <nico@fluxnic.net>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 arch/arm/mach-msm/include/mach/debug-macro.S |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-msm/include/mach/debug-macro.S b/arch/arm/mach-msm/include/mach/debug-macro.S
index fbd5d90..7796d8c 100644
--- a/arch/arm/mach-msm/include/mach/debug-macro.S
+++ b/arch/arm/mach-msm/include/mach/debug-macro.S
@@ -36,7 +36,16 @@
 	tst	\rd, #0x04
 	beq	1001b
 	.endm
+#else
+	.macro	addruart, rx, tmp
+	.endm
 
-	.macro	busyuart,rd,rx
+	.macro	senduart,rd,rx
+	.endm
+
+	.macro	waituart,rd,rx
 	.endm
 #endif
+
+	.macro	busyuart,rd,rx
+	.endm
-- 
1.7.0.4

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] msm: fix debug-macro.S build failure

[Russell King - ARM Linux]

On Wed, Oct 27, 2010 at 02:58:08PM -0700, Daniel Walker wrote:
> Originally there was an ifdef case to handle when no debug uart
> was selected. In commit 0ea1293009826da45e1019f45dfde1e557bb30df
> that case was removed which causes the following build failure,
> 
> linux-2.6/arch/arm/kernel/debug.S: Assembler messages:
> linux-2.6/arch/arm/kernel/debug.S:174: Error: bad instruction `addruart r1,r2'
> linux-2.6/arch/arm/kernel/debug.S:176: Error: bad instruction `waituart r2,r3'
> linux-2.6/arch/arm/kernel/debug.S:177: Error: bad instruction `senduart r1,r3'
> linux-2.6/arch/arm/kernel/debug.S:178: Error: bad instruction `busyuart r2,r3'
> linux-2.6/arch/arm/kernel/debug.S:190: Error: bad instruction `addruart r1,r2'
> 
> This is a partial revert to add back the case which was removed.

It wants fixing properly, not just reverting.  Even though it's only
a minor difference, it's important to reflect the API in assembly
macros, especially when it has changed.

> +#else
> +	.macro	addruart, rx, tmp

addruart is now expected to return two values, and 'tmp' ends up being
misleading.  This is a recipe for mistakes unless this is corrected.
It should be 'rp, rv' instead of 'rx, tmp'.

[PATCH -v2] msm: fix debug-macro.S build failure

[Daniel Walker]

Originally there was an ifdef case to handle when no debug uart
was selected. In commit 0ea1293009826da45e1019f45dfde1e557bb30df
that case was removed which causes the following build failure,

/local/mnt/workspace/c_dwalke/git-trees/linux-2.6/arch/arm/kernel/debug.S: Assembler messages:
/local/mnt/workspace/c_dwalke/git-trees/linux-2.6/arch/arm/kernel/debug.S:174: Error: bad instruction `addruart r1,r2'
/local/mnt/workspace/c_dwalke/git-trees/linux-2.6/arch/arm/kernel/debug.S:176: Error: bad instruction `waituart r2,r3'
/local/mnt/workspace/c_dwalke/git-trees/linux-2.6/arch/arm/kernel/debug.S:177: Error: bad instruction `senduart r1,r3'
/local/mnt/workspace/c_dwalke/git-trees/linux-2.6/arch/arm/kernel/debug.S:178: Error: bad instruction `busyuart r2,r3'
/local/mnt/workspace/c_dwalke/git-trees/linux-2.6/arch/arm/kernel/debug.S:190: Error: bad instruction `addruart r1,r2'

This is a partial revert to add back the case which was removed, with
one minor API change in addruart name the return values "rp, rv".

Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Jason Wang <jason77.wang@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Nicolas Pitre <nico@fluxnic.net>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 arch/arm/mach-msm/include/mach/debug-macro.S |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-msm/include/mach/debug-macro.S b/arch/arm/mach-msm/include/mach/debug-macro.S
index fbd5d90..d726eda 100644
--- a/arch/arm/mach-msm/include/mach/debug-macro.S
+++ b/arch/arm/mach-msm/include/mach/debug-macro.S
@@ -36,7 +36,16 @@
 	tst	\rd, #0x04
 	beq	1001b
 	.endm
+#else
+	.macro	addruart, rp, rv
+	.endm
 
-	.macro	busyuart,rd,rx
+	.macro	senduart,rd,rx
+	.endm
+
+	.macro	waituart,rd,rx
 	.endm
 #endif
+
+	.macro	busyuart,rd,rx
+	.endm
-- 
1.7.0.4

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Wed, 27 Oct 2010, Russell King - ARM Linux wrote:

> > +#else
> > +	.macro	addruart, rx, tmp
> 
> addruart is now expected to return two values, and 'tmp' ends up being
> misleading.  This is a recipe for mistakes unless this is corrected.
> It should be 'rp, rv' instead of 'rx, tmp'.

Also, it is probably not a good idea to return nothing. Otherwise the 
code using those macros will then work on random values that just 
happened to be in the corresponding register at the call location.


Nicolas

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

On Wed, 2010-10-27 at 22:30 -0400, Nicolas Pitre wrote:
> On Wed, 27 Oct 2010, Russell King - ARM Linux wrote:
> 
> > > +#else
> > > +	.macro	addruart, rx, tmp
> > 
> > addruart is now expected to return two values, and 'tmp' ends up being
> > misleading.  This is a recipe for mistakes unless this is corrected.
> > It should be 'rp, rv' instead of 'rx, tmp'.
> 
> Also, it is probably not a good idea to return nothing. Otherwise the 
> code using those macros will then work on random values that just 
> happened to be in the corresponding register at the call location.

Should we have something in generic arm code that allows this to just be
turned off? Ideally we don't want any of this stuff even running.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] msm: fix debug-macro.S build failure

[Russell King - ARM Linux]

On Thu, Oct 28, 2010 at 09:30:13AM -0700, Daniel Walker wrote:
> On Wed, 2010-10-27 at 22:30 -0400, Nicolas Pitre wrote:
> > On Wed, 27 Oct 2010, Russell King - ARM Linux wrote:
> > 
> > > > +#else
> > > > +	.macro	addruart, rx, tmp
> > > 
> > > addruart is now expected to return two values, and 'tmp' ends up being
> > > misleading.  This is a recipe for mistakes unless this is corrected.
> > > It should be 'rp, rv' instead of 'rx, tmp'.
> > 
> > Also, it is probably not a good idea to return nothing. Otherwise the 
> > code using those macros will then work on random values that just 
> > happened to be in the corresponding register at the call location.
> 
> Should we have something in generic arm code that allows this to just be
> turned off? Ideally we don't want any of this stuff even running.

If you don't want it, don't enable DEBUG_LL.  DEBUG_LL is what you
enable for initial board bring-up and once you're getting kernel
messages via standard console drivers, you disable it.

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

On Thu, 2010-10-28 at 17:35 +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 28, 2010 at 09:30:13AM -0700, Daniel Walker wrote:
> > On Wed, 2010-10-27 at 22:30 -0400, Nicolas Pitre wrote:
> > > On Wed, 27 Oct 2010, Russell King - ARM Linux wrote:
> > > 
> > > > > +#else
> > > > > +	.macro	addruart, rx, tmp
> > > > 
> > > > addruart is now expected to return two values, and 'tmp' ends up being
> > > > misleading.  This is a recipe for mistakes unless this is corrected.
> > > > It should be 'rp, rv' instead of 'rx, tmp'.
> > > 
> > > Also, it is probably not a good idea to return nothing. Otherwise the 
> > > code using those macros will then work on random values that just 
> > > happened to be in the corresponding register at the call location.
> > 
> > Should we have something in generic arm code that allows this to just be
> > turned off? Ideally we don't want any of this stuff even running.
> 
> If you don't want it, don't enable DEBUG_LL.  DEBUG_LL is what you
> enable for initial board bring-up and once you're getting kernel
> messages via standard console drivers, you disable it.

This board doesn't have the ability to support DEBUG_LL .. I don't want
the user to have the option to even select that.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] msm: fix debug-macro.S build failure

[Russell King - ARM Linux]

On Thu, Oct 28, 2010 at 09:41:06AM -0700, Daniel Walker wrote:
> On Thu, 2010-10-28 at 17:35 +0100, Russell King - ARM Linux wrote:
> > On Thu, Oct 28, 2010 at 09:30:13AM -0700, Daniel Walker wrote:
> > > On Wed, 2010-10-27 at 22:30 -0400, Nicolas Pitre wrote:
> > > > On Wed, 27 Oct 2010, Russell King - ARM Linux wrote:
> > > > 
> > > > > > +#else
> > > > > > +	.macro	addruart, rx, tmp
> > > > > 
> > > > > addruart is now expected to return two values, and 'tmp' ends up being
> > > > > misleading.  This is a recipe for mistakes unless this is corrected.
> > > > > It should be 'rp, rv' instead of 'rx, tmp'.
> > > > 
> > > > Also, it is probably not a good idea to return nothing. Otherwise the 
> > > > code using those macros will then work on random values that just 
> > > > happened to be in the corresponding register at the call location.
> > > 
> > > Should we have something in generic arm code that allows this to just be
> > > turned off? Ideally we don't want any of this stuff even running.
> > 
> > If you don't want it, don't enable DEBUG_LL.  DEBUG_LL is what you
> > enable for initial board bring-up and once you're getting kernel
> > messages via standard console drivers, you disable it.
> 
> This board doesn't have the ability to support DEBUG_LL .. I don't want
> the user to have the option to even select that.

There is no such option present - and there never has been.  This raises
the question about what you did before with .phys_io and .io_pg_offst
on these platforms which can't support it at all.

Effectively, rp is what you would've returned as .phys_io, and rv is
the virtual address corresponding to .io_pg_offst - so just replicate
that behaviour.  But do not leave this function empty otherwise if you
enable DEBUG_LL, you'll get unpredictable behaviour.

All that you'll then have is a temporary mapping which is torn-down
when the kernel initializes the page tables properly.

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

On Thu, 2010-10-28 at 18:46 +0100, Russell King - ARM Linux wrote:

> There is no such option present - and there never has been.  This raises
> the question about what you did before with .phys_io and .io_pg_offst
> on these platforms which can't support it at all.

We had this in some of the boards (or something like it),

#ifdef CONFIG_MSM_DEBUG_UART
        .phys_io        = MSM_DEBUG_UART_PHYS,
        .io_pg_offst    = ((MSM_DEBUG_UART_BASE) >> 18) & 0xfffc,
#endif 

and the code in debug-macro.S had similar ifdefs.

I should change what I said in the last email. It's not that these
boards can't support this, it's that they can't support it _right now_.
It's something we can add, but haven't yet.

> Effectively, rp is what you would've returned as .phys_io, and rv is
> the virtual address corresponding to .io_pg_offst - so just replicate
> that behaviour.  But do not leave this function empty otherwise if you
> enable DEBUG_LL, you'll get unpredictable behaviour.

This is what the function currently has,

        .macro  addruart, rp, rv
        ldr     \rp, =MSM_DEBUG_UART_PHYS
        ldr     \rv, =MSM_DEBUG_UART_BASE
        .endm

So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
returning it. We don't actually have those values for all the boards
tho. My understanding was that there are some generic arm changes
needed, but I need to confirm that.

Daniel


-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Thu, 28 Oct 2010, Daniel Walker wrote:

> This is what the function currently has,
> 
>         .macro  addruart, rp, rv
>         ldr     \rp, =MSM_DEBUG_UART_PHYS
>         ldr     \rv, =MSM_DEBUG_UART_BASE
>         .endm
> 
> So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> returning it. We don't actually have those values for all the boards
> tho. My understanding was that there are some generic arm changes
> needed, but I need to confirm that.

Just return 0 in both registers when you have nothing better to return.


Nicolas

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

On Thu, 2010-10-28 at 14:24 -0400, Nicolas Pitre wrote:
> On Thu, 28 Oct 2010, Daniel Walker wrote:
> 
> > This is what the function currently has,
> > 
> >         .macro  addruart, rp, rv
> >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> >         ldr     \rv, =MSM_DEBUG_UART_BASE
> >         .endm
> > 
> > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > returning it. We don't actually have those values for all the boards
> > tho. My understanding was that there are some generic arm changes
> > needed, but I need to confirm that.
> 
> Just return 0 in both registers when you have nothing better to return.

What about the other functions ?

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] msm: fix debug-macro.S build failure

[Russell King - ARM Linux]

On Thu, Oct 28, 2010 at 11:27:38AM -0700, Daniel Walker wrote:
> On Thu, 2010-10-28 at 14:24 -0400, Nicolas Pitre wrote:
> > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > 
> > > This is what the function currently has,
> > > 
> > >         .macro  addruart, rp, rv
> > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > >         .endm
> > > 
> > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > returning it. We don't actually have those values for all the boards
> > > tho. My understanding was that there are some generic arm changes
> > > needed, but I need to confirm that.
> > 
> > Just return 0 in both registers when you have nothing better to return.
> 
> What about the other functions ?

If you don't populate the other macros, you're only loading an address
into a register which won't be used.

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:

> On Thu, Oct 28, 2010 at 11:27:38AM -0700, Daniel Walker wrote:
> > On Thu, 2010-10-28 at 14:24 -0400, Nicolas Pitre wrote:
> > > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > > 
> > > > This is what the function currently has,
> > > > 
> > > >         .macro  addruart, rp, rv
> > > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > > >         .endm
> > > > 
> > > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > > returning it. We don't actually have those values for all the boards
> > > > tho. My understanding was that there are some generic arm changes
> > > > needed, but I need to confirm that.
> > > 
> > > Just return 0 in both registers when you have nothing better to return.
> > 
> > What about the other functions ?
> 
> If you don't populate the other macros, you're only loading an address
> into a register which won't be used.

DOH, indeed.  Nevermind my previous comment.  Still, skipping the 
mapping when given address is 0 could make sense too, instead of a magic 
0xfff00000.


Nicolas

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Thu, 28 Oct 2010, Daniel Walker wrote:

> On Thu, 2010-10-28 at 14:24 -0400, Nicolas Pitre wrote:
> > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > 
> > > This is what the function currently has,
> > > 
> > >         .macro  addruart, rp, rv
> > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > >         .endm
> > > 
> > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > returning it. We don't actually have those values for all the boards
> > > tho. My understanding was that there are some generic arm changes
> > > needed, but I need to confirm that.
> > 
> > Just return 0 in both registers when you have nothing better to return.
> 
> What about the other functions ?

We can test for rv == 0 and bail out early with a "mov pc, lr".


Nicolas

[PATCH] msm: fix debug-macro.S build failure

[Russell King - ARM Linux]

On Thu, Oct 28, 2010 at 02:24:33PM -0400, Nicolas Pitre wrote:
> On Thu, 28 Oct 2010, Daniel Walker wrote:
> 
> > This is what the function currently has,
> > 
> >         .macro  addruart, rp, rv
> >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> >         ldr     \rv, =MSM_DEBUG_UART_BASE
> >         .endm
> > 
> > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > returning it. We don't actually have those values for all the boards
> > tho. My understanding was that there are some generic arm changes
> > needed, but I need to confirm that.
> 
> Just return 0 in both registers when you have nothing better to return.

That's not a good idea - it'll cause 512MB of 1:1 mappings to be setup
at virtual location 0 using the IO flags, which may conflict on ARMv6+.
A better idea would be to return 0xfff00000, which'll cause it to only
populate the top-most 1MB.

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:

> On Thu, Oct 28, 2010 at 02:24:33PM -0400, Nicolas Pitre wrote:
> > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > 
> > > This is what the function currently has,
> > > 
> > >         .macro  addruart, rp, rv
> > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > >         .endm
> > > 
> > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > returning it. We don't actually have those values for all the boards
> > > tho. My understanding was that there are some generic arm changes
> > > needed, but I need to confirm that.
> > 
> > Just return 0 in both registers when you have nothing better to return.
> 
> That's not a good idea - it'll cause 512MB of 1:1 mappings to be setup
> at virtual location 0 using the IO flags, which may conflict on ARMv6+.
> A better idea would be to return 0xfff00000, which'll cause it to only
> populate the top-most 1MB.

Given that this a phony address, better test for 0 explicitly and skip 
the mapping as well as bailing out early from putchar, etc.


Nicolas

[PATCH] msm: fix debug-macro.S build failure

[Russell King - ARM Linux]

On Thu, Oct 28, 2010 at 02:43:16PM -0400, Nicolas Pitre wrote:
> On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:
> 
> > On Thu, Oct 28, 2010 at 02:24:33PM -0400, Nicolas Pitre wrote:
> > > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > > 
> > > > This is what the function currently has,
> > > > 
> > > >         .macro  addruart, rp, rv
> > > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > > >         .endm
> > > > 
> > > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > > returning it. We don't actually have those values for all the boards
> > > > tho. My understanding was that there are some generic arm changes
> > > > needed, but I need to confirm that.
> > > 
> > > Just return 0 in both registers when you have nothing better to return.
> > 
> > That's not a good idea - it'll cause 512MB of 1:1 mappings to be setup
> > at virtual location 0 using the IO flags, which may conflict on ARMv6+.
> > A better idea would be to return 0xfff00000, which'll cause it to only
> > populate the top-most 1MB.
> 
> Given that this a phony address, better test for 0 explicitly and skip 
> the mapping as well as bailing out early from putchar, etc.

That could be 0 phys, which given there is no defined memory layout on
ARM, I would not put it past someone to put a UART at phys location 0
one day.

As you can wire ARM CPUs to boot with highvecs enabled, this is something
that could realistically happen, with the boot rom at the high address.

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:

> On Thu, Oct 28, 2010 at 02:43:16PM -0400, Nicolas Pitre wrote:
> > On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:
> > 
> > > On Thu, Oct 28, 2010 at 02:24:33PM -0400, Nicolas Pitre wrote:
> > > > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > > > 
> > > > > This is what the function currently has,
> > > > > 
> > > > >         .macro  addruart, rp, rv
> > > > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > > > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > > > >         .endm
> > > > > 
> > > > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > > > returning it. We don't actually have those values for all the boards
> > > > > tho. My understanding was that there are some generic arm changes
> > > > > needed, but I need to confirm that.
> > > > 
> > > > Just return 0 in both registers when you have nothing better to return.
> > > 
> > > That's not a good idea - it'll cause 512MB of 1:1 mappings to be setup
> > > at virtual location 0 using the IO flags, which may conflict on ARMv6+.
> > > A better idea would be to return 0xfff00000, which'll cause it to only
> > > populate the top-most 1MB.
> > 
> > Given that this a phony address, better test for 0 explicitly and skip 
> > the mapping as well as bailing out early from putchar, etc.
> 
> That could be 0 phys, which given there is no defined memory layout on
> ARM, I would not put it past someone to put a UART at phys location 0
> one day.

Who knows.  But in this case I think it is probably cleaner to just care 
about the virtual address, and do something like this:

diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index dd6b369a..1174880 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -216,7 +216,8 @@ __create_page_tables:
 	 */
 	addruart r7, r3
 
-	mov	r3, r3, lsr #20
+	movs	r3, r3, lsr #20
+	beq	2f
 	mov	r3, r3, lsl #2
 
 	add	r0, r4, r3
@@ -231,7 +232,7 @@ __create_page_tables:
 	add	r3, r3, #1 << 20
 	teq	r0, r6
 	bne	1b
-
+2:
 #else /* CONFIG_DEBUG_ICEDCC */
 	/* we don't need any serial debugging mappings for ICEDCC */
 	ldr	r7, [r10, #PROCINFO_IO_MMUFLAGS] @ io_mmuflags
diff --git a/arch/arm/mach-msm/include/mach/debug-macro.S b/arch/arm/mach-msm/include/mach/debug-macro.S
index fbd5d90..d8ea859 100644
--- a/arch/arm/mach-msm/include/mach/debug-macro.S
+++ b/arch/arm/mach-msm/include/mach/debug-macro.S
@@ -39,4 +39,18 @@
 
 	.macro	busyuart,rd,rx
 	.endm
+
+#else
+
+	.macro	addruart, rp, rv
+	mov	\rv, #0
+	.endm
+
+	.macro	senduart,rd,rx
+	.endm
+	.macro	waituart,rd,rx
+	.endm
+	.macro	busyuart,rd,rx
+	.endm
+
 #endif


Nicolas

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

On Thu, 2010-10-28 at 23:03 -0400, Nicolas Pitre wrote:
> On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:
> 
> > On Thu, Oct 28, 2010 at 02:43:16PM -0400, Nicolas Pitre wrote:
> > > On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:
> > > 
> > > > On Thu, Oct 28, 2010 at 02:24:33PM -0400, Nicolas Pitre wrote:
> > > > > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > > > > 
> > > > > > This is what the function currently has,
> > > > > > 
> > > > > >         .macro  addruart, rp, rv
> > > > > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > > > > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > > > > >         .endm
> > > > > > 
> > > > > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > > > > returning it. We don't actually have those values for all the boards
> > > > > > tho. My understanding was that there are some generic arm changes
> > > > > > needed, but I need to confirm that.
> > > > > 
> > > > > Just return 0 in both registers when you have nothing better to return.
> > > > 
> > > > That's not a good idea - it'll cause 512MB of 1:1 mappings to be setup
> > > > at virtual location 0 using the IO flags, which may conflict on ARMv6+.
> > > > A better idea would be to return 0xfff00000, which'll cause it to only
> > > > populate the top-most 1MB.
> > > 
> > > Given that this a phony address, better test for 0 explicitly and skip 
> > > the mapping as well as bailing out early from putchar, etc.
> > 
> > That could be 0 phys, which given there is no defined memory layout on
> > ARM, I would not put it past someone to put a UART at phys location 0
> > one day.
> 
> Who knows.  But in this case I think it is probably cleaner to just care 
> about the virtual address, and do something like this:

I need something for this merge window (which is closing soon) .. So I'm
just going to go with my original revert .. It seems like anything I do
to get addruart to return something turns into too large a patch which I
don't want to force.

I think the revert should be fine until we come up with a good way to
fix this, either what you suggested or something entirely in msm.

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] msm: fix debug-macro.S build failure

[Russell King - ARM Linux]

On Fri, Oct 29, 2010 at 02:17:13PM -0700, Daniel Walker wrote:
> On Thu, 2010-10-28 at 23:03 -0400, Nicolas Pitre wrote:
> > On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:
> > 
> > > On Thu, Oct 28, 2010 at 02:43:16PM -0400, Nicolas Pitre wrote:
> > > > On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:
> > > > 
> > > > > On Thu, Oct 28, 2010 at 02:24:33PM -0400, Nicolas Pitre wrote:
> > > > > > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > > > > > 
> > > > > > > This is what the function currently has,
> > > > > > > 
> > > > > > >         .macro  addruart, rp, rv
> > > > > > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > > > > > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > > > > > >         .endm
> > > > > > > 
> > > > > > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > > > > > returning it. We don't actually have those values for all the boards
> > > > > > > tho. My understanding was that there are some generic arm changes
> > > > > > > needed, but I need to confirm that.
> > > > > > 
> > > > > > Just return 0 in both registers when you have nothing better to return.
> > > > > 
> > > > > That's not a good idea - it'll cause 512MB of 1:1 mappings to be setup
> > > > > at virtual location 0 using the IO flags, which may conflict on ARMv6+.
> > > > > A better idea would be to return 0xfff00000, which'll cause it to only
> > > > > populate the top-most 1MB.
> > > > 
> > > > Given that this a phony address, better test for 0 explicitly and skip 
> > > > the mapping as well as bailing out early from putchar, etc.
> > > 
> > > That could be 0 phys, which given there is no defined memory layout on
> > > ARM, I would not put it past someone to put a UART at phys location 0
> > > one day.
> > 
> > Who knows.  But in this case I think it is probably cleaner to just care 
> > about the virtual address, and do something like this:
> 
> I need something for this merge window (which is closing soon) .. So I'm
> just going to go with my original revert .. It seems like anything I do
> to get addruart to return something turns into too large a patch which I
> don't want to force.
> 
> I think the revert should be fine until we come up with a good way to
> fix this, either what you suggested or something entirely in msm.

Your original patch is unsuitable as it leaves the values uninitialized.

What will happen is that rp will be the value of the procinfo mmuflags,
so effectively zero.  rv will be page table value for the section
following the kernel, which is effectively the physical address of that.

This I guess will mean that it generally won't overwrite an existing
mapping, unless the folowing 512MB of mappings does so.

As I've said, the safest thing is to set rv to 0xfff00000 and have it
only setup one mapping.  Set rp to whatever you like - leave it
uninitialised if you're not filling in the other functions, it doesn't
matter (but note that.)

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

On Fri, 2010-10-29 at 23:14 +0100, Russell King - ARM Linux wrote:

> Your original patch is unsuitable as it leaves the values uninitialized.
> 

All I'm really concerned about is fixing the build issue during this
merge window. But I'll send another patch with your suggestions added.

Daniel
-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] msm: fix debug-macro.S build failure

[Russell King - ARM Linux]

On Fri, Oct 29, 2010 at 03:21:22PM -0700, Daniel Walker wrote:
> On Fri, 2010-10-29 at 23:14 +0100, Russell King - ARM Linux wrote:
> 
> > Your original patch is unsuitable as it leaves the values uninitialized.
> > 
> 
> All I'm really concerned about is fixing the build issue during this
> merge window. But I'll send another patch with your suggestions added.

As we have had a complaint from Linus about pointless churn, and this
approach causes unnecessary churn, I will refuse to apply this.  Fix
the thing properly (it's not hard) or don't bother trying to fix it at
all.

All you need do is to use your initial patch, but change the addruart to
the following:

	.macro	addruart, rp, rv
	mov	rv, #0xff000000
	orr	rv, rv, #0x00f00000
	.endm

and you commit the result.  Problem fixed in one go, both from the
compile point of view _and_ the runtime point of view.  It's really
not difficult.

[PATCH -v3] msm: fix debug-macro.S build failure

[Daniel Walker]

I was planning to send this directly to Linus assuming everyone
agrees.

---

Originally there was an ifdef case to handle when no debug uart
was selected. In commit 0ea1293009826da45e1019f45dfde1e557bb30df
that case was removed which causes the following build failure,

linux-2.6/arch/arm/kernel/debug.S: Assembler messages:
linux-2.6/arch/arm/kernel/debug.S:174: Error: bad instruction `addruart r1,r2'
linux-2.6/arch/arm/kernel/debug.S:176: Error: bad instruction `waituart r2,r3'
linux-2.6/arch/arm/kernel/debug.S:177: Error: bad instruction `senduart r1,r3'
linux-2.6/arch/arm/kernel/debug.S:178: Error: bad instruction `busyuart r2,r3'
linux-2.6/arch/arm/kernel/debug.S:190: Error: bad instruction `addruart r1,r2'

This is a partial revert to add back the case which was removed with
two caveats. First the API for the addruart macro was updated, and
the new addruart case now return 0xfff00000 so that a know IO mapping
is created instead of a random one.

Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Jason Wang <jason77.wang@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Nicolas Pitre <nico@fluxnic.net>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 arch/arm/mach-msm/include/mach/debug-macro.S |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-msm/include/mach/debug-macro.S b/arch/arm/mach-msm/include/mach/debug-macro.S
index fbd5d90..f792e01 100644
--- a/arch/arm/mach-msm/include/mach/debug-macro.S
+++ b/arch/arm/mach-msm/include/mach/debug-macro.S
@@ -36,7 +36,18 @@
 	tst	\rd, #0x04
 	beq	1001b
 	.endm
+#else
+	.macro  addruart, rp, rv
+	mov	\rv, #0xff000000
+	orr	\rv, \rv, #0x00f00000
+	.endm
 
-	.macro	busyuart,rd,rx
+	.macro	senduart,rd,rx
+	.endm
+
+	.macro	waituart,rd,rx
 	.endm
 #endif
+
+	.macro	busyuart,rd,rx
+	.endm
-- 
1.7.1

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Fri, 29 Oct 2010, Daniel Walker wrote:

> On Thu, 2010-10-28 at 23:03 -0400, Nicolas Pitre wrote:
> > On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:
> > 
> > > On Thu, Oct 28, 2010 at 02:43:16PM -0400, Nicolas Pitre wrote:
> > > > On Thu, 28 Oct 2010, Russell King - ARM Linux wrote:
> > > > 
> > > > > On Thu, Oct 28, 2010 at 02:24:33PM -0400, Nicolas Pitre wrote:
> > > > > > On Thu, 28 Oct 2010, Daniel Walker wrote:
> > > > > > 
> > > > > > > This is what the function currently has,
> > > > > > > 
> > > > > > >         .macro  addruart, rp, rv
> > > > > > >         ldr     \rp, =MSM_DEBUG_UART_PHYS
> > > > > > >         ldr     \rv, =MSM_DEBUG_UART_BASE
> > > > > > >         .endm
> > > > > > > 
> > > > > > > So if we have a MSM_DEBUG_UART_PHYS and MSM_DEBUG_UART_BASE we're
> > > > > > > returning it. We don't actually have those values for all the boards
> > > > > > > tho. My understanding was that there are some generic arm changes
> > > > > > > needed, but I need to confirm that.
> > > > > > 
> > > > > > Just return 0 in both registers when you have nothing better to return.
> > > > > 
> > > > > That's not a good idea - it'll cause 512MB of 1:1 mappings to be setup
> > > > > at virtual location 0 using the IO flags, which may conflict on ARMv6+.
> > > > > A better idea would be to return 0xfff00000, which'll cause it to only
> > > > > populate the top-most 1MB.
> > > > 
> > > > Given that this a phony address, better test for 0 explicitly and skip 
> > > > the mapping as well as bailing out early from putchar, etc.
> > > 
> > > That could be 0 phys, which given there is no defined memory layout on
> > > ARM, I would not put it past someone to put a UART at phys location 0
> > > one day.
> > 
> > Who knows.  But in this case I think it is probably cleaner to just care 
> > about the virtual address, and do something like this:
> 
> I need something for this merge window (which is closing soon) .. So I'm
> just going to go with my original revert .. It seems like anything I do
> to get addruart to return something turns into too large a patch which I
> don't want to force.

Could you at least comment the patch I posted for you?

If this fixes the issue then Russell might just merge it now.  In any 
case, that kind of fix may perfectly well be merged during the -rc 
period since that's exactly what the -rc period is all about: fixing 
those kind of fallouts after the big merge.


Nicolas

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

On Fri, 2010-10-29 at 18:17 -0400, Nicolas Pitre wrote:

> Could you at least comment the patch I posted for you?

I didn't see anything wrong with it, not from an MSM perspective.

> If this fixes the issue then Russell might just merge it now.  In any 
> case, that kind of fix may perfectly well be merged during the -rc 
> period since that's exactly what the -rc period is all about: fixing 
> those kind of fallouts after the big merge.

I'd like to have MSM building before -rc1 , that's why I was suggesting
I go with my original revert then we would fix this issue during the -rc
cycle.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Fri, 29 Oct 2010, Daniel Walker wrote:

> On Fri, 2010-10-29 at 18:17 -0400, Nicolas Pitre wrote:
> 
> > Could you at least comment the patch I posted for you?
> 
> I didn't see anything wrong with it, not from an MSM perspective.

So now you have two possibilities: my patch, or Russell's suggestions.  
Personally I find my patch is slightly better in the sense that it 
doesn't make use of a magic 0xfff00000 value with no obvious semantic.  
But you have the choice.

> I'd like to have MSM building before -rc1 , that's why I was suggesting
> I go with my original revert then we would fix this issue during the -rc
> cycle.

y
Your "fix" is just as broken as the current compilation problem.  Sure 
with your revert you may compile a kernel, but that kernel may as well 
crash at run time instead.


Nicolas

[PATCH] msm: fix debug-macro.S build failure

[Daniel Walker]

On Fri, 2010-10-29 at 18:53 -0400, Nicolas Pitre wrote:
> On Fri, 29 Oct 2010, Daniel Walker wrote:
> 
> > On Fri, 2010-10-29 at 18:17 -0400, Nicolas Pitre wrote:
> > 
> > > Could you at least comment the patch I posted for you?
> > 
> > I didn't see anything wrong with it, not from an MSM perspective.
> 
> So now you have two possibilities: my patch, or Russell's suggestions.  
> Personally I find my patch is slightly better in the sense that it 
> doesn't make use of a magic 0xfff00000 value with no obvious semantic.  
> But you have the choice.

Russell's because I think I can get that in right now, which is all I
care about.

> > I'd like to have MSM building before -rc1 , that's why I was suggesting
> > I go with my original revert then we would fix this issue during the -rc
> > cycle.
> 
> y
> Your "fix" is just as broken as the current compilation problem.  Sure 
> with your revert you may compile a kernel, but that kernel may as well 
> crash at run time instead.

The build issue is a little more dire I think, I never suggested my
revert fixed anything but the build issue (which was the point) .. It
was just suppose to bring back what was wrongfully removed.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] msm: fix debug-macro.S build failure

[David Brown]

On Fri, Oct 29, 2010 at 04:07:18PM -0700, Daniel Walker wrote:

> The build issue is a little more dire I think, I never suggested my
> revert fixed anything but the build issue (which was the point) .. It
> was just suppose to bring back what was wrongfully removed.

A compile error for an invalid configuration is better than a crash.

David

[PATCH] msm: fix debug-macro.S build failure

[Rohit Vaswani]

On 10/28/2010 10:46 AM, Russell King - ARM Linux wrote:
> On Thu, Oct 28, 2010 at 09:41:06AM -0700, Daniel Walker wrote:
>> On Thu, 2010-10-28 at 17:35 +0100, Russell King - ARM Linux wrote:
>>> On Thu, Oct 28, 2010 at 09:30:13AM -0700, Daniel Walker wrote:
>>>> On Wed, 2010-10-27 at 22:30 -0400, Nicolas Pitre wrote:
>>>>> On Wed, 27 Oct 2010, Russell King - ARM Linux wrote:
>>>>>
>>>>>>> +#else
>>>>>>> +	.macro	addruart, rx, tmp
>>>>>> addruart is now expected to return two values, and 'tmp' ends up being
>>>>>> misleading.  This is a recipe for mistakes unless this is corrected.
>>>>>> It should be 'rp, rv' instead of 'rx, tmp'.
>>>>> Also, it is probably not a good idea to return nothing. Otherwise the
>>>>> code using those macros will then work on random values that just
>>>>> happened to be in the corresponding register at the call location.
>>>> Should we have something in generic arm code that allows this to just be
>>>> turned off? Ideally we don't want any of this stuff even running.
>>> If you don't want it, don't enable DEBUG_LL.  DEBUG_LL is what you
>>> enable for initial board bring-up and once you're getting kernel
>>> messages via standard console drivers, you disable it.
>> This board doesn't have the ability to support DEBUG_LL .. I don't want
>> the user to have the option to even select that.
> But do not leave this function empty otherwise if you
> enable DEBUG_LL, you'll get unpredictable behaviour.
>
With DEBUG_LL enabled and if all the 3 macros (addruart, senduart, 
waituart) are empty
nothing references these values - so wouldn't this be okay?
> All that you'll then have is a temporary mapping which is torn-down
> when the kernel initializes the page tables properly.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Thanks,
Rohit Vaswani

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] msm: fix debug-macro.S build failure

[Nicolas Pitre]

On Thu, 28 Oct 2010, Rohit Vaswani wrote:

> On 10/28/2010 10:46 AM, Russell King - ARM Linux wrote:
> > On Thu, Oct 28, 2010 at 09:41:06AM -0700, Daniel Walker wrote:
> > > On Thu, 2010-10-28 at 17:35 +0100, Russell King - ARM Linux wrote:
> > > > If you don't want it, don't enable DEBUG_LL.  DEBUG_LL is what you
> > > > enable for initial board bring-up and once you're getting kernel
> > > > messages via standard console drivers, you disable it.
> > > This board doesn't have the ability to support DEBUG_LL .. I don't want
> > > the user to have the option to even select that.
> > But do not leave this function empty otherwise if you
> > enable DEBUG_LL, you'll get unpredictable behaviour.
> > 
> With DEBUG_LL enabled and if all the 3 macros (addruart, senduart, waituart)
> are empty
> nothing references these values - so wouldn't this be okay?

The addruart macro is used to set up a mapping so the senduart and 
waituart can access the UART port.  But even if senduart and waituart 
are empty, the empty addruart is wrong because if it doesn't initialize 
the passed registers, a random mapping will be created with whatever was 
in those register before.  That random mapping could overwrite the 
mapping that just was created for the kernel image for example.


Nicolas